Setting --storage-opt overrides unrelated settings

[martinetd]

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind bug

Description

Overriding storage options on command-line seems to override more than what I would have expected, e.g. in rootless mode this doesn't work:

podman --storage-opt additionalimagestore=/tmp/podman_store image list

I had to set the overlay mount program again like this:

podman --storage-opt additionalimagestore=/tmp/podman_store --storage-opt overlay.mount_program=/usr/bin/fuse-overlayfs image list

Note that I have no /etc/containers/storage.conf nor $HOME/.config/containers/storage.conf, so the mount_program comes from some default value when not setting any storage-opt.

Steps to reproduce the issue:

1. podman --root /tmp/podman_store pull docker.io/alpine

2. podman --storage-opt additionalimagestore=/tmp/podman_store image list

Describe the results you received:

Error: kernel does not support overlay fs: 'overlay' is not supported over btrfs at "/tmp/podmanroot2/overlay": backing file system is unsupported for this graph driver

Describe the results you expected:

REPOSITORY                TAG     IMAGE ID      CREATED      SIZE     R/O
docker.io/library/alpine  latest  28f6e2705743  2 weeks ago  5.88 MB  true

Additional information you deem important (e.g. issue happens only occasionally):

Might be related to #9650

Output of podman version:

Version:      3.1.0-dev
API Version:  3.1.0-dev
Go Version:   go1.15.8
Git Commit:   b7c00f2cc03499d5d385a7aa7e8cd35d0ab994d7
Built:        Mon Mar  8 09:56:35 2021
OS/Arch:      linux/amd64

[vrothberg]

@rhatdan @giuseppe PTAL

[giuseppe]

not sure what is the best way to handle this.

If we don't override all the storage opts, how would it be possible to drop opts that were already specified in the config file?

At least now it is not possible to set an empty value for, e.g. mount_program:

$ podman --storage-opt overlay.mount_program="" --storage-opt additionalimagestore=/tmp/podman_store image list
Error: overlay: can't stat program : stat : no such file or directory

[martinetd]

Good point, hadn't thought of testing dropping settings.

It might make sense to do it systemd-like and drop settings if set empty (so for example for additionalimagestore since it's a list the behaviour would be strictly appending to the list, unless it's first once set to empty and then set again to append) -- that is a bit heavy-ended but I think it's quite natural once used to.

In this particular case (mount_program) I believe we should allow setting it to empty string anyway as that is a special case in the code as well (in vendor/github.com/containers/storage/drivers/overlay/overlay.go and vendor/github.com/containers/buildah/pkg/overlay/overlay.go) ; someone could very well want to override just the mount-program

[giuseppe]

In this particular case (mount_program) I believe we should allow setting it to empty string anyway as that is a special case in the code as well (in vendor/github.com/containers/storage/drivers/overlay/overlay.go and vendor/github.com/containers/buildah/pkg/overlay/overlay.go) ; someone could very well want to override just the mount-program

agreed. It should be possible to override mount_program and clear its value.

Opened a PR: containers/storage#847

[giuseppe]

I don't think we should handle each separate option and trying to append to additionalimagestore, but at least we could have something like:

diff --git a/libpod/options.go b/libpod/options.go
index 6344e1acc..352ce0a4c 100644
--- a/libpod/options.go
+++ b/libpod/options.go
@@ -71,8 +71,7 @@ func WithStorageConfig(config storage.StoreOptions) RuntimeOption {
                }
 
                if config.GraphDriverOptions != nil {
-                       rt.storageConfig.GraphDriverOptions = make([]string, len(config.GraphDriverOptions))
-                       copy(rt.storageConfig.GraphDriverOptions, config.GraphDriverOptions)
+                       rt.storageConfig.GraphDriverOptions = append(rt.storageConfig.GraphDriverOptions, config.GraphDriverOptions...)
                        setField = true
                }

so that new specified options are appended to the list instead of replacing the configuration from the conf files.

What do you think?

[martinetd]

+                       rt.storageConfig.GraphDriverOptions = append(rt.storageConfig.GraphDriverOptions, config.GraphDriverOptions...)

That would make sense to me -- I've taken a minute to check precedence and command line options do seem to overwrite config settings so it looks good to me.

Also thanks for containers/storage#847 - that also LGTM

[giuseppe]

PR here: #9676