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

[5.2-rhel] libpod: fix volume copyup with idmap | convert owner IDs only with :idmap #24591

Merged
merged 3 commits into from
Jan 16, 2025

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Nov 18, 2024

Copy link
Contributor

openshift-ci bot commented Nov 18, 2024

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Nov 18, 2024
Copy link
Contributor

openshift-ci bot commented Nov 18, 2024

[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 Nov 18, 2024
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@giuseppe giuseppe marked this pull request as draft November 18, 2024 11:31
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2024
@giuseppe giuseppe marked this pull request as ready for review November 18, 2024 11:31
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2024
@Luap99
Copy link
Member

Luap99 commented Nov 18, 2024

@TomSweeneyRedHat Do we need z-stream backport request cards before merging?

@TomSweeneyRedHat
Copy link
Member

@Luap99 at this point, we don't need a Zstream card for 5.2, since 5.2 is top of tree in RHEL and this is a backport from main, we can just fix it with the existing Jira card. Good question though, you made me think for a bit.

Copy link

A friendly reminder that this PR had no activity for 30 days.

@Luap99
Copy link
Member

Luap99 commented Jan 7, 2025

CI is failing.

What the state here can/should this still be merged?

@giuseppe
Copy link
Member Author

giuseppe commented Jan 9, 2025

no idea if we still need this.

@TomSweeneyRedHat do we need this PR or can I close it?

@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Jan 9, 2025

This fixes an issue in the current release of Podman on RHEL and should be fixed if at all possible. I'm not sure what's going on with the tests here. It's not a busy repository. I'll throw up a test PR to see if it's something outside of this PR or not.

Created: #24990

TomSweeneyRedHat added a commit to TomSweeneyRedHat/podman that referenced this pull request Jan 9, 2025
This repo hasn't been added to in a while and containers#24591 is failing when
it builds.  I'm adding this doc only change to test if it's an
underlying problem with the tests in this repo, or something in the
new PR.

DO NOT MERGE.

Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
@Luap99
Copy link
Member

Luap99 commented Jan 10, 2025

This isn't testing anything anymore as we dropped all functional tests per our discussion last year: #24355

So technically a rebase will make this work.

Looking at the tests I see "failed to create idmapped mount: invalid argument" is matched to skip the test when the fs does not support idmap we however got "failed to create idmapped mount: operation not permitted".

I guess the second case also means idmap is simply not supported, not sure why the errno is different there though.

Anyway of the goal is to merge this reabse is enough and then I guess RHEL QE must verify these bugs anyway

giuseppe and others added 3 commits January 10, 2025 12:34
if idmap is specified for a volume, reverse the mappings when copying
up from the container, so that the original permissions are maintained.

Closes: containers#23467

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
(cherry picked from commit 3ae1568)
Signed-off-by: Ed Santiago <santiago@redhat.com>
(cherry picked from commit 7bb3b83)
convert the owner UID and GID into the user namespace only when
":idmap" mount is used.

This changes the behaviour of :idmap with an empty volume.  Now the
existing directory ownership is copied up as in the other case.

Closes: containers#23347
Closes: https://issues.redhat.com/browse/RHEL-67842

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
(cherry picked from commit 4323252)
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Member Author

rebased

@rhatdan rhatdan removed the stale-pr label Jan 10, 2025
@rhatdan
Copy link
Member

rhatdan commented Jan 10, 2025

LGTM

@giuseppe
Copy link
Member Author

please merge, it is also needed for: https://issues.redhat.com/browse/RHEL-73592

@rhatdan
Copy link
Member

rhatdan commented Jan 14, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2025
@rhatdan rhatdan removed lgtm Indicates that a PR is ready to be merged. do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jan 14, 2025
@giuseppe
Copy link
Member Author

@TomSweeneyRedHat PTAL

@TomSweeneyRedHat
Copy link
Member

LGTM

@TomSweeneyRedHat
Copy link
Member

/lgtm
Thanks @giuseppe !

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 76d1690 into containers:v5.2-rhel Jan 16, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants