Creating permanent sandbox namespace

This commit contains changes to create/delete permanent namespace
for a sandbox container.

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

pkg/server/sandbox_list_test.go

@@ -43,7 +43,7 @@ func TestToCRISandbox(t *testing.T) {
 		Name:      "test-name",
 		Config:    config,
 		CreatedAt: createdAt,
-		NetNS:     "test-netns",
+		NetNSPath: "test-netns",
 	}
 	state := runtime.PodSandboxState_SANDBOX_READY
 	expect := &runtime.PodSandbox{

pkg/server/sandbox_remove.go

@@ -78,8 +78,6 @@ func (c *criContainerdService) RemovePodSandbox(ctx context.Context, r *runtime.
 		}
 	}
 
-	// TODO(random-liu): [P1] Remove permanent namespace once used.
-
 	// Cleanup the sandbox root directory.
 	sandboxRootDir := getSandboxRootDir(c.rootDir, id)
 	if err := c.os.RemoveAll(sandboxRootDir); err != nil {

pkg/server/sandbox_run.go

@@ -77,9 +77,49 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run
 	if err != nil {
 		return nil, fmt.Errorf("failed to get sandbox image %q: %v", c.sandboxImage, err)
 	}
+	//Create Network Namespace if it is not in host network
+	hostNet := config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetHostNetwork()
+	if !hostNet {
+		// If it is not in host network namespace then create a namespace and set the sandbox
+		// handle. NetNSPath in sandbox metadata and NetNS is non empty only for non host network
+		// namespaces. If the pod is in host network namespace then both are empty and should not
+		// be used.
+		sandbox.NetNS, err = sandboxstore.NewNetNS()
+		if err != nil {
+			return nil, fmt.Errorf("failed to create network namespace for sandbox %q: %v", id, err)
+		}
+		sandbox.NetNSPath = sandbox.NetNS.GetPath()
+		defer func() {
+			if retErr != nil {
+				if err := sandbox.NetNS.Remove(); err != nil {
+					glog.Errorf("Failed to remove network namespace %s for sandbox %q: %v", sandbox.NetNSPath, id, err)
+				}
+				sandbox.NetNSPath = ""
+			}
+		}()
+		// Setup network for sandbox.
+		podNetwork := ocicni.PodNetwork{
+			Name:         config.GetMetadata().GetName(),
+			Namespace:    config.GetMetadata().GetNamespace(),
+			ID:           id,
+			NetNS:        sandbox.NetNSPath,
+			PortMappings: toCNIPortMappings(config.GetPortMappings()),
+		}
+		if err = c.netPlugin.SetUpPod(podNetwork); err != nil {
+			return nil, fmt.Errorf("failed to setup network for sandbox %q: %v", id, err)
+		}
+		defer func() {
+			if retErr != nil {
+				// Teardown network if an error is returned.
+				if err := c.netPlugin.TearDownPod(podNetwork); err != nil {
+					glog.Errorf("Failed to destroy network for sandbox %q: %v", id, err)
+				}
+			}
+		}()
+	}
 
 	// Create sandbox container.
-	spec, err := c.generateSandboxContainerSpec(id, config, image.Config)
+	spec, err := c.generateSandboxContainerSpec(id, config, image.Config, sandbox.NetNSPath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate sandbox container spec: %v", err)
 	}
@@ -165,30 +205,6 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run
 	}()
 
 	sandbox.Pid = task.Pid()
