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.

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

pod: fix duplicate volumes from containers.conf #19902

Merged
merged 1 commit into from
Sep 9, 2023

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Sep 8, 2023

If some volumes are specified in containers.conf, they are currently added twice to the containers spec causing the container to fail:

$ head -n2 ~/.config/containers/containers.conf
[containers]
volumes = ["/tmp:/tmp"]
$ podman pod create --name foo
7ac7f97f9b74a596332483e4a13e58cb9c8d997e9c5baae46804ae0acc26cbc6 $ podman run --pod=foo alpine true
Error: "/tmp": duplicate mount destination

The fix is to ignore the setting from containers.conf when setting the pod default configuration.

Does this PR introduce a user-facing change?

running a container in a pod doesn't fail if volumes or mounts are specified in the containers.conf file

If some volumes are specified in containers.conf, they are currently
added twice to the containers spec causing the container to fail:

$ head -n2 ~/.config/containers/containers.conf
[containers]
volumes = ["/tmp:/tmp"]
$ podman pod create --name foo
7ac7f97f9b74a596332483e4a13e58cb9c8d997e9c5baae46804ae0acc26cbc6
$ podman run --pod=foo alpine true
Error: "/tmp": duplicate mount destination

The fix is to ignore the setting from containers.conf when setting the
pod default configuration.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2023
@rhatdan
Copy link
Member

rhatdan commented Sep 8, 2023

LGTM
@giuseppe @mheon PTAL

// these settings are not applicable to pod create since they are per-container
// and they will end up being duplicated for each container in the pod.
infraOptions.Volume = nil
infraOptions.Mount = nil
Copy link
Member

Choose a reason for hiding this comment

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

Barely on the fence here. Should we add a debug statement just above this, showing the values we're wiping? I think not, but I don't know for sure if that might be useful at some far-flung point in time.

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure, I can add that if you think it is useful

@TomSweeneyRedHat
Copy link
Member

LGTM
one question for consideration

@mheon
Copy link
Member

mheon commented Sep 8, 2023

/lgtm
/hold

I wonder if we shouldn't dedup mounts in SpecGen to try and catch things like this, I can see this also happening with --volumes-from.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2023
@giuseppe
Copy link
Member Author

giuseppe commented Sep 8, 2023

I wonder if we shouldn't dedup mounts in SpecGen to try and catch things like this, I can see this also happening with --volumes-from.

I've tried that solution first, but we have different options for /proc. So effectively it is like two different structs, when we compare them

@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2023

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2023
@rhatdan rhatdan merged commit 745201e into containers:main Sep 9, 2023
@github-actions 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 9, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants