opensource/podman
1
0
Fork 0
mirror of https://github.com/containers/podman.git synced 2025-05-20 00:27:03 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix handling of static/volume dir

Browse Source 
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 <vrothberg@redhat.com>
This commit is contained in:
Valentin Rothberg
2023-09-21 11:28:22 +02:00
parent 919564e1ae
commit 6293ec2e2d
13 changed files with 51 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
cmd/podman/root.go
View File

@ -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")

2
go.mod
View File

@ -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

4
go.sum
View File

@ -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=

3
libpod/oci_conmon_common.go
View File

@ -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))
}

16
libpod/options.go
View File

@ -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
}

12
libpod/runtime.go
View File

@ -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)
}
}

2
libpod/sqlite_state.go
View File

@ -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

1
pkg/domain/entities/engine.go
View File

@ -58,4 +58,5 @@ type PodmanConfig struct {
SSHMode string
MachineMode bool
TransientStore bool
GraphRoot string
}

6
pkg/domain/infra/runtime_libpod.go
View File

@ -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 {

23
test/system/030-run.bats
View File

@ -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 <<EOF
[engine]
static_dir="$static_dir"
EOF
ctr=$(random_string)
CONTAINERS_CONF_OVERRIDE=$containersconf PODMAN_TIMEOUT=20 run_podman run --name=$ctr $IMAGE true
CONTAINERS_CONF_OVERRIDE=$containersconf PODMAN_TIMEOUT=20 run_podman inspect --format "{{.ID}}" $ctr
cid="$output"
# Since the container has been run with custom static_dir (where the libpod
# DB is stored), the default podman should not see it.
run_podman 1 container exists $ctr
run_podman 1 container exists $cid
CONTAINERS_CONF_OVERRIDE=$containersconf run_podman rm -f -t0 $ctr
}
@test "podman --authfile=nonexistent-path" {
# List of commands to be tested. These all share a common authfile check.
#

18
vendor/github.com/containers/common/pkg/config/config.go generated vendored
View File

@ -561,24 +561,6 @@ type SetOptions struct {
// backwards compatibility with older versions of libpod for which we must
// query the database configuration. Not included in the on-disk config.
StorageConfigGraphDriverNameSet bool `toml:"-"`
// StaticDirSet indicates if the StaticDir has been explicitly set by the
// config or by the user. It's required to guarantee backwards compatibility
// with older versions of libpod for which we must query the database
// configuration. Not included in the on-disk config.
StaticDirSet bool `toml:"-"`
// VolumePathSet indicates if the VolumePath has been explicitly set by the
// config or by the user. It's required to guarantee backwards compatibility
// with older versions of libpod for which we must query the database
// configuration. Not included in the on-disk config.
VolumePathSet bool `toml:"-"`
// TmpDirSet indicates if the TmpDir has been explicitly set by the config
// or by the user. It's required to guarantee backwards compatibility with
// older versions of libpod for which we must query the database
// configuration. Not included in the on-disk config.
TmpDirSet bool `toml:"-"`
}
// NetworkConfig represents the "network" TOML config table

2
vendor/github.com/containers/common/pkg/config/default.go generated vendored
View File

@ -306,8 +306,6 @@ func defaultEngineConfig() (*EngineConfig, error) {
c.graphRoot = storeOpts.GraphRoot
c.ImageCopyTmpDir = getDefaultTmpDir()
c.StaticDir = filepath.Join(storeOpts.GraphRoot, "libpod")
c.VolumePath = filepath.Join(storeOpts.GraphRoot, "volumes")
c.VolumePluginTimeout = DefaultVolumePluginTimeout
c.CompressionFormat = "gzip"

2
vendor/modules.txt vendored
View File

@ -164,7 +164,7 @@ github.com/containers/buildah/pkg/sshagent
github.com/containers/buildah/pkg/util
github.com/containers/buildah/pkg/volumes
github.com/containers/buildah/util
# github.com/containers/common v0.56.1-0.20230920191016-f4e726d4b162
# github.com/containers/common v0.56.1-0.20230922104122-56ed984ea383
## explicit; go 1.18
github.com/containers/common/libimage
github.com/containers/common/libimage/define