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

[Cleber Rosa]

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.

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
> 
>