| Message ID | 20190919171015.12681-5-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | testing/next (docker/podman, tcg, build fixes) | expand |
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.bennee@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. 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 > >
Cleber Rosa <crosa@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.bennee@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
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]) + except subprocess.CalledProcessError: + # i likely finished running before we got here + pass def clean(self): self._do_kill_instances(False, False)
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.bennee@linaro.org> --- tests/docker/docker.py | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) -- 2.20.1