11 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 d=
oes
nothing really.
Erik Skultety (4):
ci: Specify the shebang sequence for build.sh
ci: Run podman command directly without wrapping it with prepare.sh
ci: Expose CI_USER_LOGIN as a Makefile variable for users to use
ci: Drop the prepare.sh script
ci/Makefile | 43 ++++++++++++++++++++++++-------------------
ci/build.sh | 2 ++
ci/prepare.sh | 13 -------------
3 files changed, 26 insertions(+), 32 deletions(-)
delete mode 100644 ci/prepare.sh
--=20
2.29.2
11 a.m.
New subject: [libvirt PATCH 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>
---
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
6:05 a.m.
New subject: [libvirt PATCH 1/4] ci: Specify the shebang sequence for build.sh
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
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>
---
ci/build.sh | 2 ++
1 file changed, 2 insertions(+)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
11 a.m.
New subject: [libvirt PATCH 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_ARGS 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>
---
ci/Makefile | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/ci/Makefile b/ci/Makefile
index 7938e14c15..1a376a7f0c 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
6:10 a.m.
New subject: [libvirt PATCH 2/4] ci: Run podman command directly without wrapping it with prepare.sh
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
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_ARGS env variable doesn't need to propagated to the execution
s/MESON_ARGS/MESON_OPTS/
@@ -158,6 +159,11 @@ CI_ENGINE_ARGS = \
--rm \
--interactive \
--tty \
+ --user $(CI_UID):$(CI_GID) \
+ --workdir $(CI_USER_HOME) \
Please add quotes around the arguments here.
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
11 a.m.
New subject: [libvirt PATCH 3/4] ci: Expose CI_USER_LOGIN as a Makefile variable for users to use
More often than not I find myself debugging 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 | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/ci/Makefile b/ci/Makefile
index 1a376a7f0c..84f2f77526 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -47,13 +47,13 @@ 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)
+CI_UID = $(shell id -u $(CI_USER_LOGIN))
+CI_GID = $(shell id -g $(CI_USER_LOGIN))
# We also 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)")
CI_ENGINE = auto
# Container engine we are going to use, can be overridden per make
@@ -132,6 +132,13 @@ ifeq ($(CI_ENGINE),podman)
--gidmap $(CI_GID):0:1 \
--gidmap $(CI_GID_OTHER):$(CI_GID_OTHER):$(CI_GID_OTHER_RANGE) \
$(NULL)
+
+ # In case we want to debug in the container, having root is actually
+ # preferable, so reset the CI_PODMAN_ARGS and don't actually perform
+ # any uid/gid mapping
+ ifeq ($(CI_UID), 0)
+ CI_PODMAN_ARGS=
+ endif
endif
# Args to use when cloning a git repo.
@@ -238,6 +245,7 @@ ci-help:
@echo
@echo " CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after
completion"
@echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)'
content"
+ @echo " CI_USER_LOGIN= - which user should run in the container (default is
$$USER)"
@echo " CI_ENGINE=auto - container engine to use (podman, docker)"
@echo " CI_MESON_ARGS= - extra arguments passed to meson"
@echo " CI_NINJA_ARGS= - extra arguments passed to ninja"
--
2.29.2
6:27 a.m.
New subject: [libvirt PATCH 3/4] ci: Expose CI_USER_LOGIN as a Makefile variable for users to use
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
# 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)
+CI_UID = $(shell id -u $(CI_USER_LOGIN))
+CI_GID = $(shell id -g $(CI_USER_LOGIN))
Please quote uses of $(CI_USER_LOGIN) in the shell.
Also, since you're using the variable here, it would be IMHO cleaner
if you moved the block that contains its definition before this one.
# We also 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)")
This use of eval makes me a bit uncomfortable. Can we do
CI_USER_HOME = $(shell getent passwd "$(CI_USER_LOGIN)" | cut -d: -f6)
instead?
+ # In case we want to debug in the container, having root is
actually
+ # preferable, so reset the CI_PODMAN_ARGS and don't actually perform
+ # any uid/gid mapping
+ ifeq ($(CI_UID), 0)
+ CI_PODMAN_ARGS=
+ endif
Setting $(CI_PODMAN_ARGS) only to reset it moments later seems worse
than just doing
ifneq ($(CI_UID, 0)
CI_PODMAN_ARGS = \
...
$(NULL)
endif
The comment is also somewhat misleading: whether or not you're going
to debug in the container, whatever that means, is not really
relevant - the point is simply that performing these mappings is only
necessary when the container process is running as non-root.
@echo " CI_CLEAN=0 - do not delete
'$(CI_SCRATCHDIR)' after completion"
@echo " CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)'
content"
+ @echo " CI_USER_LOGIN= - which user should run in the container (default
is $$USER)"
@echo " CI_ENGINE=auto - container engine to use (podman, docker)"
@echo " CI_MESON_ARGS= - extra arguments passed to meson"
@echo " CI_NINJA_ARGS= - extra arguments passed to ninja"
We document the default, so this should be
CI_USER_LOGIN=$$USER - which user should run in the container
And please document this after CI_ENGINE.
--
Andrea Bolognani / Red Hat / Virtualization
6:41 a.m.
New subject: [libvirt PATCH 3/4] ci: Expose CI_USER_LOGIN as a Makefile variable for users to use
On Fri, Feb 12, 2021 at 01:27:42PM +0100, Andrea Bolognani wrote:
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
> # 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)
> +CI_UID = $(shell id -u $(CI_USER_LOGIN))
> +CI_GID = $(shell id -g $(CI_USER_LOGIN))
Please quote uses of $(CI_USER_LOGIN) in the shell.
Also, since you're using the variable here, it would be IMHO cleaner
if you moved the block that contains its definition before this one.
Sure I can do that, I just didn't feel like moving around code to achieve the
same thing in a declarative language.
> # We also 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)")
This use of eval makes me a bit uncomfortable. Can we do
Can you elaborate what the problem is? The argument is even quoted so I
sincerely don't see a problem and find it much clearer than what you suggest.
Erik
8:04 a.m.
New subject: [libvirt PATCH 3/4] ci: Expose CI_USER_LOGIN as a Makefile variable for users to use
On Fri, 2021-02-12 at 13:41 +0100, Erik Skultety wrote:
On Fri, Feb 12, 2021 at 01:27:42PM +0100, Andrea Bolognani wrote:
> On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
> > # We also 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)")
>
> This use of eval makes me a bit uncomfortable. Can we do
Can you elaborate what the problem is? The argument is even quoted so I
sincerely don't see a problem and find it much clearer than what you suggest.
It's just a code smell. In general, I prefer straightforward
constructs to "fancy" ones, especially in languages where quoting and
the like matter so much.
But I agree with you that it's safe in this specific scenario, so if
you'd rather keep it this way I won't NACK the patch because of that.
--
Andrea Bolognani / Red Hat / Virtualization
11 a.m.
New subject: [libvirt PATCH 4/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>
---
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
6:29 a.m.
New subject: [libvirt PATCH 4/4] ci: Drop the prepare.sh script
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
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>
---
ci/prepare.sh | 13 -------------
1 file changed, 13 deletions(-)
delete mode 100644 ci/prepare.sh
This makes IMHO more sense either as 3/4 or even squashed directly
into 2/4.
Either way,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
1379
days inactive
1381
days old
10 comments
2 participants
participants (2)
-
Andrea Bolognani -
Erik Skultety