-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
docker run: specify cgroup namespace mode with --cgroupns #2024
Conversation
Looks good, but CI failing |
f39912d
to
b560da6
Compare
Please sign your commits following these rules: $ git clone -b "1988-run-cgroupns-mode" git@github.com:rgulewich/cli.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842358791888
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f Amending updates the existing PR. You DO NOT need to open a new one. |
b560da6
to
bd828fd
Compare
Codecov Report
@@ Coverage Diff @@
## master #2024 +/- ##
=========================================
Coverage ? 56.79%
=========================================
Files ? 311
Lines ? 21849
Branches ? 0
=========================================
Hits ? 12410
Misses ? 8523
Partials ? 916 |
bd828fd
to
0883b2a
Compare
@AkihiroSuda - Fixed CI failures related to my changes - |
@rgulewich might be fixed by #2016, could you please rebase? |
0883b2a
to
4c4bb53
Compare
@kolyshkin - That did it, thanks! |
e2e/container/run_test.go
Outdated
environment.SkipIfCgroupNamespacesNotSupported(t) | ||
|
||
result := icmd.RunCommand("docker", "run", "--cgroupns=private", "--rm", fixtures.AlpineImage, | ||
"sh", "-c", "[[ $(cat /proc/1/cgroup | grep memory | cut -d: -f 3) == '/' ]]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps rewrite this to not depend on bash? Also, looks like a single grep is sufficient, e.g.
grep -q ':memory:/$' /proc/1/cgroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgulewich ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated - PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kolyshkin ^^
4c4bb53
to
b6efc48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! left some comments inline
docs/reference/commandline/create.md
Outdated
@@ -29,6 +29,10 @@ Options: | |||
--blkio-weight-device value Block IO weight (relative device weight) (default []) | |||
--cap-add value Add Linux capabilities (default []) | |||
--cap-drop value Drop Linux capabilities (default []) | |||
--cgroupns string Cgroup namespace to use | |||
'host': Run the container in the Docker host's cgroup namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be slightly easier to read if the descriptions are aligned;
--cgroupns string Cgroup namespace to use
'host': Run the container in the Docker host's cgroup namespace
'private': Run the container in its own private cgroup namespace
'': Use the default Docker daemon cgroup namespace specified by the "--default-cgroupns-mode" option (default)
man/dockerd.8.md
Outdated
@@ -177,6 +178,10 @@ $ sudo dockerd --add-runtime runc=runc --add-runtime custom=/usr/local/bin/my-ru | |||
**-D**, **--debug**=*true*|*false* | |||
Enable debug mode. Default is false. | |||
|
|||
**--default-cgroupns-mode**="**host**|**private**" | |||
Set the default cgroup namespace mode for newly created containers. The argument | |||
can either be **host** or **private**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we describe the default here as well?
@@ -471,6 +473,11 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con | |||
return nil, errors.Errorf("--userns: invalid USER mode") | |||
} | |||
|
|||
cgroupnsMode := container.CgroupnsMode(copts.cgroupnsMode) | |||
if !cgroupnsMode.Valid() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this; I'm a bit on the fence if we should validate this on the client side, or just leave it to the daemon to return an error if an invalid value was provided. OTOH, these values likely won't change in future, so perhaps it's ok
@kolyshkin wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok for now, given that it likely won't change in future, but I'll open a follow-up issue after this is merged to discuss this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bash completion LGTM, thanks.
needs rebase |
2ea22b0
to
602452f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for updating! (and sorry for the delay 😞) I left one comment about the docker-compose schema version (hoping #2073 will be merged soon), and a suggestion for the flag description output for --help
.
Otherwise looks good to me!
@@ -471,6 +473,11 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con | |||
return nil, errors.Errorf("--userns: invalid USER mode") | |||
} | |||
|
|||
cgroupnsMode := container.CgroupnsMode(copts.cgroupnsMode) | |||
if !cgroupnsMode.Valid() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok for now, given that it likely won't change in future, but I'll open a follow-up issue after this is merged to discuss this
602452f
to
fdeb2fd
Compare
@thaJeztah - Updated with your changes. Mind taking a look? |
977f2ad
to
88aba22
Compare
88aba22
to
306338f
Compare
@thaJeztah / @kolyshkin - Mind taking a look? Thanks! |
@thaJeztah PTAL? |
needs rebase |
306338f
to
298d9cd
Compare
Rebased. @thaJeztah, mind taking a look? |
298d9cd
to
15c01bb
Compare
15c01bb
to
5afc093
Compare
ping @thaJeztah |
5afc093
to
e0d3e04
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good To Me 🐯
Signed-off-by: Rob Gulewich <rgulewich@netflix.com>
e0d3e04
to
5ad1d4d
Compare
CI failure seems unrelated
|
CI green |
Whoop! Let's merge! @AkihiroSuda if you're able to assist with #2303 (that's related to CI being really flaky currently) |
- What I did
This adds the
--cgroupns=host|private
option todocker run
to allow setting the cgroup namespace mode. Fixes #1988- How I did it
Largely by copying the code for userns and ipc modes.
- How to verify it
docker run --cgroupns=private
against a daemon that's running with--default-cgroupns-mode=host
.- Description for the changelog
docker run: allow specifying cgroup namespace mode with --cgroupns
- A picture of a cute animal (not mandatory but encouraged)