1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH v2] docker: Don't enable networking as a side-effect of DEBUG=1

Daniel P. Berrange posted 1 patch 6 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170713144352.2212-1-berrange@redhat.com
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
tests/docker/Makefile.include | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH v2] docker: Don't enable networking as a side-effect of DEBUG=1
Posted by Daniel P. Berrange 6 years, 9 months ago 
When trying to debug problems with tests it is natural to set
DEBUG=1 when starting the docker environment. Unfortunately
this has a side-effect of enabling an eth0 network interface
in the container, which changes the operating environment of
the test suite. IOW tests with fail may suddenly start
working again if DEBUG=1 is set, due to changed network setup.

Add a separate NETWORK variable to allow enablement of
networking separately from DEBUG=1. This can be used in two
ways. To enable the default docker network backend

  make docker-test-build@fedora NETWORK=1

while to enable a specific network backend, eg join the network
associated with the container 'wibble':

  make docker-test-build@fedora NETWORK=container:wibble

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---

Changed in v2:

 - Remove reference to eth0
 - Allow arbitrary docker network backend to be given

 tests/docker/Makefile.include | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 037cb9e9e7..ec8e8d935d 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -106,6 +106,8 @@ docker:
 	@echo '                         (default is 1)'
 	@echo '    DEBUG=1              Stop and drop to shell in the created container'
 	@echo '                         before running the command.'
+	@echo '    NETWORK=1            Enable virtual network interface with default backend.'
+	@echo '    NETWORK=$BACKEND     Enable virtual network interface with $BACKEND.'
 	@echo '    NOUSER               Define to disable adding current user to containers passwd.'
 	@echo '    NOCACHE=1            Ignore cache when build images.'
 	@echo '    EXECUTABLE=<path>    Include executable in image.'
@@ -132,7 +134,8 @@ docker-run: docker-qemu-src
 		$(SRC_PATH)/tests/docker/docker.py run 			\
 			$(if $(NOUSER),,-u $(shell id -u)) -t 		\
 			$(if $V,,--rm) 					\
-			$(if $(DEBUG),-i,--net=none) 			\
+			$(if $(DEBUG),-i,)				\
+			$(if $(NETWORK),$(if $(subst $(NETWORK),,1)$(subst 1,,$(NETWORK)),--net=$(NETWORK),),--net=none) \
 			-e TARGET_LIST=$(TARGET_LIST) 			\
 			-e EXTRA_CONFIGURE_OPTS="$(EXTRA_CONFIGURE_OPTS)" \
 			-e V=$V -e J=$J -e DEBUG=$(DEBUG)		\
-- 
2.13.0
Re: [Qemu-devel] [PATCH v2] docker: Don't enable networking as a side-effect of DEBUG=1
Posted by Fam Zheng 6 years, 9 months ago 
On Thu, 07/13 15:43, Daniel P. Berrange wrote:
> When trying to debug problems with tests it is natural to set
> DEBUG=1 when starting the docker environment. Unfortunately
> this has a side-effect of enabling an eth0 network interface
> in the container, which changes the operating environment of
> the test suite. IOW tests with fail may suddenly start
> working again if DEBUG=1 is set, due to changed network setup.
> 
> Add a separate NETWORK variable to allow enablement of
> networking separately from DEBUG=1. This can be used in two
> ways. To enable the default docker network backend
> 
>   make docker-test-build@fedora NETWORK=1
> 
> while to enable a specific network backend, eg join the network
> associated with the container 'wibble':
> 
>   make docker-test-build@fedora NETWORK=container:wibble
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> 
> Changed in v2:
> 
>  - Remove reference to eth0
>  - Allow arbitrary docker network backend to be given
> 
>  tests/docker/Makefile.include | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index 037cb9e9e7..ec8e8d935d 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -106,6 +106,8 @@ docker:
>  	@echo '                         (default is 1)'
>  	@echo '    DEBUG=1              Stop and drop to shell in the created container'
>  	@echo '                         before running the command.'
> +	@echo '    NETWORK=1            Enable virtual network interface with default backend.'
> +	@echo '    NETWORK=$BACKEND     Enable virtual network interface with $BACKEND.'
>  	@echo '    NOUSER               Define to disable adding current user to containers passwd.'
>  	@echo '    NOCACHE=1            Ignore cache when build images.'
>  	@echo '    EXECUTABLE=<path>    Include executable in image.'
> @@ -132,7 +134,8 @@ docker-run: docker-qemu-src
>  		$(SRC_PATH)/tests/docker/docker.py run 			\
>  			$(if $(NOUSER),,-u $(shell id -u)) -t 		\
>  			$(if $V,,--rm) 					\
> -			$(if $(DEBUG),-i,--net=none) 			\
> +			$(if $(DEBUG),-i,)				\
> +			$(if $(NETWORK),$(if $(subst $(NETWORK),,1)$(subst 1,,$(NETWORK)),--net=$(NETWORK),),--net=none) \

