[Qemu-devel] [PATCH v2 2/5] tests/docker: add podman support

Marc-André Lureau posted 5 patches 6 years, 7 months ago
Maintainers: "Alex Bennée" <alex.bennee@linaro.org>, Fam Zheng <fam@euphon.net>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>
There is a newer version of this series
[Qemu-devel] [PATCH v2 2/5] tests/docker: add podman support
Posted by Marc-André Lureau 6 years, 7 months ago
Allow to specify the container engine to run with ENGINE variable.

By default, ENGINE=auto and will select either podman or docker.

With current podman, we have to use a uidmap trick in order to be able
to rw-share the ccache directory with the container user.

With a user 1000, the default mapping is:
1000 (host) -> 0 (container).

So write access to /var/tmp/ccache ends will end with permission
denied error.

With "--uidmap 1000:0:1 --uidmap 0:1:1000", the mapping is:
1000 (host) -> 0 (container, 1st namespace) -> 1000 (container, 2nd namespace).

(the rest is mumbo jumbo to avoid holes in the range of UIDs)

A future podman version may have an option such as --userns-keep-uid.
Thanks to Debarshi Ray for the help!

Cc: Debarshi Ray <rishi@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 Makefile                      |  2 +-
 tests/docker/Makefile.include | 17 ++++++++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index c63de4e36c..2bcf9bc674 100644
--- a/Makefile
+++ b/Makefile
@@ -1152,7 +1152,7 @@ endif
 	@echo  ''
 	@echo  'Test targets:'
 	@echo  '  check           - Run all tests (check-help for details)'
-	@echo  '  docker          - Help about targets running tests inside Docker containers'
+	@echo  '  docker          - Help about targets running tests inside containers'
 	@echo  '  vm-help         - Help about targets running tests inside VM'
 	@echo  ''
 	@echo  'Documentation targets:'
diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index aaf5396b85..0abd2ab0c9 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -17,7 +17,9 @@ DOCKER_TESTS := $(notdir $(shell \
 
 DOCKER_TOOLS := travis
 
-DOCKER_SCRIPT=$(SRC_PATH)/tests/docker/docker.py
+ENGINE := auto
+
+DOCKER_SCRIPT=$(SRC_PATH)/tests/docker/docker.py --engine $(ENGINE)
 
 TESTS ?= %
 IMAGES ?= %
@@ -146,7 +148,7 @@ $(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES) $(DOCKER_DEPR
 )
 
 docker:
-	@echo 'Build QEMU and run tests inside Docker containers'
+	@echo 'Build QEMU and run tests inside Docker or Podman containers'
 	@echo
 	@echo 'Available targets:'
 	@echo
@@ -193,6 +195,14 @@ endif
 	@echo '    EXECUTABLE=<path>    Include executable in image.'
 	@echo '    EXTRA_FILES="<path> [... <path>]"'
 	@echo '                         Include extra files in image.'
+	@echo '    ENGINE=auto/docker/podman'
+	@echo '                         Specify which container engine to run.'
+
+UID=$(shell id -u)
+UID1=$(shell expr $(UID) + 1)
+ifeq ($(shell $(DOCKER_SCRIPT) probe),podman)
+PODMAN=1
+endif
 
 # This rule if for directly running against an arbitrary docker target.
 # It is called by the expanded docker targets (e.g. make
@@ -212,7 +222,8 @@ docker-run: docker-qemu-src
 			"  COPYING $(EXECUTABLE) to $(IMAGE)"))
 	$(call quiet-command,						\
 		$(DOCKER_SCRIPT) run 					\
-			$(if $(NOUSER),,-u $(shell id -u)) 		\
+			$(if $(NOUSER),,-u $(UID)			\
+				$(if $(PODMAN),--uidmap $(UID):0:1 --uidmap 0:1:$(UID) --uidmap $(UID1):$(UID1):64536)) 		\
 			--security-opt seccomp=unconfined		\
 			$(if $V,,--rm) 					\
 			$(if $(DEBUG),-ti,)				\
-- 
2.22.0.214.g8dca754b1e


Re: [Qemu-devel] [PATCH v2 2/5] tests/docker: add podman support
Posted by Debarshi Ray 6 years, 6 months ago
Hey,

Sorry for the late response. I was on vacation and away from my keyboard.

On Tue, Jul 9, 2019 at 9:44 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> With current podman, we have to use a uidmap trick in order to be able
> to rw-share the ccache directory with the container user.
>
> With a user 1000, the default mapping is:
> 1000 (host) -> 0 (container).
>
> So write access to /var/tmp/ccache ends will end with permission
> denied error.
>
> With "--uidmap 1000:0:1 --uidmap 0:1:1000", the mapping is:
> 1000 (host) -> 0 (container, 1st namespace) -> 1000 (container, 2nd namespace).
>
> (the rest is mumbo jumbo to avoid holes in the range of UIDs)
>
> A future podman version may have an option such as --userns-keep-uid.

The future is here! :)

Since Podman 1.4.0, released on 7th June 2019, you can use
--userns=keep-id instead of typing out the entire UID mapping. The
relevant Podman pull request is:
https://github.com/containers/libpod/pull/3196

Cheers,
Rishi

Re: [Qemu-devel] [PATCH v2 2/5] tests/docker: add podman support
Posted by Paolo Bonzini 6 years, 7 months ago
On 09/07/19 21:43, Marc-André Lureau wrote:
> With current podman, we have to use a uidmap trick in order to be able
> to rw-share the ccache directory with the container user.
> 
> With a user 1000, the default mapping is:
> 1000 (host) -> 0 (container).

Why not do this in docker.py (either as part of patch 1 or separately)?
 Also, can you document in a comment why this is not needed with docker?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v2 2/5] tests/docker: add podman support
Posted by Marc-André Lureau 6 years, 7 months ago
Hi

On Wed, Jul 10, 2019 at 12:27 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 09/07/19 21:43, Marc-André Lureau wrote:
> > With current podman, we have to use a uidmap trick in order to be able
> > to rw-share the ccache directory with the container user.
> >
> > With a user 1000, the default mapping is:
> > 1000 (host) -> 0 (container).
>
> Why not do this in docker.py (either as part of patch 1 or separately)?
>  Also, can you document in a comment why this is not needed with docker?
>

Doing it in docker.py would probably mean parsing and tweaking
arguments given to Docker.run(). Since it's a "temporary" work around,
I would rather have it at the top-level caller, in the Makefile.

I am not very familiar with podman or docker, so I am not able to tell
you why docker does work by default.  @Debarshi Ray might know, as he
helped me finding a workaround.

thanks

Re: [Qemu-devel] [PATCH v2 2/5] tests/docker: add podman support
Posted by Debarshi Ray 6 years, 6 months ago
On Wed, Jul 10, 2019 at 10:40 AM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> I am not very familiar with podman or docker, so I am not able to tell
> you why docker does work by default.  @Debarshi Ray might know, as he
> helped me finding a workaround.

You need to mention the UID mapping via --uidmap arguments (or
--userns=keep-id) because you are using Podman without involving root
on the host anywhere. With Docker the daemon always runs as root. You
either run the user-facing client also as root (with sudo and such) or
you add your user to the special 'docker' group.

These days, very recently, rootless Docker is a thing too:
https://engineering.docker.com/2019/02/experimenting-with-rootless-docker/

However, I don't think that's what the QEMU test suite has been using. :)

When running rootless, you can only map your current UID from the host
into the new user namespace, and usually that gets mapped to UID 0
inside the namespace. Hence the need to override the UID mapping. This
limitation isn't present when root is involved on the host.

Read this commit message for some more details on exactly what happens
when you specify the UID mapping like that:
https://github.com/debarshiray/toolbox/commit/cfcf4eb31e14b3a3

Re: [Qemu-devel] [PATCH v2 2/5] tests/docker: add podman support
Posted by Paolo Bonzini 6 years, 7 months ago
On 10/07/19 10:39, Marc-André Lureau wrote:
>> Why not do this in docker.py (either as part of patch 1 or separately)?
>>  Also, can you document in a comment why this is not needed with docker?
>
> Doing it in docker.py would probably mean parsing and tweaking
> arguments given to Docker.run(). Since it's a "temporary" work around,
> I would rather have it at the top-level caller, in the Makefile.

On the other hand that splits the choice of docker vs. podman in two 
places, and Python is a better place to implement workarounds.

It's not hard to move the workaround there.  The "-u $(shell id -u)" 
option could be replaced by a "--run-as-current-user" option parsed by 
RunCommand, not unlike --add-current-user that BuildCommand already 
supports.

Something like this (untested of course :)):

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index aaf5396b85..019191f1a1 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -212,7 +212,7 @@ docker-run: docker-qemu-src
 			"  COPYING $(EXECUTABLE) to $(IMAGE)"))
 	$(call quiet-command,						\
 		$(DOCKER_SCRIPT) run 					\
-			$(if $(NOUSER),,-u $(shell id -u)) 		\
+			$(if $(NOUSER),,--run-as-current-user) 		\
 			--security-opt seccomp=unconfined		\
 			$(if $V,,--rm) 					\
 			$(if $(DEBUG),-ti,)				\
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 53a8c9c801..92c02aeed8 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -333,8 +333,12 @@ class RunCommand(SubCommand):
     def args(self, parser):
         parser.add_argument("--keep", action="store_true",
                             help="Don't remove image when command completes")
+        parser.add_argument("--run-as-current-user", action="store_true",
+                            help="Run container using the current user's uid")
 
     def run(self, args, argv):
+        if args.use_current_user:
+            argv = [ "-u", str(os.getuid()) ] + argv
         return Docker().run(argv, args.keep, quiet=args.quiet)
 
 

Paolo

> I am not very familiar with podman or docker, so I am not able to tell
> you why docker does work by default.  @Debarshi Ray might know, as he
> helped me finding a workaround.


Re: [Qemu-devel] [PATCH v2 2/5] tests/docker: add podman support
Posted by Alex Bennée 6 years, 7 months ago
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/07/19 10:39, Marc-André Lureau wrote:
>>> Why not do this in docker.py (either as part of patch 1 or separately)?
>>>  Also, can you document in a comment why this is not needed with docker?
>>
>> Doing it in docker.py would probably mean parsing and tweaking
>> arguments given to Docker.run(). Since it's a "temporary" work around,
>> I would rather have it at the top-level caller, in the Makefile.
>
> On the other hand that splits the choice of docker vs. podman in two
> places, and Python is a better place to implement workarounds.

Yeah I agree we should move this trickery away from the Makefiles.

--
Alex Bennée