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

Refactor storage.conf loading #1469

Open
dcermak opened this issue Jan 16, 2023 · 10 comments
Open

Refactor storage.conf loading #1469

dcermak opened this issue Jan 16, 2023 · 10 comments

Comments

@dcermak
Copy link
Contributor

dcermak commented Jan 16, 2023

I would like to propose that we refactor where & when storage.conf is loaded into the storageOptions structs. Currently it happens in various places, it depends on whether we are using rootless mode or not and most of the value copying from the loaded values into the structs is done by hand on a field by field basis. This leads to subtle bugs that required amendments like #1468 (and ftr, this one is 100% my fault, because I didn't test the changes from #1460 properly).

I would propose that we refactor the config loading, so that it happens in one place only to reduce the cognitive load when dealing with the config loading.

@vrothberg
Copy link
Member

SGTM

@rhatdan @giuseppe @nalind WDYT?

@rhatdan
Copy link
Member

rhatdan commented Jan 18, 2023

I think that would be a good idea.

@giuseppe
Copy link
Member

I agree, this part of the code is quite messy and hard to follow

@danishprakash
Copy link

Looks like there's a consensus on this, I'd like to take a shot at this If that sounds okay.

@neverpanic
Copy link

This seems like a good idea, given that there seems to be a bug related to the order in which /etc/containers/storage.conf and /usr/share/containers/storage.conf are processed. See https://bodhi.fedoraproject.org/updates/FEDORA-2023-a0f754c701#comment-2853529.

@Meister1593
Copy link

#1587 related

@asteven
Copy link
Contributor

asteven commented Aug 9, 2023

It would be nice to have a /etc/containers/storage.conf.d/ directory for overrides.
Similar structures already exists for e.g for /etc/containers/registries.conf.d.

This makes it so much easier for config management or Dockerfile authors.

Drop a override file in a folder instead of parsing/reading/setting a key:value in a file that changes on every package update.

e.g. my current primary use case:
set driver=zfs in /etc/containers/storage.conf

That's easy to do with config mangement tools like cdist or ansible.
But it's a PITA to do from a Dockerfile.

@rhatdan
Copy link
Member

rhatdan commented Aug 9, 2023

I think this would be a good idea, the issue is no one has stepped forward to do the work.

@danishprakash
Copy link

I can start a draft PR and perhaps we can continue this discussion there while consolidating all the ideas shared so far. Wdyt?

@vrothberg
Copy link
Member

vrothberg commented Aug 9, 2023

Thanks, @danishprakash!

I think it's best to first open a separate issue and discuss how this should look like. Designing inside a PR can be very time consuming, especially for those driving the PR.

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

8 participants