Returns more than one additional image store #2096
Conversation
Closes containers#2094 Signed-off-by: Mario Loriedo <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: l0rd The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| storeOptions.GraphDriverOptions = append(storeOptions.GraphDriverOptions, fmt.Sprintf("%s.imagestore=%s", config.Storage.Driver, s)) | ||
| if len(config.Storage.Options.AdditionalImageStores) > 0 { | ||
| imageStore := config.Storage.Options.AdditionalImageStores[0] | ||
| additionalImageStores := strings.Join(config.Storage.Options.AdditionalImageStores, ",") |
There was a problem hiding this comment.
I'm not opposed to this way of doing things, but I think it'd be conceptually cleaner to add a new API field that was an array/list.
Squashing an array to a string like this comma-separated raises questions around e.g. "what happens if the store path has a comma in it" - to be clear not something that really we'd expect to happen in this case.
But as a general case these things can happen, and ad-hoc "stringification" can become a problem.
There's also the flip side in that in theory something could have been reading the prior field and relying on it only containing (the top?) store, and suddenly seeing multiple paths there would break a prior consumer.
There was a problem hiding this comment.
this can be specified multiple times, e.g. podman --storage-opt overlay.additionalimagestore=/tmp/1 --storage-opt overlay.additionalimagestore=/tmp/2 ... works fine.
So instead of combining multiple additional image stores in a single option, we can have multiple additionalimagestore, one for each directory.
Said so, we don't currently support commas in the path, as we have an explicit strings.Split(val, ",") in overlay.go.
|
On the commit message: this repository like many others in this organization uses "Linux kernel style" commit messages, see the git log - this blog is one I reference often. In this case I'd say something like:
? |
There was a problem hiding this comment.
It seems to me that this is at best a workaround for this specific case.
The root cause, as I understand it, is that GraphDriverOptions is a list of (key, value) pairs, where nothing prohibits duplicate keys; meanwhile Podman treats it as a map of key: value, where duplicate keys are impossible.
This modifies one code path where duplicate keys can happen; but GraphDriverOptions can contain user-specified items, via several different input paths. So this doesn’t actually fix things to accurately represent the configuration.
My best guess is that the podman info command needs to change its output format to be able to represent graphOptions as they truly exist; or, maybe, give up on that general extensible field entirely, and have a specific “image stores” output field, getting only that data (not parsing the config, but asking c/storage via .AdditionalImageStores() or maybe some to-be-added higher level interface over it) and presenting it in some user-appropriate way.
(Generally I’m seriously unhappy about the flow where we have well-typed Storage.Options, turn that into strings, and then parse strings in the graph driver again; but that’s not all that relevant for this PR, and because GraphDriverOptions can be specified by the user directly, a string parser must exist somewhere.)
| storeOptions.GraphDriverOptions = append(storeOptions.GraphDriverOptions, fmt.Sprintf("%s.imagestore=%s", config.Storage.Driver, imageStore)) | ||
| storeOptions.GraphDriverOptions = append(storeOptions.GraphDriverOptions, fmt.Sprintf("%s.additionalimagestores=%s", config.Storage.Driver, additionalImageStores)) |
There was a problem hiding this comment.
I can’t see any parser of a driver additionalimagestores option; AFAICS that just doesn’t work.
All entries in Options.AdditionalImageStores have the same semantics (and that’s different from Options.ImageStore). I don’t know why we would treat the first item differently.
I especially don’t know why we would now add the first item twice.
Also, if you look at the drivers’ option parsers, for the overlay driver both option names are treated the same, but for VFS, additionalimagestore is not recognized at all. So this breaks VFS.
| } | ||
| } | ||
| assert.Equal(t, actualStore, expectedStore) | ||
| assert.Equal(t, actualAdditionalStores, expectedAdditionalStores) |
There was a problem hiding this comment.
(The parameter order is (t, expected, actual).)
|
@l0rd still working on this one? |
|
@rhatdan yes. I mean I should resume working on this next week. |
Closes #2094