-	sandbox.NetNS = getNetworkNamespace(task.Pid())
-	if !config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetHostNetwork() {
-		// Setup network for sandbox.
-		// TODO(random-liu): [P2] Replace with permanent network namespace.
-		podNetwork := ocicni.PodNetwork{
-			Name:         config.GetMetadata().GetName(),
-			Namespace:    config.GetMetadata().GetNamespace(),
-			ID:           id,
-			NetNS:        sandbox.NetNS,
-			PortMappings: toCNIPortMappings(config.GetPortMappings()),
-		}
-		if err = c.netPlugin.SetUpPod(podNetwork); err != nil {
-			return nil, fmt.Errorf("failed to setup network for sandbox %q: %v", id, err)
-		}
-		defer func() {
-			if retErr != nil {
-				// Teardown network if an error is returned.
-				if err := c.netPlugin.TearDownPod(podNetwork); err != nil {
-					glog.Errorf("failed to destroy network for sandbox %q: %v", id, err)
-				}
-			}
-		}()
-	}
-
 	if err = task.Start(ctx); err != nil {
 		return nil, fmt.Errorf("failed to start sandbox container task %q: %v",
 			id, err)
@@ -205,7 +221,7 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run
 }
 
 func (c *criContainerdService) generateSandboxContainerSpec(id string, config *runtime.PodSandboxConfig,
-	imageConfig *imagespec.ImageConfig) (*runtimespec.Spec, error) {
+	imageConfig *imagespec.ImageConfig, nsPath string) (*runtimespec.Spec, error) {
 	// Creates a spec Generator with the default spec.
 	// TODO(random-liu): [P1] Compare the default settings with docker and containerd default.
 	spec, err := containerd.GenerateSpec()
@@ -252,15 +268,12 @@ func (c *criContainerdService) generateSandboxContainerSpec(id string, config *r
 
 	// Set namespace options.
 	nsOptions := config.GetLinux().GetSecurityContext().GetNamespaceOptions()
-	// TODO(random-liu): [P1] Create permanent network namespace, so that we could still cleanup
-	// network namespace after sandbox container dies unexpectedly.
-	// By default, all namespaces are enabled for the container, runc will create a new namespace
-	// for it. By removing the namespace, the container will inherit the namespace of the runtime.
 	if nsOptions.GetHostNetwork() {
 		g.RemoveLinuxNamespace(string(runtimespec.NetworkNamespace)) // nolint: errcheck
-		// TODO(random-liu): [P1] Figure out how to handle UTS namespace.
+	} else {
+		//TODO(Abhi): May be move this to containerd spec opts (WithLinuxSpaceOption)
+		g.AddOrReplaceLinuxNamespace(string(runtimespec.NetworkNamespace), nsPath) // nolint: errcheck
 	}
-
 	if nsOptions.GetHostPid() {
 		g.RemoveLinuxNamespace(string(runtimespec.PIDNamespace)) // nolint: errcheck
 	}

pkg/server/sandbox_run_test.go

@@ -68,6 +68,7 @@ func getRunPodSandboxTestData() (*runtime.PodSandboxConfig, *imagespec.ImageConf
 
 func TestGenerateSandboxContainerSpec(t *testing.T) {
 	testID := "test-id"
+	nsPath := "test-cni"
 	for desc, test := range map[string]struct {
 		configChange      func(*runtime.PodSandboxConfig)
 		imageConfigChange func(*imagespec.ImageConfig)
@@ -80,6 +81,7 @@ func TestGenerateSandboxContainerSpec(t *testing.T) {
 				require.NotNil(t, spec.Linux)
 				assert.Contains(t, spec.Linux.Namespaces, runtimespec.LinuxNamespace{
 					Type: runtimespec.NetworkNamespace,
+					Path: nsPath,
 				})
 				assert.Contains(t, spec.Linux.Namespaces, runtimespec.LinuxNamespace{
 					Type: runtimespec.PIDNamespace,
@@ -136,7 +138,7 @@ func TestGenerateSandboxContainerSpec(t *testing.T) {
 		if test.imageConfigChange != nil {
 			test.imageConfigChange(imageConfig)
 		}
-		spec, err := c.generateSandboxContainerSpec(testID, config, imageConfig)
+		spec, err := c.generateSandboxContainerSpec(testID, config, imageConfig, nsPath)
 		if test.expectErr {
 			assert.Error(t, err)
 			assert.Nil(t, spec)

pkg/server/sandbox_status.go

@@ -64,7 +64,7 @@ func (c *criContainerdService) PodSandboxStatus(ctx context.Context, r *runtime.
 			state = runtime.PodSandboxState_SANDBOX_READY
 		}
 	}
-	ip, err := c.netPlugin.GetPodNetworkStatus(sandbox.NetNS)
+	ip, err := c.netPlugin.GetPodNetworkStatus(sandbox.NetNSPath)
 	if err != nil {
 		// Ignore the error on network status
 		ip = ""

pkg/server/sandbox_stop.go

@@ -63,22 +63,29 @@ func (c *criContainerdService) StopPodSandbox(ctx context.Context, r *runtime.St
 	}
 
 	// Teardown network for sandbox.
-	_, err = c.os.Stat(sandbox.NetNS)
-	if err == nil {
-		if !sandbox.Config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetHostNetwork() {
-			if teardownErr := c.netPlugin.TearDownPod(ocicni.PodNetwork{
-				Name:         sandbox.Config.GetMetadata().GetName(),
-				Namespace:    sandbox.Config.GetMetadata().GetNamespace(),
-				ID:           id,
-				NetNS:        sandbox.NetNS,
-				PortMappings: toCNIPortMappings(sandbox.Config.GetPortMappings()),
-			}); teardownErr != nil {
-				return nil, fmt.Errorf("failed to destroy network for sandbox %q: %v", id, teardownErr)
-			}
+	if sandbox.NetNSPath != "" {
+		if _, err := os.Stat(sandbox.NetNSPath); err != nil {
+			return nil, fmt.Errorf("failed to stat network namespace path %s :%v", sandbox.NetNSPath, err)
+		}
+		if teardownErr := c.netPlugin.TearDownPod(ocicni.PodNetwork{
+			Name:         sandbox.Config.GetMetadata().GetName(),
+			Namespace:    sandbox.Config.GetMetadata().GetNamespace(),
+			ID:           id,
+			NetNS:        sandbox.NetNSPath,
+			PortMappings: toCNIPortMappings(sandbox.Config.GetPortMappings()),
+		}); teardownErr != nil {
+			return nil, fmt.Errorf("failed to destroy network for sandbox %q: %v", id, teardownErr)
+		}
+		/*TODO:It is still possible that cri-containerd crashes after we teardown the network, but before we remove the network namespace.
+		In that case, we'll not be able to remove the sandbox anymore. The chance is slim, but we should be aware of that.
+		In the future, once TearDownPod is idempotent, this will be fixed.*/
+
+		//Close the sandbox network namespace if it was created
+		if err = sandbox.NetNS.Remove(); err != nil {
+			return nil, fmt.Errorf("failed to remove network namespace for sandbox %q:  %v", id, err)
 		}
-	} else if !os.IsNotExist(err) { // It's ok for sandbox.NetNS to *not* exist
-		return nil, fmt.Errorf("failed to stat netns path for sandbox %q before tearing down the network: %v", id, err)
 	}
+
 	glog.V(2).Infof("TearDown network for sandbox %q successfully", id)
 
 	sandboxRoot := getSandboxRootDir(c.rootDir, id)

pkg/store/sandbox/metadata.go

@@ -51,8 +51,8 @@ type Metadata struct {
 	CreatedAt int64
 	// Pid is the process id of the sandbox.
 	Pid uint32
-	// NetNS is the network namespace used by the sandbox.
-	NetNS string
+	// NetNSPath is the network namespace used by the sandbox.
+	NetNSPath string
 }
 
 // Encode encodes Metadata into bytes in json format.

pkg/store/sandbox/netns.go

@@ -0,0 +1,64 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package sandbox
+
+import (
+	"fmt"
+	"sync"
+
+	cnins "github.com/containernetworking/plugins/pkg/ns"
+)
+
+// NetNS holds network namespace for sandbox
+type NetNS struct {
+	sync.Mutex
+	ns     cnins.NetNS
+	closed bool
+}
+
+// NewNetNS creates a network namespace for the sandbox
+func NewNetNS() (*NetNS, error) {
+	netns, err := cnins.NewNS()
+	if err != nil {
+		return nil, fmt.Errorf("failed to setup network namespace %v", err)
+	}
+	n := new(NetNS)
+	n.ns = netns
+	return n, nil
+}
+
+// Remove removes network namepace if it exists and not closed. Remove is idempotent,
+// meaning it might be invoked multiple times and provides consistent result.
+func (n *NetNS) Remove() error {
+	n.Lock()
+	defer n.Unlock()
+	if !n.closed {
+		err := n.ns.Close()
+		if err != nil {
+			return err
+		}
+		n.closed = true
+	}
+	return nil
+}
+
+// GetPath returns network namespace path for sandbox container
+func (n *NetNS) GetPath() string {
+	n.Lock()
+	defer n.Unlock()
+	return n.ns.Path()
+}

pkg/store/sandbox/sandbox.go

@@ -31,7 +31,8 @@ type Sandbox struct {
 	Metadata
 	// Containerd sandbox container
 	Container containerd.Container
-	// TODO(random-liu): Add cni network namespace client.
+	// CNI network namespace client
+	NetNS *NetNS
 }
 
 // Store stores all sandboxes.

pkg/store/sandbox/sandbox_test.go

@@ -42,7 +42,7 @@ func TestSandboxStore(t *testing.T) {
 			},
 			CreatedAt: time.Now().UnixNano(),
 			Pid:       1001,
-			NetNS:     "TestNetNS-1",
+			NetNSPath: "TestNetNS-1",
 		},
 		"2": {
 			ID:   "2",
@@ -57,7 +57,7 @@ func TestSandboxStore(t *testing.T) {
 			},
 			CreatedAt: time.Now().UnixNano(),
 			Pid:       1002,
-			NetNS:     "TestNetNS-2",
+			NetNSPath: "TestNetNS-2",
 		},
 		"3": {
 			ID:   "3",
@@ -72,7 +72,7 @@ func TestSandboxStore(t *testing.T) {
 			},
 			CreatedAt: time.Now().UnixNano(),
 			Pid:       1003,
-			NetNS:     "TestNetNS-3",
+			NetNSPath: "TestNetNS-3",
 		},
 	}
 	assert := assertlib.New(t)