Isn't the first subst enough? We already know $(NETWORK) is non-empty. If it is
"1", $(subst 1,,1) is empty; otherwise, $(subst foo,,1) is non-empty:

			$(if $(NETWORK),$(if $(subst $(NETWORK),,1),,--net=$(NETWORK)),--net=none) \

The second subst is only a necessary condition.

Another way to write it is:

			$(if $(NETWORK),$(if $(findstring $(NETWORK),1),,--net=$(NETWORK)),--net=none) \

Fam

>  			-e TARGET_LIST=$(TARGET_LIST) 			\
>  			-e EXTRA_CONFIGURE_OPTS="$(EXTRA_CONFIGURE_OPTS)" \
>  			-e V=$V -e J=$J -e DEBUG=$(DEBUG)		\
> -- 
> 2.13.0
>
Re: [Qemu-devel] [PATCH v2] docker: Don't enable networking as a side-effect of DEBUG=1
Posted by Daniel P. Berrange 6 years, 9 months ago 
On Fri, Jul 14, 2017 at 10:29:30AM +0800, Fam Zheng wrote:
> On Thu, 07/13 15:43, Daniel P. Berrange wrote:
> > When trying to debug problems with tests it is natural to set
> > DEBUG=1 when starting the docker environment. Unfortunately
> > this has a side-effect of enabling an eth0 network interface
> > in the container, which changes the operating environment of
> > the test suite. IOW tests with fail may suddenly start
> > working again if DEBUG=1 is set, due to changed network setup.
> > 
> > Add a separate NETWORK variable to allow enablement of
> > networking separately from DEBUG=1. This can be used in two
> > ways. To enable the default docker network backend
> > 
> >   make docker-test-build@fedora NETWORK=1
> > 
> > while to enable a specific network backend, eg join the network
> > associated with the container 'wibble':
> > 
> >   make docker-test-build@fedora NETWORK=container:wibble
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> > 
> > Changed in v2:
> > 
> >  - Remove reference to eth0
> >  - Allow arbitrary docker network backend to be given
> > 
> >  tests/docker/Makefile.include | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> > index 037cb9e9e7..ec8e8d935d 100644
> > --- a/tests/docker/Makefile.include
> > +++ b/tests/docker/Makefile.include
> > @@ -106,6 +106,8 @@ docker:
> >  	@echo '                         (default is 1)'
> >  	@echo '    DEBUG=1              Stop and drop to shell in the created container'
> >  	@echo '                         before running the command.'
> > +	@echo '    NETWORK=1            Enable virtual network interface with default backend.'
> > +	@echo '    NETWORK=$BACKEND     Enable virtual network interface with $BACKEND.'
> >  	@echo '    NOUSER               Define to disable adding current user to containers passwd.'
> >  	@echo '    NOCACHE=1            Ignore cache when build images.'
> >  	@echo '    EXECUTABLE=<path>    Include executable in image.'
> > @@ -132,7 +134,8 @@ docker-run: docker-qemu-src
> >  		$(SRC_PATH)/tests/docker/docker.py run 			\
> >  			$(if $(NOUSER),,-u $(shell id -u)) -t 		\
> >  			$(if $V,,--rm) 					\
> > -			$(if $(DEBUG),-i,--net=none) 			\
> > +			$(if $(DEBUG),-i,)				\
> > +			$(if $(NETWORK),$(if $(subst $(NETWORK),,1)$(subst 1,,$(NETWORK)),--net=$(NETWORK),),--net=none) \
> 
> Isn't the first subst enough? We already know $(NETWORK) is non-empty. If it is
> "1", $(subst 1,,1) is empty; otherwise, $(subst foo,,1) is non-empty:
> 
> 			$(if $(NETWORK),$(if $(subst $(NETWORK),,1),,--net=$(NETWORK)),--net=none) \

