Cleber Rosa <cr...@redhat.com> 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 <alex.ben...@linaro.org>
>> ---
>> 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
