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

Returns more than one additional image store #2096

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+58 −11
Open

Returns more than one additional image store #2096

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions types/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,8 +457,11 @@ func ReloadConfigurationFile(configFile string, storeOptions *StoreOptions) erro
if config.Storage.RootlessStoragePath != "" {
storeOptions.RootlessStoragePath = config.Storage.RootlessStoragePath
}
for _, s := range config.Storage.Options.AdditionalImageStores {
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, ",")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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))
Comment on lines +463 to +464
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
for _, s := range config.Storage.Options.AdditionalLayerStores {
storeOptions.GraphDriverOptions = append(storeOptions.GraphDriverOptions, fmt.Sprintf("%s.additionallayerstore=%s", config.Storage.Driver, s))
Expand Down
57 changes: 48 additions & 9 deletions types/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,52 @@ func TestGetRootlessStorageOpts2(t *testing.T) {
}

func TestReloadConfigurationFile(t *testing.T) {
content := bytes.NewBufferString("")
logrus.SetOutput(content)
var storageOpts StoreOptions
err := ReloadConfigurationFile("./storage_broken.conf", &storageOpts)
require.NoError(t, err)
assert.Equal(t, storageOpts.RunRoot, "/run/containers/test")
logrus.SetOutput(os.Stderr)

assert.Equal(t, strings.Contains(content.String(), "Failed to decode the keys [\\\"foo\\\" \\\"storage.options.graphroot\\\"] from \\\"./storage_broken.conf\\\"\""), true)
t.Run("broken", func(t *testing.T) {
content := bytes.NewBufferString("")
logrus.SetOutput(content)
var storageOpts StoreOptions
err := ReloadConfigurationFile("./storage_broken.conf", &storageOpts)
require.NoError(t, err)
assert.Equal(t, storageOpts.RunRoot, "/run/containers/test")
logrus.SetOutput(os.Stderr)
assert.Equal(t, strings.Contains(content.String(), "Failed to decode the keys [\\\"foo\\\" \\\"storage.options.graphroot\\\"] from \\\"./storage_broken.conf\\\"\""), true)
})
t.Run("imagestore-empty", func(t *testing.T) {
expectedStore := ""
expectedAdditionalStores := ""
var storageOpts StoreOptions
err := ReloadConfigurationFile("./storage_test.conf", &storageOpts)
require.NoError(t, err)
var actualStore, actualAdditionalStores string
for _, o := range storageOpts.GraphDriverOptions {
option := strings.Split(o, "=")
switch option[0] {
case storageOpts.GraphDriverName + ".imagestore":
actualStore = option[1]
case storageOpts.GraphDriverName + ".additionalimagestores":
actualAdditionalStores = option[1]
}
}
assert.Equal(t, actualStore, expectedStore)
assert.Equal(t, actualAdditionalStores, expectedAdditionalStores)
Copy link
Collaborator

Choose a reason for hiding this comment

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

(The parameter order is (t, expected, actual).)

})
t.Run("imagestore-many", func(t *testing.T) {
expectedStore := "/var/lib/containers/storage1"
expectedAdditionalStores := "/var/lib/containers/storage1,/var/lib/containers/storage2"
var storageOpts StoreOptions
err := ReloadConfigurationFile("./storage_imagestores_test.conf", &storageOpts)
require.NoError(t, err)
var actualStore, actualAdditionalStores string
for _, o := range storageOpts.GraphDriverOptions {
option := strings.Split(o, "=")
switch option[0] {
case storageOpts.GraphDriverName + ".imagestore":
actualStore = option[1]
case storageOpts.GraphDriverName + ".additionalimagestores":
actualAdditionalStores = option[1]
}
}
assert.Equal(t, actualStore, expectedStore)
assert.Equal(t, actualAdditionalStores, expectedAdditionalStores)
})
}
5 changes: 5 additions & 0 deletions types/storage_imagestores_test.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[storage.options]
additionalimagestores = [
"/var/lib/containers/storage1",
"/var/lib/containers/storage2"
]