From e08f13ea319c5ecb9584fc3e12d886aca6ee9d20 Mon Sep 17 00:00:00 2001
From: gabrielle beyer <gabrielle.n.beyer@intel.com>
Date: Tue, 11 Jun 2019 16:57:20 +0000
Subject: [PATCH] vc: error handling for bindUnmount functionalities

Add error handling surrounding the syscall of unmounting the
container rootfs. Include a unit test to check that missing
files are not considered errors when attempting to unmount.

Fixes: #164

Signed-off-by: gabrielle beyer <gabrielle.n.beyer@intel.com>
---
 virtcontainers/api_test.go   |  8 ++++++--
 virtcontainers/mount.go      | 22 +++++++++++++++-------
 virtcontainers/mount_test.go | 20 ++++++++++++++++++++
 3 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go
index a3895a8853..3d97b4b631 100644
--- a/virtcontainers/api_test.go
+++ b/virtcontainers/api_test.go
@@ -327,7 +327,9 @@ func TestStartSandboxKataAgentSuccessful(t *testing.T) {
 
 	// TODO: defaultSharedDir is a hyper var = /run/hyper/shared/sandboxes
 	// do we need to unmount sandboxes and containers?
-	bindUnmountAllRootfs(ctx, testDir, pImpl)
+	if err := bindUnmountAllRootfs(ctx, testDir, pImpl); err != nil {
+		t.Fatal(err)
+	}
 
 }
 
@@ -535,7 +537,9 @@ func TestRunSandboxKataAgentSuccessful(t *testing.T) {
 	pImpl, ok := p.(*Sandbox)
 	assert.True(t, ok)
 
-	bindUnmountAllRootfs(ctx, testDir, pImpl)
+	if err := bindUnmountAllRootfs(ctx, testDir, pImpl); err != nil {
+		t.Fatal(err)
+	}
 }
 
 func TestRunSandboxFailing(t *testing.T) {
diff --git a/virtcontainers/mount.go b/virtcontainers/mount.go
index 8d7a201549..2a77285ece 100644
--- a/virtcontainers/mount.go
+++ b/virtcontainers/mount.go
@@ -15,6 +15,9 @@ import (
 	"path/filepath"
 	"strings"
 	"syscall"
+
+	merr "github.com/hashicorp/go-multierror"
+	"github.com/sirupsen/logrus"
 )
 
 // DefaultShmSize is the default shm size to be used in case host
@@ -330,23 +333,28 @@ func bindUnmountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID s
 	defer span.Finish()
 
 	rootfsDest := filepath.Join(sharedDir, sandboxID, cID, rootfsDir)
-	syscall.Unmount(rootfsDest, syscall.MNT_DETACH)
-
-	return nil
+	err := syscall.Unmount(rootfsDest, syscall.MNT_DETACH)
+	if err == syscall.ENOENT {
+		logrus.Warnf("%s: %s", err, rootfsDest)
+		return nil
+	}
+	return err
 }
 
-func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbox) {
+func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbox) error {
 	span, _ := trace(ctx, "bindUnmountAllRootfs")
 	defer span.Finish()
 
+	var errors *merr.Error
 	for _, c := range sandbox.containers {
 		c.unmountHostMounts()
 		if c.state.Fstype == "" {
-			// Need to check for error returned by this call.
-			// See: https://github.com/containers/virtcontainers/issues/295
-			bindUnmountContainerRootfs(c.ctx, sharedDir, sandbox.id, c.id)
+			// even if error found, don't break out of loop until all mounts attempted
+			// to be unmounted, and collect all errors
+			errors = merr.Append(errors, bindUnmountContainerRootfs(c.ctx, sharedDir, sandbox.id, c.id))
 		}
 	}
+	return errors.ErrorOrNil()
 }
 
 const (
diff --git a/virtcontainers/mount_test.go b/virtcontainers/mount_test.go
index 64cbe61b64..1809d36eb6 100644
--- a/virtcontainers/mount_test.go
+++ b/virtcontainers/mount_test.go
@@ -339,3 +339,23 @@ func TestIsEphemeralStorage(t *testing.T) {
 	isHostEmptyDir = Isk8sHostEmptyDir(sampleEphePath)
 	assert.False(t, isHostEmptyDir)
 }
+
+// TestBindUnmountContainerRootfsENOENTNotError tests that if a file
+// or directory attempting to be unmounted doesn't exist, then it
+// is not considered an error
+func TestBindUnmountContainerRootfsENOENTNotError(t *testing.T) {
+	testMnt := "/tmp/test_mount"
+	sID := "sandIDTest"
+	cID := "contIDTest"
+
+	// check to make sure the file doesn't exist
+	testPath := filepath.Join(testMnt, sID, cID, rootfsDir)
+	if _, err := os.Stat(testPath); !os.IsNotExist(err) {
+		if err := os.Remove(testPath); err != nil {
+			t.Fatalf("test mount file should not exist, and cannot be removed: %s", err)
+		}
+	}
+
+	err := bindUnmountContainerRootfs(context.Background(), testMnt, sID, cID)
+	assert.Nil(t, err)
+}