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

store: prefer /etc/containers/storage.conf #1471

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Jan 16, 2023

when running in rootful mode, if it is present, prefer the override path /etc/containers/storage.conf instead of using the default storage.conf provided by the package under the /usr/share/containers/ directory.

Signed-off-by: Giuseppe Scrivano [email protected]

@giuseppe giuseppe marked this pull request as draft January 17, 2023 09:55
@giuseppe giuseppe force-pushed the default-override-for-rootless branch from c4e9b9d to 8358880 Compare January 17, 2023 10:01
@giuseppe giuseppe marked this pull request as ready for review January 17, 2023 10:01
when running in rootful mode, if it is present, prefer the override path
/etc/containers/storage.conf instead of using the default storage.conf
provided by the package under the /usr/share/containers/ directory.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the default-override-for-rootless branch from 8358880 to f64e1bd Compare January 17, 2023 10:02
Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Member

rhatdan commented Jan 17, 2023

@rhatdan
Copy link
Member

rhatdan commented Jan 17, 2023

@danishprakash PTAL

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Doesn’t this need documentation?


Does this actually work? At least Podman callers use podman/pkg/rootless, which is a simple EUID check, not handling _CONTAINERS_ROOTLESS_UID. I would expect Podman to set !rootless inside a user namespace, then.

OTOH c/storage itself, in DefaultStoreOptionsAutoDetectUID, does check for _CONTAINERS_ROOTLESS_UID.

Is that intentional? If so, how does this work?

Please write these things down. What is the intended semantics of this rootless parameter? Then we can discuss whether it is being set correctly by callers; without that we have nothing to really discuss.

@giuseppe
Copy link
Member Author

rootless is an argument to DefaultConfigFile so how it is calculated it is outside the scope of this function.

I agree the code that deals with configuration files is a complete mess and we handle it from so many different places that honestly I don't know how it all is supposed to fit. It took me a while to understand what is going with the issue I was looking at.

What this PR changes is to honor /etc/containers/storage.conf when it exists instead of using /usr/containers/storage.conf as the base for the default values, because when we do that, we lose all the settings configured in /etc/containers/storage.conf.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 17, 2023

rootless is an argument to DefaultConfigFile so how it is calculated it is outside the scope of this function.

I mean, depending on what this parameter means, this could either be a correct PR or a clear bug vs. the stated intent.

I can’t see how propagating the current undefined state is sustainable, and I’m personally not interested. (There are enough other people on Cc:, and I won’t stand in the way of merging if they like it.)

@rhatdan
Copy link
Member

rhatdan commented Jan 17, 2023

Lets merge and then work on fixing the documentation. I think the documentation does state that /etc/containers/storage.conf overrides /usr/share/containers/containers.conf.

@rhatdan rhatdan merged commit d5792d2 into containers:main Jan 17, 2023
@lukasmrtvy
Copy link

Is possible to backport this for 4.4.0 ? /usr/share/containers/ filesystem path is not writable in FCOS and AFAIK theres no other way, how to override storage.conf for API.

@rhatdan
Copy link
Member

rhatdan commented Jan 20, 2023

Yes podman 4.4 will get the latest update containers/storage.

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

Successfully merging this pull request may close these issues.

5 participants