From f9655d92d660bad46a992831605960767fedc44a Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 28 Jul 2020 15:59:58 -0400 Subject: [PATCH 1/4] When given OCI runtime by path, use path as name Say I start a container with the flag `--runtime /usr/local/sbin/crun`. I then stop the container, and restart it without the flag. We previously stored the runtime in use by a container only by basename when given a path, so the container only knows that it's using the `crun` OCI runtime - and on being restarted without the flag, it will use the system crun, not my special crun build. Using the full path as the name in these cases ensures we will still use the correct runtime, even on subsequent runs of Podman. Signed-off-by: Matthew Heon --- libpod/runtime.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/libpod/runtime.go b/libpod/runtime.go index 7da8b181f4..6d5ea9c34e 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -383,14 +383,12 @@ func makeRuntime(ctx context.Context, runtime *Runtime) (retErr error) { // If the string starts with / it's a path to a runtime // executable. if strings.HasPrefix(runtime.config.Engine.OCIRuntime, "/") { - name := filepath.Base(runtime.config.Engine.OCIRuntime) - - ociRuntime, err := newConmonOCIRuntime(name, []string{runtime.config.Engine.OCIRuntime}, runtime.conmonPath, runtime.runtimeFlags, runtime.config) + ociRuntime, err := newConmonOCIRuntime(runtime.config.Engine.OCIRuntime, []string{runtime.config.Engine.OCIRuntime}, runtime.conmonPath, runtime.runtimeFlags, runtime.config) if err != nil { return err } - runtime.ociRuntimes[name] = ociRuntime + runtime.ociRuntimes[runtime.config.Engine.OCIRuntime] = ociRuntime runtime.defaultOCIRuntime = ociRuntime } else { ociRuntime, ok := runtime.ociRuntimes[runtime.config.Engine.OCIRuntime] From 338d521782727daca90efc2601b16502626aa53c Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 28 Jul 2020 16:22:48 -0400 Subject: [PATCH 2/4] Re-create OCI runtimes by path when it is missing When an OCI runtime is given by full path, we need to ensure we use the same runtime on subsequent use. Unfortunately, users are often not considerate enough to use the same `--runtime` flag every time they invoke runtime - and if the runtime was not in containers.conf, that means we don't have it stored inn the libpod Runtime. Fortunately, since we have the full path, we can initialize the OCI runtime for use at the point where we pull the container from the database. Signed-off-by: Matthew Heon --- libpod/boltdb_state_internal.go | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/libpod/boltdb_state_internal.go b/libpod/boltdb_state_internal.go index 9be753d267..e195ca3149 100644 --- a/libpod/boltdb_state_internal.go +++ b/libpod/boltdb_state_internal.go @@ -2,7 +2,7 @@ package libpod import ( "bytes" - "path/filepath" + "os" "runtime" "strings" @@ -400,14 +400,30 @@ func (s *BoltState) getContainerFromDB(id []byte, ctr *Container, ctrsBkt *bolt. // Handle legacy containers which might use a literal path for // their OCI runtime name. runtimeName := ctr.config.OCIRuntime - if strings.HasPrefix(runtimeName, "/") { - runtimeName = filepath.Base(runtimeName) - } - ociRuntime, ok := s.runtime.ociRuntimes[runtimeName] if !ok { - // Use a MissingRuntime implementation - ociRuntime = getMissingRuntime(runtimeName, s.runtime) + runtimeSet := false + + // If the path starts with a / and exists, make a new + // OCI runtime for it using the full path. + if strings.HasPrefix(runtimeName, "/") { + if stat, err := os.Stat(runtimeName); err == nil && !stat.IsDir() { + newOCIRuntime, err := newConmonOCIRuntime(runtimeName, []string{runtimeName}, s.runtime.conmonPath, s.runtime.runtimeFlags, s.runtime.config) + if err == nil { + // The runtime lock should + // protect against concurrent + // modification of the map. + ociRuntime = newOCIRuntime + s.runtime.ociRuntimes[runtimeName] = ociRuntime + runtimeSet = true + } + } + } + + if !runtimeSet { + // Use a MissingRuntime implementation + ociRuntime = getMissingRuntime(runtimeName, s.runtime) + } } ctr.ociRuntime = ociRuntime } From 3858fc1d019d13fe3378c129d133fd22789ac0dc Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 29 Jul 2020 14:39:02 -0400 Subject: [PATCH 3/4] Use runtime names instead of paths in E2E tests My patches to fix `--runtime /usr/bin/crun` being allowed to use a different version of the crun runtime revealed a problem: we were actually relying on that exact behavior in our E2E tests. We specified the runtime path as `/usr/bin/runc` for the Ubuntu tests, but that didn't exist, so Podman was actively looking for a different, usable runc binary and using that, instead of the path we explicitly hardcoded. Fixing the bug broke this, and thus broke the tests. Instead of hard-coding OCI runtime paths, swap to just using the runtime name, `runc` or `crun`, and letting Podman figure out where the runtime lives - it's quite good at that. This should un-break the tests and make them more durable. Signed-off-by: Matthew Heon --- contrib/cirrus/setup_environment.sh | 7 +++++-- test/endpoint/setup.go | 9 +-------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/contrib/cirrus/setup_environment.sh b/contrib/cirrus/setup_environment.sh index 1b992711f6..df52804398 100755 --- a/contrib/cirrus/setup_environment.sh +++ b/contrib/cirrus/setup_environment.sh @@ -52,7 +52,7 @@ case "$CG_FS_TYPE" in if [[ "$OS_RELEASE_ID" == "ubuntu" ]]; then echo "export OCI_RUNTIME=/usr/lib/cri-o-runc/sbin/runc" >> /etc/environment else - echo "export OCI_RUNTIME=/usr/bin/runc" >> /etc/environment + echo "export OCI_RUNTIME=runc" >> /etc/environment fi fi ;; @@ -61,7 +61,7 @@ case "$CG_FS_TYPE" in # This is necessary since we've built/installed from source, # which uses runc as the default. warn "Forcing testing with crun instead of runc" - echo "export OCI_RUNTIME=/usr/bin/crun" >> /etc/environment + echo "export OCI_RUNTIME=crun" >> /etc/environment fi ;; *) die_unknown CG_FS_TYPE @@ -127,6 +127,9 @@ case "$PRIV_NAME" in echo "$_suns" >> /etc/environment source /etc/environment fi + +# Reload to incorporate any changes from above +source "$SCRIPT_BASE/lib.sh" ;; rootless) _ru="export ROOTLESS_USER='${ROOTLESS_USER:-some${RANDOM}dude}'" diff --git a/test/endpoint/setup.go b/test/endpoint/setup.go index 56cab06b02..6bbc8d2bce 100644 --- a/test/endpoint/setup.go +++ b/test/endpoint/setup.go @@ -51,14 +51,7 @@ func Setup(tempDir string) *EndpointTestIntegration { ociRuntime := os.Getenv("OCI_RUNTIME") if ociRuntime == "" { - var err error - ociRuntime, err = exec.LookPath("runc") - // If we cannot find the runc binary, setting to something static as we have no way - // to return an error. The tests will fail and point out that the runc binary could - // not be found nicely. - if err != nil { - ociRuntime = "/usr/bin/runc" - } + ociRuntime = "runc" } os.Setenv("DISABLE_HC_SYSTEMD", "true") CNIConfigDir := "/etc/cni/net.d" From 1b4933376f4e6738ff3a0c42a2e27c6d21c07e7c Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 29 Jul 2020 16:42:40 -0400 Subject: [PATCH 4/4] Add a system test to verify --runtime is preserved Signed-off-by: Matthew Heon --- contrib/cirrus/setup_environment.sh | 3 --- test/e2e/common_test.go | 9 +-------- test/system/030-run.bats | 13 +++++++++++++ test/system/helpers.bash | 10 ++++++++++ 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/contrib/cirrus/setup_environment.sh b/contrib/cirrus/setup_environment.sh index df52804398..3135a5e65a 100755 --- a/contrib/cirrus/setup_environment.sh +++ b/contrib/cirrus/setup_environment.sh @@ -127,9 +127,6 @@ case "$PRIV_NAME" in echo "$_suns" >> /etc/environment source /etc/environment fi - -# Reload to incorporate any changes from above -source "$SCRIPT_BASE/lib.sh" ;; rootless) _ru="export ROOTLESS_USER='${ROOTLESS_USER:-some${RANDOM}dude}'" diff --git a/test/e2e/common_test.go b/test/e2e/common_test.go index e36c866900..226b716278 100644 --- a/test/e2e/common_test.go +++ b/test/e2e/common_test.go @@ -235,14 +235,7 @@ func PodmanTestCreateUtil(tempDir string, remote bool) *PodmanTestIntegration { ociRuntime := os.Getenv("OCI_RUNTIME") if ociRuntime == "" { - var err error - ociRuntime, err = exec.LookPath("crun") - // If we cannot find the crun binary, setting to something static as we have no way - // to return an error. The tests will fail and point out that the runc binary could - // not be found nicely. - if err != nil { - ociRuntime = "/usr/bin/runc" - } + ociRuntime = "crun" } os.Setenv("DISABLE_HC_SYSTEMD", "true") CNIConfigDir := "/etc/cni/net.d" diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 28dc7c7a7f..9f40377306 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -460,4 +460,17 @@ json-file | f is "$output" "$expect" "podman run with --tz=local, matches host" } +# run with --runtime should preserve the named runtime +@test "podman run : full path to --runtime is preserved" { + skip_if_cgroupsv1 + skip_if_remote + run_podman run -d --runtime '/usr/bin/crun' $IMAGE sleep 60 + cid="$output" + + run_podman inspect --format '{{.OCIRuntime}}' $cid + is "$output" "/usr/bin/crun" + + run_podman kill $cid +} + # vim: filetype=sh diff --git a/test/system/helpers.bash b/test/system/helpers.bash index 73cf1e5b21..2cced10c26 100644 --- a/test/system/helpers.bash +++ b/test/system/helpers.bash @@ -253,6 +253,7 @@ function is_cgroupsv1() { ! is_cgroupsv2 } +# True if cgroups v2 are enabled function is_cgroupsv2() { cgroup_type=$(stat -f -c %T /sys/fs/cgroup) test "$cgroup_type" = "cgroup2fs" @@ -305,6 +306,15 @@ function skip_if_no_selinux() { fi } +####################### +# skip_if_cgroupsv1 # ...with an optional message +####################### +function skip_if_cgroupsv1() { + if ! is_cgroupsv2; then + skip "${1:-test requires cgroupsv2}" + fi +} + ######### # die # Abort with helpful message #########