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

Disable mount options when running --privileged #1317

Merged
merged 2 commits into from
Nov 30, 2018
Merged

Disable mount options when running --privileged #1317

merged 2 commits into from
Nov 30, 2018

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Aug 21, 2018

We now default to setting storage options to "nodev", when running
privileged containers, we need to turn this off so the processes can
manipulate the image.

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

@rhatdan
Copy link
Member Author

rhatdan commented Aug 21, 2018

@cgwalters PTAL

mheon
mheon reviewed Aug 21, 2018
View reviewed changes
libpod/runtime.go Outdated
}
opts = append(opts, i)
}
r.config.StorageConfig.GraphDriverOptions = opts
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this without modifying the storage.Store?

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 how?

Copy link
Member Author

Choose a reason for hiding this comment

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

It no longer modifies the store. Just sends the options in on the container creation.

@mheon
Copy link
Member

mheon commented Aug 21, 2018

Would be a lot better if we had an API to toggle per-container in c/storage - this will make it so that launching a single container will disable mount opts for the entire container once we move CRI-O to libpod

@TomSweeneyRedHat
Copy link
Member

bot, retest this please

giuseppe
giuseppe reviewed Aug 25, 2018
View reviewed changes
libpod/runtime.go Outdated
@@ -714,3 +715,15 @@ func (r *Runtime) generateName() (string, error) {
func (r *Runtime) ImageRuntime() *image.Runtime {
return r.imageRuntime
}

// RemoveMountOptions removes mount ouptions from the graphdriver
Copy link
Member

Choose a reason for hiding this comment

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

typo "options"

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

@TomSweeneyRedHat TomSweeneyRedHat changed the title Disable mount options when running --priviged Disable mount options when running --privileged Aug 25, 2018
@rhatdan rhatdan force-pushed the privileged branch 2 times, most recently from 2f753b8 to 85fc280 Compare August 26, 2018 08:38
@rhatdan
Copy link
Member Author

rhatdan commented Aug 26, 2018

Opened containers/storage#211 so we could pass this down the the graphdriver per container,

@rhatdan rhatdan changed the title Disable mount options when running --privileged [WIP] Disable mount options when running --privileged Aug 26, 2018
@rhatdan
Copy link
Member Author

rhatdan commented Sep 20, 2018

bot, retest this please

@rhatdan rhatdan changed the title [WIP] Disable mount options when running --privileged Disable mount options when running --privileged Sep 20, 2018
@rh-atomic-bot
Copy link
Collaborator

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

@rhatdan rhatdan changed the title Disable mount options when running --privileged [WIP] Disable mount options when running --privileged Oct 30, 2018
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2018
@rhatdan
Copy link
Member Author

rhatdan commented Oct 30, 2018

Requires containers/storage#226

@rh-atomic-bot
Copy link
Collaborator

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

@rhatdan
Vendor in latest containers/storage
193e619 
This allows us to modify the containers mount option on a per/container basis

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Disable mount options when running --privileged
3beacb7 
We now default to setting storage options to "nodev", when running
privileged containers, we need to turn this off so the processes can
manipulate the image.

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan rhatdan changed the title [WIP] Disable mount options when running --privileged Disable mount options when running --privileged Nov 28, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2018
@rhatdan
Copy link
Member Author

rhatdan commented Nov 30, 2018

@cgwalters @mheon @baude @umohnani8 @giuseppe @vrothberg PTAL
I believe this is ready to merge.

@mheon
Copy link
Member

mheon commented Nov 30, 2018

LGTM

@mheon
Copy link
Member

mheon commented Nov 30, 2018

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

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 Nov 30, 2018
@TomSweeneyRedHat
Copy link
Member

LGTM

@mheon
Copy link
Member

mheon commented Nov 30, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2018
@openshift-merge-robot openshift-merge-robot merged commit b504623 into containers:master Nov 30, 2018
This was referenced Nov 30, 2018
Merged
Merged
@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.
Reviewers

@mheon mheon mheon left review comments

@giuseppe giuseppe giuseppe left review comments

Assignees

@mheon mheon

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 @mheon @TomSweeneyRedHat @rh-atomic-bot @openshift-ci-robot @giuseppe @openshift-merge-robot