This looks inverted to me - NETWORK=1, will generate --net=1 and
NETWORK=foo generates empty string.


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: [Qemu-devel] [PATCH v2] docker: Don't enable networking as a side-effect of DEBUG=1
Posted by Fam Zheng 6 years, 9 months ago 
On Fri, 07/14 12:41, Daniel P. Berrange wrote:
> On Fri, Jul 14, 2017 at 10:29:30AM +0800, Fam Zheng wrote:
> > On Thu, 07/13 15:43, Daniel P. Berrange wrote:
> > > When trying to debug problems with tests it is natural to set
> > > DEBUG=1 when starting the docker environment. Unfortunately
> > > this has a side-effect of enabling an eth0 network interface
> > > in the container, which changes the operating environment of
> > > the test suite. IOW tests with fail may suddenly start
> > > working again if DEBUG=1 is set, due to changed network setup.
> > > 
> > > Add a separate NETWORK variable to allow enablement of
> > > networking separately from DEBUG=1. This can be used in two
> > > ways. To enable the default docker network backend
> > > 
> > >   make docker-test-build@fedora NETWORK=1
> > > 
> > > while to enable a specific network backend, eg join the network
> > > associated with the container 'wibble':
> > > 
> > >   make docker-test-build@fedora NETWORK=container:wibble
> > > 
> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > ---
> > > 
> > > Changed in v2:
> > > 
> > >  - Remove reference to eth0
> > >  - Allow arbitrary docker network backend to be given
> > > 
> > >  tests/docker/Makefile.include | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> > > index 037cb9e9e7..ec8e8d935d 100644
> > > --- a/tests/docker/Makefile.include
> > > +++ b/tests/docker/Makefile.include
> > > @@ -106,6 +106,8 @@ docker:
> > >  	@echo '                         (default is 1)'
> > >  	@echo '    DEBUG=1              Stop and drop to shell in the created container'
> > >  	@echo '                         before running the command.'
> > > +	@echo '    NETWORK=1            Enable virtual network interface with default backend.'
> > > +	@echo '    NETWORK=$BACKEND     Enable virtual network interface with $BACKEND.'
> > >  	@echo '    NOUSER               Define to disable adding current user to containers passwd.'
> > >  	@echo '    NOCACHE=1            Ignore cache when build images.'
> > >  	@echo '    EXECUTABLE=<path>    Include executable in image.'
> > > @@ -132,7 +134,8 @@ docker-run: docker-qemu-src
> > >  		$(SRC_PATH)/tests/docker/docker.py run 			\
> > >  			$(if $(NOUSER),,-u $(shell id -u)) -t 		\
> > >  			$(if $V,,--rm) 					\
> > > -			$(if $(DEBUG),-i,--net=none) 			\
> > > +			$(if $(DEBUG),-i,)				\
> > > +			$(if $(NETWORK),$(if $(subst $(NETWORK),,1)$(subst 1,,$(NETWORK)),--net=$(NETWORK),),--net=none) \
> > 
> > Isn't the first subst enough? We already know $(NETWORK) is non-empty. If it is
> > "1", $(subst 1,,1) is empty; otherwise, $(subst foo,,1) is non-empty:
> > 
> > 			$(if $(NETWORK),$(if $(subst $(NETWORK),,1),,--net=$(NETWORK)),--net=none) \
> 
> This looks inverted to me - NETWORK=1, will generate --net=1 and
> NETWORK=foo generates empty string.

You are right, I was confused (subst succeed is FALSE branch of if). I think
s/,,/,/ and it will work?

