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

[v3.0.1-rhel] podman cp: ignore EPERMs in rootless mode #9732

Conversation

vrothberg
Copy link
Member

Ignore permission errors when copying from a rootless container.
TTY devices inside rootless containers are owned by the host's
root user which is "nobody" inside the container's user namespace
rendering us unable to even read them.

Enable the integration test which was temporarily disabled for rootless
users.

Signed-off-by: Valentin Rothberg [email protected]

This one slipped through in the backports. Last bit needed to fix https://bugzilla.redhat.com/show_bug.cgi?id=1936927.

Signed-off-by: Valentin Rothberg <[email protected]>
Ignore permission errors when copying from a rootless container.
TTY devices inside rootless containers are owned by the host's
root user which is "nobody" inside the container's user namespace
rendering us unable to even read them.

Enable the integration test which was temporarily disabled for rootless
users.

Signed-off-by: Valentin Rothberg <[email protected]>
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2021
@vrothberg
Copy link
Member Author

Had to add a third commit to fix the containers.conf timezone e2e test. I do not know why that worked before since New York yields EDT, not EST.

@@ -10,7 +10,7 @@ require (
github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd // indirect
github.com/containernetworking/cni v0.8.1
github.com/containernetworking/plugins v0.9.0
github.com/containers/buildah v1.19.7
Copy link
Member

Choose a reason for hiding this comment

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

This will give us a higher version of Buildah vendored into Podman than we have available on RHEL (v1.19.6).

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 what that implies. We need .7 to fix the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Just concerned that if you run Buildah you could get different behavior than running podman build. I'm probably over-fretting.

@@ -311,7 +311,7 @@ var _ = Describe("Podman run", func() {
session = podmanTest.Podman([]string{"run", ALPINE, "date", "+'%H %Z'"})
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))
Expect(session.OutputToString()).To(ContainSubstring("EST"))
Expect(session.OutputToString()).To(ContainSubstring("EDT"))
Copy link
Member

Choose a reason for hiding this comment

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

Won't this change/fail in the fall when we go back to EST? Seems like a check that needs to be modified....

Copy link
Member

Choose a reason for hiding this comment

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

DO NOT MERGE THIS. Use 8de5607 instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks!

@TomSweeneyRedHat
Copy link
Member

@rhatdan @mheon @baude Please put this at the top of your list for reviews this morning.

The New York timezone changes between summer and winter time.
Make sure the test allows both timezones.

Signed-off-by: Paul Holzinger <[email protected]>
@vrothberg vrothberg force-pushed the v3.0.1-rhel-cp-backports branch from dc90ab4 to 82c35bf Compare March 17, 2021 12:45
@mheon
Copy link
Member

mheon commented Mar 17, 2021

I can live with Buildah patch release not matching. If it was a minor release I'd be more concerned.

LGTM, but CI is red even after your patch.

@vrothberg
Copy link
Member Author

LGTM, but CI is red even after your patch.

The APIv2 tests flake. I want to wait with restarting until the other tests turn green.

@edsantiago
Copy link
Member

Maybe this would be a good time to cherry-pick #9699

@TomSweeneyRedHat
Copy link
Member

LGTM
and all happy green test buttons
@jnovy FYI
@mheon, want to press the shiny merge button?

@mheon
Copy link
Member

mheon commented Mar 17, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2021
@openshift-merge-robot openshift-merge-robot merged commit ad1aaba into containers:v3.0.1-rhel Mar 17, 2021
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants