Skip to content

User namespaces: graduate from experimental - #19187

Merged
thaJeztah merged 3 commits into
moby:masterfrom
estesp:lets-do-this
Jan 12, 2016
Merged

User namespaces: graduate from experimental#19187
thaJeztah merged 3 commits into
moby:masterfrom
estesp:lets-do-this

Conversation

@estesp

@estesp estesp commented Jan 8, 2016

Copy link
Copy Markdown
Contributor

Moves experimental user namespaces to master in preparation for the 1.10 release. Adds documentation to regular docker daemon command docs. Properly warns of incompatible flags for namespace sharing + privileged.

Closes: #18448
Closes: #18430
Closes: #18247

@estesp

estesp commented Jan 8, 2016

Copy link
Copy Markdown
Contributor Author

ping @thaJeztah : I spent some extra time trying to make the docs more readable/clear. Feel free to start reviewing/providing feedback as I think the docs may need more help than the code, which is basically just a migration of the flag and settings.

@thaJeztah

Copy link
Copy Markdown
Member

ping @moxiegirl as well 😄

@vdemeester

Copy link
Copy Markdown
Member

😻

@jessfraz

jessfraz commented Jan 8, 2016

Copy link
Copy Markdown
Contributor

yayayayayayayayayayayayyayaaya! LGTM

@LK4D4

LK4D4 commented Jan 8, 2016

Copy link
Copy Markdown
Contributor

yolo
LGTM

@icecrime

icecrime commented Jan 8, 2016

Copy link
Copy Markdown
Contributor

Looks YOLO To Me 👍

Adds the `--userns-remap` flag to the master build

Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
@estesp

estesp commented Jan 8, 2016

Copy link
Copy Markdown
Contributor Author

@jfrazelle just FYI--updated the patch to stop hack/make.sh from adding experimental build tag when DOCKER_REMAP_ROOT is set, given it will now be "testable" from master. Let me know if you see any issues with that, but the CI userns run should verify that's correct.

@jessfraz

jessfraz commented Jan 8, 2016

Copy link
Copy Markdown
Contributor

sweet thats perfect thanks!

On Fri, Jan 8, 2016 at 12:10 PM, Phil Estes notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle just FYI--updated the patch to
stop hack/make.sh from adding experimental build tag when
DOCKER_REMAP_ROOT is set, given it will now be "testable" from master.
Let me know if you see any issues with that, but the CI userns run
should verify that's correct.


Reply to this email directly or view it on GitHub
#19187 (comment).

@thaJeztah

Copy link
Copy Markdown
Member

@estesp don't forget to update the daemon man page as well, to include the new flag; https://github.com/docker/docker/blob/master/man/docker-daemon.8.md

@HackToday

Copy link
Copy Markdown
Contributor

@thaJeztah do you know how to test it when it was not merged ? Do we have binary build for that ? Or need I cherry-pick these code in my repo and build binary by myself ? Thanks

@vdemeester

Copy link
Copy Markdown
Member

@HackToday You can try out a pull request by checking it out using git and then build it (using make binary for example 😉)

@HackToday

Copy link
Copy Markdown
Contributor

Thanks @vdemeester I tried this clone
https://github.com/estesp/docker/tree/lets-do-this

