8:19 a.m.
Our Debian containers don't have sudo pre-installed and the only reason we
actually needed it was to run a custom prepare script which as it turns out
does nothing really.
Since v1:
- applied quoting to the variables as asked during the review process
- simplified setting of CI_PODMAN_ARGS
- moved 4/4 to 3/4
Erik Skultety (4):
ci: Specify the shebang sequence for build.sh
ci: Run podman command directly without wrapping it with prepare.sh
ci: Drop the prepare.sh script
ci: Makefile: Expose the CI_USER_LOGIN variable for users to use
ci/Makefile | 62 +++++++++++++++++++++++++--------------------------
ci/build.sh | 2 ++
ci/prepare.sh | 13 -----------
3 files changed, 33 insertions(+), 44 deletions(-)
delete mode 100644 ci/prepare.sh
--
2.29.2
8:19 a.m.
New subject: [libvirt PATCH v2 1/4] ci: Specify the shebang sequence for build.sh
This is necessary for the follow up patch, because the default
entrypoint for a Dockerfile is exec.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
ci/build.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/ci/build.sh b/ci/build.sh
index 740b46a935..0f23df1fa3 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -1,3 +1,5 @@
+#!/bin/sh
+
# This script is used to build libvirt inside the container.
#
# You can customize it to your liking, or alternatively use a
--
2.29.2
8:19 a.m.
New subject: [libvirt PATCH v2 2/4] ci: Run podman command directly without wrapping it with prepare.sh
The prepare.sh script isn't currently used and forces us to make use
of sudo to switch the user inside the container from root to $USER
which created a problem on our Debian Slim-based containers which don't
have the 'sudo' package installed.
This patch removes the sudo invocation and instead runs the CMD
directly with podman.
Summary of the changes:
- move the corresponding env variables which we need to be set in the
environment from the sudo invocation to the podman invocation
- pass --workdir to podman to retain the original behaviour we had with
sudo spawning a login shell.
- MESON_OPTS env variable doesn't need to propagated to the execution
environment anymore (like we had to do with sudo), because it's
defined in the Dockerfile
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
ci/Makefile | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/ci/Makefile b/ci/Makefile
index 7938e14c15..9308738d2d 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -82,7 +82,6 @@ CI_HOME_MOUNTS = \
$(NULL)
CI_SCRIPT_MOUNTS = \
- --volume $(CI_SCRATCHDIR)/prepare:$(CI_USER_HOME)/prepare:z \
--volume $(CI_SCRATCHDIR)/build:$(CI_USER_HOME)/build:z \
$(NULL)
@@ -150,6 +149,8 @@ CI_GIT_ARGS = \
# --user we execute as the same user & group account
# as dev so that file ownership matches host
# instead of root:root
+# --workdir we change to user's home dir in the container
+# before running the workload
# --volume to pass in the cloned git repo & config
# --ulimit lower files limit for performance reasons
# --interactive
@@ -158,6 +159,11 @@ CI_ENGINE_ARGS = \
--rm \
--interactive \
--tty \
+ --user "$(CI_UID)":"$(CI_GID)" \
+ --workdir "$(CI_USER_HOME)" \
+ --env CI_CONT_SRCDIR="$(CI_CONT_SRCDIR)" \
+ --env CI_MESON_ARGS="$(CI_MESON_ARGS)" \
+ --env CI_NINJA_ARGS="$(CI_NINJA_ARGS)" \
$(CI_PODMAN_ARGS) \
$(CI_PWDB_MOUNTS) \
$(CI_HOME_MOUNTS) \
@@ -178,9 +184,8 @@ ci-prepare-tree: ci-check-engine
cp /etc/passwd $(CI_SCRATCHDIR); \
cp /etc/group $(CI_SCRATCHDIR); \
mkdir -p $(CI_SCRATCHDIR)/home; \
- cp "$(CI_PREPARE_SCRIPT)" $(CI_SCRATCHDIR)/prepare; \
cp "$(CI_BUILD_SCRIPT)" $(CI_SCRATCHDIR)/build; \
- chmod +x "$(CI_SCRATCHDIR)/prepare" "$(CI_SCRATCHDIR)/build"; \
+ chmod +x "$(CI_SCRATCHDIR)/build"; \
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 $$(git submodule | awk '{ print $$2 }' | sed -E 's,^../,,g')
; \
@@ -192,18 +197,10 @@ ci-prepare-tree: ci-check-engine
fi
ci-run-command@%: ci-prepare-tree
- $(CI_ENGINE) run $(CI_ENGINE_ARGS) $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) \
- /bin/bash -c ' \
- $(CI_USER_HOME)/prepare || exit 1; \
- sudo \
- --login \
- --user="#$(CI_UID)" \
- --group="#$(CI_GID)" \
- MESON_OPTS="$$MESON_OPTS" \
- CI_CONT_SRCDIR="$(CI_CONT_SRCDIR)" \
- CI_MESON_ARGS="$(CI_MESON_ARGS)" \
- CI_NINJA_ARGS="$(CI_NINJA_ARGS)" \
- $(CI_COMMAND) || exit 1'
+ $(CI_ENGINE) run \
+ $(CI_ENGINE_ARGS) \
+ $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) \
+ $(CI_COMMAND)
@test "$(CI_CLEAN)" = "1" && rm -rf $(CI_SCRATCHDIR) || :
ci-shell@%:
--
2.29.2
8:19 a.m.
New subject: [libvirt PATCH v2 3/4] ci: Drop the prepare.sh script
The purpose of this script was to prepare a customized environment in
the container, but was actually never used and it required the usage of
sudo to switch the environment from root's context to a regular user's
one.
The thing is that once someone needs a custom script they would very
likely to debug something and would also benefit from root privileges
in general, so the usage of 'sudo' in such case was a bit cumbersome.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
ci/prepare.sh | 13 -------------
1 file changed, 13 deletions(-)
delete mode 100644 ci/prepare.sh
diff --git a/ci/prepare.sh b/ci/prepare.sh
deleted file mode 100644
index da6fc9a1b5..0000000000
--- a/ci/prepare.sh
+++ /dev/null
@@ -1,13 +0,0 @@
-# This script is used to prepare the environment that will be used
-# to build libvirt inside the container.
-#
-# You can customize it to your liking, or alternatively use a
-# completely different script by passing
-#
-# CI_PREPARE_SCRIPT=/path/to/your/prepare/script
-#
-# to make.
-#
-# Note that this script will have root privileges inside the
-# container, so it can be used for things like installing additional
-# packages.
--
2.29.2
8:19 a.m.
New subject: [libvirt PATCH v2 4/4] ci: Makefile: Expose the CI_USER_LOGIN variable for users to use
More often than not I find myself debugging in the containers which
means that I need to have root inside, but without manually tweaking
the Makefile each time the execution would simply fail thanks to the
uid/gid mapping we do. What if we expose the CI_USER_LOGIN variable, so
that when needed, the root can be simply passed with this variable and
voila - you have a root shell inside the container with CWD=~root.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
ci/Makefile | 35 +++++++++++++++++++----------------
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/ci/Makefile b/ci/Makefile
index 9308738d2d..05a50c021c 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -45,15 +45,15 @@ CI_CLEAN = 1
# preserved env
CI_REUSE = 0
-# We need the container process to run with current host IDs
-# so that it can access the passed in build directory
-CI_UID = $(shell id -u)
-CI_GID = $(shell id -g)
-
-# We also need the user's login and home directory to prepare the
+# We need the user's login and home directory to prepare the
# environment the way some programs expect it
-CI_USER_LOGIN = $(shell echo "$$USER")
-CI_USER_HOME = $(shell echo "$$HOME")
+CI_USER_LOGIN = $(shell whoami)
+CI_USER_HOME = $(shell eval echo "~$(CI_USER_LOGIN)")
+
+# We also need the container process to run with current host IDs
+# so that it can access the passed in build directory
+CI_UID = $(shell id -u "$(CI_USER_LOGIN)")
+CI_GID = $(shell id -g "$(CI_USER_LOGIN)")
CI_ENGINE = auto
# Container engine we are going to use, can be overridden per make
@@ -124,14 +124,16 @@ ifeq ($(CI_ENGINE),podman)
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)
+ ifneq ($(CI_UID), 0)
+ 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)
+ endif
endif
# Args to use when cloning a git repo.
@@ -239,6 +241,7 @@ ci-help:
@echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after
completion"
@echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)'
content"
@echo " CI_ENGINE=auto - container engine to use (podman, docker)"
+ @echo " CI_USER_LOGIN=$$USER - which user should run in the
container"
@echo " CI_MESON_ARGS= - extra arguments passed to meson"
@echo " CI_NINJA_ARGS= - extra arguments passed to ninja"
@echo
--
2.29.2
9:04 a.m.
New subject: [libvirt PATCH v2 4/4] ci: Makefile: Expose the CI_USER_LOGIN variable for users to use
On Fri, 2021-02-12 at 15:19 +0100, Erik Skultety wrote:
@@ -239,6 +241,7 @@ ci-help:
@echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after
completion"
@echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)'
content"
@echo " CI_ENGINE=auto - container engine to use (podman, docker)"
+ @echo " CI_USER_LOGIN=$$USER - which user should run in the
container"
Alignment is completely broken here. But I just realized that $USER
will get expanded here, so we have basically no way of keeping that
consistent...
Basically, your original version of the help message was better,
please go back to that one :)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
9:16 a.m.
New subject: [libvirt PATCH v2 4/4] ci: Makefile: Expose the CI_USER_LOGIN variable for users to use
On Fri, Feb 12, 2021 at 04:04:04PM +0100, Andrea Bolognani wrote:
On Fri, 2021-02-12 at 15:19 +0100, Erik Skultety wrote:
> @@ -239,6 +241,7 @@ ci-help:
> @echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)'
after completion"
> @echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)'
content"
> @echo " CI_ENGINE=auto - container engine to use (podman,
docker)"
> + @echo " CI_USER_LOGIN=$$USER - which user should run in the
container"
Alignment is completely broken here. But I just realized that $USER
will get expanded here, so we have basically no way of keeping that
consistent...
I didn't want to point it out earlier, since to me it's not a deal breaker :).
Basically, your original version of the help message was better,
please go back to that one :)
Sure, will do, thanks for the review.
Erik
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
1379
days inactive
1379
days old
6 comments
2 participants
participants (2)
-
Andrea Bolognani -
Erik Skultety