storage: port storage.conf to new configfile package and rework option parsing by Luap99 · Pull Request #680 · containers/container-libs

Luap99 · 2026-03-04T17:47:15Z

see commits

mtrmac

5-minute look:

“storage/types: remove Save() and StorageConfig()”: ACK assuming there really are no users
“storage/types: rework config files parsing”: no opinion yet (I know, not helpful)
‘Revert "Ignore failure to ignore thinpool keys"’: Why should we expect that users touched storage.conf in the meantime, e.g. a personal one in rootless setups? (A genuine question, not a NAK)
“move common/internal/attributedstring into storage/pkg/configfile”: ACK. I wonder a bit about the subpackage name, but the functionality does only make sense in configs.
“storage.conf: use custom slice type to allow appending”: Sure, I didn’t check whether that is comprehensive
“storage: rewrite option parsing”: I think all of that existing code is a bad idea. If we start with structured data in Go structs, we shouldn’t be round-tripping through a set of strings and parsing that again into Go structs. Admittedly that would be a graph driver API change, but we have been changing that recently all the time. OTOH I think it’s probably unreasonable to block this work on changing all of that — and there may well be something I’m missing. (Also I worry about a bit about not detecting use of options unsupported by the graph driver if we just gave the graph driver a full Go struct.)
“storage: no longer warn when the config file has no driver set”: I suppose this is contingent on the above.

mtrmac · 2026-03-04T19:34:50Z