Fam
Re: [Qemu-devel] [PATCH v2] docker: Don't enable networking as a side-effect of DEBUG=1
Posted by Daniel P. Berrange 6 years, 9 months ago 
On Fri, Jul 14, 2017 at 07:50:03PM +0800, Fam Zheng wrote:
> On Fri, 07/14 12:41, Daniel P. Berrange wrote:
> > On Fri, Jul 14, 2017 at 10:29:30AM +0800, Fam Zheng wrote:
> > > On Thu, 07/13 15:43, Daniel P. Berrange wrote:
> > > > When trying to debug problems with tests it is natural to set
> > > > DEBUG=1 when starting the docker environment. Unfortunately
> > > > this has a side-effect of enabling an eth0 network interface
> > > > in the container, which changes the operating environment of
> > > > the test suite. IOW tests with fail may suddenly start
> > > > working again if DEBUG=1 is set, due to changed network setup.
> > > > 
> > > > Add a separate NETWORK variable to allow enablement of
> > > > networking separately from DEBUG=1. This can be used in two
> > > > ways. To enable the default docker network backend
> > > > 
> > > >   make docker-test-build@fedora NETWORK=1
> > > > 
> > > > while to enable a specific network backend, eg join the network
> > > > associated with the container 'wibble':
> > > > 
> > > >   make docker-test-build@fedora NETWORK=container:wibble
> > > > 
> > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > > ---
> > > > 
> > > > Changed in v2:
> > > > 
> > > >  - Remove reference to eth0
> > > >  - Allow arbitrary docker network backend to be given
> > > > 
> > > >  tests/docker/Makefile.include | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> > > > index 037cb9e9e7..ec8e8d935d 100644
> > > > --- a/tests/docker/Makefile.include
> > > > +++ b/tests/docker/Makefile.include
> > > > @@ -106,6 +106,8 @@ docker:
> > > >  	@echo '                         (default is 1)'
> > > >  	@echo '    DEBUG=1              Stop and drop to shell in the created container'
> > > >  	@echo '                         before running the command.'
> > > > +	@echo '    NETWORK=1            Enable virtual network interface with default backend.'
> > > > +	@echo '    NETWORK=$BACKEND     Enable virtual network interface with $BACKEND.'
> > > >  	@echo '    NOUSER               Define to disable adding current user to containers passwd.'
> > > >  	@echo '    NOCACHE=1            Ignore cache when build images.'
> > > >  	@echo '    EXECUTABLE=<path>    Include executable in image.'
> > > > @@ -132,7 +134,8 @@ docker-run: docker-qemu-src
> > > >  		$(SRC_PATH)/tests/docker/docker.py run 			\
> > > >  			$(if $(NOUSER),,-u $(shell id -u)) -t 		\
> > > >  			$(if $V,,--rm) 					\
> > > > -			$(if $(DEBUG),-i,--net=none) 			\
> > > > +			$(if $(DEBUG),-i,)				\
> > > > +			$(if $(NETWORK),$(if $(subst $(NETWORK),,1)$(subst 1,,$(NETWORK)),--net=$(NETWORK),),--net=none) \
> > > 
> > > Isn't the first subst enough? We already know $(NETWORK) is non-empty. If it is
> > > "1", $(subst 1,,1) is empty; otherwise, $(subst foo,,1) is non-empty:
> > > 
> > > 			$(if $(NETWORK),$(if $(subst $(NETWORK),,1),,--net=$(NETWORK)),--net=none) \
> > 
> > This looks inverted to me - NETWORK=1, will generate --net=1 and
> > NETWORK=foo generates empty string.
> 
> You are right, I was confused (subst succeed is FALSE branch of if). I think
> s/,,/,/ and it will work?

Yep


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: [Qemu-devel] [PATCH v2] docker: Don't enable networking as a side-effect of DEBUG=1
Posted by Fam Zheng 6 years, 9 months ago 
On Fri, 07/14 12:52, Daniel P. Berrange wrote:
> > > > > @@ -132,7 +134,8 @@ docker-run: docker-qemu-src
> > > > >  		$(SRC_PATH)/tests/docker/docker.py run 			\
> > > > >  			$(if $(NOUSER),,-u $(shell id -u)) -t 		\
> > > > >  			$(if $V,,--rm) 					\
> > > > > -			$(if $(DEBUG),-i,--net=none) 			\
> > > > > +			$(if $(DEBUG),-i,)				\
> > > > > +			$(if $(NETWORK),$(if $(subst $(NETWORK),,1)$(subst 1,,$(NETWORK)),--net=$(NETWORK),),--net=none) \
> > > > 
> > > > Isn't the first subst enough? We already know $(NETWORK) is non-empty. If it is
> > > > "1", $(subst 1,,1) is empty; otherwise, $(subst foo,,1) is non-empty:
> > > > 
> > > > 			$(if $(NETWORK),$(if $(subst $(NETWORK),,1),,--net=$(NETWORK)),--net=none) \
> > > 
> > > This looks inverted to me - NETWORK=1, will generate --net=1 and
> > > NETWORK=foo generates empty string.
> > 
> > You are right, I was confused (subst succeed is FALSE branch of if). I think
> > s/,,/,/ and it will work?
> 
> Yep

OK, so dropping the second "$(subst ...)" and the comma we have:

			$(if $(NETWORK),$(if $(subst $(NETWORK),,1),--net=$(NETWORK)),--net=none) \

With that, queued for 2.10:

    https://github.com/famz/qemu/tree/staging

Fam

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.