In SQLite state, use defaults for empty-string checks #24775

mheon · 2024-12-05T13:42:21Z

As part of our database init, we perform a check of the current values for a few fields (graph driver, graph root, static dir, and a few more) to validate that Libpod is being started with a sane & sensible config, and the user's containers can actually be expected to work. Basically, we take the current runtime config and compare against values cached in the database from the first time Podman was run.

We've had some issues with this logic before this year around symlink resolution, but this is a new edge case. Somehow, the database is being loaded with the empty string for some fields (at least graph driver) which is causing comparisons to fail because we will never compare against "" for those fields - we insert the default value instead, assuming we have one.

Having a value of "" in the database largely invalidates the check so arguably we could just drop it, but what BoltDB did - and what SQLite does after this patch - is to use the default value for comparison instead of "". This should still catch some edge cases, and shouldn't be too harmful.

What this does not do is identify or solve the reason that we are seeing the empty string in the database at all. From my read on the logic, it must mean that the graph driver is explicitly set to "" in the c/storage config at the time Podman is first run and I'm not precisely sure how that happens.

Fixes #24738

Does this PR introduce a user-facing change?

Fixed a bug where Podman would fail to start due to a database configuration mismatch when certain fields were configured to the empty string.

mheon · 2024-12-05T13:46:07Z

Adding no new tests as we don't do database config tests like this in CI

Luap99 · 2024-12-05T13:57:32Z

The way to trigger this is create an empty storage.conf without driver then run podman to init db (you must reset the storage before of course)

$ podman ps
WARN[0000] The storage 'driver' option should be set in /home/pholzing/.config/containers/storage.conf. A driver was picked automatically. 
WARN[0000] The storage 'driver' option should be set in /home/pholzing/.config/containers/storage.conf. A driver was picked automatically. 
CONTAINER ID  IMAGE       COMMAND     CREATED     STATUS      PORTS       NAMES

# Follow the advice and add the driver, i.e. overlay
$ vi ~/.config/containers/storage.conf
$ cat ~/.config/containers/storage.conf
[storage]
driver="overlay"
$ podman ps
Error: database graph driver "" does not match our graph driver "overlay": database configuration mismatch

I think we can must test this somehow in CI.

I really must say they for every bug I dislike the validation logic more and more

baude · 2024-12-05T14:03:45Z

could we just use the podman cli and the --storage-driver string? I think if you did an e2e test you could skip the step to prepopulate the image cache and then add the storage driver option as a one-off ?

Luap99 · 2024-12-05T14:08:03Z

--storage-driver "" is ignored AFAIk so it must come from the config file I think. But that should not be much more difficult as we can use the CONTAINERS_STORAGE_CONF env to set it for a single test

Luap99 · 2024-12-10T16:41:27Z

test/e2e/info_test.go

@@ -291,4 +291,52 @@ var _ = Describe("Podman Info", func() {
 		Expect(info).ToNot(ExitCleanly())
 		podmanTest.StartRemoteService() // Start service again so teardown runs clean
 	})
