Skip to content

volumes: Fix idmap not working for volumes#16249

Merged
openshift-merge-robot merged 1 commit intocontainers:mainfrom
klausenbusk:fix-idmap-for-volumes
Oct 27, 2022
Merged

volumes: Fix idmap not working for volumes#16249
openshift-merge-robot merged 1 commit intocontainers:mainfrom
klausenbusk:fix-idmap-for-volumes

Conversation

@klausenbusk
Copy link
Copy Markdown
Contributor

@klausenbusk klausenbusk commented Oct 21, 2022 •
edited
Loading

idmap is documented as supported for volumes, but it was not added to the getNamedVolume() function.

Fixes: e83d366 ("volumes: add new option idmap")
Signed-off-by: Kristian Klausen kristian@klausen.dk

Does this PR introduce a user-facing change?

None

cc @giuseppe as you added the idmap option.

@klausenbusk klausenbusk force-pushed the fix-idmap-for-volumes branch from fa518d6 to c040409 Compare October 21, 2022 14:19
@klausenbusk klausenbusk changed the title volumes: Fix idmap not working for named volumes volumes: Fix idmap not working for volumes Oct 21, 2022
giuseppe
giuseppe approved these changes Oct 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2022
@klausenbusk klausenbusk force-pushed the fix-idmap-for-volumes branch from c040409 to 106f5c6 Compare October 21, 2022 19:47
@mheon
Copy link
Copy Markdown
Member

mheon commented Oct 21, 2022

Test failures are all search flakes - Dockerhub acting up again.

@mheon
Copy link
Copy Markdown
Member

mheon commented Oct 21, 2022

Code LGTM; any chance you could add a test?

@klausenbusk
Copy link
Copy Markdown
Contributor Author

klausenbusk commented Oct 21, 2022 •
edited
Loading

Code LGTM; any chance you could add a test?

Hmm, I added a quick test, but apparently neither Ubuntu nor Fedora 36 has a new enough kernel (5.19 or newer). At least that is why I think it is failing with:

Error: crun: mount_setattr `/tmp/podman_test1978745283/root/volumes/vol-1/_data`: Operation not permitted: OCI permission denied

I guess I could add a test to pkg/specgenutil/volumes_test.go testing getNamedVolume(), but I'm not sure if it is worth it (the test would be tightly coupled to the implementation).

Any suggestions?

@mheon
Copy link
Copy Markdown
Member

mheon commented Oct 21, 2022

I would expect F36 to have a new enough kernel, but maybe the VMs haven't been updated. @cevich Do we have 5.19 anywhere in CI? We can skip the test everywhere else if we can get at least one distro working.

@klausenbusk
Copy link
Copy Markdown
Contributor Author

klausenbusk commented Oct 21, 2022

I would expect F36 to have a new enough kernel, but maybe the VMs haven't been updated.

The log for F36 shows 5.18.19-200.fc36.x86_64, so I guess the VM must be updated :)

@giuseppe
Copy link
Copy Markdown
Member

giuseppe commented Oct 24, 2022

it also depends on the underlying file system. My suggestion is to ignore the error if it contains "Operation not permitted"

@brauner
Copy link
Copy Markdown

brauner commented Oct 24, 2022

it also depends on the underlying file system. My suggestion is to ignore the error if it contains "Operation not permitted"

If the filesystem doesn't support it but you are sufficiently privileged you should always see EINVAL fyi.

@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Oct 24, 2022

@cevich PTAL

@klausenbusk klausenbusk force-pushed the fix-idmap-for-volumes branch 4 times, most recently from 24d1659 to fca3584 Compare October 26, 2022 21:49
@klausenbusk
volumes: Fix idmap not working for volumes
3e6637a 
idmap is documented as supported for volumes, but it was not added to
the getNamedVolume() function.

Fixes: e83d366 ("volumes: add new option idmap")
Signed-off-by: Kristian Klausen <kristian@klausen.dk>
@klausenbusk klausenbusk force-pushed the fix-idmap-for-volumes branch from fca3584 to 3e6637a Compare October 26, 2022 23:24
@klausenbusk
Copy link
Copy Markdown
Contributor Author

klausenbusk commented Oct 27, 2022

CI is green🎉I mixed up the kernel versions, 5.19 added ovl: support idmapped layers and 5.12 added fs: introduce MOUNT_ATTR_IDMAP. Only the latter is needed for the test.

giuseppe
giuseppe approved these changes Oct 27, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Oct 27, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, klausenbusk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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-merge-robot openshift-merge-robot merged commit c577fe3 into containers:main Oct 27, 2022
@klausenbusk klausenbusk deleted the fix-idmap-for-volumes branch October 27, 2022 20:45
@M1cha M1cha mentioned this pull request Dec 3, 2022
Closed
@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 Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@giuseppe giuseppe giuseppe approved these changes

Assignees

@giuseppe giuseppe

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-none

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@klausenbusk @mheon @giuseppe @brauner @rhatdan @openshift-merge-robot