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

Jump to bottom

Cleanup handling of podman mount/unmount #7085

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Cleanup handling of podman mount/unmount #7085

merged 1 commit into from
Jul 28, 2020

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jul 26, 2020

We should default to the user name unmount rather then the internal
name of umount.

Also User namespace was not being handled correctly. We want to inform
the user that if they do a mount when in rootless mode that they have
to be first in the podman unshare state.

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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 Jul 26, 2020
@rhatdan
Copy link
Member Author

rhatdan commented Jul 26, 2020

Splitting this PR away from the bigger podmain image mount PR. Hopefully this will help me figure out what is causing the podman tag/untag issues, that are preventing that PR from merging.

vrothberg
vrothberg reviewed Jul 27, 2020
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Code LGTM but I'd love to have some tests for the "unknown format" case and when mounting outside a user namespace.

edsantiago
edsantiago reviewed Jul 27, 2020
View reviewed changes
pkg/rootless/rootless_linux.c
@@ -210,6 +210,13 @@ can_use_shortcut ()
ret = false;
break;
}

if (strcmp (argv[argc], "container") == 0 &&
strcmp (argv[argc+1], "mount") == 0)
Copy link
Member

Choose a reason for hiding this comment

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

index out of range

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@rhatdan rhatdan force-pushed the cmount branch 2 times, most recently from 5592fd1 to 60f4c98 Compare July 27, 2020 13:04
TomSweeneyRedHat
TomSweeneyRedHat reviewed Jul 27, 2020
View reviewed changes
docs/source/markdown/links/podman-unmount.1
@@ -1 +0,0 @@
.so man1/podman-umount.1
Copy link
Member

Choose a reason for hiding this comment

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

Was this intentionally deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, It is actually renamed. podman-umount.1 now links to man1/podman-unmount.1

@TomSweeneyRedHat
Copy link
Member

One question, and tests as @vrothberg mentioned would be good. Otherwise LGTM

@rhatdan
Copy link
Member Author

rhatdan commented Jul 27, 2020

Tests added, but I could not get them to run locally. Will see what happens here.

@rhatdan rhatdan force-pushed the cmount branch 2 times, most recently from 6598cab to cc57807 Compare July 27, 2020 17:04
mheon
mheon reviewed Jul 27, 2020
View reviewed changes
cmd/podman/registry/config.go Outdated
@@ -16,6 +16,7 @@ import (

const (
ParentNSRequired = "ParentNSRequired"
UserNSRequired = "UserNSRequired"
Copy link
Member

Choose a reason for hiding this comment

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

Could this be renamed? Maybe "RootRequired"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that is helpful. How about UnshareUserNSRequired Or UnshareNSRequired

Copy link
Member

Choose a reason for hiding this comment

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

UnshareNSRequired SGTM

@mheon
Copy link
Member

mheon commented Jul 27, 2020

LGTM once the UserNSRequired field is renamed

@rhatdan
Cleanup handling of podman mount/unmount
8f7ed50 
We should default to the user name unmount rather then the internal
name of umount.

Also User namespace was not being handled correctly. We want to inform
the user that if they do a mount when in rootless mode that they have
to be first in the podman unshare state.

Signed-off-by: Daniel J Walsh <[email protected]>
vrothberg
vrothberg reviewed Jul 28, 2020
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2020
@vrothberg
Copy link
Member

Restarted flakes

@edsantiago edsantiago mentioned this pull request Jul 28, 2020
Closed
@rhatdan
Copy link
Member Author

rhatdan commented Jul 28, 2020

/hold cancel

@rhatdan rhatdan removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2020
@openshift-merge-robot openshift-merge-robot merged commit 91c92d1 into containers:master Jul 28, 2020
@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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@vrothberg vrothberg vrothberg left review comments

@mheon mheon mheon left review comments

@edsantiago edsantiago edsantiago left review comments

@baude baude Awaiting requested review from baude

@umohnani8 umohnani8 Awaiting requested review from umohnani8

Assignees

@vrothberg vrothberg

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rhatdan @openshift-ci-robot @TomSweeneyRedHat @mheon @vrothberg @edsantiago @openshift-merge-robot