[libvirt] [PATCH 0/2] Support more container engines in Makefile.ci
Maybe someone can add support for libvirt-lxc later on =) Martin Kletzander (2): Don't include Makefile.ci in Makefile.am Add support for podman in Makefile.ci Makefile.am | 4 +- Makefile.ci | 125 ++++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 96 insertions(+), 33 deletions(-) -- 2.21.0
The way it works now the Makefile needs to be both make valid and automake valid. That is fine for now, but if we want to use anything more advanced, like conditionals, we cannot have it like that any more. So instead forward all ci-* rules to that file. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Makefile.am | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Makefile.am b/Makefile.am index 0d8bb733e6d2..442bae511828 100644 --- a/Makefile.am +++ b/Makefile.am @@ -35,6 +35,7 @@ EXTRA_DIST = \ libvirt-qemu.pc.in \ libvirt-lxc.pc.in \ libvirt-admin.pc.in \ + Makefile.ci \ Makefile.nonreentrant \ autogen.sh \ cfg.mk \ @@ -107,4 +108,5 @@ gen-AUTHORS: rm -f all.list maint.list contrib.list; \ fi -include Makefile.ci +ci-%: + $(MAKE) -f Makefile.ci $@ -- 2.21.0
On Tue, May 07, 2019 at 05:45:30PM +0200, Martin Kletzander wrote:
The way it works now the Makefile needs to be both make valid and automake valid. That is fine for now, but if we want to use anything more advanced, like conditionals, we cannot have it like that any more.
So instead forward all ci-* rules to that file.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Makefile.am | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 0d8bb733e6d2..442bae511828 100644 --- a/Makefile.am +++ b/Makefile.am @@ -35,6 +35,7 @@ EXTRA_DIST = \ libvirt-qemu.pc.in \ libvirt-lxc.pc.in \ libvirt-admin.pc.in \ + Makefile.ci \ Makefile.nonreentrant \ autogen.sh \ cfg.mk \
Indentation is not consistent here - tabs vs non-tabs.
@@ -107,4 +108,5 @@ gen-AUTHORS: rm -f all.list maint.list contrib.list; \ fi
-include Makefile.ci +ci-%: + $(MAKE) -f Makefile.ci $@
Will this cause all variables to be forwarded ? eg will make ci-build@fedora-29 CI_IMAGE_TAG=:latest result in make -f Makefile.ci ci-build@fedora-29 CI_IMAGE_TAG=:latest 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 :|
On Thu, May 09, 2019 at 01:54:42PM +0100, Daniel P. Berrangé wrote:
On Tue, May 07, 2019 at 05:45:30PM +0200, Martin Kletzander wrote:
The way it works now the Makefile needs to be both make valid and automake valid. That is fine for now, but if we want to use anything more advanced, like conditionals, we cannot have it like that any more.
So instead forward all ci-* rules to that file.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Makefile.am | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 0d8bb733e6d2..442bae511828 100644 --- a/Makefile.am +++ b/Makefile.am @@ -35,6 +35,7 @@ EXTRA_DIST = \ libvirt-qemu.pc.in \ libvirt-lxc.pc.in \ libvirt-admin.pc.in \ + Makefile.ci \ Makefile.nonreentrant \ autogen.sh \ cfg.mk \
Indentation is not consistent here - tabs vs non-tabs.
@@ -107,4 +108,5 @@ gen-AUTHORS: rm -f all.list maint.list contrib.list; \ fi
-include Makefile.ci +ci-%: + $(MAKE) -f Makefile.ci $@
Will this cause all variables to be forwarded ?
eg will
make ci-build@fedora-29 CI_IMAGE_TAG=:latest
result in
make -f Makefile.ci ci-build@fedora-29 CI_IMAGE_TAG=:latest
It worked for me with CI_CENGINE, I thing this is forwarded thanks to $(MAKE).
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 :|
On Thu, May 09, 2019 at 03:56:20PM +0200, Martin Kletzander wrote:
On Thu, May 09, 2019 at 01:54:42PM +0100, Daniel P. Berrangé wrote:
On Tue, May 07, 2019 at 05:45:30PM +0200, Martin Kletzander wrote:
The way it works now the Makefile needs to be both make valid and automake valid. That is fine for now, but if we want to use anything more advanced, like conditionals, we cannot have it like that any more.
So instead forward all ci-* rules to that file.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Makefile.am | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am index 0d8bb733e6d2..442bae511828 100644 --- a/Makefile.am +++ b/Makefile.am @@ -35,6 +35,7 @@ EXTRA_DIST = \ libvirt-qemu.pc.in \ libvirt-lxc.pc.in \ libvirt-admin.pc.in \ + Makefile.ci \ Makefile.nonreentrant \ autogen.sh \ cfg.mk \
Indentation is not consistent here - tabs vs non-tabs.
@@ -107,4 +108,5 @@ gen-AUTHORS: rm -f all.list maint.list contrib.list; \ fi
-include Makefile.ci +ci-%: + $(MAKE) -f Makefile.ci $@
Will this cause all variables to be forwarded ?
eg will
make ci-build@fedora-29 CI_IMAGE_TAG=:latest
result in
make -f Makefile.ci ci-build@fedora-29 CI_IMAGE_TAG=:latest
It worked for me with CI_CENGINE, I thing this is forwarded thanks to $(MAKE).
I don't think it is $(MAKE), as that's just a variable that expands to the string "make". It looks like any variables in the makefile and turned into environment variables that are inherited by the child make process. I did a quick test and it all worked as desired. $ cat bar.mak FISH=food fish: $(MAKE) -f foo.mak $@ $ cat foo.mak FISH=cake fish: @echo ">>$(FISH)<<" $ make -f bar.mak fish FISH=pond make -f foo.mak fish make[1]: Entering directory '/home/berrange/tmp'
pond<< make[1]: Leaving directory '/home/berrange/tmp'
So on that basis, with the indent fix: Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
This way more users can run our CI builds locally. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Makefile.ci | 125 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 93 insertions(+), 32 deletions(-) diff --git a/Makefile.ci b/Makefile.ci index 12a62167cc67..e2989ada313c 100644 --- a/Makefile.ci +++ b/Makefile.ci @@ -17,7 +17,7 @@ CI_GIT_ROOT = $(shell git rev-parse --show-toplevel) CI_HOST_SRCDIR = $(CI_SCRATCHDIR)/src # The directory holding the source inside the -# container. ie where we told Docker to expose +# container. ie where we want to expose # the $(CI_HOST_SRCDIR) directory from the host CI_CONT_SRCDIR = /src @@ -46,14 +46,13 @@ CI_CONFIGURE_ARGS = # cloning them CI_SUBMODULES = $(shell git submodule | awk '{ print $$2 }') -# Location of the Docker images we're going to pull +# Location of the container images we're going to pull # Can be useful to overridde to use a locally built # image instead CI_IMAGE_PREFIX = quay.io/libvirt/buildenv- -# Docker defaults to pulling the ':latest' tag but -# if the Docker repo above uses different conventions -# this can override it +# The default tag is ':latest' but if the container +# repo above uses different conventions this can override it CI_IMAGE_TAG = :master # We delete the virtual root after completion, set @@ -71,24 +70,82 @@ CI_REUSE = 0 CI_UID = $(shell id -u) CI_GID = $(shell id -g) -# Docker doesn't require the IDs you run as to exist in +# Container engine runtime we are going to use, can be overridden per make +# invocation, if it is not, we try podman and then default to docker. +ifeq ($(CI_CENGINE),) + CI_CENGINE = $(shell podman version >/dev/null && echo podman || echo docker) +endif + +# IDs you run as do not need to exist in # the container's /etc/passwd & /etc/group files, but -# if they do not, then libvirt's 'make check' will fail +# if they do not, then libvirt's 'make check' will fail # many tests. -# -# We do not directly mount /etc/{passwd,group} as Docker -# is liable to mess with SELinux labelling which will -# then prevent the host accessing them. Copying them -# first is safer. -CI_PWDB_MOUNTS = \ - --volume $(CI_SCRATCHDIR)/group:/etc/group:ro,z \ - --volume $(CI_SCRATCHDIR)/passwd:/etc/passwd:ro,z \ - $(NULL) +ifeq ($(CI_CENGINE),podman) + CI_PWDB_MOUNTS = \ + --volume /etc/group:/etc/group:ro,z \ + --volume /etc/passwd:/etc/passwd:ro,z \ + $(NULL) +else + # We do not directly mount /etc/{passwd,group} as Docker + # is liable to mess with SELinux labelling which will + # then prevent the host accessing them. Copying them + # first is safer. + CI_PWDB_MOUNTS = \ + --volume $(CI_SCRATCHDIR)/group:/etc/group:ro,z \ + --volume $(CI_SCRATCHDIR)/passwd:/etc/passwd:ro,z \ + $(NULL) +endif + +ifeq ($(CI_CENGINE),docker) + # Docker containers can have very large ulimits + # for nofiles - as much as 1048576. This makes + # libvirt very slow at exec'ing programs. + CI_ULIMIT_FILES = 1024 +endif -# Docker containers can have very large ulimits -# for nofiles - as much as 1048576. This makes -# libvirt very slow at exec'ing programs. -CI_ULIMIT_FILES = 1024 +ifeq ($(CI_CENGINE),podman) + # Podman cannot reuse host namespace when running non-root containers. Until + # support for --keep-uid is added we can just create another mapping that will + # do that for us. Beware, that in {uid,git}map=container_id:host_id:range, + # the host_id does actually refer to the uid in the first mapping where 0 + # (root) is mapped to the current user and rest is offset. + + # In order to set up this mapping, we need to keep all the user IDs to prevent + # possible errors as some images might expect UIDs up to 90000 (looking at you + # fedora), so we don't want the overflowuid to be used for them. For mapping + # all the other users properly ther eneeds to be some math done. Don't worry, + # it's just addition and subtraction. + + # 65536 ought to be enough (tm), but for really rare cases the maximums might + # need to be higher, but that only happens when your /etc/sub{u,g}id allow + # users to have more IDs. Unless --keep-uid is supported, let's do this in a + # way that should work for everyone. + CI_MAX_UID = $(shell sed -n "s/^$USER:[^:]\+://p" /etc/subuid) + CI_MAX_GID = $(shell sed -n "s/^$USER:[^:]\+://p" /etc/subgid) + ifeq ($(CI_MAX_UID),) + CI_MAX_UID = 65536 + endif + ifeq ($(CI_MAX_GID),) + CI_MAX_GID = 65536 + endif + CI_UID_OTHER = $(shell echo $$(($(CI_UID)+1))) + CI_GID_OTHER = $(shell echo $$(($(CI_GID)+1))) + CI_UID_OTHER_RANGE = $(shell echo $$(($(CI_MAX_UID)-$(CI_UID)))) + CI_GID_OTHER_RANGE = $(shell echo $$(($(CI_MAX_GID)-$(CI_GID)))) + + CI_PODMAN_ARGS = \ + --uidmap 0:1:$(CI_UID) \ + --uidmap $(CI_UID):0:1 \ + --uidmap $(CI_UID_OTHER):$(CI_UID_OTHER):$(CI_UID_OTHER_RANGE) \ + --gidmap 0:1:$(CI_GID) \ + --gidmap $(CI_GID):0:1 \ + --gidmap $(CI_GID_OTHER):$(CI_GID_OTHER):$(CI_GID_OTHER_RANGE) \ + $(NULL) +else + CI_DOCKER_ARGS = \ + --ulimit nofile=$(CI_ULIMIT_FILES):$(CI_ULIMIT_FILES) \ + $(NULL) +endif # Args to use when cloning a git repo. # -c stop it complaining about checking out a random hash @@ -100,7 +157,7 @@ CI_GIT_ARGS = \ --local \ $(NULL) -# Args to use when running the Docker env +# Args to use when running the container # --rm stop inactive containers getting left behind # --user we execute as the same user & group account # as dev so that file ownership matches host @@ -110,27 +167,30 @@ CI_GIT_ARGS = \ # --ulimit lower files limit for performance reasons # --interactive # --tty Ensure we have ability to Ctrl-C the build -CI_DOCKER_ARGS = \ +CI_CENGINE_ARGS = \ --rm \ --user $(CI_UID):$(CI_GID) \ --interactive \ --tty \ + $(CI_PODMAN_ARGS) \ + $(CI_DOCKER_ARGS) \ $(CI_PWDB_MOUNTS) \ --volume $(CI_HOST_SRCDIR):$(CI_CONT_SRCDIR):z \ --workdir $(CI_CONT_SRCDIR) \ - --ulimit nofile=$(CI_ULIMIT_FILES):$(CI_ULIMIT_FILES) \ $(NULL) -ci-check-docker: - @echo -n "Checking if Docker is available and running..." && \ - docker version 1>/dev/null && echo "yes" +ci-check-cengine: + @echo -n "Checking if $(CI_CENGINE) is available..." && \ + $(CI_CENGINE) version 1>/dev/null && echo "yes" -ci-prepare-tree: ci-check-docker +ci-prepare-tree: ci-check-cengine @test "$(CI_REUSE)" != "1" && rm -rf $(CI_SCRATCHDIR) || : @if ! test -d $(CI_SCRATCHDIR) ; then \ mkdir -p $(CI_SCRATCHDIR); \ - cp /etc/passwd $(CI_SCRATCHDIR); \ - cp /etc/group $(CI_SCRATCHDIR); \ + if test "$(CI_CENGINE)" != "podman"; then \ + cp /etc/passwd $(CI_SCRATCHDIR); \ + cp /etc/group $(CI_SCRATCHDIR); \ + fi; \ echo "Cloning $(CI_GIT_ROOT) to $(CI_HOST_SRCDIR)"; \ git clone $(CI_GIT_ARGS) $(CI_GIT_ROOT) $(CI_HOST_SRCDIR) || exit 1; \ for mod in $(CI_SUBMODULES) ; \ @@ -150,7 +210,7 @@ ci-prepare-tree: ci-check-docker # gl_public_submodule_commit= to disable gnulib's submodule check # which breaks due to way we clone the submodules ci-build@%: ci-prepare-tree - docker run $(CI_DOCKER_ARGS) $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) \ + $(CI_CENGINE) run $(CI_CENGINE_ARGS) $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) \ /bin/bash -c '\ mkdir -p $(CI_CONT_BUILDDIR) || exit 1 ; \ cd $(CI_CONT_BUILDDIR) ; \ @@ -179,11 +239,11 @@ ci-check@%: $(MAKE) -f $(CI_MAKEFILE) ci-build@$* CI_MAKE_ARGS="check" ci-shell@%: ci-prepare-tree - docker run $(CI_DOCKER_ARGS) $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) /bin/bash + $(CI_CENGINE) run $(CI_CENGINE_ARGS) $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) /bin/bash @test "$(CI_CLEAN)" = "1" && rm -rf $(CI_SCRATCHDIR) || : ci-help: - @echo "Build libvirt inside Docker containers used for CI" + @echo "Build libvirt inside containers used for CI" @echo @echo "Available targets:" @echo @@ -215,6 +275,7 @@ ci-help: @echo @echo "Available make variables:" @echo + @echo " CI_CENGINE=engine - container engine to use (podman (default) or docker)" @echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after completion" @echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' content" @echo -- 2.21.0
On Tue, May 07, 2019 at 05:45:31PM +0200, Martin Kletzander wrote:
This way more users can run our CI builds locally.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Makefile.ci | 125 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 93 insertions(+), 32 deletions(-)
diff --git a/Makefile.ci b/Makefile.ci index 12a62167cc67..e2989ada313c 100644 --- a/Makefile.ci +++ b/Makefile.ci @@ -17,7 +17,7 @@ CI_GIT_ROOT = $(shell git rev-parse --show-toplevel) CI_HOST_SRCDIR = $(CI_SCRATCHDIR)/src
# The directory holding the source inside the -# container. ie where we told Docker to expose +# container. ie where we want to expose # the $(CI_HOST_SRCDIR) directory from the host CI_CONT_SRCDIR = /src
@@ -46,14 +46,13 @@ CI_CONFIGURE_ARGS = # cloning them CI_SUBMODULES = $(shell git submodule | awk '{ print $$2 }')
-# Location of the Docker images we're going to pull +# Location of the container images we're going to pull # Can be useful to overridde to use a locally built # image instead CI_IMAGE_PREFIX = quay.io/libvirt/buildenv-
-# Docker defaults to pulling the ':latest' tag but -# if the Docker repo above uses different conventions -# this can override it +# The default tag is ':latest' but if the container +# repo above uses different conventions this can override it CI_IMAGE_TAG = :master
# We delete the virtual root after completion, set @@ -71,24 +70,82 @@ CI_REUSE = 0 CI_UID = $(shell id -u) CI_GID = $(shell id -g)
-# Docker doesn't require the IDs you run as to exist in +# Container engine runtime we are going to use, can be overridden per make +# invocation, if it is not, we try podman and then default to docker. +ifeq ($(CI_CENGINE),) + CI_CENGINE = $(shell podman version >/dev/null && echo podman || echo docker) +endif + +# IDs you run as do not need to exist in # the container's /etc/passwd & /etc/group files, but -# if they do not, then libvirt's 'make check' will fail +# if they do not, then libvirt's 'make check' will fail # many tests. -# -# We do not directly mount /etc/{passwd,group} as Docker -# is liable to mess with SELinux labelling which will -# then prevent the host accessing them. Copying them -# first is safer. -CI_PWDB_MOUNTS = \ - --volume $(CI_SCRATCHDIR)/group:/etc/group:ro,z \ - --volume $(CI_SCRATCHDIR)/passwd:/etc/passwd:ro,z \ - $(NULL) +ifeq ($(CI_CENGINE),podman) + CI_PWDB_MOUNTS = \ + --volume /etc/group:/etc/group:ro,z \ + --volume /etc/passwd:/etc/passwd:ro,z \ + $(NULL) +else + # We do not directly mount /etc/{passwd,group} as Docker + # is liable to mess with SELinux labelling which will + # then prevent the host accessing them. Copying them + # first is safer. + CI_PWDB_MOUNTS = \ + --volume $(CI_SCRATCHDIR)/group:/etc/group:ro,z \ + --volume $(CI_SCRATCHDIR)/passwd:/etc/passwd:ro,z \ + $(NULL) +endif
Does this need to be conditionalized ? Wouldn't podman just work with the existing code. How does podman end up giving access to these files if it isn't changing the SELinux label on them ?
+ +ifeq ($(CI_CENGINE),docker) + # Docker containers can have very large ulimits + # for nofiles - as much as 1048576. This makes + # libvirt very slow at exec'ing programs. + CI_ULIMIT_FILES = 1024 +endif
Again, does this really need to be conditionalized ?
-# Docker containers can have very large ulimits -# for nofiles - as much as 1048576. This makes -# libvirt very slow at exec'ing programs. -CI_ULIMIT_FILES = 1024 +ifeq ($(CI_CENGINE),podman) + # Podman cannot reuse host namespace when running non-root containers. Until + # support for --keep-uid is added we can just create another mapping that will + # do that for us. Beware, that in {uid,git}map=container_id:host_id:range, + # the host_id does actually refer to the uid in the first mapping where 0 + # (root) is mapped to the current user and rest is offset. + + # In order to set up this mapping, we need to keep all the user IDs to prevent + # possible errors as some images might expect UIDs up to 90000 (looking at you + # fedora), so we don't want the overflowuid to be used for them. For mapping + # all the other users properly ther eneeds to be some math done. Don't worry, + # it's just addition and subtraction. + + # 65536 ought to be enough (tm), but for really rare cases the maximums might + # need to be higher, but that only happens when your /etc/sub{u,g}id allow + # users to have more IDs. Unless --keep-uid is supported, let's do this in a + # way that should work for everyone. + CI_MAX_UID = $(shell sed -n "s/^$USER:[^:]\+://p" /etc/subuid) + CI_MAX_GID = $(shell sed -n "s/^$USER:[^:]\+://p" /etc/subgid) + ifeq ($(CI_MAX_UID),) + CI_MAX_UID = 65536 + endif + ifeq ($(CI_MAX_GID),) + CI_MAX_GID = 65536 + endif + CI_UID_OTHER = $(shell echo $$(($(CI_UID)+1))) + CI_GID_OTHER = $(shell echo $$(($(CI_GID)+1))) + CI_UID_OTHER_RANGE = $(shell echo $$(($(CI_MAX_UID)-$(CI_UID)))) + CI_GID_OTHER_RANGE = $(shell echo $$(($(CI_MAX_GID)-$(CI_GID)))) + + CI_PODMAN_ARGS = \ + --uidmap 0:1:$(CI_UID) \ + --uidmap $(CI_UID):0:1 \ + --uidmap $(CI_UID_OTHER):$(CI_UID_OTHER):$(CI_UID_OTHER_RANGE) \ + --gidmap 0:1:$(CI_GID) \ + --gidmap $(CI_GID):0:1 \ + --gidmap $(CI_GID_OTHER):$(CI_GID_OTHER):$(CI_GID_OTHER_RANGE) \ + $(NULL) +else + CI_DOCKER_ARGS = \ + --ulimit nofile=$(CI_ULIMIT_FILES):$(CI_ULIMIT_FILES) \ + $(NULL) +endif
# Args to use when cloning a git repo. # -c stop it complaining about checking out a random hash @@ -100,7 +157,7 @@ CI_GIT_ARGS = \ --local \ $(NULL)
-# Args to use when running the Docker env +# Args to use when running the container # --rm stop inactive containers getting left behind # --user we execute as the same user & group account # as dev so that file ownership matches host @@ -110,27 +167,30 @@ CI_GIT_ARGS = \ # --ulimit lower files limit for performance reasons # --interactive # --tty Ensure we have ability to Ctrl-C the build -CI_DOCKER_ARGS = \ +CI_CENGINE_ARGS = \ --rm \ --user $(CI_UID):$(CI_GID) \ --interactive \ --tty \ + $(CI_PODMAN_ARGS) \ + $(CI_DOCKER_ARGS) \ $(CI_PWDB_MOUNTS) \ --volume $(CI_HOST_SRCDIR):$(CI_CONT_SRCDIR):z \ --workdir $(CI_CONT_SRCDIR) \ - --ulimit nofile=$(CI_ULIMIT_FILES):$(CI_ULIMIT_FILES) \ $(NULL)
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 :|
On Thu, May 09, 2019 at 02:06:18PM +0100, Daniel P. Berrangé wrote:
On Tue, May 07, 2019 at 05:45:31PM +0200, Martin Kletzander wrote:
This way more users can run our CI builds locally.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Makefile.ci | 125 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 93 insertions(+), 32 deletions(-)
diff --git a/Makefile.ci b/Makefile.ci index 12a62167cc67..e2989ada313c 100644 --- a/Makefile.ci +++ b/Makefile.ci @@ -17,7 +17,7 @@ CI_GIT_ROOT = $(shell git rev-parse --show-toplevel) CI_HOST_SRCDIR = $(CI_SCRATCHDIR)/src
# The directory holding the source inside the -# container. ie where we told Docker to expose +# container. ie where we want to expose # the $(CI_HOST_SRCDIR) directory from the host CI_CONT_SRCDIR = /src
@@ -46,14 +46,13 @@ CI_CONFIGURE_ARGS = # cloning them CI_SUBMODULES = $(shell git submodule | awk '{ print $$2 }')
-# Location of the Docker images we're going to pull +# Location of the container images we're going to pull # Can be useful to overridde to use a locally built # image instead CI_IMAGE_PREFIX = quay.io/libvirt/buildenv-
-# Docker defaults to pulling the ':latest' tag but -# if the Docker repo above uses different conventions -# this can override it +# The default tag is ':latest' but if the container +# repo above uses different conventions this can override it CI_IMAGE_TAG = :master
# We delete the virtual root after completion, set @@ -71,24 +70,82 @@ CI_REUSE = 0 CI_UID = $(shell id -u) CI_GID = $(shell id -g)
-# Docker doesn't require the IDs you run as to exist in +# Container engine runtime we are going to use, can be overridden per make +# invocation, if it is not, we try podman and then default to docker. +ifeq ($(CI_CENGINE),) + CI_CENGINE = $(shell podman version >/dev/null && echo podman || echo docker) +endif + +# IDs you run as do not need to exist in # the container's /etc/passwd & /etc/group files, but -# if they do not, then libvirt's 'make check' will fail +# if they do not, then libvirt's 'make check' will fail # many tests. -# -# We do not directly mount /etc/{passwd,group} as Docker -# is liable to mess with SELinux labelling which will -# then prevent the host accessing them. Copying them -# first is safer. -CI_PWDB_MOUNTS = \ - --volume $(CI_SCRATCHDIR)/group:/etc/group:ro,z \ - --volume $(CI_SCRATCHDIR)/passwd:/etc/passwd:ro,z \ - $(NULL) +ifeq ($(CI_CENGINE),podman) + CI_PWDB_MOUNTS = \ + --volume /etc/group:/etc/group:ro,z \ + --volume /etc/passwd:/etc/passwd:ro,z \ + $(NULL) +else + # We do not directly mount /etc/{passwd,group} as Docker + # is liable to mess with SELinux labelling which will + # then prevent the host accessing them. Copying them + # first is safer. + CI_PWDB_MOUNTS = \ + --volume $(CI_SCRATCHDIR)/group:/etc/group:ro,z \ + --volume $(CI_SCRATCHDIR)/passwd:/etc/passwd:ro,z \ + $(NULL) +endif
Does this need to be conditionalized ? Wouldn't podman just work with the existing code. How does podman end up giving
It would, but it seemed pointless to copy the files when it is not needed.
access to these files if it isn't changing the SELinux label on them ?
Oh right, I haven't tried with SELinux. I kind of hope that fuse-overlayfs would solve that, but they are just starting with these things, so yeah, you're right, this needs to be there due to SELinux for both podman and docker.
+ +ifeq ($(CI_CENGINE),docker) + # Docker containers can have very large ulimits + # for nofiles - as much as 1048576. This makes + # libvirt very slow at exec'ing programs. + CI_ULIMIT_FILES = 1024 +endif
Again, does this really need to be conditionalized ?
Not really, it can be left unconditional. I'll send a clean v2.
-# Docker containers can have very large ulimits -# for nofiles - as much as 1048576. This makes -# libvirt very slow at exec'ing programs. -CI_ULIMIT_FILES = 1024 +ifeq ($(CI_CENGINE),podman) + # Podman cannot reuse host namespace when running non-root containers. Until + # support for --keep-uid is added we can just create another mapping that will + # do that for us. Beware, that in {uid,git}map=container_id:host_id:range, + # the host_id does actually refer to the uid in the first mapping where 0 + # (root) is mapped to the current user and rest is offset. + + # In order to set up this mapping, we need to keep all the user IDs to prevent + # possible errors as some images might expect UIDs up to 90000 (looking at you + # fedora), so we don't want the overflowuid to be used for them. For mapping + # all the other users properly ther eneeds to be some math done. Don't worry, + # it's just addition and subtraction. + + # 65536 ought to be enough (tm), but for really rare cases the maximums might + # need to be higher, but that only happens when your /etc/sub{u,g}id allow + # users to have more IDs. Unless --keep-uid is supported, let's do this in a + # way that should work for everyone. + CI_MAX_UID = $(shell sed -n "s/^$USER:[^:]\+://p" /etc/subuid) + CI_MAX_GID = $(shell sed -n "s/^$USER:[^:]\+://p" /etc/subgid) + ifeq ($(CI_MAX_UID),) + CI_MAX_UID = 65536 + endif + ifeq ($(CI_MAX_GID),) + CI_MAX_GID = 65536 + endif + CI_UID_OTHER = $(shell echo $$(($(CI_UID)+1))) + CI_GID_OTHER = $(shell echo $$(($(CI_GID)+1))) + CI_UID_OTHER_RANGE = $(shell echo $$(($(CI_MAX_UID)-$(CI_UID)))) + CI_GID_OTHER_RANGE = $(shell echo $$(($(CI_MAX_GID)-$(CI_GID)))) + + CI_PODMAN_ARGS = \ + --uidmap 0:1:$(CI_UID) \ + --uidmap $(CI_UID):0:1 \ + --uidmap $(CI_UID_OTHER):$(CI_UID_OTHER):$(CI_UID_OTHER_RANGE) \ + --gidmap 0:1:$(CI_GID) \ + --gidmap $(CI_GID):0:1 \ + --gidmap $(CI_GID_OTHER):$(CI_GID_OTHER):$(CI_GID_OTHER_RANGE) \ + $(NULL) +else + CI_DOCKER_ARGS = \ + --ulimit nofile=$(CI_ULIMIT_FILES):$(CI_ULIMIT_FILES) \ + $(NULL) +endif
# Args to use when cloning a git repo. # -c stop it complaining about checking out a random hash @@ -100,7 +157,7 @@ CI_GIT_ARGS = \ --local \ $(NULL)
-# Args to use when running the Docker env +# Args to use when running the container # --rm stop inactive containers getting left behind # --user we execute as the same user & group account # as dev so that file ownership matches host @@ -110,27 +167,30 @@ CI_GIT_ARGS = \ # --ulimit lower files limit for performance reasons # --interactive # --tty Ensure we have ability to Ctrl-C the build -CI_DOCKER_ARGS = \ +CI_CENGINE_ARGS = \ --rm \ --user $(CI_UID):$(CI_GID) \ --interactive \ --tty \ + $(CI_PODMAN_ARGS) \ + $(CI_DOCKER_ARGS) \ $(CI_PWDB_MOUNTS) \ --volume $(CI_HOST_SRCDIR):$(CI_CONT_SRCDIR):z \ --workdir $(CI_CONT_SRCDIR) \ - --ulimit nofile=$(CI_ULIMIT_FILES):$(CI_ULIMIT_FILES) \ $(NULL)
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 :|
On Tue, 2019-05-07 at 17:45 +0200, Martin Kletzander wrote:
@echo @echo "Available make variables:" @echo + @echo " CI_CENGINE=engine - container engine to use (podman (default) or docker)" @echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after completion" @echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' content" @echo
I have not looked at the code in detail, but I wanted to point this out before you respin: I really don't like the name CI_CENGINE :) Since we're dealing with containers exclusively I'm pretty sure we can just go with CI_ENGINE and not cause any confusion for users. The description given above is also not accurate, as far as I can tell: we use Podman instead of Docker only if we detect that the former is available, so what we really do is *prefer* Podman, not default to it. I would document it along the lines of CI_ENGINE=auto - container engine to use (podman, docker) and implement it like CI_ENGINE=auto # ... ifeq ($(CI_ENGINE),auto) CI_ENGINE = $(shell ...) endif # Validate that CI_ENGINE is either podman or docker at this # point, and error out otherwise -- Andrea Bolognani / Red Hat / Virtualization
On Fri, May 10, 2019 at 10:08:05AM +0200, Andrea Bolognani wrote:
On Tue, 2019-05-07 at 17:45 +0200, Martin Kletzander wrote:
@echo @echo "Available make variables:" @echo + @echo " CI_CENGINE=engine - container engine to use (podman (default) or docker)" @echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after completion" @echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' content" @echo
I have not looked at the code in detail, but I wanted to point this out before you respin: I really don't like the name CI_CENGINE :)
Since we're dealing with containers exclusively I'm pretty sure we can just go with CI_ENGINE and not cause any confusion for users.
The description given above is also not accurate, as far as I can tell: we use Podman instead of Docker only if we detect that the former is available, so what we really do is *prefer* Podman, not default to it. I would document it along the lines of
CI_ENGINE=auto - container engine to use (podman, docker)
and implement it like
CI_ENGINE=auto
# ...
ifeq ($(CI_ENGINE),auto) CI_ENGINE = $(shell ...)
This needs an override directive [1], because make, see "Overriding" [2]. But it still looks better, so I'll change v2 with this. [1] https://www.gnu.org/software/make/manual/html_node/Override-Directive.html#O... [2] https://www.gnu.org/software/make/manual/html_node/Overriding.html
endif
# Validate that CI_ENGINE is either podman or docker at this # point, and error out otherwise
I can't really do that, that would have to be in a rule to fail properly. But it's (kind of) done in ci-check-engine. To check that it is one of the strings I would have to parse it and check the first word, otherwise I couldn't do what I can do now, e.g.: make -f Makefile.ci ci-check@debian-9 CI_ENGINE="podman --log-level=debug"
-- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Martin Kletzander