don't try to create devices in a user namespace#19182
don't try to create devices in a user namespace#19182hallyn wants to merge 1 commit intomoby:masterfrom
Conversation
|
Hey @hallyn; this seems reasonable at first blush, but if you want the lxd import to work, you will need to modify |
pkg/archive/archive_unix.go
Outdated
There was a problem hiding this comment.
we shouldn't have to import a lib for this functionality though :) this shouldnt be hard to test by checking proc or something
There was a problem hiding this comment.
True. I went and looked at the function; it reads /proc/self/uid_map with some validation that maps are actually in use. As much as code reuse is good, vendoring in that tree seems potentially overkill. This may be a useful as a utility fn across the engine--what do you think of adding the same function to Docker @hallyn?
There was a problem hiding this comment.
On Fri, Jan 08, 2016 at 09:27:46AM -0800, Phil Estes wrote:
@@ -57,6 +58,10 @@ func minor(device uint64) uint64 {
// handleTarTypeBlockCharFifo is an OS-specific helper function used by
// createTarFile to handle the following types of header: Block; Char; Fifo
func handleTarTypeBlockCharFifo(hdr *tar.Header, path string) error {
- if shared.RunningInUserNS() {
True. I went and looked at the function; it reads
/proc/self/uid_mapwith some validation that maps are actually in use. As much as code reuse is good, vendoring in that tree seems potentially overkill. This may be a useful as a utility fn across the engine--what do you think of adding the same function to Docker @hallyn?
100% agreed - since I'm not particularly familiar with the coding style I
was hoping someone who was would reply with a counter-PR and avoid any
bike-shedding :)
But I'll go ahead and take a stab in a few hours
|
Hi - I will put a shared function in opencontainer |
|
That would be preferable over importing lxd for one function. |
80e1422 to
6723fd5
Compare
|
So I would guess that the failures are due to my updating the vendor libcontainer source. I don't know how to best handle that. I could simply copy the RunningInUserNS() function into docker's own system package and use it from there, but sharing it from libcontainer seemed cleaner. But I'm not up to fixing failures from a libcontainer update. What is preferred? |
|
I can see the same issues in #19250, it looks like there's some upstream bug in libcontainer. I'm happy to take a look to see what I can do about it. Once #19250 is merged, you can Also, you should have two separate commits IMO (one for the vendor update and one for using the new bits in Docker) -- it makes it much nicer for people in the future who are trying to bisect. |
|
awesome, thanks
|
|
@crosbymichael hi - this is also waiting for the libcontainer bump, as #19250 is. |
|
thank - i'll split it into two commits, and i think i'll post two competing branches. One which has the second commit updating vendor, and one which puts the needed function into docker's system library itself. |
|
@hallyn There's no need to do two PRs, just split it into two commits in the same PR (just because it makes bisecting much, much easier). |
7c2b2c9 to
a007deb
Compare
|
@cyphar, no, that was going to be two different, conflicting PRs. I decided to just do the one which moves the function into docker's own pkg/system and see what feedback that gets. |
34e9069 to
cfff8ef
Compare
|
The 'experimental' test failure looks to my untrained eye like a transient network error. Could that be? |
If we are running in a user namespace, don't try to mknod as it won't be allowed. libcontainer will bind-mount the host's devices over files in the container anyway, so it's not needed. This adds a RunningInUserNS function to docker/pkg/system. The same function exists in libcontainer, but that cannot currently be imported due to unrelated bugs. Once those bugs are fixed there is no reason why we could not start using the one from libcontainer. Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
|
@estesp what should we do with this? move to code review? |
|
So, I think the only issue is that this same function has been added to libcontainer (opencontainers/runc#458) by @hallyn and, even though we are somewhat stuck until post-1.10 on doing a full libcontainer vendor update, it seems reasonable to wait for that rather than duplicate the function again in |
|
@estesp no, no rush, just trying to get the count of PRs down. So can we close this one then per your comment? |
|
@tiborvass I think that's fine. @hallyn once we have a libcontainer update feel free to create a new PR using the imported function from libcontainer. Thanks! |
If docker is being run inside a user namespace, it won't be able
to create devices. Since libcontainer will try to bind mount the
devices anyway this is fine - ignore it.
Signed-off-by: Serge Hallyn serge.hallyn@ubuntu.com