Fix additional image stores in podman info #24731

l0rd · 2024-12-02T12:28:28Z

The command podman info returned only one imagestore in store.graphOptions.<driver>.imagestore even if multiple
image stores were configured.

This change replaces the field <driver>.imagestore with the field <driver>.additionalImageStores, that is an array of strings that includes all the configured additional image stores.

Fix containers/storage#2094

Does this PR introduce a user-facing change?

Fix `podman info` when there are multiple additional image stores

Luap99 · 2024-12-02T12:44:25Z

test/system/005-info.bats

+    if [[ -z "$CI_DESIRED_STORAGE" ]]; then
+        # When running in Cirrus, CI_DESIRED_STORAGE *must* be defined
+        # in .cirrus.yml so we can double-check that all CI VMs are
+        # using overlay or vfs as desired.
+        if [[ -n "$CIRRUS_CI" ]]; then
+            die "CIRRUS_CI is set, but CI_DESIRED_STORAGE is not! See #20161"
+        fi
+
+        # Not running under Cirrus (e.g., gating tests, or dev laptop).
+        # Totally OK to skip this test.
+        skip "CI_DESIRED_STORAGE is unset--OK, because we're not in Cirrus"
+    fi
+    is "$(podman_storage_driver)" "$CI_DESIRED_STORAGE" "podman storage driver is not CI_DESIRED_STORAGE (from .cirrus.yml)"


This seems unnecessary, the test does not have to depend on any CI setup so it shouldn't as we should be able to run that locally.

Just call $(podman_storage_driver) to know the current driver

Ok will do that

Luap99 · 2024-12-02T12:45:00Z

test/system/005-info.bats

+    is "$(podman_storage_driver)" "$CI_DESIRED_STORAGE" "podman storage driver is not CI_DESIRED_STORAGE (from .cirrus.yml)"
+    skip_if_remote "--storage-opt flag is not supported for remote"
+    # Only overlay storage driver supports additional image stores
+    if [[ "$CI_DESIRED_STORAGE" = "overlay" ]]; then


That isn't true, vfs supports extra imagestores as well so the condition is not needed.

Luap99 · 2024-12-02T12:46:42Z

libpod/info.go

@@ -260,7 +261,14 @@ func (r *Runtime) storeInfo() (*define.StoreInfo, error) {
 			program["Version"] = ver
 			program["Package"] = version.Package(split[1])
 			graphOptions[split[0]] = program
-		} else {
+		case strings.HasSuffix(split[0], "imagestore"):
+			key := strings.ReplaceAll(split[0], "imagestore", "additionalImageStores")


this means the old imagestore field is no longer included while the certainly was buggy it might be that users depend on that field somehow so we should not remove it and keep for backwards compat at least until 6.0 IMO

Sure, I can keep that key as well, with the same logic if you think it can break someone.

I think we need both. imagestore refers to a path where images will be stored. This feature is mostly used by CRI-O to split containers and images on two different file systems. It is not the same as additionalimagestores.

@giuseppe I see that there are 2 different configs in storage.conf:

[storage] imagestore = "/first/path" [storage.options] additionalimagestores = [ "second/path" ]

But podman info returns only the additionalimagestores path:

... store: ... graphOptions: overlay.imagestore: "second/path" overlay.additionalImageStores: ["second/path"]

CRI-O shouldn't be affected by what podman info returns (this PR change), right?

Luap99 · 2024-12-02T15:13:49Z

test/system/005-info.bats

+    store1=$PODMAN_TMPDIR/store1
+    store2=$PODMAN_TMPDIR/store2
+    mkdir -p $store1 $store2
+    run_podman info --storage-opt=$(podman_storage_driver)'.imagestore='$store1 \


can you cache $(podman_storage_driver) in a var? It seems the function calls podman info each time and info is a rather slow command.

Luap99 · 2024-12-02T15:24:48Z

test/system/005-info.bats

+    mkdir -p $store1 $store2
+    run_podman info --storage-opt=$(podman_storage_driver)'.imagestore='$store1 \
+                    --storage-opt=$(podman_storage_driver)'.imagestore='$store2 \
+                    --format '{{index .Store.GraphOptions "'$(podman_storage_driver)'.additionalImageStores"}}'


{{index .Store.GraphOptions "'$(podman_storage_driver)'.additionalImageStores"}}\n{{index .Store.GraphOptions "'$(podman_storage_driver)'.imagestore"}}

Can you use this format then check

assert "${lines[0]}" == "["$store1" "$store2"]" "output includes additional image stores" assert "${lines[1]}" == "$store2" "old imagestore output"

In general assert should be preferred over is as is has some rather dangerous pitfalls, i.e. your current test can end up broken as [] is valid regex so is may assume it should perfom a regex match not an exact match.
With assert you must specify == or =~ so there is no ambiguity

The command `podman info` returned only one imagestore in `store.graphOptions.<driver>.imagestore` even if multiple image stores were configured. This change replaces the field `<driver>.imagestore` with the field `<driver>.additionalImageStores`, that instead of a string is an array of strings and that includes all the configured additional image stores. Fix containers/storage#2094 Signed-off-by: Mario Loriedo <[email protected]>

Luap99

LGTM, thanks

openshift-ci · 2024-12-02T15:41:07Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

baude · 2024-12-03T14:12:09Z

/lgtm

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

l0rd mentioned this pull request Dec 2, 2024

Returns more than one additional image store containers/storage#2096

Closed

Luap99 reviewed Dec 2, 2024

View reviewed changes

l0rd force-pushed the additionalimagestores branch from 0b6899b to 0aa347a Compare December 2, 2024 15:11

Luap99 reviewed Dec 2, 2024

View reviewed changes

l0rd force-pushed the additionalimagestores branch from 0aa347a to 0d3a653 Compare December 2, 2024 15:37

Luap99 approved these changes Dec 2, 2024

View reviewed changes

openshift-ci bot assigned baude Dec 3, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2024

openshift-merge-bot bot merged commit 45ac4e2 into containers:main Dec 3, 2024
79 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix additional image stores in podman info #24731

Fix additional image stores in podman info #24731

l0rd commented Dec 2, 2024

Luap99 Dec 2, 2024

l0rd Dec 2, 2024

Luap99 Dec 2, 2024

l0rd Dec 2, 2024

Luap99 Dec 2, 2024

l0rd Dec 2, 2024

l0rd Dec 2, 2024

giuseppe Dec 12, 2024

l0rd Dec 13, 2024

l0rd Dec 13, 2024

Luap99 Dec 2, 2024

l0rd Dec 2, 2024

Luap99 Dec 2, 2024

l0rd Dec 2, 2024

Luap99 left a comment

openshift-ci bot commented Dec 2, 2024

baude commented Dec 3, 2024

Fix additional image stores in podman info #24731

Fix additional image stores in podman info #24731

Conversation

l0rd commented Dec 2, 2024

Does this PR introduce a user-facing change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 2, 2024

baude commented Dec 3, 2024