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

Add support for privileged containers #123 #132

Merged
merged 1 commit into from
Mar 13, 2014
Merged

Add support for privileged containers #123 #132

merged 1 commit into from
Mar 13, 2014

Conversation

kvz
Copy link

@kvz kvz commented Mar 3, 2014

This was required when I tried to mount external volumes and
addresses errors such as mount.nfs: Operation not permitted (similar for smbfs, and afs)

You can add privileged: True to any service in your fig.yml for the change to take effect.

I don't code Python normally, so please be gentle : )

@kvz kvz mentioned this pull request Mar 4, 2014
Closed
@bfirsh
Copy link

bfirsh commented Mar 4, 2014

fig/packages/docker/client.py already has an option to set privileged when starting – does it also need it when creating?

@kvz
Copy link
Author

kvz commented Mar 4, 2014

I think create_container complained about being passed the privileged option as it gets the same config as start_container. It might not be needed in the actual create though, will investigate

@bfirsh
Copy link

bfirsh commented Mar 4, 2014

Ah gotchya – yes, that's worth checking, and making sure it doesn't get passed through.

fig/packages/docker/client.py is in fact a vendorised version of docker-py so we don't maintain it here. If there's something missing from that, then we can push it upstream.

@j0hnsmith
Copy link

I'd love to run via fig with privileged.

@kvz
Copy link
Author

kvz commented Mar 9, 2014

Hey @bfirsh I found some more time to work on this and made it so that service.py filters out privileged when container_options are passed to create_container.

You were right, the option is only needed for running containers, and this allowed me to also revert the changes I had made to client.py in the docker package.

It works for me (I can mount network volumes from within containers) but I'm not sure if I did it in the best way/place, so let me know what you think.

@gregwebs
Copy link

@bfirsh is this pull waiting on anything (other than a review)?

bfirsh
bfirsh reviewed Mar 13, 2014
View reviewed changes
fig/service.py
@@ -241,7 +249,7 @@ def _get_links(self, link_to_self):
links.append((container.name, container.name_without_project))
return links

def _get_container_options(self, override_options, one_off=False):
def _get_container_options(self, override_options, one_off=False, for_create=False):
Copy link

Choose a reason for hiding this comment

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

_get_container_options is only used for creating, afaik? Is this needed?

This method should probably be called _get_container_create_options or something, and we could have a new _get_container_start_options to make it clear what is going on.

@bfirsh
Copy link

bfirsh commented Mar 13, 2014

Thanks for taking the time on this @kvz! Added a couple of comments. Would be great to get tests too.

@kvz
Copy link
Author

kvz commented Mar 13, 2014

np, how's this?

@bfirsh
Copy link

bfirsh commented Mar 13, 2014

Brilliant, thanks! Will let Travis do its thing. Maybe we should also have a test to check that without the privileged flag, containers aren't privileged.

@kvz
Add support for privileged containers #123
0f5a56b 
This is required for mounting external volumes and
addresses errors such as `mount.nfs: Operation not permitted`

Be gentle, I don't normally use Python :)
@kvz
Copy link
Author

kvz commented Mar 13, 2014

Alright, added that as well : )

@bfirsh
Copy link

bfirsh commented Mar 13, 2014

Huzzah! Thanks!

bfirsh added a commit that referenced this pull request Mar 13, 2014
@bfirsh
Merge pull request #132 from kvz/privileged
a96ab41 
Add support for privileged containers #123
@bfirsh bfirsh merged commit a96ab41 into docker:master Mar 13, 2014
@bfirsh
Copy link

bfirsh commented Mar 13, 2014

privilege_spalsh_image 2

@kvz kvz deleted the privileged branch March 13, 2014 13:46
@kvz
Copy link
Author

kvz commented Mar 13, 2014

😄

yuval-k pushed a commit to yuval-k/compose that referenced this pull request Apr 10, 2015
@bfirsh
Merge pull request docker#132 from kvz/privileged
53fc17e 
Add support for privileged containers docker#123
Signed-off-by: Yuval Kohavi <[email protected]>
xulike666 pushed a commit to xulike666/compose that referenced this pull request Jan 19, 2017
@crosbymichael
Merge pull request docker#132 from marcosnils/vendor_layout
3041015 
Switch to new vendor directory layout
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kvz @bfirsh @j0hnsmith @gregwebs