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

Fix mount propagation. #1305

Closed
wants to merge 1 commit into from
Closed

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Aug 20, 2018

All volumes mount points should default to "private".

Root Propagation needs to default to "shared", since users
could mount a volume in as shared.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

@mheon
Copy link
Member

mheon commented Aug 20, 2018

@rhatdan I take it this fixes our mount issues?

LGTM regardless

@rhatdan
Copy link
Member Author

rhatdan commented Aug 20, 2018

@mheon Partially. I have a PR for runc to really fix it. But we will see how that goes.

@rhatdan
Copy link
Member Author

rhatdan commented Aug 20, 2018

opencontainers/runc#1873

@rhatdan
Copy link
Member Author

rhatdan commented Aug 22, 2018

bot, retest this please

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably 4a95ef4) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan rhatdan force-pushed the propagation branch 5 times, most recently from c723fb4 to 8209fde Compare August 26, 2018 08:36
@rhatdan
Copy link
Member Author

rhatdan commented Aug 26, 2018

bot, retest this please

@giuseppe
Copy link
Member

could the default be slave or wouldn't it solve the issue? It is usually safer to use than private as changes in the source are propagated to the destination as well, and there is no risk of creating new mounts in the source

@rhatdan
Copy link
Member Author

rhatdan commented Aug 27, 2018

I was just going with the default that we went with in Docker. We could always move this to a setting in libpod if it was considered important. But for now I think SLAVE keeps us the safest.

@rhatdan
Copy link
Member Author

rhatdan commented Aug 27, 2018

Expect(match).Should(BeFalse())
})

It("podman run findmnt nothing shared", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Remove "nothing"

Default mount propagation inside of containes should be private

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@vrothberg
Copy link
Member

LGTM
I grepped through the source and all locations seem to be updated 👍

@mheon
Copy link
Member

mheon commented Aug 27, 2018

LGTM

@mheon
Copy link
Member

mheon commented Aug 27, 2018

@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Collaborator

📌 Commit 8072f34 has been approved by mheon

@TomSweeneyRedHat
Copy link
Member

LGTM

@rh-atomic-bot
Copy link
Collaborator

⌛ Testing commit 8072f34 with merge 663ee91...

@umohnani8
Copy link
Member

LGTM

@rh-atomic-bot
Copy link
Collaborator

☀️ Test successful - status-papr
Approved by: mheon
Pushing 663ee91 to master...

@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 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

7 participants