In SQLite state, use defaults for empty-string checks #24775
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
Adding no new tests as we don't do database config tests like this in CI |
|
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) 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 |
|
could we just use the podman cli and the |
|
|
mheon
force-pushed
the
fix_24738
branch
2 times, most recently
from
7df556c to
1e04b52
Compare
test/e2e/info_test.go
Outdated
| @@ -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() { | |||
There was a problem hiding this comment.
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
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 <mheon@redhat.com>
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?