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.

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

proposal: refactor loading storage options #2098

Open
danishprakash opened this issue Sep 19, 2024 · 0 comments
Open

proposal: refactor loading storage options #2098

danishprakash opened this issue Sep 19, 2024 · 0 comments

Comments

@danishprakash
Copy link

Contd. from #1469

Background - loading store options from storage.conf seems to be a bit unintuitive at present. For instance, differentiating between rootless and rootful loading is not apparent. All of the loading seems to happen within types/options.go but it'd be good to refactor this piece to make it more readable and maintainable.


@dcermak and I sat on this for a while and tried to reason out the problem and possible solution. We're creating this issue to discuss the approach to refactoring if at all one is desired.

Current

  • The entrypoint to config loading seems to be DefaultStoreOptions() which leads to loadStoreOptions
  • Moving further: loadStoreOptionsFromConfFile takes care of virtually everything
    • Loading defaults
    • Loading rootless options
    • Loading all other options
    • Various validation checks
      ...

Proposed

We can consolidate this into a single entrypoint (as before) but preferably branch out depending upon whether we're rootful or rootless, for instance:

// GetStoreOptions will act as the entrypoint
// Alternatively, we'll have `GetDefaultStoreOptions`
// that would *only* return default options for
// special use-cases i.e. `GetDefaultMountOptions`
func GetStoreOptions() (StoreOptions, error) {
	var storeOpts StoreOptions
	if unshare.IsRootless() {
		storeOpts = getRootlessDefaults()
	} else {
		storeOpts = getRootfulDefaults()
	}

	return loadStoreOptionsFromConfFile(storeOpts)
}

Branching out to load default store options conditionally and then abstracting away file-based config loading to loadStoreOptionsFromConfFile.

func loadStoreOptionsFromConfFile(storeOpts *StoreOptions) (*StoreOptions, error) {
	// Load distro-shipped or default configuration
	// onto the storeOpts first
	storeOpts = loadStoreOptionsFromUsrShare(storeOpts)

	// Override storeOpts with admin modified
	// configuration if any
	storeOpts = loadStoreOptionsFromEtc(storeOpts)

	// Finally, if we're rootless, override options
	// with those set in ~/.config/containers/storage.conf
	if unshare.IsRootless() {
		storeOpts = loadStoreOptionsFromUserHome(storeOpts)
	}

	return storeOpts
}

Here, read the config from (at least 3 config files) in the following order:

  1. (loaded) defaults
  2. Load configuration shipped with the distro from /usr/share/containers/storage.conf
  3. Load configuration added/modified by administrator from /etc/containers/storage.conf
  4. Finally, override with anything that the user has set in ~/.config/containers/storage.conf

This is still quite high-level and we're not yet aware of any issues we might run into with the proposed changes, but hopefully enough to get the discussion started.

Auxiliary changes

  • loadStoreOptionsFromConfFile is also loading defaults, not clear from the signature
  • types.Options() is intended to be only used for fetching default store options but the signature suggests otherwise.
  • Why aren't we using types.DefaultStoreOptions and using storage.DefaultStoreOptions from within podman, readability?
  • Preferably attach all operations on StoreOptions on the struct itself. Given we should have just one instance of the options, it'll make it easier to override attributes this way.
  • In loadStoreOptionsFromConfFile above, independently get a slice of paths that want to iterate on, in order and have a generic loadFromFile(path, storeOpts) to reduce redundancy and potentially duplication.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant