fix handling of static/volume dir #20088

vrothberg · 2023-09-21T12:52:51Z

The processing and setting of the static and volume directories was scattered across the code base (including c/common) leading to subtle errors that surfaced in #19938.

There were multiple issues that I try to summarize below:

c/common loaded the graphroot from c/storage to set the defaults for static and volume dir. That ignored Podman's --root flag and surfaced in podman with custom static_dir slow to tear-down containers #19938 and other bugs. c/common does not set the defaults anymore which gives Podman the ability to detect when the user/admin configured a custom directory (not empty value).
When parsing the CLI, Podman (ab)uses containers.conf structures to set the defaults but also to override them in case the user specified a flag. The --root flag overrode the static dir which is wrong and broke a couple of use cases. Now there is a dedicated field for in the "PodmanConfig" which also includes a containers.conf struct.
The defaults for static and volume dir and now being set correctly and adhere to --root.
The CONTAINERS_CONF_OVERRIDE env variable has not been passed to the cleanup process. I believe that all env variables should be passed to conmon to avoid such subtle bugs.

Overall I find that the code and logic is scattered and hard to understand and follow. I refrained from larger refactorings as I really just want to get #19938 fixed and then go back to other priorities.

Fixes: #19938

Does this PR introduce a user-facing change?

Fix a bug where setting the static or volume directory in containers.conf would lead to cleanup errors.

vrothberg · 2023-09-21T12:53:22Z

Marked as a draft for now as it's vendoring containers/common#1660. Once green and Acked we can merge containers/common#1660 and do the dance.

pkg/domain/infra/runtime_libpod.go

packit-as-a-service · 2023-09-21T13:12:34Z

Cockpit tests failed for commit 020c8dd6e96d9e95a5144c2444e229ddf7056abb. @martinpitt, @jelly, @mvollmer please check.

packit-as-a-service · 2023-09-21T13:21:43Z

Cockpit tests failed for commit 6156986252c282035bcddd2056dea7483a22350d. @martinpitt, @jelly, @mvollmer please check.

vrothberg · 2023-09-21T13:48:21Z

@mheon PTAL

martinpitt · 2023-09-21T13:55:59Z

Thanks @vrothberg for fixing. Tests previously failed on

 podman rmi --all
Error: creating runtime static files directory "": mkdir : no such file or directory

It's a bit curious that no other test picked this up. The test setup script doesn't do anything super weird though. Anyway, water under the bridge!

vrothberg · 2023-09-21T14:01:26Z

Thanks, @martinpitt ! The mkdir issue has been fixed in the meantime.

test/system/030-run.bats

mheon · 2023-09-21T14:59:46Z

cmd/podman/root.go

@@ -540,7 +540,7 @@ func rootFlags(cmd *cobra.Command, podmanConfig *entities.PodmanConfig) {
 		_ = pFlags.MarkHidden(networkBackendFlagName)

 		rootFlagName := "root"
-		pFlags.StringVar(&podmanConfig.ContainersConf.Engine.StaticDir, rootFlagName, podmanConfig.ContainersConfDefaultsRO.Engine.StaticDir, "Path to the root directory in which data, including images, is stored")
+		pFlags.StringVar(&podmanConfig.GraphRoot, rootFlagName, "", "Path to the graph root directory where images, containers, etc. are stored")


Are we concerned about this breaking folks? Setting --root is now doing something very different than it did before.

I honestly don't know. This stuff is really broken :(

I think we're good to go. It essentially overrode the graph root before as well (that's how it was being used).

vrothberg · 2023-09-22T08:43:54Z

libpod/oci_conmon_common.go

