* Re: [PATCH] docker.py: always use --rm
2020-09-17 10:44 [PATCH] docker.py: always use --rm Paolo Bonzini
@ 2020-09-17 10:48 ` Daniel P. Berrangé
2020-09-17 10:49 ` Paolo Bonzini
2020-09-17 10:48 ` no-reply
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Daniel P. Berrangé @ 2020-09-17 10:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
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 :|
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] docker.py: always use --rm
2020-09-17 10:48 ` Daniel P. Berrangé
@ 2020-09-17 10:49 ` Paolo Bonzini
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2020-09-17 10:49 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: Peter Maydell, qemu-devel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] docker.py: always use --rm
2020-09-17 10:44 [PATCH] docker.py: always use --rm Paolo Bonzini
2020-09-17 10:48 ` Daniel P. Berrangé
@ 2020-09-17 10:48 ` no-reply
2020-09-17 10:49 ` no-reply
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: no-reply @ 2020-09-17 10:48 UTC (permalink / raw)
To: pbonzini; +Cc: berrange, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] docker.py: always use --rm
2020-09-17 10:44 [PATCH] docker.py: always use --rm Paolo Bonzini
2020-09-17 10:48 ` Daniel P. Berrangé
2020-09-17 10:48 ` no-reply
@ 2020-09-17 10:49 ` no-reply
2020-09-17 13:38 ` Philippe Mathieu-Daudé
2020-09-18 12:36 ` Peter Maydell
4 siblings, 0 replies; 10+ messages in thread
From: no-reply @ 2020-09-17 10:49 UTC (permalink / raw)
To: pbonzini; +Cc: berrange, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] docker.py: always use --rm
2020-09-17 10:44 [PATCH] docker.py: always use --rm Paolo Bonzini
` (2 preceding siblings ...)
2020-09-17 10:49 ` no-reply
@ 2020-09-17 13:38 ` Philippe Mathieu-Daudé
2020-09-17 16:52 ` Alex Bennée
2020-09-18 12:36 ` Peter Maydell
4 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-17 13:38 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Alex Bennée, berrange
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:
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] docker.py: always use --rm
2020-09-17 13:38 ` Philippe Mathieu-Daudé
@ 2020-09-17 16:52 ` Alex Bennée
2020-09-17 16:58 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Alex Bennée @ 2020-09-17 16:52 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Paolo Bonzini, berrange, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] docker.py: always use --rm
2020-09-17 16:52 ` Alex Bennée
@ 2020-09-17 16:58 ` Paolo Bonzini
2020-09-17 18:50 ` Alex Bennée
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2020-09-17 16:58 UTC (permalink / raw)
To: Alex Bennée; +Cc: berrange, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]
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
>
[-- Attachment #2: Type: text/html, Size: 2779 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] docker.py: always use --rm
2020-09-17 16:58 ` Paolo Bonzini
@ 2020-09-17 18:50 ` Alex Bennée
0 siblings, 0 replies; 10+ messages in thread
From: Alex Bennée @ 2020-09-17 18:50 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: berrange, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]
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
>
>>
[-- Attachment #2: Type: text/html, Size: 3178 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] docker.py: always use --rm
2020-09-17 10:44 [PATCH] docker.py: always use --rm Paolo Bonzini
` (3 preceding siblings ...)
2020-09-17 13:38 ` Philippe Mathieu-Daudé
@ 2020-09-18 12:36 ` Peter Maydell
4 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2020-09-18 12:36 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Daniel P. Berrange, QEMU Developers
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
^ permalink raw reply [flat|nested] 10+ messages in thread