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.

Sign up for GitHub

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

Fix additional image stores in podman info #24731

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Fix additional image stores in podman info #24731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions libpod/info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,8 @@ func (r *Runtime) storeInfo() (*define.StoreInfo, error) {
graphOptions := map[string]interface{}{}
for _, o := range r.store.GraphOptions() {
split := strings.SplitN(o, "=", 2)
if strings.HasSuffix(split[0], "mount_program") {
switch {
case strings.HasSuffix(split[0], "mount_program"):
ver, err := version.Program(split[1])
if err != nil {
logrus.Warnf("Failed to retrieve program version for %s: %v", split[1], err)
Expand All @@ -260,7 +261,17 @@ 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")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

PR updated

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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"]

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if graphOptions[key] == nil {
graphOptions[key] = []string{split[1]}
} else {
graphOptions[key] = append(graphOptions[key].([]string), split[1])
}
// Fallthrough to include the `imagestore` key to avoid breaking
// Podman v5 API. Should be removed in Podman v6.0.0.
fallthrough
default:
graphOptions[split[0]] = split[1]
}
}
Expand Down
13 changes: 13 additions & 0 deletions test/system/005-info.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,19 @@ host.slirp4netns.executable | $expr_path
is "$output" ".*graphOptions: {}" "output includes graphOptions: {}"
}

@test "podman info - additional image stores" {
skip_if_remote "--storage-opt flag is not supported for remote"
driver=$(podman_storage_driver)
store1=$PODMAN_TMPDIR/store1
store2=$PODMAN_TMPDIR/store2
mkdir -p $store1 $store2
run_podman info --storage-opt=$driver'.imagestore='$store1 \
--storage-opt=$driver'.imagestore='$store2 \
--format '{{index .Store.GraphOptions "'$driver'.additionalImageStores"}}\n{{index .Store.GraphOptions "'$driver'.imagestore"}}'
assert "${lines[0]}" == "["$store1" "$store2"]" "output includes additional image stores"
assert "${lines[1]}" == "$store2" "old imagestore output"
}

@test "podman info netavark " {
# Confirm netavark in use when explicitly required by execution environment.
if [[ "$NETWORK_BACKEND" == "netavark" ]]; then
Expand Down