+
+	It("podman startup: ensure database checks for graph driver accommodate empty string", func() {


Actually this will never work because all the e2e test pass all option via cli such as --storage-driver so it not matter what we set in the config.

I think this might have to be in test/system and you pass your own --root/-runroot/--tmpdir and just leave the driver to the storage .conf

github-actions · 2025-01-10T00:09:22Z

A friendly reminder that this PR had no activity for 30 days.

Luap99 · 2025-02-06T10:59:57Z

test/system/005-info.bats

+    # Force all custom directories so we don't pick up an existing database
+    CONTAINERS_STORAGE_CONF=$PODMAN_TMPDIR/storage.conf run_podman --root $PODMAN_TMPDIR/root --runroot $PODMAN_TMPDIR/runroot --tmpdir $PODMAN_TMPDIR/tmpdir info


use safe_opts=$(podman_isolation_opts ${PODMAN_TMPDIR})

and then just add $safe_opts as args here and below, see the test above as example.

Second storage prints an warning for the unset driver so you need to tell the test to ignore it

run_podman 0+w $safe_opts info require_warning "The storage 'driver' option should be set" \ "c/storage should warn on empty storage driver"

mheon · 2025-02-06T15:22:33Z

F40 rootless is using VFS by default. That seems odd.

Luap99 · 2025-02-06T16:36:40Z

F40 rootless is using VFS by default. That seems odd.

Because we should test all storage drivers we actively "support" so vfs for fedora -1 and overlayfs for 41/rawhide AFAIK to have good coverage.

That said we are using a custom --root here and custom storage conf so I don't follow with the env around it matters at all...

mheon · 2025-02-06T16:38:30Z

That was my thinking - everything that would configure storage driver is being overridden... Maybe I should do a system reset first to make sure there isn't a database at the location?

Luap99 · 2025-02-06T16:45:06Z

system reset would not help, podman_isolation_opts is just an echo an the tmpdir is unique for each test so there really should not be any podman content prior to running the first command.

The test environment creates /etc/containers/storage.conf with vfs so maybe the empty storage conf file does not actual overwrite it somehow despite c/storage complaining about it?

mheon · 2025-02-06T16:57:11Z

@nalind CONTAINERS_STORAGE_CONF should fully override the default /etc/containers/storage.conf and not layer on top of it, right?

nalind · 2025-02-06T18:05:38Z

It should, yes. Experimentally verified using ~~the~~ a sample file from the storage library's repository and env CONTAINERS_STORAGE_CONF=./types/storage_test.conf strace -f podman info as root.

Luap99 · 2025-02-10T16:17:26Z

test/system/005-info.bats

+    # Now add a valid graph driver to storage.conf
+    cat >$PODMAN_TMPDIR/storage.conf <<EOF
+[storage]
+driver="overlay"


Suggested change

driver="overlay"

driver="$(podman_storage_driver)"

That should use the right storage driver and turn into vfs on fedora -1, I am somewhat sure this is the right fix for the test instead of ignoring the vfs error

As part of our database init, we perform a check of the current values for a few fields (graph driver, graph root, static dir, and a few more) to validate that Libpod is being started with a sane & sensible config, and the user's containers can actually be expected to work. Basically, we take the current runtime config and compare against values cached in the database from the first time Podman was run. We've had some issues with this logic before this year around symlink resolution, but this is a new edge case. Somehow, the database is being loaded with the empty string for some fields (at least graph driver) which is causing comparisons to fail because we will never compare against "" for those fields - we insert the default value instead, assuming we have one. Having a value of "" in the database largely invalidates the check so arguably we could just drop it, but what BoltDB did - and what SQLite does after this patch - is to use the default value for comparison instead of "". This should still catch some edge cases, and shouldn't be too harmful. What this does not do is identify or solve the reason that we are seeing the empty string in the database at all. From my read on the logic, it must mean that the graph driver is explicitly set to "" in the c/storage config at the time Podman is first run and I'm not precisely sure how that happens. Fixes containers#24738 Signed-off-by: Matt Heon <[email protected]>

packit-as-a-service · 2025-02-10T18:43:40Z

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

Luap99

LGTM

openshift-ci · 2025-02-10T18:55:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, mheon

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,mheon]

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

mheon · 2025-02-10T18:57:16Z

@containers/podman-maintainers PTAL and merge

mheon · 2025-02-10T18:58:03Z

/cherry-pick v5.4

openshift-cherrypick-robot · 2025-02-10T18:58:07Z

@mheon: once the present PR merges, I will cherry-pick it on top of v5.4 in a new PR and assign it to you.

In response to this:

/cherry-pick v5.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

mheon · 2025-02-11T15:02:42Z

@containers/podman-maintainers Anyone care to merge?

rhatdan · 2025-02-11T15:09:24Z

/lgtm

openshift-cherrypick-robot · 2025-02-11T15:44:06Z

@mheon: new pull request created: #25294

In response to this:

/cherry-pick v5.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 5, 2024

mheon added the No New Tests Allow PR to proceed without adding regression tests label Dec 5, 2024

mheon force-pushed the fix_24738 branch 2 times, most recently from 7df556c to 1e04b52 Compare December 10, 2024 15:46

Luap99 reviewed Dec 10, 2024

View reviewed changes

mheon force-pushed the fix_24738 branch from 1e04b52 to b54620d Compare December 10, 2024 20:15

github-actions bot added the stale-pr label Jan 10, 2025

mheon force-pushed the fix_24738 branch 2 times, most recently from 1614605 to 34e3f22 Compare February 5, 2025 20:57

Luap99 reviewed Feb 6, 2025

View reviewed changes

mheon force-pushed the fix_24738 branch 2 times, most recently from 6c42f8f to b1418b8 Compare February 6, 2025 14:49

Luap99 reviewed Feb 10, 2025

View reviewed changes

mheon force-pushed the fix_24738 branch from b1418b8 to cb53abc Compare February 10, 2025 17:42

Luap99 approved these changes Feb 10, 2025

View reviewed changes

openshift-ci bot assigned rhatdan Feb 11, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2025

rhatdan removed lgtm Indicates that a PR is ready to be merged. stale-pr labels Feb 11, 2025

mheon added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2025

openshift-merge-bot bot merged commit 4a0b230 into containers:main Feb 11, 2025
82 of 83 checks passed

openshift-cherrypick-robot mentioned this pull request Feb 11, 2025

[v5.4] In SQLite state, use defaults for empty-string checks #25294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In SQLite state, use defaults for empty-string checks #24775

In SQLite state, use defaults for empty-string checks #24775

mheon commented Dec 5, 2024

mheon commented Dec 5, 2024

Luap99 commented Dec 5, 2024

baude commented Dec 5, 2024

Luap99 commented Dec 5, 2024

Luap99 Dec 10, 2024

github-actions bot commented Jan 10, 2025

Luap99 Feb 6, 2025

mheon commented Feb 6, 2025

Luap99 commented Feb 6, 2025

mheon commented Feb 6, 2025

Luap99 commented Feb 6, 2025

mheon commented Feb 6, 2025

nalind commented Feb 6, 2025 •

edited

Loading

Luap99 Feb 10, 2025

packit-as-a-service bot commented Feb 10, 2025

Luap99 left a comment

openshift-ci bot commented Feb 10, 2025

mheon commented Feb 10, 2025

mheon commented Feb 10, 2025

openshift-cherrypick-robot commented Feb 10, 2025

mheon commented Feb 11, 2025

rhatdan commented Feb 11, 2025

openshift-cherrypick-robot commented Feb 11, 2025

		# Force all custom directories so we don't pick up an existing database
		CONTAINERS_STORAGE_CONF=$PODMAN_TMPDIR/storage.conf run_podman --root $PODMAN_TMPDIR/root --runroot $PODMAN_TMPDIR/runroot --tmpdir $PODMAN_TMPDIR/tmpdir info

In SQLite state, use defaults for empty-string checks #24775

In SQLite state, use defaults for empty-string checks #24775

Conversation

mheon commented Dec 5, 2024

Does this PR introduce a user-facing change?

mheon commented Dec 5, 2024

Luap99 commented Dec 5, 2024

baude commented Dec 5, 2024

Luap99 commented Dec 5, 2024

Luap99 Dec 10, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2025

Luap99 Feb 6, 2025

Choose a reason for hiding this comment

mheon commented Feb 6, 2025

Luap99 commented Feb 6, 2025

mheon commented Feb 6, 2025

Luap99 commented Feb 6, 2025

mheon commented Feb 6, 2025

nalind commented Feb 6, 2025 • edited Loading

Luap99 Feb 10, 2025

Choose a reason for hiding this comment

packit-as-a-service bot commented Feb 10, 2025

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 10, 2025

mheon commented Feb 10, 2025

mheon commented Feb 10, 2025

openshift-cherrypick-robot commented Feb 10, 2025

mheon commented Feb 11, 2025

rhatdan commented Feb 11, 2025

openshift-cherrypick-robot commented Feb 11, 2025

nalind commented Feb 6, 2025 •

edited

Loading