Add support for drop-in config #1885

harche · 2024-04-12T17:49:45Z

Helps in use cases where a drop-in config could help by allowing users to update a subsection of the config instead of touching an entire config file.

e.g.

openshift-ci · 2024-04-12T17:49:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: harche
Once this PR has been reviewed and has the lgtm label, please assign nalind for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

harche · 2024-04-12T17:49:52Z

/hold

haircommander · 2024-04-12T17:57:39Z

types/options.go

+}
+
+func mergeStoreOptions(base, dropIn StoreOptions) StoreOptions {
+	if dropIn.RunRoot != "" {


instead of individually adding fields (which won't scale well, can we just serialize into the struct: https://github.com/cri-o/cri-o/blob/256fda5ac98de1d67e26133d0b25df2a74e2ebd2/pkg/config/config.go#L705

Thanks. Turns out they already have a function that does that,

storage/types/options.go

Line 415 in 1fd0dc1

func ReloadConfigurationFile(configFile string, storeOptions *StoreOptions) error {

Not really, they don't serialize and automatically update.

@haircommander I updated the code to reuse existing ReloadConfigurationFile (instead of writing a new merge config logic) which seem to be doing more than just updating the options. ReloadConfigurationFile is in use already, so I think we can safely rely on it. Although I admit it is not as scalable as the cri-o approach is, but using ReloadConfigurationFile allows us to reuse their existing way of reading and parsing a config file.

WDYT?

haircommander · 2024-04-12T19:02:02Z

types/options.go

+	// defaultOverrideConfigFile path to override the default system wide storage.conf file
+	defaultOverrideConfigFile = "/etc/containers/storage.conf"
+
+	// defaultDropInConfigDir path to the folder containing drop in config files
+	defaultDropInConfigDir = "/etc/containers/storage.conf.d"


these may need to remain in the platform specific paths. i'm not sure if there are windows paths that need to be supported.

I moved it here because all platform specific files had same value for that variable, including windows one. Although it does look incorrect for the windows, but I just moved it from options_windows.go

Also, if I don't add it here, then just like existing defaultOverrideConfigFile, I will have to add new defaultDropInConfigDir to all platform specific files, otherwise the compilation would fail on those platforms.

rphillips · 2024-04-16T14:44:39Z

types/options.go

+	}
+
+	if _, err := os.Stat(defaultDropInConfigDir); err != nil && os.IsNotExist(err) {
+		return defaultStoreOptions, err


Do you mean to return an err here, since the file does not exist?

yes, you are right. even if the defaultDropInConfigDir does not exist then there is no need to return the error. Thanks for point it out. I will update the changes.

rphillips · 2024-04-16T14:45:40Z

types/options.go

+
+		return nil
+	})
+	if err != nil {


return filepath.Walk(...)... The if statements are not necessary.

Thanks. updated.

rphillips · 2024-04-16T15:05:47Z

types/options.go

+		if info.IsDir() {
+			return nil
+		}
+


Nice update. Should check for file extension here.

But a text file without a .conf or .toml can also contain a valid drop-in config. For any reason the content is not valid (irrespective of the extension) it will fail in ReloadConfigurationFile below while parsing the toml.

Hmm. We support a file without an extension?

as long as the content of the file is valid, it shouldn't matter, right?

crio doesn't enforce the extension either - https://github.com/cri-o/cri-o/blob/256fda5ac98de1d67e26133d0b25df2a74e2ebd2/pkg/config/config.go#L776

Usually there is a filter, but this is fine.

one risk is temporary files written by editor and the like. It mostly only is an issue if we're reloading automatically. If it's manually triggered (like here) then the user usually has written and edited the editor before the reload happens...

I think requiring a .conf or .toml extension could be a nice change both here and cri-o. @sohankunkerkar added a filter for the kubelet's drop-in work

Yes require a .conf extension, that follows what we have done with other dropin files.

Thanks @rphillips @haircommander @rhatdan for this feedback. I have updated the code to select files with .conf extension only and also updated corresponding unit test to verify that.

rhatdan · 2024-04-20T10:30:55Z

Could you open a [WIP] PR against Podman to test this feature, and make sure it does not break anything in it's ci/cd system.

mtrmac · 2024-04-22T19:54:40Z

types/options.go

+		}
+
+		// Load drop-in options from the current file
+		err = ReloadConfigurationFile(path, baseOptions)


Very quickly skimming the implementation of ReloadConfigurationFile, it is not designed to merge things at all; it starts with *storeOptions = StoreOptions{}.

A reminder: ^^^. I can’t see how this works at all. What am I missing?

The included unit test TestMergeConfigFromDirectory gives me the impression that the change is working as expected, but maybe I am missing something. Can you suggest a change to that test which can bring out the failure you are anticipating?

The way I am looking at it is, for every drop-in config file we are initializing *storeOptions = StoreOptions{} and then merging it with the earlier values. But again, there is a good chance I might have overlooked something. A test case would definitely help me understand it better.

That test is not testing merging at all.

As just a starting point (I don’t understand all of the c/storage option loading code, too many functions and too many code paths and too many hooks; I’m scared of that):

diff --git a/types/options_test.go b/types/options_test.go index bd5a70c8b..92d35dc6e 100644 --- a/types/options_test.go +++ b/types/options_test.go @@ -227,8 +227,7 @@ graphroot = 'temp/graph2'`, runroot = 'should/ignore' graphroot = 'should/ignore'`, `[storage] -runroot = 'temp/run3' -graphroot = 'temp/graph3'`, +runroot = 'temp/run3'`, `[storage] runroot = 'temp/run4' graphroot = 'temp/graph4'`, @@ -242,14 +241,16 @@ graphroot = 'temp/graph4'`, // Set base options baseOptions := StoreOptions{ - RunRoot: "initial/run", - GraphRoot: "initial/graph", + RunRoot: "initial/run", + GraphRoot: "initial/graph", + TransientStore: true, } // Expected results after merging configurations from only .conf files expectedOptions := StoreOptions{ - RunRoot: "temp/run3", // Last .conf file (config3.conf) read overrides earlier values - GraphRoot: "temp/graph3", + RunRoot: "temp/run3", // Last .conf file (config3.conf) read overrides earlier values + GraphRoot: "temp/graph2", + TransientStore: true, } // Run the merging function @@ -258,7 +259,5 @@ graphroot = 'temp/graph4'`, t.Fatalf("Error merging config from directory: %v", err) } - if baseOptions.RunRoot != expectedOptions.RunRoot || baseOptions.GraphRoot != expectedOptions.GraphRoot { - t.Errorf("Expected RunRoot to be %q and GraphRoot to be %q, got RunRoot %q and GraphRoot %q", expectedOptions.RunRoot, expectedOptions.GraphRoot, baseOptions.RunRoot, baseOptions.GraphRoot) - } + assert.DeepEqual(t, expectedOptions, baseOptions) }

Thanks @mtrmac .

/hold until we fix it.

harche · 2024-04-24T12:49:45Z

Could you open a [WIP] PR against Podman to test this feature, and make sure it does not break anything in it's ci/cd system.

@rhatdan I opened containers/podman#22484. Most of the jobs were green, apart from the one for the mac. I am not able to trigger the retest of that job, it needs /ok-to-test label.

rhatdan · 2024-04-24T13:23:14Z

@nalind @giuseppe @saschagrunert @vrothberg PTAL

giuseppe · 2024-04-24T14:14:57Z

types/options.go

+
+	if _, err := os.Stat(defaultDropInConfigDir); err == nil {
+		// The directory exists, so merge the configuration from this directory
+		err = mergeConfigFromDirectory(&baseOptions, defaultDropInConfigDir)


I am not sure we should hardcode a specific directory, but instead try to load each file from the $STORAGE_CONF + ".d" directory, where $STORAGE_CONF is whatever file we decided to use

Thanks @giuseppe, that's a good idea. I modified the code to set the drop-in config dir to storage conf path + .d

types/options.go

saschagrunert · 2024-04-24T14:53:45Z

types/options.go

+	// defaultOverrideConfigFile path to override the default system wide storage.conf file
+	defaultOverrideConfigFile = "/etc/containers/storage.conf"
+
+	// defaultDropInConfigDir path to the folder containing drop in config files
+	defaultDropInConfigDir = "/etc/containers/storage.conf.d"


Let's make those const if they're not variable.

Because of this change #1885 (comment), we may not be able to make it a const.

Signed-off-by: Harshal Patil <harpatil@redhat.com>

mtrmac · 2024-04-26T18:07:38Z

types/options.go

@@ -519,8 +570,13 @@ func ReloadConfigurationFile(configFile string, storeOptions *StoreOptions) erro
 		storeOptions.PullOptions = config.Storage.Options.PullOptions
 	}

-	storeOptions.DisableVolatile = config.Storage.Options.DisableVolatile
-	storeOptions.TransientStore = config.Storage.TransientStore
+	if config.Storage.Options.DisableVolatile {


How do I set disableFolatile = false in the drop-in?

It seems to me that all of this is going to require some other approach, and probably very detailed testing.

Or, alternatively, be very strict and very restrictive about which options are supported in drop-ins, support/test only those, and hard-fail if any others are present. Then we can add support for more options over time … assuming it’s sufficiently certain adding the others will actually be possible in the future.

Agreed, we need to call meta.Keys() to pull out the keys that are being set, then selectively set those values on a storeOptions instance.

Thanks @rphillips. If I get the toml.DecodeFile(configFile, &config).Keys() and update the values only if the key is defined in a drop-in config then it seem to fix the issue.

keySet := make(map[string]bool) for _, key := range meta.Keys() { keySet[key.String()] = true } if _, ok := keySet["storage.transient_store"]; ok { storeOptions.TransientStore = config.Storage.TransientStore }

cc @mtrmac @haircommander @sohankunkerkar

The updated snippet of the included test looks like this,

contents := []string{ `[storage] runroot = 'temp/run1' graphroot = 'temp/graph1' transient_store = false`, `[storage] runroot = 'temp/run2' graphroot = 'temp/graph2'`, `[storage] runroot = 'should/ignore' graphroot = 'should/ignore'`, `[storage] runroot = 'temp/run3'`, `[storage] runroot = 'temp/run4' graphroot = 'temp/graph4'`, } for i, fileName := range fileNames { filePath := filepath.Join(tempDir, fileName) if err := os.WriteFile(filePath, []byte(contents[i]), 0o666); err != nil { t.Fatalf("Failed to write to temp file: %v", err) } } // Set base options baseOptions := StoreOptions{ RunRoot: "initial/run", GraphRoot: "initial/graph", TransientStore: true, } // Expected results after merging configurations from only .conf files expectedOptions := StoreOptions{ RunRoot: "temp/run3", // Last .conf file (config3.conf) read overrides earlier values GraphRoot: "temp/graph2", TransientStore: false, }

@haircommander pointed me to how kubelet handles drop-in configs,

https://github.com/kubernetes/kubernetes/blob/02365ecec1cb1ddf993cf4ed12407737db950cea/cmd/kubelet/app/server.go#L322-L332

One more option is to use mergo to merge the StorageOptions.

I sorta worry about mergo setting options to false. Did we get past this issue in MCO where values being set to false wouldn’t get set?

Yeah the mergo option didn't work. It does set the option to false as you predicated.

code is at, harche@f0b92df#diff-5c0b7c9ae084e6c4613c438d00cede106862b604f184a331df1c3806cfbd11d2R164

So it seems like our only option is to use meta.Keys()?

BTW, which MCO issue are you talking about?

https://issues.redhat.com/browse/OCPBUGS-14399 I remember we had to hack around it.

openshift-ci bot added the do-not-merge/hold label Apr 12, 2024

harche marked this pull request as draft April 12, 2024 17:50

openshift-ci bot added the do-not-merge/work-in-progress label Apr 12, 2024

haircommander reviewed Apr 12, 2024

View reviewed changes

harche force-pushed the dropin branch 2 times, most recently from cce7ffd to b13d446 Compare April 12, 2024 18:49

haircommander reviewed Apr 12, 2024

View reviewed changes

rphillips reviewed Apr 16, 2024

View reviewed changes

harche force-pushed the dropin branch from b13d446 to 6ee0c3f Compare April 16, 2024 14:59

rphillips reviewed Apr 16, 2024

View reviewed changes

harche force-pushed the dropin branch 5 times, most recently from d56119b to 697c412 Compare April 19, 2024 15:05

harche marked this pull request as ready for review April 19, 2024 15:05

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 19, 2024

mtrmac reviewed Apr 22, 2024

View reviewed changes

giuseppe reviewed Apr 24, 2024

View reviewed changes

saschagrunert reviewed Apr 24, 2024

View reviewed changes

harche force-pushed the dropin branch 2 times, most recently from 7ca8f82 to 9192784 Compare April 26, 2024 17:54

Add support for drop-in config

25f1558

Signed-off-by: Harshal Patil <harpatil@redhat.com>

harche force-pushed the dropin branch from 9192784 to 25f1558 Compare April 26, 2024 17:54

mtrmac reviewed Apr 26, 2024

View reviewed changes

Add support for drop-in config #1885

Are you sure you want to change the base?

Add support for drop-in config #1885

Conversation

harche commented Apr 12, 2024 • edited

openshift-ci bot commented Apr 12, 2024

harche commented Apr 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harche Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Apr 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harche commented Apr 24, 2024

rhatdan commented Apr 24, 2024

giuseppe Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harche Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harche commented Apr 12, 2024 •

edited

harche Apr 16, 2024 •

edited

giuseppe Apr 24, 2024 •

edited

harche Apr 30, 2024 •

edited