storage: port storage.conf to new configfile package and rework option parsing#680
storage: port storage.conf to new configfile package and rework option parsing#680mtrmac merged 14 commits intocontainers:mainfrom
Conversation
github-actions
bot
added
storage
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
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.confin 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.
| 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
This part is in cmd/containers-storage which I think is mostly a test/debug command and hopefully has no real users?
Yes.
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.
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. |
Thanks, that’s a good reason. |
|
Packit jobs failed. @containers/packit-build please check. |
| // 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"` |
There was a problem hiding this comment.
Would a rename be appropriate? configfile.Slice doesn't really express why I shouldn't be using an ordinary array? configfile.LayerableSlice maybe?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
|
This should be good to review Podman PR containers/podman#28194 crio PR cri-o/cri-o#9797 AFAICT the failures there are not related to this so crio should work without any changes |
|
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. |
| # Storage path for rootless users | ||
| # | ||
| rootless_storage_path = "$HOME/$UID/containers/storage" | ||
| rootless_storage_path = "$HOME/$UID/rootless/storage" |
There was a problem hiding this comment.
Not for this PR, but this is something we probably need to highlight in the Red Hat Docs when 6.0 comes out @ddarrah
There was a problem hiding this comment.
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
left a comment
TomSweeneyRedHat
left a comment
There was a problem hiding this comment.
LGTM
and happy green test buttons
|
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 |
|
I am LGTM on everything except 81cf9d1 which I have not had time to fully review. Will try to get to it today. |
|
I have some nitpicky comments on reworking argument parsing but otherwise LGTM |
There was a problem hiding this comment.
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.)
| // 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 != "" { |
There was a problem hiding this comment.
(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…)
There was a problem hiding this comment.
I did not touch the order, I think we can reorder later if really wanted
| } | ||
| if !searchOptions(doptions, s200) { | ||
| t.Fatalf("Expected to find size %q options, got %v", s200, doptions) | ||
| } |
There was a problem hiding this comment.
(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.)
There was a problem hiding this comment.
I have not pushed more tests for now, if this is a blocker for you and I should add more let me know
| storeOptions.GraphDriverName = overlayDriver | ||
| } | ||
| storeOptions.GraphDriverPriority = config.Storage.DriverPriority | ||
| if storeOptions.GraphDriverName == "" && len(storeOptions.GraphDriverPriority) == 0 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ah I was looking at at one point but forget again, thanks for the find I think
71ab900 is enough to address that part?
There was a problem hiding this comment.
Yes, that works:
Most graph drivers start their init function with some variant of MkdirAll (typically MkdirAll(filepath.Join(home, "something"), fair enough), so this might technically be not quite necessary, ultimately.
Due to ScanPriorDrivers, I think pre-creating is very risky; given all of the above, the MkdirAll in store.go only after we successfully get a graph driver, the way this PR does, is at least a safe transformation.
(I suppose, ideally, we should separate a “check whether a driver can be used” and “actually create the graph driver” in the driver API, and have the generic code create the drivers’ home, rather than the current reliance on an ~accidental MkdirAll.)
storage/drivers/btrfs/btrfs.go
Outdated
| @@ -102,18 +102,22 @@ func parseOptions(opt []string) (btrfsOptions, bool, error) { | |||
| return options, userDiskQuota, err | |||
| } | |||
| key = strings.ToLower(key) | |||
There was a problem hiding this comment.
Can we combine these and graphdriver.IsDriverPrefixedOption? Do the slicing in that function, return a pair of (driver, option)?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
ParseOption(driver, input):
- split into key=value
ToLower(key)- see if key is prefixed; return (unprefixed key, should report failures)
?
There was a problem hiding this comment.
… rather, return (unprefixed key, value, should report failures)
There was a problem hiding this comment.
ok I reworked it again, I think it looks a bit better now?
giuseppe
left a comment
giuseppe
left a comment
There was a problem hiding this comment.
thanks, tested locally.
LGTM, with the fix for mount_program= without a whitespace
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 containers.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 change 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 actually have working individual commits and tests. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
The option logic was changed 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>
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>
If both graphroot and rootless_storage_path we use graphroot so add a warning in such case and mention rootless_storage_path is deprecated. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
The value is unused and it is confusing to write into the struct which should hold only the toml file data. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
The storage options can contain an empty graph driver name as the code has logic to pick a suitable default later. As such the name is not valid there and we must move the MkdirAll call to after s.graphDriverName is assigned in load(). Signed-off-by: Paul Holzinger <pholzing@redhat.com>
As pointed out by Miloslav during review. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
As pointed out by Miloslav. This here is a bug as we should still parse all other options even if min_space is set. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
|
@Luap99 FYI I’m trying to track this down, I see The difference is that previously, the defaults for creating a store came from Now, the defaults come from I think the new version is much better, but it is different. Do we need to worry about that? I think Podman would usually get a store via Outside of Podman, this is probably user-visible as a change to But I think that was somewhat unanticipated; can you think of other consequences of this? |
Are there known users of cotnainers-storage: transport on macos, does that even work, I guess vfs might? I mean logically the hard coded root paths are just broken when you run rootless on macos so I would say bug fix and doubt it will cause much complains.
Podman Buildah and crio use DefaultStoreOptions so they should be fine generally.
Is it unexpected? I guess sure I did not consider that mainly because I didn't want to try to make sense between the ever so slightly differences between the old DefaultStoreOptions() and Options() calls. But what should break? I mean as root should should still get the same paths as before and as non root you used to get unusable paths (assuming an admin did not chown them to your user) so I would say change is a bug in that sense that rootless users can now get "sane" paths. |
No, generally: the Basically, this boils down to another(?) c/storage API incompatibility: |
For some reason, c/storage has a very different default behavior on macOS, where it ~defaults to force_mask (storing file ownership and permissions in an extended attribute). I don’t know who is using this other than our tests, but it exists.
Certainly the c/image test is wrong, given how c/storage actually processes graphDir/rootDir options. I’ll look into fixing it. |
Personally I am unhappy with that GetStore API, why do we pass the options as struct instead of pointer so we could have a clear use defaults vs use all my struct values? IMO the the most logical thing would be have all callers provide valid options and do not try to handle defaults within GetStore() at all and then avoid the caching part as well but I did not study the git history for these so I likely miss the real reason this was done. |
We have per-field "set/unset" logic in option handling either way…
(To a large extent, this shouldn’t be a problem because the code must be ready to deal with concurrently running other processes, so having multiple instances of a
(
And then we would need to pay extra attention that everything calls the same Anyway, existing code calling |
|
There’s an argument that there should not be any |
5 participants
see commits