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

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.

How do I set disableVolatile = 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,
	}

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.

systemd has definitely benefited from a single consistent config file format with consistent semantics and merge support.

Explicitly iterating over the keys seems like it'd work, but another thing to do is to use nullable values for all entries. This is much more elegant in Rust of course...here's how I recently did a basic "mergeable toml" in bootc:

https://github.com/containers/bootc/blob/f152bfe8da47c3bfbafca33c2142d10611375fa3/lib/src/install/config.rs#L47 (Note each entry is Option<T>) and there's a Mergeable trait which can be called recursively e.g. https://github.com/containers/bootc/blob/f152bfe8da47c3bfbafca33c2142d10611375fa3/lib/src/install/config.rs#L106