Introduce/document new IPC modes#367
Conversation
kolyshkin
requested review from
mdlinville,
thaJeztah and
vdemeester
as code owners
|
I guess the tests will fail as it requires vendoring moby/moby |
Codecov Report
@@ Coverage Diff @@
## master #367 +/- ##
==========================================
- Coverage 46.25% 46.23% -0.02%
==========================================
Files 193 193
Lines 16097 16093 -4
==========================================
- Hits 7445 7441 -4
Misses 8263 8263
Partials 389 389 |
mdlinville
left a comment
mdlinville
left a comment
There was a problem hiding this comment.
LGTM assuming that you also added these command-line args to the code in moby/moby so that the auto-generation of the CLI docs will pick them up.
docs/reference/run.md
Outdated
| ## IPC settings (--ipc) | ||
|
|
||
| --ipc="" : Set the IPC mode for the container, | ||
| 'private': own IPC namespace |
There was a problem hiding this comment.
This needs the new none mode as well
Also we should remove the client-side validation of acceptable options, per moby/moby#34087 (review)
One thing I noticed that has to be fixed is this;
$ docker run --ipc=none -dit --name bar busybox
docker: --ipc: invalid IPC mode.Looks like there's validation performed in the CLI; cli/command/container/opts.go#L424-L427.
We should remove that, as it's the daemon that should validate if a value is valid (important if the CLI version doesn't match the daemon version)
kolyshkin
force-pushed
the
ipcmode
branch
3 times, most recently
from
847088f to
682b7b8
Compare
This builds (and depends) on moby/moby#34087 Version 2: - remove --ipc argument validation (it is now done by daemon) - add/document 'none' value - docs/reference/run.md: add a table with better modes description - dockerd(8) typesetting fixes Version 3: - remove ipc mode tests from cli/command/container/opts_test.go Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
changed the title
|
This is no longer WIP |
|
Anything else I need to do in order to get it merged? |
|
@kolyshkin was waiting for the moby/moby PR to get merge 😛. It's merge so let's merge this one too 👍 |
6 participants
This builds (and depends) on moby/moby#34087