From 6293ec2e2dea80d0806b7ea07866236765d576e5 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 21 Sep 2023 11:28:22 +0200 Subject: [PATCH] fix handling of static/volume dir The processing and setting of the static and volume directories was scattered across the code base (including c/common) leading to subtle errors that surfaced in #19938. There were multiple issues that I try to summarize below: - c/common loaded the graphroot from c/storage to set the defaults for static and volume dir. That ignored Podman's --root flag and surfaced in #19938 and other bugs. c/common does not set the defaults anymore which gives Podman the ability to detect when the user/admin configured a custom directory (not empty value). - When parsing the CLI, Podman (ab)uses containers.conf structures to set the defaults but also to override them in case the user specified a flag. The --root flag overrode the static dir which is wrong and broke a couple of use cases. Now there is a dedicated field for in the "PodmanConfig" which also includes a containers.conf struct. - The defaults for static and volume dir and now being set correctly and adhere to --root. - The CONTAINERS_CONF_OVERRIDE env variable has not been passed to the cleanup process. I believe that _all_ env variables should be passed to conmon to avoid such subtle bugs. Overall I find that the code and logic is scattered and hard to understand and follow. I refrained from larger refactorings as I really just want to get #19938 fixed and then go back to other priorities. https://github.com/containers/common/pull/1659 broke three pkg/machine tests. Those have been commented out until getting fixed. Fixes: #19938 Signed-off-by: Valentin Rothberg --- cmd/podman/root.go | 2 +- go.mod | 2 +- go.sum | 4 ++-- libpod/oci_conmon_common.go | 3 +++ libpod/options.go | 16 +------------ libpod/runtime.go | 12 +++++++++- libpod/sqlite_state.go | 2 ++ pkg/domain/entities/engine.go | 1 + pkg/domain/infra/runtime_libpod.go | 6 ++++- test/system/030-run.bats | 23 +++++++++++++++++++ .../containers/common/pkg/config/config.go | 18 --------------- .../containers/common/pkg/config/default.go | 2 -- vendor/modules.txt | 2 +- 13 files changed, 51 insertions(+), 42 deletions(-) diff --git a/cmd/podman/root.go b/cmd/podman/root.go index 698267c5e7..8f132c053f 100644 --- a/cmd/podman/root.go +++ b/cmd/podman/root.go @@ -540,7 +540,7 @@ func rootFlags(cmd *cobra.Command, podmanConfig *entities.PodmanConfig) { _ = pFlags.MarkHidden(networkBackendFlagName) rootFlagName := "root" - pFlags.StringVar(&podmanConfig.ContainersConf.Engine.StaticDir, rootFlagName, podmanConfig.ContainersConfDefaultsRO.Engine.StaticDir, "Path to the root directory in which data, including images, is stored") + pFlags.StringVar(&podmanConfig.GraphRoot, rootFlagName, "", "Path to the graph root directory where images, containers, etc. are stored") _ = cmd.RegisterFlagCompletionFunc(rootFlagName, completion.AutocompleteDefault) pFlags.StringVar(&podmanConfig.RegistriesConf, "registries-conf", "", "Path to a registries.conf to use for image processing") diff --git a/go.mod b/go.mod index 2dc4baac56..20c429b6e8 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/containernetworking/cni v1.1.2 github.com/containernetworking/plugins v1.3.0 github.com/containers/buildah v1.32.0 - github.com/containers/common v0.56.1-0.20230920191016-f4e726d4b162 + github.com/containers/common v0.56.1-0.20230922104122-56ed984ea383 github.com/containers/conmon v2.0.20+incompatible github.com/containers/gvisor-tap-vsock v0.7.1-0.20230922151156-97028a6a6d6a github.com/containers/image/v5 v5.28.0 diff --git a/go.sum b/go.sum index 18e7af6e0a..a51e2ce64c 100644 --- a/go.sum +++ b/go.sum @@ -249,8 +249,8 @@ github.com/containernetworking/plugins v1.3.0 h1:QVNXMT6XloyMUoO2wUOqWTC1hWFV62Q github.com/containernetworking/plugins v1.3.0/go.mod h1:Pc2wcedTQQCVuROOOaLBPPxrEXqqXBFt3cZ+/yVg6l0= github.com/containers/buildah v1.32.0 h1:uz5Rcf7lGeStj7iPTBgO4UdhQYZqMMzyt9suDf16k1k= github.com/containers/buildah v1.32.0/go.mod h1:sN3rA3DbnqekNz3bNdkqWduuirYDuMs54LUCOZOomBE= -github.com/containers/common v0.56.1-0.20230920191016-f4e726d4b162 h1:94djwIUmnWY9tzQmbMsZ58Kpjg2vx1e0R1HfpTYDkMM= -github.com/containers/common v0.56.1-0.20230920191016-f4e726d4b162/go.mod h1:ABFEglmyt48WWWQv80kGhitfbVfR1Br35wk3gBQdrIk= +github.com/containers/common v0.56.1-0.20230922104122-56ed984ea383 h1:+SPOIY+DIO5nExB66n9aVWZi/yzcLpks6Ys4IwVxOLY= +github.com/containers/common v0.56.1-0.20230922104122-56ed984ea383/go.mod h1:ABFEglmyt48WWWQv80kGhitfbVfR1Br35wk3gBQdrIk= github.com/containers/conmon v2.0.20+incompatible h1:YbCVSFSCqFjjVwHTPINGdMX1F6JXHGTUje2ZYobNrkg= github.com/containers/conmon v2.0.20+incompatible/go.mod h1:hgwZ2mtuDrppv78a/cOBNiCm6O0UMWGx1mu7P00nu5I= github.com/containers/gvisor-tap-vsock v0.7.1-0.20230922151156-97028a6a6d6a h1:AytlbDLdlu6fZxulV3sHrXYQpQpkipNCZA6LGwcL37M= diff --git a/libpod/oci_conmon_common.go b/libpod/oci_conmon_common.go index 3e077b6822..8cca1622f2 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -1328,6 +1328,9 @@ func (r *ConmonOCIRuntime) configureConmonEnv(runtimeDir string) []string { if conf, ok := os.LookupEnv("CONTAINERS_CONF"); ok { env = append(env, fmt.Sprintf("CONTAINERS_CONF=%s", conf)) } + if conf, ok := os.LookupEnv("CONTAINERS_CONF_OVERRIDE"); ok { + env = append(env, fmt.Sprintf("CONTAINERS_CONF_OVERRIDE=%s", conf)) + } if conf, ok := os.LookupEnv("CONTAINERS_HELPER_BINARY_DIR"); ok { env = append(env, fmt.Sprintf("CONTAINERS_HELPER_BINARY_DIR=%s", conf)) } diff --git a/libpod/options.go b/libpod/options.go index 98ff4a34ea..cd5af912a0 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -5,7 +5,6 @@ import ( "fmt" "net" "os" - "path/filepath" "strings" "syscall" "time" @@ -52,17 +51,6 @@ func WithStorageConfig(config storage.StoreOptions) RuntimeOption { if config.GraphRoot != "" { rt.storageConfig.GraphRoot = config.GraphRoot rt.storageSet.GraphRootSet = true - - // Also set libpod static dir, so we are a subdirectory - // of the c/storage store by default - rt.config.Engine.StaticDir = filepath.Join(config.GraphRoot, "libpod") - rt.storageSet.StaticDirSet = true - - // Also set libpod volume path, so we are a subdirectory - // of the c/storage store by default - rt.config.Engine.VolumePath = filepath.Join(config.GraphRoot, "volumes") - rt.storageSet.VolumePathSet = true - setField = true } @@ -270,7 +258,6 @@ func WithStaticDir(dir string) RuntimeOption { } rt.config.Engine.StaticDir = dir - rt.config.Engine.StaticDirSet = true return nil } @@ -372,7 +359,6 @@ func WithTmpDir(dir string) RuntimeOption { return define.ErrRuntimeFinalized } rt.config.Engine.TmpDir = dir - rt.config.Engine.TmpDirSet = true return nil } @@ -457,7 +443,7 @@ func WithVolumePath(volPath string) RuntimeOption { } rt.config.Engine.VolumePath = volPath - rt.config.Engine.VolumePathSet = true + rt.storageSet.VolumePathSet = true return nil } diff --git a/libpod/runtime.go b/libpod/runtime.go index c67b70c6ba..31255ad2e4 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -311,11 +311,21 @@ func makeRuntime(runtime *Runtime) (retErr error) { return fmt.Errorf("cannot perform system reset while renumbering locks: %w", define.ErrInvalidArg) } + if runtime.config.Engine.StaticDir == "" { + runtime.config.Engine.StaticDir = filepath.Join(runtime.storageConfig.GraphRoot, "libpod") + runtime.storageSet.StaticDirSet = true + } + + if runtime.config.Engine.VolumePath == "" { + runtime.config.Engine.VolumePath = filepath.Join(runtime.storageConfig.GraphRoot, "volumes") + runtime.storageSet.VolumePathSet = true + } + // Make the static files directory if it does not exist if err := os.MkdirAll(runtime.config.Engine.StaticDir, 0700); err != nil { // The directory is allowed to exist if !errors.Is(err, os.ErrExist) { - return fmt.Errorf("creating runtime static files directory: %w", err) + return fmt.Errorf("creating runtime static files directory %q: %w", runtime.config.Engine.StaticDir, err) } } diff --git a/libpod/sqlite_state.go b/libpod/sqlite_state.go index de30988adf..344137b23b 100644 --- a/libpod/sqlite_state.go +++ b/libpod/sqlite_state.go @@ -54,6 +54,8 @@ func NewSqliteState(runtime *Runtime) (_ State, defErr error) { basePath := runtime.storageConfig.GraphRoot if runtime.storageConfig.TransientStore { basePath = runtime.storageConfig.RunRoot + } else if !runtime.storageSet.StaticDirSet { + basePath = runtime.config.Engine.StaticDir } // c/storage is set up *after* the DB - so even though we use the c/s diff --git a/pkg/domain/entities/engine.go b/pkg/domain/entities/engine.go index 70f0906d98..c268c6298f 100644 --- a/pkg/domain/entities/engine.go +++ b/pkg/domain/entities/engine.go @@ -58,4 +58,5 @@ type PodmanConfig struct { SSHMode string MachineMode bool TransientStore bool + GraphRoot string } diff --git a/pkg/domain/infra/runtime_libpod.go b/pkg/domain/infra/runtime_libpod.go index 33af1018a8..7dae29ed19 100644 --- a/pkg/domain/infra/runtime_libpod.go +++ b/pkg/domain/infra/runtime_libpod.go @@ -148,7 +148,7 @@ func getRuntime(ctx context.Context, fs *flag.FlagSet, opts *engineOpts) (*libpo if fs.Changed("root") { storageSet = true - storageOpts.GraphRoot = cfg.ContainersConf.Engine.StaticDir + storageOpts.GraphRoot = cfg.GraphRoot storageOpts.GraphDriverOptions = []string{} } if fs.Changed("runroot") { @@ -279,6 +279,10 @@ func getRuntime(ctx context.Context, fs *flag.FlagSet, opts *engineOpts) (*libpo options = append(options, libpod.WithSyslog()) } + if opts.config.ContainersConfDefaultsRO.Engine.StaticDir != "" { + options = append(options, libpod.WithStaticDir(opts.config.ContainersConfDefaultsRO.Engine.StaticDir)) + } + // TODO flag to set CNI plugins dir? if !opts.withFDS { diff --git a/test/system/030-run.bats b/test/system/030-run.bats index f5f8c09133..e84af4be4e 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -1216,6 +1216,29 @@ EOF run_podman rm -f -t0 $ctr } +@test "podman run - custom static_dir" { + # regression test for #19938 to make sure the cleanup process uses the same + # static_dir and writes the exit code. If not, podman-run will run into + # it's 20 sec timeout waiting for the exit code to be written. + + skip_if_remote "CONTAINERS_CONF_OVERRIDE redirect does not work on remote" + containersconf=$PODMAN_TMPDIR/containers.conf + static_dir=$PODMAN_TMPDIR/static_dir +cat >$containersconf <