Re: [PATCH v2 04/16] tests/docker: reduce scary warnings from failed inspect

[Alex Bennée]

Cleber Rosa <address@hidden> writes:

> On Thu, Sep 19, 2019 at 06:10:03PM +0100, Alex Bennée wrote:
>> There is a race here in the clean-up code so lets just accept that
>> sometimes the active task we just looked up might have finished before
>> we got to inspect it.
>>
>> Signed-off-by: Alex Bennée <address@hidden>
>> ---
>>  tests/docker/docker.py | 32 ++++++++++++++++++--------------
>>  1 file changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
>> index 29613afd489..4dca6006d2f 100755
>> --- a/tests/docker/docker.py
>> +++ b/tests/docker/docker.py
>> @@ -235,20 +235,24 @@ class Docker(object):
>>          if not only_active:
>>              cmd.append("-a")
>>          for i in self._output(cmd).split():
>> -            resp = self._output(["inspect", i])
>> -            labels = json.loads(resp)[0]["Config"]["Labels"]
>> -            active = json.loads(resp)[0]["State"]["Running"]
>> -            if not labels:
>> -                continue
>> -            instance_uuid = labels.get("com.qemu.instance.uuid", None)
>> -            if not instance_uuid:
>> -                continue
>> -            if only_known and instance_uuid not in self._instances:
>> -                continue
>> -            print("Terminating", i)
>> -            if active:
>> -                self._do(["kill", i])
>> -            self._do(["rm", i])
>> +            try:
>> +                resp = self._output(["inspect", i])
>> +                labels = json.loads(resp)[0]["Config"]["Labels"]
>> +                active = json.loads(resp)[0]["State"]["Running"]
>> +                if not labels:
>> +                    continue
>> +                instance_uuid = labels.get("com.qemu.instance.uuid", None)
>> +                if not instance_uuid:
>> +                    continue
>> +                if only_known and instance_uuid not in self._instances:
>> +                    continue
>> +                print("Terminating", i)
>> +                if active:
>> +                    self._do(["kill", i])
>> +                    self._do(["rm", i])
>
> Both podman and docker seem to handle "rm -f $INSTANCE" well, which
> would by itself consolidate the two commands in one and minimize the
> race condition.

It's the self.__output which is the real race condition because
check_output complains if the command doesn't succeed with 0. What we
really need is to find somewhat of filtering the ps just for qemu
instances and then just rm -f them as you suggest.

>
> For unexisting containers, and no other errors, podman will return "1
> if one of the specified containers did not exist, and no other
> failure", as per its man page.  I couldn't find any guarantee that
> docker will also return 1 on a similar situation, but that's what
> I've experienced too:
>
>  $ docker rm -f CONTAINER_IS_NOW_FONE
>  Error response from daemon: No such container: CONTAINER_IS_NOW_GONE
>  $ echo $?
>  1
>
> Maybe waiving exit status 1 and nothing else would do the trick here
> and not hide other real problems?
>
> - Cleber.
>
>> +            except subprocess.CalledProcessError:
>> +                # i likely finished running before we got here
>> +                pass
>>
>>      def clean(self):
>>          self._do_kill_instances(False, False)
>> --
>> 2.20.1
>>
>>


--
Alex Bennée