@@ -1328,6 +1328,9 @@ func (r *ConmonOCIRuntime) configureConmonEnv(runtimeDir string) []string {
 	if conf, ok := os.LookupEnv("CONTAINERS_CONF"); ok {
 		env = append(env, fmt.Sprintf("CONTAINERS_CONF=%s", conf))
 	}
+	if conf, ok := os.LookupEnv("CONTAINERS_CONF_OVERRIDE"); ok {


@mheon do you recall why we're not passing all env variables to conmon? I feel like we should do this since there is a growing number of those which I do not see being passed down.

I think it dates back to before we forked off Conmon, so this is probably older than my involvement in the project. I don't see a good reason not to forward on full environment?

Thanks! I will open a follow-up PR 👍

test/system/030-run.bats

vrothberg · 2023-09-22T10:46:02Z

OK, this is ready from end ✔️

Luap99 · 2023-09-22T11:01:40Z

If this is fully rebased on the latest c/common I expect the machine tests to fail: #20094

vrothberg · 2023-09-22T11:03:21Z

If this is fully rebased on the latest c/common I expect the machine tests to fail: #20094

This is the 2nd time in 24 hours that a commit in common got merged and broke Podman CI :( I'll take a look at fixing the machine tests.

vrothberg · 2023-09-22T11:10:33Z

Repushed and commented out the failures. I let @ashley-cui adjust the tests once this PR here got in.

Luap99

LGTM

openshift-ci · 2023-09-22T11:56:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, 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:

~~OWNERS~~ [Luap99,vrothberg]

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

vrothberg · 2023-09-22T12:30:25Z

sqlite behaves differently than boltdb :(

Not sure whether I can get this fixed in time today, apologies.

vrothberg · 2023-09-22T13:19:46Z

Opened containers/common#1663 to revert my two commits, so I can work on the remaining issues in peace.

vrothberg · 2023-09-22T13:35:22Z

sqlite behaves differently than boltdb :(

Found the issue. The problem was that the sqlite DB was always placed on the graph root and not on the static dir. Now, since the static dir can really be configured it blew up.

ashley-cui · 2023-09-22T14:02:01Z

Sorry about that, opened #20101 and will rebase/adjust once this PR goes in.

vrothberg · 2023-09-22T14:03:22Z

Sorry about that, opened #20101 and will rebase/adjust once this PR goes in.

Thanks! In case this PR here fails again, feel free to just merge yours and I will rebase this one here on Monday :)

vrothberg · 2023-09-22T15:47:30Z

The fight is still not over. Transient store e2e tests are crying. I'll calm them Monday morning.

vrothberg · 2023-09-25T09:06:48Z

Hitting #20119 now

The processing and setting of the static and volume directories was scattered across the code base (including c/common) leading to subtle errors that surfaced in containers#19938. There were multiple issues that I try to summarize below: - c/common loaded the graphroot from c/storage to set the defaults for static and volume dir. That ignored Podman's --root flag and surfaced in containers#19938 and other bugs. c/common does not set the defaults anymore which gives Podman the ability to detect when the user/admin configured a custom directory (not empty value). - When parsing the CLI, Podman (ab)uses containers.conf structures to set the defaults but also to override them in case the user specified a flag. The --root flag overrode the static dir which is wrong and broke a couple of use cases. Now there is a dedicated field for in the "PodmanConfig" which also includes a containers.conf struct. - The defaults for static and volume dir and now being set correctly and adhere to --root. - The CONTAINERS_CONF_OVERRIDE env variable has not been passed to the cleanup process. I believe that _all_ env variables should be passed to conmon to avoid such subtle bugs. Overall I find that the code and logic is scattered and hard to understand and follow. I refrained from larger refactorings as I really just want to get containers#19938 fixed and then go back to other priorities. containers/common#1659 broke three pkg/machine tests. Those have been commented out until getting fixed. Fixes: containers#19938 Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2023-09-25T14:22:05Z

@mheon @baude this should finally be green in a jiffy. Please take a look. I did not change existing tests, so I am fairly confident it's not introducing a regression but this code is quite fragile.

baude · 2023-09-25T14:52:38Z

code LGTM

mheon · 2023-09-25T16:26:02Z

/lgtm

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 21, 2023

vrothberg commented Sep 21, 2023

View reviewed changes

pkg/domain/infra/runtime_libpod.go Outdated Show resolved Hide resolved

vrothberg mentioned this pull request Sep 21, 2023

pkg/config: do NOT set StaticDir and VolumeDir containers/common#1660

Merged

vrothberg force-pushed the fix-19938 branch 3 times, most recently from d6c569a to 4bd5ca0 Compare September 21, 2023 13:11

edsantiago reviewed Sep 21, 2023

View reviewed changes

test/system/030-run.bats Show resolved Hide resolved

mheon reviewed Sep 21, 2023

View reviewed changes

vrothberg force-pushed the fix-19938 branch from 4bd5ca0 to a4b1cf1 Compare September 22, 2023 08:42

vrothberg commented Sep 22, 2023

View reviewed changes

test/system/030-run.bats Outdated Show resolved Hide resolved

vrothberg force-pushed the fix-19938 branch from a4b1cf1 to 712a9db Compare September 22, 2023 10:45

vrothberg marked this pull request as ready for review September 22, 2023 10:45

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2023

vrothberg mentioned this pull request Sep 22, 2023

Default machine CPUs to Cores/2 containers/common#1659

Merged

vrothberg force-pushed the fix-19938 branch from 712a9db to ba70184 Compare September 22, 2023 11:09

vrothberg force-pushed the fix-19938 branch from ba70184 to 35ad588 Compare September 22, 2023 11:25

Luap99 approved these changes Sep 22, 2023

View reviewed changes

vrothberg mentioned this pull request Sep 22, 2023

revert two commits for containers/podman/pull/20088 containers/common#1663

Closed

vrothberg force-pushed the fix-19938 branch from 35ad588 to 422dbe6 Compare September 22, 2023 13:34

ashley-cui mentioned this pull request Sep 22, 2023

Adjust machine CPU tests #20101

Merged

vrothberg force-pushed the fix-19938 branch from 422dbe6 to a1519a4 Compare September 25, 2023 07:46

vrothberg force-pushed the fix-19938 branch from a1519a4 to 5b285ed Compare September 25, 2023 12:13

vrothberg force-pushed the fix-19938 branch from 5b285ed to 6293ec2 Compare September 25, 2023 12:14

openshift-ci bot assigned mheon Sep 25, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2023

openshift-merge-robot merged commit 2fef4c7 into containers:main Sep 25, 2023
97 checks passed

vrothberg deleted the fix-19938 branch September 26, 2023 07:06

Luap99 mentioned this pull request Dec 1, 2023

Error: database libpod root directory (staticdir) does not match our libpod root directory (staticdir) #20872

Closed

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 Dec 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix handling of static/volume dir #20088

fix handling of static/volume dir #20088

vrothberg commented Sep 21, 2023 •

edited

Loading

vrothberg commented Sep 21, 2023

packit-as-a-service bot commented Sep 21, 2023

packit-as-a-service bot commented Sep 21, 2023

vrothberg commented Sep 21, 2023

martinpitt commented Sep 21, 2023

vrothberg commented Sep 21, 2023

mheon Sep 21, 2023

vrothberg Sep 21, 2023

vrothberg Sep 22, 2023

vrothberg Sep 22, 2023

mheon Sep 25, 2023

vrothberg Sep 25, 2023

vrothberg commented Sep 22, 2023

Luap99 commented Sep 22, 2023

vrothberg commented Sep 22, 2023

vrothberg commented Sep 22, 2023

Luap99 left a comment

openshift-ci bot commented Sep 22, 2023

vrothberg commented Sep 22, 2023

vrothberg commented Sep 22, 2023

vrothberg commented Sep 22, 2023

ashley-cui commented Sep 22, 2023

vrothberg commented Sep 22, 2023

vrothberg commented Sep 22, 2023

vrothberg commented Sep 25, 2023

vrothberg commented Sep 25, 2023

baude commented Sep 25, 2023

mheon commented Sep 25, 2023

fix handling of static/volume dir #20088

fix handling of static/volume dir #20088

Conversation

vrothberg commented Sep 21, 2023 • edited Loading

Does this PR introduce a user-facing change?

vrothberg commented Sep 21, 2023

packit-as-a-service bot commented Sep 21, 2023

packit-as-a-service bot commented Sep 21, 2023

vrothberg commented Sep 21, 2023

martinpitt commented Sep 21, 2023

vrothberg commented Sep 21, 2023

mheon Sep 21, 2023

Choose a reason for hiding this comment

vrothberg Sep 21, 2023

Choose a reason for hiding this comment

vrothberg Sep 22, 2023

Choose a reason for hiding this comment

vrothberg Sep 22, 2023

Choose a reason for hiding this comment

mheon Sep 25, 2023

Choose a reason for hiding this comment

vrothberg Sep 25, 2023

Choose a reason for hiding this comment

vrothberg commented Sep 22, 2023

Luap99 commented Sep 22, 2023

vrothberg commented Sep 22, 2023

vrothberg commented Sep 22, 2023

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 22, 2023

vrothberg commented Sep 22, 2023

vrothberg commented Sep 22, 2023

vrothberg commented Sep 22, 2023

ashley-cui commented Sep 22, 2023

vrothberg commented Sep 22, 2023

vrothberg commented Sep 22, 2023

vrothberg commented Sep 25, 2023

vrothberg commented Sep 25, 2023

baude commented Sep 25, 2023

mheon commented Sep 25, 2023

vrothberg commented Sep 21, 2023 •

edited

Loading