storage/cmd/containers-storage/config.go

 	if len(args) > 0 {
-		if err = types.ReloadConfigurationFileIfNeeded(args[0], &options); err != nil {
-			return 1, fmt.Errorf("reload: %+v", err)
+		if err := os.Setenv("CONTAINERS_STORAGE_CONF", args[0]); err != nil {


This makes me wonder. Should the config file choice really be done by environment variables exclusively?

On the one hand, I suppose with drop-ins it is really not that meaningful to allow individual overrides.

On the other hand, os.Setenv is process-wide and not safe to use from goroutines in this sense. If using environment variables is really all that great, would we consider dropping this parameter and telling users to set the environment variable manually?! Purely from a Go code structure, adding another field to LoadOptions would be much more natural.

This is probably fine as is, an a natural consequence of the (settled and approved!) design.

This part is in cmd/containers-storage which I think is mostly a test/debug command and hopefully has no real users? And AFAIK this command exists again right away after printing the config file.

So if we give a path we want to make to only read this one file? We can add a interface to configfile to set that one file via an options struct sure... but then I don't think we need that for our actual users and this here seems like the easiest/quickest thing to switch and I am a bit limited in term of time so I am afraid not every choice will be the cleanest

This part is in cmd/containers-storage which I think is mostly a test/debug command and hopefully has no real users?

Yes.

Luap99 · 2026-03-04T19:54:26Z

“storage: rewrite option parsing”: I think all of that existing code is a bad idea. If we start with structured data in Go structs, we shouldn’t be round-tripping through a set of strings and parsing that again into Go structs. Admittedly that would be a graph driver API change, but we have been changing that recently all the time. OTOH I think it’s probably unreasonable to block this work on changing all of that — and there may well be something I’m missing. (Also I worry about a bit about not detecting use of options unsupported by the graph driver if we just gave the graph driver a full Go struct.)

I really thought about just passing the options struct to the graphdriver init instead. And I do agree this is much better than this string nonsense to some extend. But then we still need to support option parsing via the old API I guess unless we really want to break more stuff and AFAIK the string syntax has to be supported no matter what via container-storage: transport and --storage-opt cli options we expose.
But yes we could avoid converting the existing config file to a string slice and I would be in favour though with the time restricts I Am not sure I want to commit into doing this as well right now.

‘Revert "Ignore failure to ignore thinpool keys"’: Why should we expect that users touched storage.conf in the meantime, e.g. a personal one in rootless setups? (A genuine question, not a NAK)

Because that was not about users setting this option, literally the default fedora storage.conf file had this embeeded when this work around was added. AFAICT this is no longer the case so it should not be needed and if users still have that it is time for them to update. And well actually the configfile package logs these things at debug level (bc containers.conf did that) so I guess they would not notice either way which make this field even less needed.
That said this commit is indeed 100% optional to my work and can be dropped just fine if wanted.

mtrmac · 2026-03-05T14:57:06Z

Because that was not about users setting this option, literally the default fedora storage.conf file had this embeeded when this work around was added. AFAICT this is no longer the case so it should not be needed and if users still have that it is time for them to update.

Thanks, that’s a good reason.

packit-as-a-service · 2026-03-05T18:02:38Z

Packit jobs failed. @containers/packit-build please check.

mheon · 2026-03-06T20:59:32Z

common/pkg/config/config.go

 	// ConmonEnvVars are environment variables to pass to the Conmon binary
 	// when it is launched.
-	ConmonEnvVars attributedstring.Slice `toml:"conmon_env_vars,omitempty"`
+	ConmonEnvVars configfile.Slice `toml:"conmon_env_vars,omitempty"`


Would a rename be appropriate? configfile.Slice doesn't really express why I shouldn't be using an ordinary array? configfile.LayerableSlice maybe?

I mean that is what the type documentation is for? And that is pretty clear there IMO.

I don't mind renaming it but personally I see little reason for that, even with any other name someone still has to read the docs to get why this type exists and what is actually does different.

Yeah I'm not firm on this, I can live with the current name

Because the new storage.conf parsing will always read the storage.conf file also under $HOME it means in this unshare env it tries to access /root which will not work due missing permissions. To work around set CONTAINERS_STORAGE_CONF=/dev/null to make it not parse the normal storage.conf locations here. The tests are using their own storage options via the cli anyway so this should not alter the behavior. This is need in preparation for the storage.conf rewrite[1]. [1] containers/container-libs#680 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

This option is not actually used in GetStore() for anything. It only is used during config parsing in storage.conf but that is of no relevance here. Context: I remove this option for the storage.conf rework [1] and as it does not do anything we can already remove it to avoid breaking the compile for the vendor step once the upstream PR is merged. [1] containers/container-libs#680 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Luap99 · 2026-03-10T17:38:58Z

This should be good to review

Podman PR containers/podman#28194
Buildah PR containers/buildah#6708, submitted the fixes we can merge ahead as containers/buildah#6711 and containers/buildah#6714 there so buildah should not get broken by the vendor

crio PR cri-o/cri-o#9797 AFAICT the failures there are not related to this so crio should work without any changes

storage/docs/containers-storage.conf.5.md

TomSweeneyRedHat · 2026-03-10T18:56:18Z

Just a man page nit, and I'd like to see happy green test buttons in Buildah/Podman. At the moment, there's a bit of red in each, I think, due to flakes.

Luap99 · 2026-03-11T17:18:41Z

Just a man page nit, and I'd like to see happy green test buttons in Buildah/Podman. At the moment, there's a bit of red in each, I think, due to flakes.

They are flakes, I normally do not bother to click rerun on known flakes on PRs that cannot yet be merged as it just wastes resources for nothing IMO.
But looks like buildah is green now, I clicked rerun on the podman ones as well so I guess they turn green soon

AFAICT these have been added at some point to allow storage.conf rewriting but that has never been implemented anywhere. As such drop these as the concept of writing to a signle default file becomes very questionable with the new incoming config loading as it uses many files not just one. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Use the new pkg/configfile to allow parsing multiple files including drop ins per our design doc[1]. I went with the hard way of removing a lot of complexity and public APIs to start with a much more simple approach. Now there are only public functions left, LoadStoreOptions() which loads all files every time it gets called, this could be used for callers who like to re-read the files and a later point. Otherwsie the main existing DefaultStoreOptions() which caches the result on first call so we do not load the same files multiple times. Then a lot of this custom code to load rootless options were dropped. I think that should generally be fine to there might be subtile things I miss. One point is the choice of the rootless storage driver. The new code leaves that empty, the store load path already has logic to get a default driver. One important note is the handling of rootless_storage_path, which we read before graphroot now. So if graphroot is set in any configfile then rootless_storage_path is ignored. Overall we should deprecate rootless_storage_path as both the runroot and graphroot already uses the env expand code. And with the new configfile code users who only want to set the storage path of rootless users should create /etc/containers/storage.rootless.conf.d/99-myconf.conf with the graphroot set. That file will not be read by the rootful process. And to set a rootful default use /etc/containers/storage.rootful.conf.d/99-myconf.conf. [1] https://github.com/containers/podman/blob/7e78e842a05b4c924d9adae263bbdab340ab031f/contrib/design-docs/config-file-parsing.md Signed-off-by: Paul Holzinger <pholzing@redhat.com>

This reverts commit 9d71cd1. This was only added to avoid warning with older storage.conf that have that key. This problem is now close to two years old so all storage.conf should have been updated in the meantime and if they are not the current log level is only debug in pkg/configfile and no longer a wanring so it will not spam users either way. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

The motivation is that the new configwork should use that to allow appending values in all files. As such move it into the existing configfile package so it can be reused, this does mean the package is no longer internal but it cannot be as we use it in common which is a different module. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Use the custom Slice type we have had in cotnainers.conf also for the storage.conf arrays. With drop in files users should have a way to append and not just replace array values. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Overhaul option parsing to not require driver prefix option names. When creating a driver it makes little sense to require all callers to send in option with the driver name prefixed. As such chnage the driver parsing so it accepts just the option name or the existing name. syntax. With that we can have a better option parsing out of the storage.conf file because we no longer need the full driver name already while parsing the file. Even when in practise the actual driver name mind not be known until way later when we init the graphdriver. Now we still need the driver.optname syntax so that the config file can pass down options for different drivers without knowing the actual driver that will be used. So the option parsing in the drivers now should ignore options meant for a different driver to not cause hard errors when a config file sets a vfs and overlay option at the same time. Also I know the commit is a bit large but I was not able to split this in a useful way to would actualyl have working induvidual commits and tests. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

The option logic was chnaged in the prior commit to handle the case without driver name properly. Based on the comment in [1] the reason we had this warning was only because of this special option handling. [1] containers/storage#486 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Using the dot without a driver name is not valid, while the old parsing code accepted that it was due the incorrect passing of options. Any use of that syntax seems more of a mistake to me so lets not try to support that any longer. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

With the new config parsing graphroot/runroot in the main file will be used by all process root and rootless so it makes no sense to set them in the default config file as rootless would try to use them. The library already picks the right defaults so there is no need to set them. If one would want to set them for only root they should go into the new storage.rootful.conf.d/ directory. Note I kept mountopt for now as this is wanted per commit cf28ef6 ("Add default mount options to pass to drivers") and I don't think the library uses that default internally. I think we should change that eventually but for now I like to not make this even bigger than it already is. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Update the docs based on the new parsing behavior. Note as far as the new drop in and file precedence works I plan to add a new man page and then have all the man pages link to that instead of duplication how the ordering works in each man page. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

TomSweeneyRedHat · 2026-03-13T15:50:52Z

storage/types/storage_test.conf

 # Storage path for rootless users
 #
-rootless_storage_path = "$HOME/$UID/containers/storage"
+rootless_storage_path = "$HOME/$UID/rootless/storage"


Not for this PR, but this is something we probably need to highlight in the Red Hat Docs when 6.0 comes out @ddarrah

This has nothing to do with anything external, it is a test file used in the unit tests, nothing more.

The line is changed so we can test for difference between graphroot and rootless_storage_path fields as they used the same value before

TomSweeneyRedHat

LGTM
and happy green test buttons

Luap99 · 2026-03-16T16:55:35Z

I have fixed the merge conflicts locally but will not push as I wait for a another review and I don't want this merged until we fixed the current CNI removal vendor in containers/buildah#6453 and containers/podman#28202

mheon · 2026-03-16T16:58:55Z

I am LGTM on everything except 81cf9d1 which I have not had time to fully review. Will try to get to it today.

mheon · 2026-03-16T18:55:41Z

I have some nitpicky comments on reworking argument parsing but otherwise LGTM

mtrmac

A ~full review now.

I did not quite analyze the differences line by line to identify every single behavior change — we are knowingly breaking at a larger scale, and, I’m sure the previous code structure is not worth preserving…

(Perhaps also clean up some typos in commit messages.)

mtrmac · 2026-03-16T19:54:59Z

storage/types/options.go

-	f, err := os.Create(configFile)
-	if err != nil {
-		return err
+	// FIXME: Should this be before or after the graphroot setting?


Would it make sense to warn if both are set?

mtrmac · 2026-03-16T19:58:54Z

storage/types/options.go

-// StorageConfig is used to retrieve the storage.conf toml in order to overwrite it
-func StorageConfig() (*TomlConfig, error) {
-	config := new(TomlConfig)
+	if config.Storage.GraphRoot != "" {


(Absolutely non-blocking: Ordering the field processing in the order of fields in config.Storage would make it a tiny bit easier to verify that everything is handled. OTOH the structure is not that regular…)

mtrmac · 2026-03-16T20:00:18Z

storage/types/options.go

 		storeOptions.GraphDriverName = config.Storage.Driver
 	}
 	if os.Getenv("STORAGE_DRIVER") != "" {
 		config.Storage.Driver = os.Getenv("STORAGE_DRIVER")


(Pre-existing: This modification of config seems a bit unexpected, I’d expect the mapping to be a ~pure function.)

mtrmac · 2026-03-16T20:20:44Z

storage/types/options.go

-func ReloadConfigurationFileIfNeeded(configFile string, storeOptions *StoreOptions) error {
-	prevReloadConfig.mutex.Lock()
-	defer prevReloadConfig.mutex.Unlock()
+func LoadStoreOptions(opts LoadOptions) (StoreOptions, error) {


A bit of documentation please, at least pointing at DefaultStoreOptions.

mtrmac · 2026-03-16T20:25:22Z

storage/types/options.go

 	}
 	if config.Storage.Options.MountProgram != "" {
-		storeOptions.GraphDriverOptions = append(storeOptions.GraphDriverOptions, fmt.Sprintf("%s.mount_program=%s", config.Storage.Driver, config.Storage.Options.MountProgram))
+		storeOptions.GraphDriverOptions = append(storeOptions.GraphDriverOptions, " mount_program="+config.Storage.Options.MountProgram)


The space looks unexpected. (It probably happens to work because ParseKeyValueOpt trims the returned values, eww.)

mtrmac · 2026-03-16T20:39:19Z

storage/pkg/config/config_test.go

-	}
-	if !searchOptions(doptions, s200) {
-		t.Fatalf("Expected to find size %q options, got %v", s200, doptions)
-	}


(Some testing of the interactions between per-driver options and the top options in OptionsConfig would be valuable. Admittedly the previous version of GetGraphDriverOptions wasn’t really tested for it that much…, and in particular, we never had tests going all the way to the driver and verifying that it processes the global option set in LoadStoreOptions + the per-graph-driver one in GetGraphDriverOptions prioritizing the expected one, anyway…)

(Generally I wonder a bit about splitting the functional "mapping" part of LoadStoreOptions from the more imperative loading part. But is not actually necessary for testing, we can just create test-only config files from Go data.)

mtrmac · 2026-03-16T20:43:47Z

storage/types/options.go

 		storeOptions.GraphDriverName = overlayDriver
 	}
-	storeOptions.GraphDriverPriority = config.Storage.DriverPriority
-	if storeOptions.GraphDriverName == "" && len(storeOptions.GraphDriverPriority) == 0 {


I can’t see for sure how GraphDriverName == "" works.

Yes, drivers.New figures something out, but GetStore is using GraphDriverName to create paths. That might be unnecessary/superfluous; is it really?

mtrmac · 2026-03-16T20:52:26Z

storage/docs/containers-storage.conf.5.md

 ## FILES

-Distributions often provide a `/usr/share/containers/storage.conf` file to define default storage configuration. Administrators can override this file by creating `/etc/containers/storage.conf` to specify their own configuration. Likewise rootless users can create a storage.conf file to override the system storage.conf files. Files should be stored in the `$XDG_CONFIG_HOME/containers/storage.conf` file.  If `$XDG_CONFIG_HOME` is not set then the file `$HOME/.config/containers/storage.conf` is used.
+The following search locations are used:


Please document the priority, and the per-base name overrides (at least by linking to the systemd spec)

As mention in the commit message the plan would be to link a common man page that describes the ordering for each
filled as https://redhat.atlassian.net/browse/RUN-4440
is it ok to wait for that work?

Oh I have missed that. Fair enough. Maybe a FIXME HTML comment?

mtrmac · 2026-03-16T20:54:44Z

storage/types/options.go

-	if err != nil {
-		return nil, err
+	if storeOptions.ImageStore != "" && storeOptions.ImageStore == storeOptions.GraphRoot {
+		return storeOptions, fmt.Errorf("imagestore %s must either be not set or be a different than graphroot", storeOptions.ImageStore)


(pre-existing, s/a //)

mheon · 2026-03-17T00:03:08Z

storage/drivers/btrfs/btrfs.go

@@ -102,18 +102,22 @@ func parseOptions(opt []string) (btrfsOptions, bool, error) {
 			return options, userDiskQuota, err
 		}
 		key = strings.ToLower(key)


Can we combine these and graphdriver.IsDriverPrefixedOption? Do the slicing in that function, return a pair of (driver, option)?

so I had a look, while I agree it sounds nice it would introduce one big problem.

IsDriverPrefixedOption() like right now is only called in case when the driver code does not understand the option.
I think what you say is move this above in which case splitting at . can return the wrong thing if you consider that . can be a valid option value imagestore=/mypath/withdot/my.image then we split wrong and confuse the parser.

So I don't think it would work like you suggest unfortunately

ParseOption(driver, input):

split into key=value

ToLower(key)

see if key is prefixed; return (unprefixed key, should report failures)

?

… rather, return (unprefixed key, value, should report failures)

giuseppe

thanks, tested locally.

LGTM, with the fix for mount_program= without a whitespace

mtrmac · 2026-03-17T17:28:31Z

storage/store.go

 // GetMountOptions returns the mountoptions for the specified driver and graphDriverOptions
 func GetMountOptions(driver string, graphDriverOptions []string) ([]string, error) {
 	mountOpts := []string{
 		".mountopt",


This also expects a dot-prefixed option…
That probably needs to be adjusted, but it’s one more weird data point in favor of supporting that. I didn’t look further into the history of this.

Seems like we want to expose the options to podman.
containers/podman@3beacb7
87880e1

That seems as often the weirdest way to get this info, I feel like it would be much more saner to expose a function to return the mountOptions field from the driver interface to the store and then return that instead of doing another round of option parsing.

I fail to see why this function would need to be in storage at all. Any caller passes in the mount options here again so they could process it themselves without us having to expose the storage API. I feel like it would be best to just remove this function.

I think the intent is for the container’s RW layers to generally follow the mountopt option from storage.conf. I don’t know who needs / uses that option (other than the nodev default) but it seems reasonable as a principle that containers should follow that.

And then the behavior with --privileged is a sort of a hack breaking that model — and if there should be such a hack, having it in Podman is not too bad. I can imagine ContainerOptions.DoNotUseTheseMountOptions where Podman would still contain the privOpt list but the option value would not travel storage → Podman → storage to do the filtering; I don’t have a strong opinion on that.

I feel like it would be much more saner to expose a function to return the mountOptions field from the driver interface to the store and then return that

If the driver is responsible for parsing/determining options, yes, that would be cleaner.

Alternatively the flow could go storage.conf → generic option processing / parsing code → driver just gets a value to use, with no parsing logic in the driver. (That e.g. ensure the prioritization of top-level vs. driver-specific variants of these options is handled consistently.) OTOH that might make it harder to handle exceptions (like drivers that don’t support mount options at all… right now they refuse the storage.conf option but silently ignore the ContainerOptions… not great).

Anyway for this PR, I think the minimal action is

decide whether we should accept .-prefixed options

ensure this function accepts the raw mountopt option name.

github-actions bot added storage Related to "storage" package common Related to "common" package labels Mar 4, 2026

mtrmac reviewed Mar 4, 2026

View reviewed changes

Luap99 force-pushed the storage-conf branch from 9a92999 to b5ae39e Compare March 5, 2026 18:01

github-actions bot added the image Related to "image" package label Mar 5, 2026

Luap99 force-pushed the storage-conf branch from b5ae39e to 5479ee5 Compare March 5, 2026 18:07

Luap99 mentioned this pull request Mar 6, 2026

DNM: test storage.conf rewrite containers/podman#28194

Draft

6 tasks

mheon reviewed Mar 6, 2026

View reviewed changes

Luap99 mentioned this pull request Mar 10, 2026

tests: some more storage.conf rewrite prep containers/buildah#6714

Merged

Luap99 marked this pull request as ready for review March 10, 2026 17:39

TomSweeneyRedHat reviewed Mar 10, 2026

View reviewed changes

storage/docs/containers-storage.conf.5.md Outdated Show resolved Hide resolved

Luap99 force-pushed the storage-conf branch from 5479ee5 to 38de879 Compare March 11, 2026 17:14

Luap99 added 10 commits March 11, 2026 20:43

storage.conf: use custom slice type to allow appending

0aea2be

Use the custom Slice type we have had in cotnainers.conf also for the storage.conf arrays. With drop in files users should have a way to append and not just replace array values. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Luap99 force-pushed the storage-conf branch from 38de879 to 6d5d416 Compare March 11, 2026 19:43

TomSweeneyRedHat reviewed Mar 13, 2026

View reviewed changes

mtrmac reviewed Mar 16, 2026

View reviewed changes

mheon reviewed Mar 17, 2026

View reviewed changes

giuseppe approved these changes Mar 17, 2026

View reviewed changes

mtrmac reviewed Mar 17, 2026

View reviewed changes

Conversation

Luap99 commented Mar 4, 2026

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luap99 commented Mar 4, 2026

Uh oh!

mtrmac commented Mar 5, 2026

Uh oh!

packit-as-a-service bot commented Mar 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luap99 commented Mar 10, 2026

Uh oh!

Uh oh!

TomSweeneyRedHat commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Luap99 commented Mar 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

Uh oh!

Luap99 commented Mar 16, 2026

Uh oh!

mheon commented Mar 16, 2026

Uh oh!

mheon commented Mar 16, 2026

Uh oh!

mtrmac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe left a comment

TomSweeneyRedHat commented Mar 10, 2026 •

edited

Loading

mtrmac left a comment •

edited

Loading