1. Patchew
  2. QEMU
  3. View series

[PATCH] docker.py: always use --rm

Paolo Bonzini posted 1 patch 3 years, 7 months ago
Test docker-quick@centos7 failed
Test docker-mingw@fedora failed
Test checkpatch failed
Test FreeBSD failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200917104441.31738-1-pbonzini@redhat.com
Maintainers: Fam Zheng <fam@euphon.net>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>
tests/docker/Makefile.include | 1 -
tests/docker/docker.py        | 4 ++--
2 files changed, 2 insertions(+), 3 deletions(-)
[PATCH] docker.py: always use --rm
Posted by Paolo Bonzini 3 years, 7 months ago 
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,)				\
 			$(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
Re: [PATCH] docker.py: always use --rm
Posted by no-reply@patchew.org 3 years, 7 months ago 
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
Re: [PATCH] docker.py: always use --rm
Posted by no-reply@patchew.org 3 years, 7 months ago 
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
Re: [PATCH] docker.py: always use --rm
Posted by Philippe Mathieu-Daudé 3 years, 7 months ago 
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:
>
Re: [PATCH] docker.py: always use --rm
Posted by Alex Bennée 3 years, 7 months ago 
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
Re: [PATCH] docker.py: always use --rm
Posted by Paolo Bonzini 3 years, 7 months ago 
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

>
Re: [PATCH] docker.py: always use --rm
Posted by Alex Bennée 3 years, 7 months ago 
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
>
>>
Re: [PATCH] docker.py: always use --rm
Posted by Peter Maydell 3 years, 7 months ago 
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
Re: [PATCH] docker.py: always use --rm
Posted by Daniel P. Berrangé 3 years, 7 months ago 
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 :|
Re: [PATCH] docker.py: always use --rm
Posted by Paolo Bonzini 3 years, 7 months ago 
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
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.