Since I did not figure it out just cherry-pick easy ways to do that :(

@HackToday

Copy link
Copy Markdown
Contributor

Found following potential issues:

  1. use host with --uts=host or --ipc=host failed with following strange error message:

root@003d1da3734a:/go/src/github.com/docker/docker# docker run -it --uts=host myhtop pidtest-1
mount: permission denied
Could not mount /sys/kernel/security.
AppArmor detection and --privileged mode might break.
mount: permission denied
root@003d1da3734a:/go/src/github.com/docker/docker# docker run -it --ipc=host myhtop pidtest-1
mount: permission denied
Could not mount /sys/kernel/security.
AppArmor detection and --privileged mode might break.
mount: permission denied

  1. for --ipc use container:ID

root@003d1da3734a:/go/src/github.com/docker/docker# docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
5040226021d9 busybox "sh" 23 minutes ago Up 23 minutes testok
2dc6bb7c5ebb busybox "sh" 25 minutes ago Up 25 minutes goofy_northcutt
root@003d1da3734a:/go/src/github.com/docker/docker#
root@003d1da3734a:/go/src/github.com/docker/docker#
root@003d1da3734a:/go/src/github.com/docker/docker# docker run --name testsecond -it --ipc=container:testok busybox sh
docker: Error response from daemon: Container command could not be invoked..
root@003d1da3734a:/go/src/github.com/docker/docker# docker ps -a
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
98268acac542 busybox "sh" 8 seconds ago Created testsecond
5040226021d9 busybox "sh" 23 minutes ago Up 23 minutes testok
2dc6bb7c5ebb busybox "sh" 25 minutes ago Up 25 minutes goofy_northcutt

And the container was created.

@HackToday

Copy link
Copy Markdown
Contributor

And another issue:

When run with docker run --read-only -v /icanwrite busybox sh still have long go trace message like below:

5: main
Package: main
File: docker.go@18


6: main
Package: runtime
File: proc.go@111


7: goexit
Package: runtime
File: asm_amd64.s@1721
docker: Error response from daemon: Cannot start container c8bd26e061fe5d4b229581c912b0204c0dee4b3bebf82e72940895591bf74f09: [9] System error: operation not permitted.

It seems still an issue mentioned like this one #18430 (although the old one is about pid namespace)

Comment thread daemon/daemon_unix.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest replace use with share, as keep consistent above network namespace error message.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@estesp

estesp commented Jan 11, 2016

Copy link
Copy Markdown
Contributor Author

And another issue:

When run with docker run --read-only -v /icanwrite busybox sh still have long go trace message like below:

I have added an early error similar to the namespace sharing errors when using --read-only=true. The docs mention this is not possible with user namespaces, but adding the error makes it much clearer to the user. Thanks.

@estesp

estesp commented Jan 11, 2016

Copy link
Copy Markdown
Contributor Author

@thaJeztah I have made the modifications you suggested as well as added the additional info to the docker-daemon man page. Please let me know if the man page additions are sufficient of if the more complete detail from the markdown docs should be added here as well.

@HackToday I have made corrections to the docs per your suggestions and added an error message when --read-only is in use (to prevent the long complicated Go dump output). Your other issues (with shared ipc/uts) seem to be related to something your specific container is doing at startup? If I run a simple "busybox" or "ubuntu" image, I don't get these errors. I can look into it further, but maybe you can try with a standard image and then understand what your "myhtop" container is trying to do

Comment thread docs/reference/commandline/daemon.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

single dashes if you must use dashes.

- and therefore a container - 

@jamtur01

Copy link
Copy Markdown
Contributor

Nice Docs - minor edits.

@estesp

estesp commented Jan 12, 2016

Copy link
Copy Markdown
Contributor Author

Thanks for the docs review @jamtur01. I have updated the PR per your comments.

@jamtur01

Copy link
Copy Markdown
Contributor

One minor fix but otherwise Docs LGTM. Thanks!

@estesp

estesp commented Jan 12, 2016

Copy link
Copy Markdown
Contributor Author

Great! @jamtur01 I've used "superuser" instead, and the PR is updated. Thanks!

This prevents strange errors and clarifies which namespace options are
incompatible with user namespaces (at this time).

Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
Remove the experimental docs for user namespaces and add similar content
to the `docker daemon` command documentation.

Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
@cyphar

cyphar commented Jan 12, 2016

Copy link
Copy Markdown
Contributor

I think that #19251 should be considered before we make this non-experimental (quite a few systems don't have the latest version of shadow).

@HackToday

Copy link
Copy Markdown
Contributor

LGTM

@cyphar

cyphar commented Jan 12, 2016

Copy link
Copy Markdown
Contributor

@HackToday "LGTM"s should only be used by Docker maintainers (they are used by other maintainers to figure out whether there is a consensus on a feature being merged). It's just confusing to have non-maintainers LGTM things.

@HackToday

Copy link
Copy Markdown
Contributor

@cyphar sure, Ignore LGTM, I will +1 about this

@HackToday

Copy link
Copy Markdown
Contributor

👍

@cpuguy83

Copy link
Copy Markdown
Member

ping @thaJeztah for docs signoff

@thaJeztah

Copy link
Copy Markdown
Member

docs LGTM, let's merge!

thaJeztah added a commit that referenced this pull request Jan 12, 2016
User namespaces: graduate from experimental
@thaJeztah
thaJeztah merged commit c72be04 into moby:master Jan 12, 2016
@tiborvass

Copy link
Copy Markdown
Contributor

\o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.