From bab95de9a24a169397aaeef32316f581428ffda8 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 6 Apr 2023 14:41:10 +0200 Subject: [PATCH] rootless: make sure we only use a single pause process Currently --tmpdir changes the location of the pause.pid file. this causes issues because the c code in pkg/rootless does not know about that. I tried to fix this[1] by fixing the c code to not use the shortcut. While this fix worked it will result in many pause processes leaking in the integrration tests. Commit ab88632 added this behavior but following the disccusion it was never the intention that we end up having more than one pause process. The issues that was trying to fix was caused by somthing else AFAICT, the main problem seems to be that the pause.pid file parent directory may not be created when we try to create the pid file so it failed with ENOENT. This patch fixes it by creating this directory always and revert the change to no longer depend on the tmpdir value. With this commit we now always use XDG_RUNTIME_DIR/libpod/tmp/pause.pid for all podman processes. This allows the c shortcut to work reliably and should therefore improve perfomance over my other approach. A system test is added to ensure we see the right behavior and that podman system migrate actually stops the pause process. Thanks to Ed Santiago for the improved test to make it work for both `catatonit` and `podman pause`. This should fix the issues with namespace missmatches that we can see in CI as flakes. [1] https://github.com/containers/podman/pull/18057 Fixes #18057 Signed-off-by: Paul Holzinger --- libpod/runtime.go | 9 +++- libpod/runtime_migrate.go | 2 +- pkg/domain/infra/abi/system.go | 6 +-- pkg/util/utils_supported.go | 16 ++---- pkg/util/utils_windows.go | 6 --- test/system/550-pause-process.bats | 79 ++++++++++++++++++++++++++++++ 6 files changed, 93 insertions(+), 25 deletions(-) create mode 100644 test/system/550-pause-process.bats diff --git a/libpod/runtime.go b/libpod/runtime.go index 8899b90506..83c5a1eba1 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -579,10 +579,17 @@ func makeRuntime(runtime *Runtime) (retErr error) { } unLockFunc() unLockFunc = nil - pausePid, err := util.GetRootlessPauseProcessPidPathGivenDir(runtime.config.Engine.TmpDir) + pausePid, err := util.GetRootlessPauseProcessPidPath() if err != nil { return fmt.Errorf("could not get pause process pid file path: %w", err) } + + // create the path in case it does not already exists + // https://github.com/containers/podman/issues/8539 + if err := os.MkdirAll(filepath.Dir(pausePid), 0o700); err != nil { + return fmt.Errorf("could not create pause process pid file directory: %w", err) + } + became, ret, err := rootless.BecomeRootInUserNS(pausePid) if err != nil { return err diff --git a/libpod/runtime_migrate.go b/libpod/runtime_migrate.go index f279eaf9d1..4764271845 100644 --- a/libpod/runtime_migrate.go +++ b/libpod/runtime_migrate.go @@ -18,7 +18,7 @@ import ( func (r *Runtime) stopPauseProcess() error { if rootless.IsRootless() { - pausePidPath, err := util.GetRootlessPauseProcessPidPathGivenDir(r.config.Engine.TmpDir) + pausePidPath, err := util.GetRootlessPauseProcessPidPath() if err != nil { return fmt.Errorf("could not get pause process pid file path: %w", err) } diff --git a/pkg/domain/infra/abi/system.go b/pkg/domain/infra/abi/system.go index 45731f4e07..4a58b87e80 100644 --- a/pkg/domain/infra/abi/system.go +++ b/pkg/domain/infra/abi/system.go @@ -107,11 +107,7 @@ func (ic *ContainerEngine) SetupRootless(_ context.Context, noMoveProcess bool) return nil } - tmpDir, err := ic.Libpod.TmpDir() - if err != nil { - return err - } - pausePidPath, err := util.GetRootlessPauseProcessPidPathGivenDir(tmpDir) + pausePidPath, err := util.GetRootlessPauseProcessPidPath() if err != nil { return fmt.Errorf("could not get pause process pid file path: %w", err) } diff --git a/pkg/util/utils_supported.go b/pkg/util/utils_supported.go index 90a2ecf862..b269b97011 100644 --- a/pkg/util/utils_supported.go +++ b/pkg/util/utils_supported.go @@ -107,21 +107,13 @@ func GetRootlessConfigHomeDir() (string, error) { // GetRootlessPauseProcessPidPath returns the path to the file that holds the pid for // the pause process. -// DEPRECATED - switch to GetRootlessPauseProcessPidPathGivenDir func GetRootlessPauseProcessPidPath() (string, error) { runtimeDir, err := GetRuntimeDir() if err != nil { return "", err } - return filepath.Join(runtimeDir, "libpod", "pause.pid"), nil -} - -// GetRootlessPauseProcessPidPathGivenDir returns the path to the file that -// holds the PID of the pause process, given the location of Libpod's temporary -// files. -func GetRootlessPauseProcessPidPathGivenDir(libpodTmpDir string) (string, error) { - if libpodTmpDir == "" { - return "", errors.New("must provide non-empty temporary directory") - } - return filepath.Join(libpodTmpDir, "pause.pid"), nil + // Note this path must be kept in sync with pkg/rootless/rootless_linux.go + // We only want a single pause process per user, so we do not want to use + // the tmpdir which can be changed via --tmpdir. + return filepath.Join(runtimeDir, "libpod", "tmp", "pause.pid"), nil } diff --git a/pkg/util/utils_windows.go b/pkg/util/utils_windows.go index 703e5472a6..1e48eb5721 100644 --- a/pkg/util/utils_windows.go +++ b/pkg/util/utils_windows.go @@ -30,12 +30,6 @@ func GetRootlessPauseProcessPidPath() (string, error) { return "", fmt.Errorf("GetRootlessPauseProcessPidPath: %w", errNotImplemented) } -// GetRootlessPauseProcessPidPath returns the path to the file that holds the pid for -// the pause process -func GetRootlessPauseProcessPidPathGivenDir(unused string) (string, error) { - return "", fmt.Errorf("GetRootlessPauseProcessPidPath: %w", errNotImplemented) -} - // GetRuntimeDir returns the runtime directory func GetRuntimeDir() (string, error) { data, err := homedir.GetDataHome() diff --git a/test/system/550-pause-process.bats b/test/system/550-pause-process.bats new file mode 100644 index 0000000000..733ad81a50 --- /dev/null +++ b/test/system/550-pause-process.bats @@ -0,0 +1,79 @@ +#!/usr/bin/env bats -*- bats -*- +# +# test to make sure we use the correct podman pause process +# + +load helpers + +function _check_pause_process() { + pause_pid= + if [[ -z "$pause_pid_file" ]]; then + return + fi + + test -e $pause_pid_file || die "Pause pid file $pause_pid_file missing" + + # do not mark this variable as local; our parent expects it + pause_pid=$(<$pause_pid_file) + test -d /proc/$pause_pid || die "Pause process $pause_pid (from $pause_pid_file) is not running" + + assert "$(