Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix handling of static/volume dir #20088

Merged
merged 1 commit into from Sep 25, 2023
Merged

fix handling of static/volume dir #20088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/podman/root.go
View file
Open in desktop
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we concerned about this breaking folks? Setting --root is now doing something very different than it did before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't know. This stuff is really broken :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good to go. It essentially overrode the graph root before as well (that's how it was being used).

_ = cmd.RegisterFlagCompletionFunc(rootFlagName, completion.AutocompleteDefault)

pFlags.StringVar(&podmanConfig.RegistriesConf, "registries-conf", "", "Path to a registries.conf to use for image processing")
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Expand Up @@ -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=
Expand Down
3 changes: 3 additions & 0 deletions libpod/oci_conmon_common.go
View file
Open in desktop
Expand Up @@ -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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mheon do you recall why we're not passing all env variables to conmon? I feel like we should do this since there is a growing number of those which I do not see being passed down.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it dates back to before we forked off Conmon, so this is probably older than my involvement in the project. I don't see a good reason not to forward on full environment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will open a follow-up PR 👍

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))
}
Expand Down
16 changes: 1 addition & 15 deletions libpod/options.go
View file
Open in desktop
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"net"
"os"
"path/filepath"
"strings"
"syscall"
"time"
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -270,7 +258,6 @@ func WithStaticDir(dir string) RuntimeOption {
}

rt.config.Engine.StaticDir = dir
rt.config.Engine.StaticDirSet = true

return nil
}
Expand Down Expand Up @@ -372,7 +359,6 @@ func WithTmpDir(dir string) RuntimeOption {
return define.ErrRuntimeFinalized
}
rt.config.Engine.TmpDir = dir
rt.config.Engine.TmpDirSet = true

return nil
}
Expand Down Expand Up @@ -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
}
Expand Down
12 changes: 11 additions & 1 deletion libpod/runtime.go
View file
Open in desktop
Expand Up @@ -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)
}
}

Expand Down
2 changes: 2 additions & 0 deletions libpod/sqlite_state.go
View file
Open in desktop
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions pkg/domain/entities/engine.go
View file
Open in desktop
Expand Up @@ -58,4 +58,5 @@ type PodmanConfig struct {
SSHMode string
MachineMode bool
TransientStore bool
GraphRoot string
}
6 changes: 5 additions & 1 deletion pkg/domain/infra/runtime_libpod.go
View file
Open in desktop
Expand Up @@ -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") {
Expand Down Expand Up @@ -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 {
Expand Down
23 changes: 23 additions & 0 deletions test/system/030-run.bats
View file
Open in desktop
Expand Up @@ -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
vrothberg marked this conversation as resolved.
Show resolved Hide resolved
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.
#
Expand Down
18 changes: 0 additions & 18 deletions vendor/github.com/containers/common/pkg/config/config.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions vendor/github.com/containers/common/pkg/config/default.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion vendor/modules.txt
View file
Open in desktop
Expand Up @@ -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
Expand Down