shim: Start shims inside appropriate namespaces #637

sboeuf · 2018-02-24T00:46:47Z

This PR first creates a new package nsenter so that we can spawn any code into a set of namespaces. Then, the virtcontainers code is modified to rely on this new package. At last, it starts shims from agents within the right PID namespace. In details, a shim representing a container process should enter its own PID namespace so that we can spawn other shims corresponding to exec'ed processes inside the same namespace. This is needed if we don't want some race conditions to happen, due to the exec'ed shims to never return their exit codes if the container process returned first.

Fixes #613

jodh-intel · 2018-02-26T09:11:23Z

pkg/nsenter/nsenter.go

+}
+
+func getNSPathFromPID(pid int, nsType NSType) string {
+	return fmt.Sprintf("/proc/%d/ns/%s", pid, nsType)


Nit: (file)path.Join() maybe (here and below)?

yes I will replace it.

jodh-intel · 2018-02-26T09:12:40Z

pkg/nsenter/nsenter.go

@@ -0,0 +1,183 @@
+//
+// Copyright (c) 2018 Intel Corporation


The commit mentions "Based on the CNI implementation of the
package 'ns' on the plugins repository", so do you need to reference some of those details here? Or is this a clean-room re-implementation?

This is really a brand new re-implementation here. I have just got inspiration from what the ns package is doing for CNI.

If there's anything that was loosely copied from the ns package, we have to include the right copyrights.

Ok I understand, then I'll add them.

jodh-intel · 2018-02-26T09:13:46Z

pkg/nsenter/nsenter.go

+
+// List of namespace types.
+const (
+	NSTypeCGroup NSType = "cgroup"


Bonus points for sorting the NSType's and CloneFlagsTable alphabetically 😄

jodh-intel · 2018-02-26T09:15:32Z

pkg/nsenter/nsenter.go

+	return file, nil
+}
+
+func sortNSList(nsList []Namespace) []Namespace {


Could you add a unit-test for this?

Definitely !

jodh-intel · 2018-02-26T09:21:24Z

pkg/nsenter/nsenter.go

+
+// NsEnter executes the passed closure under the given namespace,
+// restoring the original namespace afterwards.
+func NsEnter(unsortedNSList []Namespace, toRun func() error) error {


Given the importance and delicacy of this change, we really could do with some unit tests. I think you should be able to write unit tests for these functions that for examle create a new ns, switch to it and record their (hopefully new) namespace, assuming they all start with:

if os.Geteuid() != 0 { t.Skip(testDisabledAsNonRoot) }

Yes, I was planning on implementing some testing here. I'll work on that today.

amshinde · 2018-02-26T17:25:57Z

pkg/nsenter/nsenter.go

+	NSTypeCGroup NSType = "cgroup"
+	NSTypeIPC           = "ipc"
+	NSTypeNet           = "net"
+	NSTypeMount         = "mnt"


@sboeuf As discussed I had tried doing the exact same thing taking inspiration from the CNI implementation for net namespaces. Unfortunately setns in golang does not work for mount namespaces due to multithreaded nature of golang. We may have to resort to C code to be able to switch to mount namespaces, similar to what libcontainer does.

If we are trying to solve the shim exit bug for exec, we may get away with solving this by just switching to PID namespaces.

amshinde · 2018-02-26T22:27:36Z

@sboeuf Did some digging up on the issues filed surrounding the mount namespace issue when I was implementing support for devices. See this:
golang/go#8676

Also, that issue has code that checks if the namespace change was successful, we could do something similar in our tests:
https://storage.googleapis.com/go-attachment/8676/0/checkns.c

amshinde · 2018-02-26T22:36:20Z

@sboeuf Doing things the way you are doing currently works (most of the time) and provides for a simple solution (instead of reexecing a new process), but I came across this article which does state that it is not 100% foolproof : https://www.weave.works/blog/linux-namespaces-and-go-don-t-mix

Basically golang may spawn a new thread for a blocking syscall even after LockOsThread()
We have an issue in vishwanda/netlink repo related to the above:
vishvananda/netns#17

The issue seems to have been resolved in golang 1.10, but we may see an unsuccessful setns call in earlier versions.

Doing things in the shim code, may not be such a bad approach after all.

sboeuf · 2018-02-27T06:24:38Z

@jodh-intel PR updated with almost full coverage for unit tests and more than that a real test that we're entering namespaces.
As @amshinde explained, we have some limitations with Go, even if this code is supposed to be foolproof.

About limitations, both mount and user namespaces cannot be entered with the Setns() syscall from a Go program because it's multithreaded. I honestly don't get the "why" of this limitation but after all I have read and my local testing, I can confirm we cannot enter those namespaces if we don't involve some C code, or the use of an external binary such as nsenter.
About being foolproof, I think this is safe since we know that we should not pass a callback involving a go routine through this package, otherwise it won't be executed in the expected namespace. Also, the second issue about having some new threads being created in case the current one takes too long should not happen in our case and will be solved with Go 1.10.

Basically, this package does the most that a Go package can do with netns. So my proposal is that we could take it in since it will help spawning our shims into expected PID and network namespaces, and worst case, it won't do any harm in case we hit the issues explained above once every 10000000000 times.

At least this whole PR brings the structure for the shims to be switched to the expected namespaces.

I think we should try to look into a better way to solve this, but this will involve more researches.

sboeuf · 2018-02-27T22:08:25Z

@sameo @jodh-intel @amshinde PR is ready for another review ;)

amshinde · 2018-02-28T19:38:31Z

pkg/nsenter/nsenter.go

+	return nil
+}
+
+func getFileFromNS(nsPath string) (*os.File, error) {


I know this function is not exported, but it is not very intuitive what this function does without looking at the code. Could you add some documentation comments here?

amshinde · 2018-02-28T19:48:31Z

pkg/nsenter/nsenter.go

+	go func() {
+		defer wg.Done()
+		runtime.LockOSThread()
+		innerError = containedCall()


Shouldn't there be an UnlockOsThread() call here?

Yes sure I think we can do that. But it won't change anything since this goroutine ends after that.

amshinde · 2018-02-28T19:59:17Z

pkg/nsenter/nsenter_test.go

+	assert.Nil(t, err, "Should not fail since closure should return nil: %v", err)
+}
+
+func areNSIdentical(nsPath1, nsPath2 string) (bool, error) {


I dont think you are calling this function anywhere in the tests.

Oh yeah you're right, that was a temp function that I should have removed !

amshinde · 2018-02-28T20:04:08Z

pkg/nsenter/nsenter_test.go

+
+		evalExpectedNSPath, err := filepath.EvalSymlinks(ns.Path)
+		if err != nil {
+			evalExpectedNSPath = err.(*os.PathError).Path


Might be useful to add comments here describing how you are doing the comparison.

Yes good point

amshinde · 2018-02-28T20:12:33Z

hyperstart_agent.go

@@ -314,7 +315,22 @@ func (h *hyper) exec(pod *Pod, c Container, cmd Cmd) (*Process, error) {
 		Process:   *hyperProcess,
 	}

-	process, err := prepareAndStartShim(pod, h.shim, c.id, token, h.state.URL, cmd)


I prefer passing additional arguments here rather than duplicating the code.

Fair enough ! I have removed it.

This package goal is to make very simple the execution of any code inside any set of namespaces. Based on the CNI implementation of the package 'ns' on the plugins repository, this new package takes a list of namespaces as input and a routine that should be called once all those namespaces have been entered. This way, this package provide a generic way to spawn any binary into the expected set of namespaces. It exports only one function NsEnter() making the usage very simple. Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

This commit gives the ability to start a new shim process inside a set of new namespaces. This will be useful to start the container process which should be "init" process of its PID namespace. Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

This new commit adds to the shim the ability to be run within any existing namespace. This will be very useful in order to start shims representing exec'ed processes related to an existing container. In this case, we will need to start those shims in the namespaces of the container process shim. Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

The shim representing the container process needs to be in its own PID namespace as the "init" process. The reason is that we need to make sure it will wait for its exec'ed processes in the same PID namespace to be killed before to return. This happens inside each agent, but if this is not reproduced on the host, we get some race conditions leading the shim container process to return before its exec'ed processes are killed. And this leads those shims to never return their exit codes. This commit solves this issue by making sure a new shim representing a container process enters a new PID namespace, and any exec'ed process related to this container enters this PID namespace previously created. Fixes #613 Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

sboeuf · 2018-02-28T23:24:00Z

@amshinde PR updated, PTAL

amshinde

lgtm

sboeuf · 2018-03-02T06:39:47Z

@grahamwhaley @jodh-intel ready to be merged

sboeuf added in progress review and removed in progress labels Feb 24, 2018

sboeuf force-pushed the shim_enter_ns branch 2 times, most recently from b206a54 to a3b5c9b Compare February 24, 2018 01:47

jodh-intel reviewed Feb 26, 2018

View reviewed changes

amshinde reviewed Feb 26, 2018

View reviewed changes

sboeuf force-pushed the shim_enter_ns branch 2 times, most recently from cbf2a11 to acfe46d Compare February 27, 2018 01:02

sboeuf force-pushed the shim_enter_ns branch from acfe46d to 9f1b05c Compare February 27, 2018 18:42

amshinde suggested changes Feb 28, 2018

View reviewed changes

Sebastien Boeuf added 4 commits February 28, 2018 14:59

sboeuf force-pushed the shim_enter_ns branch from 9f1b05c to 5c73774 Compare February 28, 2018 23:23

sboeuf mentioned this pull request Mar 1, 2018

vendor: update virtcontainers to pull in latest kata gRPC updates clearcontainers/runtime#1039

Closed

amshinde approved these changes Mar 1, 2018

View reviewed changes

grahamwhaley merged commit 24ee778 into master Mar 2, 2018

grahamwhaley removed the review label Mar 2, 2018

sboeuf deleted the shim_enter_ns branch March 2, 2018 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shim: Start shims inside appropriate namespaces #637

shim: Start shims inside appropriate namespaces #637

sboeuf commented Feb 24, 2018

jodh-intel Feb 26, 2018

sboeuf Feb 26, 2018

jodh-intel Feb 26, 2018

sboeuf Feb 26, 2018

sameo Feb 27, 2018

sboeuf Feb 27, 2018

jodh-intel Feb 26, 2018

sboeuf Feb 26, 2018

jodh-intel Feb 26, 2018

sboeuf Feb 26, 2018

jodh-intel Feb 26, 2018

sboeuf Feb 26, 2018

amshinde Feb 26, 2018

amshinde commented Feb 26, 2018

amshinde commented Feb 26, 2018

sboeuf commented Feb 27, 2018

sboeuf commented Feb 27, 2018

amshinde Feb 28, 2018

sboeuf Feb 28, 2018

amshinde Feb 28, 2018

sboeuf Feb 28, 2018

amshinde Feb 28, 2018

sboeuf Feb 28, 2018

amshinde Feb 28, 2018

sboeuf Feb 28, 2018

amshinde Feb 28, 2018

sboeuf Feb 28, 2018

sboeuf commented Feb 28, 2018

amshinde left a comment

sboeuf commented Mar 2, 2018

shim: Start shims inside appropriate namespaces #637

shim: Start shims inside appropriate namespaces #637

Conversation

sboeuf commented Feb 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshinde commented Feb 26, 2018

amshinde commented Feb 26, 2018

sboeuf commented Feb 27, 2018

sboeuf commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sboeuf commented Feb 28, 2018

amshinde left a comment

Choose a reason for hiding this comment

sboeuf commented Mar 2, 2018