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

use cached containers.conf #16238

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

vrothberg
Copy link
Member

Use Default() instead of re-loading containers.conf.

[NO NEW TESTS NEEDED] as it's not a functional change.

Fixes: containers/common/issues/1200
Signed-off-by: Valentin Rothberg [email protected]

Does this PR introduce a user-facing change?

Fix a bug where the containers.conf files are reloaded redundantly.

@Luap99 @alexlarsson PTAL
I did not measure performance impacts.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 20, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2022
@vrothberg
Copy link
Member Author

@baude @rhatdan PTAL

@vrothberg
Copy link
Member Author

Performance benefits on my machine are in ns but I assume that's because containers.conf is mostly commented out. Parsing a custom containers.conf would take longer where the benefits would be more visible.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@vrothberg
Copy link
Member Author

An unexpected armada of errors.

@vrothberg
Copy link
Member Author

@Luap99 any suspicion what's causing the farts?

@vrothberg
Copy link
Member Author

OK, I think I got it. Must have been the volume path.

@vrothberg
Copy link
Member Author

@containers/podman-maintainers PTAL

@baude
Copy link
Member

baude commented Oct 20, 2022

/hold
/lgtm

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2022
@mheon
Copy link
Member

mheon commented Oct 20, 2022

Tests are very angry

@vrothberg
Copy link
Member Author

Tests are very angry

Down to one failing test. I'll tackle that tomorrow o/

@rhatdan
Copy link
Member

rhatdan commented Oct 20, 2022

I have an alternative to this.
#16242 Might want to combine them.

@vrothberg
Copy link
Member Author

I really don't like how we are using the pointer to the default config and mess with it. There's still some issue when creating the reset runtime which is painful to debug.

@vrothberg
Copy link
Member Author

OK, I see what's going on. The static dir (i.e., root) is being joined with "libpod" later on which in turn changes the default config. We really need to separate the concerns of "displaying defaults on the CLI", "loading a default config", "storing and altering a config in the libpod runtime". At the moment, we conflate them into one and I think that's too risky to cause hard-to-debug issues in the future.

@vrothberg
Copy link
Member Author

Updated the PR to not use the default config for storing the CLI values.

I will have a closer look at how we pass around the config.Config in the stack in another commit; I don't want to start yak shaving.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2022
@vrothberg
Copy link
Member Author

OK, I started shaving the yak. I thought this will be a quick fix.

Use `Default()` instead of re-loading containers.conf.

Also rework how the containers.conf objects are handled for parsing the
CLI.  Previously, we were conflating "loading the defaults" with
"storing values from the CLI" with "libpod may further change fields"
which ultimately led to various bugs and test failues.

To address the issue, separate the defaults from the values from the CLI
and properly name the fields to make the semantics less ambiguous.

[NO NEW TESTS NEEDED] as it's not a functional change.

Fixes: containers/common/issues/1200
Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
Copy link
Member Author

I reworked PodmanConfig a bit to make it less ambiguous which values come from the defaults loaded from containers.conf and which ones came from the CLI. For that purpose, there are two fields now.

@vrothberg
Copy link
Member Author

Search flake is kicking in hard :( I opened #16248 but think I need to tackle that here as well.

There's no guarantee that the searched image will be returned, so only
make sure that "alpine" is mentioned somewhere.

Fixes: containers#16248
Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
Copy link
Member Author

@baude @rhatdan can you take another look? I restarted the flake but tests are green now.

@vrothberg
Copy link
Member Author

@giuseppe @Luap99 PTAL

@rhatdan
Copy link
Member

rhatdan commented Oct 24, 2022

LGTM
/hold cancel
@containers/podman-maintainers PTAL

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2022
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

I am not sure why we would need two configs? This will increase memory usage, unsafe.Sizeof(*config) returns 1704 bytes right now, with this change we would double it.

Also I don't like that the original field is also renamed since this creates a unnecessary large diff IMO.

@vrothberg
Copy link
Member Author

vrothberg commented Oct 24, 2022

I am not sure why we would need two configs? This will increase memory usage, unsafe.Sizeof(*config) returns 1704 bytes right now, with this change we would double it.

Did you read the commit message? I personally prefer a couple of bytes over messing with the default config. If so, please give actionable proposals. "I don't like XY" is not something I can work with.

There's an inherent design flaw in c/common/pkg/config to expose the internal data (i.e., config.Config) which is used for parsing. That's not possible with registries.conf because the package clearly separates the concerns of reading/parsing the config (in an internal package) from the runtime data that external users/callers are allowed to use.

Podman ran exactly into this trap as it was abusing the default config for storing CLI values. A clearer separation of concerns would be to not use a config.Config at all internally but I don't aim for perfection; one issue at a time.

Also I don't like that the original field is also renamed since this creates a unnecessary large diff IMO.

Please read the commit message. I don't care much about the diff but care about error potentials.

@Luap99
Copy link
Member

Luap99 commented Oct 24, 2022

I try to understand this but I think I missing something. I see that we overwrite the config fields with cli options and you fix this by using another empty Config for that. But what is the point of that when in libpod.NewRuntime() you use the default config, which is then modified in newRuntimeFromConfig() with the values from the empty config. Therefore the default is still changed so I would assume we need a copy there as well? Or maybe it is better to just make config.Default() return a copy so that callers cannot modify the internal data.

@vrothberg
Copy link
Member Author

I try to understand this but I think I missing something. I see that we overwrite the config fields with cli options and you fix this by using another empty Config for that. But what is the point of that when in libpod.NewRuntime() you use the default config, which is then modified in newRuntimeFromConfig() with the values from the empty config. Therefore the default is still changed so I would assume we need a copy there as well? Or maybe it is better to just make config.Default() return a copy so that callers cannot modify the internal data.

There is definitely more to improve. I really don't like how the config.Config is abused and think that Podman should only use the Getter/Accessor API instead of reading fields from config.Confg. But that is beyond what I intended to do with this PR. Looking at the commit message:

we were conflating "loading the defaults" with "storing values from the CLI" with "libpod may further change fields"

This PR stops conflating "loading the defaults" with "storing values from the CLI". It does not stop "libpod may further change fields".

I can volunteer to have another look this week at the issue. But I think we should merge the PR as the issue is now at least more obvious than before. I was surprised by how many things fell apart after the initial change.

@Luap99
Copy link
Member

Luap99 commented Oct 24, 2022

Ok fine for me, this is definitely an improvement but we should keep an eye on this because it is super fragile.
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2022
@vrothberg
Copy link
Member Author

Ok fine for me, this is definitely an improvement but we should keep an eye on this because it is super fragile. /lgtm

Thanks! I agree and will take another look this week.

@openshift-merge-robot openshift-merge-robot merged commit 1b94470 into containers:main Oct 24, 2022
@vrothberg vrothberg deleted the fix-common-1200 branch October 24, 2022 12:33
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

containers.conf should be cached
6 participants