Unmount /dev/shm when stop sandbox.

Signed-off-by: Lantao Liu <lantaol@google.com>

pkg/os/os.go

@@ -22,6 +22,7 @@ import (
 	"os"
 
 	"github.com/containerd/fifo"
+	"github.com/docker/docker/pkg/mount"
 	"golang.org/x/net/context"
 	"golang.org/x/sys/unix"
 )
@@ -90,7 +91,12 @@ func (RealOS) Mount(source string, target string, fstype string, flags uintptr,
 	return unix.Mount(source, target, fstype, flags, data)
 }
 
-// Unmount will call unix.Unmount to unmount the file.
+// Unmount will call unix.Unmount to unmount the file. The function doesn't
+// return error if target is not mounted.
 func (RealOS) Unmount(target string, flags int) error {
+	// TODO(random-liu): Follow symlink to make sure the result is correct.
+	if mounted, err := mount.Mounted(target); err != nil || !mounted {
+		return err
+	}
 	return unix.Unmount(target, flags)
 }

pkg/server/sandbox_remove.go

@@ -73,7 +73,6 @@ func (c *criContainerdService) RemovePodSandbox(ctx context.Context, r *runtime.
 		glog.V(5).Infof("Remove called for snapshot %q that does not exist", id)
 	}
 
-	// TODO(random-liu): [P0] Cleanup shm created in RunPodSandbox.
 	// TODO(random-liu): [P1] Remove permanent namespace once used.
 
 	// Cleanup the sandbox root directory.

pkg/server/sandbox_run.go

@@ -183,7 +183,10 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run
 	}
 	defer func() {
 		if retErr != nil {
-			c.cleanupSandboxFiles(sandboxRootDir, config)
+			if err = c.unmountSandboxFiles(sandboxRootDir, config); err != nil {
+				glog.Errorf("Failed to unmount sandbox files in %q: %v",
+					sandboxRootDir, err)
+			}
 		}
 	}()
 
@@ -403,12 +406,15 @@ func parseDNSOptions(servers, searches, options []string) (string, error) {
 	return resolvContent, nil
 }
 
-// cleanupSandboxFiles only unmount files, we rely on the removal of sandbox root directory to remove files.
-// Each cleanup task should log error instead of returning, so as to keep on cleanup on error.
-func (c *criContainerdService) cleanupSandboxFiles(rootDir string, config *runtime.PodSandboxConfig) {
+// unmountSandboxFiles unmount some sandbox files, we rely on the removal of sandbox root directory to
+// remove these files. Unmount should *NOT* return error when:
+//  1) The mount point is already unmounted.
+//  2) The mount point doesn't exist.
+func (c *criContainerdService) unmountSandboxFiles(rootDir string, config *runtime.PodSandboxConfig) error {
 	if !config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetHostIpc() {
-		if err := c.os.Unmount(getSandboxDevShm(rootDir), unix.MNT_DETACH); err != nil && os.IsNotExist(err) {
-			glog.Errorf("failed to unmount sandbox shm: %v", err)
+		if err := c.os.Unmount(getSandboxDevShm(rootDir), unix.MNT_DETACH); err != nil && !os.IsNotExist(err) {
+			return err
 		}
 	}
+	return nil
 }

pkg/server/sandbox_stop.go

@@ -58,6 +58,11 @@ func (c *criContainerdService) StopPodSandbox(ctx context.Context, r *runtime.St
 	}
 	glog.V(2).Infof("TearDown network for sandbox %q successfully", id)
 
+	sandboxRoot := getSandboxRootDir(c.rootDir, id)
+	if err = c.unmountSandboxFiles(sandboxRoot, sandbox.Config); err != nil {
+		return nil, fmt.Errorf("failed to unmount sandbox files in %q: %v", sandboxRoot, err)
+	}
+
 	// TODO(random-liu): [P1] Handle sandbox container graceful deletion.
 	// Delete the sandbox container from containerd.
 	_, err = c.taskService.Delete(ctx, &execution.DeleteRequest{ContainerID: id})

pkg/server/sandbox_stop_test.go

@@ -58,6 +58,7 @@ func TestStopPodSandbox(t *testing.T) {
 		injectErr        error
 		injectStatErr    error
 		injectCNIErr     error
+		injectUnmountErr error
 		expectErr        bool
 		expectCalls      []string
 		expectedCNICalls []string
@@ -115,6 +116,15 @@ func TestStopPodSandbox(t *testing.T) {
 			injectCNIErr:     errors.New("arbitrary error"),
 			expectCalls:      []string{},
 		},
+		"stop sandbox with unmount error": {
+			sandboxTasks:     []task.Task{testContainer},
+			injectSandbox:    true,
+			expectErr:        true,
+			expectedCNICalls: []string{"TearDownPod"},
+			injectCNIErr:     errors.New("arbitrary error"),
+			injectUnmountErr: errors.New("arbitrary error"),
+			expectCalls:      []string{},
+		},
 	} {
 		t.Logf("TestCase %q", desc)
 		c := newTestCRIContainerdService()
@@ -136,6 +146,9 @@ func TestStopPodSandbox(t *testing.T) {
 		if test.injectStatErr != nil {
 			fakeOS.InjectError("Stat", test.injectStatErr)
 		}
+		if test.injectUnmountErr != nil {
+			fakeOS.InjectError("Unmount", test.injectUnmountErr)
+		}
 		fakeCNIPlugin.SetFakePodNetwork(testSandbox.NetNS, testSandbox.Config.GetMetadata().GetNamespace(),
 			testSandbox.Config.GetMetadata().GetName(), testID, sandboxStatusTestIP)