| Message ID | 20200917104441.31738-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | docker.py: always use --rm | expand |
On Thu, Sep 17, 2020 at 12:44:41PM +0200, Paolo Bonzini wrote: > Avoid that containers pile up. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tests/docker/Makefile.include | 1 - > tests/docker/docker.py | 4 ++-- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include > index 3daabaa2fd..75704268ff 100644 > --- a/tests/docker/Makefile.include > +++ b/tests/docker/Makefile.include > @@ -243,7 +243,6 @@ docker-run: docker-qemu-src > $(DOCKER_SCRIPT) run \ > $(if $(NOUSER),,--run-as-current-user) \ > --security-opt seccomp=unconfined \ > - $(if $V,,--rm) \ I guess the intention was that if someone is running verbose they might want to get back into the container after a failure. This is certainly a pain for CI though, because running verbose is desirable but keeping around dead containers is not. The DEBUG=1 option is likely a better general purpose debugging approach than relying on the dead container being left behind, so Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > $(if $(DEBUG),-ti,) \ > $(if $(NETWORK),$(if $(subst $(NETWORK),,1),--net=$(NETWORK)),--net=none) \ > -e TARGET_LIST=$(subst $(SPACE),$(COMMA),$(TARGET_LIST)) \ > diff --git a/tests/docker/docker.py b/tests/docker/docker.py > index 356d7618f1..36b7868406 100755 > --- a/tests/docker/docker.py > +++ b/tests/docker/docker.py > @@ -377,7 +377,7 @@ class Docker(object): > if self._command[0] == "podman": > cmd.insert(0, '--userns=keep-id') > > - ret = self._do_check(["run", "--label", > + ret = self._do_check(["run", "--rm", "--label", > "com.qemu.instance.uuid=" + label] + cmd, > quiet=quiet) > if not keep: > @@ -616,7 +616,7 @@ class CcCommand(SubCommand): > if argv and argv[0] == "--": > argv = argv[1:] > cwd = os.getcwd() > - cmd = ["--rm", "-w", cwd, > + cmd = ["-w", cwd, > "-v", "%s:%s:rw" % (cwd, cwd)] > if args.paths: > for p in args.paths: > -- > 2.26.2 > Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Patchew URL: https://patchew.org/QEMU/20200917104441.31738-1-pbonzini@redhat.com/ Hi, This series failed build test on FreeBSD host. Please find the details below. The full log is available at http://patchew.org/logs/20200917104441.31738-1-pbonzini@redhat.com/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20200917104441.31738-1-pbonzini@redhat.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. The full log is available at http://patchew.org/logs/20200917104441.31738-1-pbonzini@redhat.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On 17/09/20 12:48, Daniel P. Berrangé wrote: > On Thu, Sep 17, 2020 at 12:44:41PM +0200, Paolo Bonzini wrote: >> Avoid that containers pile up. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> tests/docker/Makefile.include | 1 - >> tests/docker/docker.py | 4 ++-- >> 2 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include >> index 3daabaa2fd..75704268ff 100644 >> --- a/tests/docker/Makefile.include >> +++ b/tests/docker/Makefile.include >> @@ -243,7 +243,6 @@ docker-run: docker-qemu-src >> $(DOCKER_SCRIPT) run \ >> $(if $(NOUSER),,--run-as-current-user) \ >> --security-opt seccomp=unconfined \ >> - $(if $V,,--rm) \ > > I guess the intention was that if someone is running verbose they might > want to get back into the container after a failure. This is certainly > a pain for CI though, because running verbose is desirable but keeping > around dead containers is not. > > The DEBUG=1 option is likely a better general purpose debugging approach > than relying on the dead container being left behind, so > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Peter, can you apply this directly in order to unbreak Patchew? Paolo > > >> $(if $(DEBUG),-ti,) \ >> $(if $(NETWORK),$(if $(subst $(NETWORK),,1),--net=$(NETWORK)),--net=none) \ >> -e TARGET_LIST=$(subst $(SPACE),$(COMMA),$(TARGET_LIST)) \ >> diff --git a/tests/docker/docker.py b/tests/docker/docker.py >> index 356d7618f1..36b7868406 100755 >> --- a/tests/docker/docker.py >> +++ b/tests/docker/docker.py >> @@ -377,7 +377,7 @@ class Docker(object): >> if self._command[0] == "podman": >> cmd.insert(0, '--userns=keep-id') >> >> - ret = self._do_check(["run", "--label", >> + ret = self._do_check(["run", "--rm", "--label", >> "com.qemu.instance.uuid=" + label] + cmd, >> quiet=quiet) >> if not keep: >> @@ -616,7 +616,7 @@ class CcCommand(SubCommand): >> if argv and argv[0] == "--": >> argv = argv[1:] >> cwd = os.getcwd() >> - cmd = ["--rm", "-w", cwd, >> + cmd = ["-w", cwd, >> "-v", "%s:%s:rw" % (cwd, cwd)] >> if args.paths: >> for p in args.paths: >> -- >> 2.26.2 >> > > Regards, > Daniel >
On 9/17/20 12:44 PM, Paolo Bonzini wrote: > Avoid that containers pile up. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tests/docker/Makefile.include | 1 - > tests/docker/docker.py | 4 ++-- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include > index 3daabaa2fd..75704268ff 100644 > --- a/tests/docker/Makefile.include > +++ b/tests/docker/Makefile.include > @@ -243,7 +243,6 @@ docker-run: docker-qemu-src > $(DOCKER_SCRIPT) run \ > $(if $(NOUSER),,--run-as-current-user) \ > --security-opt seccomp=unconfined \ > - $(if $V,,--rm) \ > $(if $(DEBUG),-ti,) \ Alternatively: - $(if $V,,--rm) - $(if $(DEBUG),-ti,) + $(if $(DEBUG),-ti,--rm) Anyhow: Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > $(if $(NETWORK),$(if $(subst $(NETWORK),,1),--net=$(NETWORK)),--net=none) \ > -e TARGET_LIST=$(subst $(SPACE),$(COMMA),$(TARGET_LIST)) \ > diff --git a/tests/docker/docker.py b/tests/docker/docker.py > index 356d7618f1..36b7868406 100755 > --- a/tests/docker/docker.py > +++ b/tests/docker/docker.py > @@ -377,7 +377,7 @@ class Docker(object): > if self._command[0] == "podman": > cmd.insert(0, '--userns=keep-id') > > - ret = self._do_check(["run", "--label", > + ret = self._do_check(["run", "--rm", "--label", > "com.qemu.instance.uuid=" + label] + cmd, > quiet=quiet) > if not keep: > @@ -616,7 +616,7 @@ class CcCommand(SubCommand): > if argv and argv[0] == "--": > argv = argv[1:] > cwd = os.getcwd() > - cmd = ["--rm", "-w", cwd, > + cmd = ["-w", cwd, > "-v", "%s:%s:rw" % (cwd, cwd)] > if args.paths: > for p in args.paths: >
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 9/17/20 12:44 PM, Paolo Bonzini wrote: >> Avoid that containers pile up. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> tests/docker/Makefile.include | 1 - >> tests/docker/docker.py | 4 ++-- >> 2 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include >> index 3daabaa2fd..75704268ff 100644 >> --- a/tests/docker/Makefile.include >> +++ b/tests/docker/Makefile.include >> @@ -243,7 +243,6 @@ docker-run: docker-qemu-src >> $(DOCKER_SCRIPT) run \ >> $(if $(NOUSER),,--run-as-current-user) \ >> --security-opt seccomp=unconfined \ >> - $(if $V,,--rm) \ >> $(if $(DEBUG),-ti,) \ > > Alternatively: > > - $(if $V,,--rm) > - $(if $(DEBUG),-ti,) > + $(if $(DEBUG),-ti,--rm) Surely that should b: $(if $(DEBUG),-ti --rm,) I think being able to keep the container around is useful for debug, I have no problem with changing the behaviour for V=1. > > Anyhow: > Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> $(if $(NETWORK),$(if $(subst $(NETWORK),,1),--net=$(NETWORK)),--net=none) \ >> -e TARGET_LIST=$(subst $(SPACE),$(COMMA),$(TARGET_LIST)) \ >> diff --git a/tests/docker/docker.py b/tests/docker/docker.py >> index 356d7618f1..36b7868406 100755 >> --- a/tests/docker/docker.py >> +++ b/tests/docker/docker.py >> @@ -377,7 +377,7 @@ class Docker(object): >> if self._command[0] == "podman": >> cmd.insert(0, '--userns=keep-id') >> >> - ret = self._do_check(["run", "--label", >> + ret = self._do_check(["run", "--rm", "--label", >> "com.qemu.instance.uuid=" + label] + cmd, >> quiet=quiet) >> if not keep: >> @@ -616,7 +616,7 @@ class CcCommand(SubCommand): >> if argv and argv[0] == "--": >> argv = argv[1:] >> cwd = os.getcwd() >> - cmd = ["--rm", "-w", cwd, >> + cmd = ["-w", cwd, >> "-v", "%s:%s:rw" % (cwd, cwd)] >> if args.paths: >> for p in args.paths: >> -- Alex Bennée
Il gio 17 set 2020, 18:53 Alex Bennée <alex.bennee@linaro.org> ha scritto: > > Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > > > On 9/17/20 12:44 PM, Paolo Bonzini wrote: > >> Avoid that containers pile up. > >> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >> --- > >> tests/docker/Makefile.include | 1 - > >> tests/docker/docker.py | 4 ++-- > >> 2 files changed, 2 insertions(+), 3 deletions(-) > >> > >> diff --git a/tests/docker/Makefile.include > b/tests/docker/Makefile.include > >> index 3daabaa2fd..75704268ff 100644 > >> --- a/tests/docker/Makefile.include > >> +++ b/tests/docker/Makefile.include > >> @@ -243,7 +243,6 @@ docker-run: docker-qemu-src > >> $(DOCKER_SCRIPT) run \ > >> $(if $(NOUSER),,--run-as-current-user) \ > >> --security-opt seccomp=unconfined \ > >> - $(if $V,,--rm) \ > >> $(if $(DEBUG),-ti,) \ > > > > Alternatively: > > > > - $(if $V,,--rm) > > - $(if $(DEBUG),-ti,) > > + $(if $(DEBUG),-ti,--rm) > > Surely that should b: > > $(if $(DEBUG),-ti --rm,) > > I think being able to keep the container around is useful for debug, I > have no problem with changing the behaviour for V=1. > You probably mean $(if $(DEBUG),-ti,--rm) but that would not work because the patch adds --rm unconditionally in docker.py. But $(DEBUG) gives you a shell to run the test from, so I don't think skipping --rm is useful even in the $(DEBUG) case. Paolo > <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Il gio 17 set 2020, 18:53 Alex Bennée <<a href="mailto:alex.bennee@linaro.org">alex.bennee@linaro.org</a>> ha scritto:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br> Philippe Mathieu-Daudé <<a href="mailto:f4bug@amsat.org" target="_blank" rel="noreferrer">f4bug@amsat.org</a>> writes:<br> <br> > On 9/17/20 12:44 PM, Paolo Bonzini wrote:<br> >> Avoid that containers pile up.<br> >> <br> >> Signed-off-by: Paolo Bonzini <<a href="mailto:pbonzini@redhat.com" target="_blank" rel="noreferrer">pbonzini@redhat.com</a>><br> >> ---<br> >> tests/docker/Makefile.include | 1 -<br> >> tests/docker/docker.py | 4 ++--<br> >> 2 files changed, 2 insertions(+), 3 deletions(-)<br> >> <br> >> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include<br> >> index 3daabaa2fd..75704268ff 100644<br> >> --- a/tests/docker/Makefile.include<br> >> +++ b/tests/docker/Makefile.include<br> >> @@ -243,7 +243,6 @@ docker-run: docker-qemu-src<br> >> $(DOCKER_SCRIPT) run \<br> >> $(if $(NOUSER),,--run-as-current-user) \<br> >> --security-opt seccomp=unconfined \<br> >> - $(if $V,,--rm) \<br> >> $(if $(DEBUG),-ti,) \<br> ><br> > Alternatively:<br> ><br> > - $(if $V,,--rm)<br> > - $(if $(DEBUG),-ti,)<br> > + $(if $(DEBUG),-ti,--rm)<br> <br> Surely that should b:<br> <br> $(if $(DEBUG),-ti --rm,)<br> <br> I think being able to keep the container around is useful for debug, I<br> have no problem with changing the behaviour for V=1.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">You probably mean $(if $(DEBUG),-ti,--rm) but that would not work because the patch adds --rm unconditionally in docker.py. But $(DEBUG) gives you a shell to run the test from, so I don't think skipping --rm is useful even in the $(DEBUG) case.</div><div dir="auto"><br></div><div dir="auto">Paolo</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote></div></div></div>
Got it. On Thu, 17 Sep 2020, 17:58 Paolo Bonzini, <pbonzini@redhat.com> wrote: > > > Il gio 17 set 2020, 18:53 Alex Bennée <alex.bennee@linaro.org> ha scritto: > >> >> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >> >> > On 9/17/20 12:44 PM, Paolo Bonzini wrote: >> >> Avoid that containers pile up. >> >> >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> >> --- >> >> tests/docker/Makefile.include | 1 - >> >> tests/docker/docker.py | 4 ++-- >> >> 2 files changed, 2 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/tests/docker/Makefile.include >> b/tests/docker/Makefile.include >> >> index 3daabaa2fd..75704268ff 100644 >> >> --- a/tests/docker/Makefile.include >> >> +++ b/tests/docker/Makefile.include >> >> @@ -243,7 +243,6 @@ docker-run: docker-qemu-src >> >> $(DOCKER_SCRIPT) run \ >> >> $(if $(NOUSER),,--run-as-current-user) \ >> >> --security-opt seccomp=unconfined \ >> >> - $(if $V,,--rm) \ >> >> $(if $(DEBUG),-ti,) \ >> > >> > Alternatively: >> > >> > - $(if $V,,--rm) >> > - $(if $(DEBUG),-ti,) >> > + $(if $(DEBUG),-ti,--rm) >> >> Surely that should b: >> >> $(if $(DEBUG),-ti --rm,) >> >> I think being able to keep the container around is useful for debug, I >> have no problem with changing the behaviour for V=1. >> > > You probably mean $(if $(DEBUG),-ti,--rm) but that would not work because > the patch adds --rm unconditionally in docker.py. But $(DEBUG) gives you a > shell to run the test from, so I don't think skipping --rm is useful even > in the $(DEBUG) case. > > Paolo > >> <div dir="auto">Got it. </div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 17 Sep 2020, 17:58 Paolo Bonzini, <<a href="mailto:pbonzini@redhat.com">pbonzini@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Il gio 17 set 2020, 18:53 Alex Bennée <<a href="mailto:alex.bennee@linaro.org" target="_blank" rel="noreferrer">alex.bennee@linaro.org</a>> ha scritto:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br> Philippe Mathieu-Daudé <<a href="mailto:f4bug@amsat.org" rel="noreferrer noreferrer" target="_blank">f4bug@amsat.org</a>> writes:<br> <br> > On 9/17/20 12:44 PM, Paolo Bonzini wrote:<br> >> Avoid that containers pile up.<br> >> <br> >> Signed-off-by: Paolo Bonzini <<a href="mailto:pbonzini@redhat.com" rel="noreferrer noreferrer" target="_blank">pbonzini@redhat.com</a>><br> >> ---<br> >> tests/docker/Makefile.include | 1 -<br> >> tests/docker/docker.py | 4 ++--<br> >> 2 files changed, 2 insertions(+), 3 deletions(-)<br> >> <br> >> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include<br> >> index 3daabaa2fd..75704268ff 100644<br> >> --- a/tests/docker/Makefile.include<br> >> +++ b/tests/docker/Makefile.include<br> >> @@ -243,7 +243,6 @@ docker-run: docker-qemu-src<br> >> $(DOCKER_SCRIPT) run \<br> >> $(if $(NOUSER),,--run-as-current-user) \<br> >> --security-opt seccomp=unconfined \<br> >> - $(if $V,,--rm) \<br> >> $(if $(DEBUG),-ti,) \<br> ><br> > Alternatively:<br> ><br> > - $(if $V,,--rm)<br> > - $(if $(DEBUG),-ti,)<br> > + $(if $(DEBUG),-ti,--rm)<br> <br> Surely that should b:<br> <br> $(if $(DEBUG),-ti --rm,)<br> <br> I think being able to keep the container around is useful for debug, I<br> have no problem with changing the behaviour for V=1.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">You probably mean $(if $(DEBUG),-ti,--rm) but that would not work because the patch adds --rm unconditionally in docker.py. But $(DEBUG) gives you a shell to run the test from, so I don't think skipping --rm is useful even in the $(DEBUG) case.</div><div dir="auto"><br></div><div dir="auto">Paolo</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote></div></div></div> </blockquote></div>
On Thu, 17 Sep 2020 at 11:45, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Avoid that containers pile up. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tests/docker/Makefile.include | 1 - > tests/docker/docker.py | 4 ++-- > 2 files changed, 2 insertions(+), 3 deletions(-) Applied to master, thanks. -- PMM
diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include index 3daabaa2fd..75704268ff 100644 --- a/tests/docker/Makefile.include +++ b/tests/docker/Makefile.include @@ -243,7 +243,6 @@ docker-run: docker-qemu-src $(DOCKER_SCRIPT) run \ $(if $(NOUSER),,--run-as-current-user) \ --security-opt seccomp=unconfined \ - $(if $V,,--rm) \ $(if $(DEBUG),-ti,) \ $(if $(NETWORK),$(if $(subst $(NETWORK),,1),--net=$(NETWORK)),--net=none) \ -e TARGET_LIST=$(subst $(SPACE),$(COMMA),$(TARGET_LIST)) \ diff --git a/tests/docker/docker.py b/tests/docker/docker.py index 356d7618f1..36b7868406 100755 --- a/tests/docker/docker.py +++ b/tests/docker/docker.py @@ -377,7 +377,7 @@ class Docker(object): if self._command[0] == "podman": cmd.insert(0, '--userns=keep-id') - ret = self._do_check(["run", "--label", + ret = self._do_check(["run", "--rm", "--label", "com.qemu.instance.uuid=" + label] + cmd, quiet=quiet) if not keep: @@ -616,7 +616,7 @@ class CcCommand(SubCommand): if argv and argv[0] == "--": argv = argv[1:] cwd = os.getcwd() - cmd = ["--rm", "-w", cwd, + cmd = ["-w", cwd, "-v", "%s:%s:rw" % (cwd, cwd)] if args.paths: for p in args.paths:
Avoid that containers pile up. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- tests/docker/Makefile.include | 1 - tests/docker/docker.py | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-)