Merge pull request #2394 from hashicorp/b-remount

Ensure dev/, proc/, and alloc/ are mounted every time a task is run

GNUmakefile

@@ -49,6 +49,7 @@ vet:
 		echo ""; \
 		echo "[LINT] Vet found suspicious constructs. Please check the reported constructs"; \
 		echo "and fix them if necessary before submitting the code for review."; \
+		exit 1; \
 	fi
 
 	@git grep -n `echo "log"".Print"` | grep -v 'vendor/' ; if [ $$? -eq 0 ]; then \

client/alloc_runner_test.go

@@ -321,7 +321,8 @@ func TestAllocRunner_SaveRestoreState(t *testing.T) {
 	}
 
 	// Create a new alloc runner
-	ar2 := NewAllocRunner(ar.logger, ar.config, upd.Update,
+	l2 := prefixedTestLogger("----- ar2:  ")
+	ar2 := NewAllocRunner(l2, ar.config, upd.Update,
 		&structs.Allocation{ID: ar.alloc.ID}, ar.vaultClient)
 	err = ar2.RestoreState()
 	if err != nil {
@@ -341,7 +342,7 @@ func TestAllocRunner_SaveRestoreState(t *testing.T) {
 		last := upd.Allocs[upd.Count-1]
 		return last.ClientStatus == structs.AllocClientStatusRunning, nil
 	}, func(err error) {
-		t.Fatalf("err: %v %#v %#v", err, upd.Allocs[0], ar.alloc.TaskStates)
+		t.Fatalf("err: %v %#v %#v", err, upd.Allocs[0], ar2.alloc.TaskStates["web"])
 	})
 
 	// Destroy and wait

client/allocdir/alloc_dir.go

@@ -416,6 +416,21 @@ func pathExists(path string) bool {
 	return true
 }
 
+// pathEmpty returns true if a path exists, is listable, and is empty. If the
+// path does not exist or is not listable an error is returned.
+func pathEmpty(path string) (bool, error) {
+	f, err := os.Open(path)
+	if err != nil {
+		return false, err
+	}
+	defer f.Close()
+	entries, err := f.Readdir(1)
+	if err != nil && err != io.EOF {
+		return false, err
+	}
+	return len(entries) == 0, nil
+}
+
 // createDir creates a directory structure inside the basepath. This functions
 // preserves the permissions of each of the subdirectories in the relative path
 // by looking up the permissions in the host.

client/allocdir/alloc_dir_test.go

@@ -106,11 +106,11 @@ func TestAllocDir_MountSharedAlloc(t *testing.T) {
 
 	// Build 2 task dirs
 	td1 := d.NewTaskDir(t1.Name)
-	if err := td1.Build(nil, cstructs.FSIsolationChroot); err != nil {
+	if err := td1.Build(false, nil, cstructs.FSIsolationChroot); err != nil {
 		t.Fatalf("error build task=%q dir: %v", t1.Name, err)
 	}
 	td2 := d.NewTaskDir(t2.Name)
-	if err := td2.Build(nil, cstructs.FSIsolationChroot); err != nil {
+	if err := td2.Build(false, nil, cstructs.FSIsolationChroot); err != nil {
 		t.Fatalf("error build task=%q dir: %v", t2.Name, err)
 	}
 
@@ -151,11 +151,11 @@ func TestAllocDir_Snapshot(t *testing.T) {
 
 	// Build 2 task dirs
 	td1 := d.NewTaskDir(t1.Name)
-	if err := td1.Build(nil, cstructs.FSIsolationImage); err != nil {
+	if err := td1.Build(false, nil, cstructs.FSIsolationImage); err != nil {
 		t.Fatalf("error build task=%q dir: %v", t1.Name, err)
 	}
 	td2 := d.NewTaskDir(t2.Name)
-	if err := td2.Build(nil, cstructs.FSIsolationImage); err != nil {
+	if err := td2.Build(false, nil, cstructs.FSIsolationImage); err != nil {
 		t.Fatalf("error build task=%q dir: %v", t2.Name, err)
 	}
 
@@ -225,7 +225,7 @@ func TestAllocDir_Move(t *testing.T) {
 	defer d2.Destroy()
 
 	td1 := d1.NewTaskDir(t1.Name)
-	if err := td1.Build(nil, cstructs.FSIsolationImage); err != nil {
+	if err := td1.Build(false, nil, cstructs.FSIsolationImage); err != nil {
 		t.Fatalf("TaskDir.Build() faild: %v", err)
 	}
 
@@ -322,7 +322,7 @@ func TestAllocDir_ReadAt_SecretDir(t *testing.T) {
 	defer d.Destroy()
 
 	td := d.NewTaskDir(t1.Name)
-	if err := td.Build(nil, cstructs.FSIsolationImage); err != nil {
+	if err := td.Build(false, nil, cstructs.FSIsolationImage); err != nil {
 		t.Fatalf("TaskDir.Build() failed: %v", err)
 	}
 
@@ -334,7 +334,7 @@ func TestAllocDir_ReadAt_SecretDir(t *testing.T) {
 }
 
 func TestAllocDir_SplitPath(t *testing.T) {
-	dir, err := ioutil.TempDir("/tmp", "tmpdirtest")
+	dir, err := ioutil.TempDir("", "tmpdirtest")
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -355,7 +355,7 @@ func TestAllocDir_SplitPath(t *testing.T) {
 }
 
 func TestAllocDir_CreateDir(t *testing.T) {
-	dir, err := ioutil.TempDir("/tmp", "tmpdirtest")
+	dir, err := ioutil.TempDir("", "tmpdirtest")
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -390,3 +390,38 @@ func TestAllocDir_CreateDir(t *testing.T) {
 		t.Fatalf("wrong file mode: %v, expected: %v", fi.Mode(), subdirMode.Mode())
 	}
 }
+
+func TestPathFuncs(t *testing.T) {
+	dir, err := ioutil.TempDir("", "nomadtest-pathfuncs")
+	if err != nil {
+		t.Fatalf("error creating temp dir: %v", err)
+	}
+	defer os.RemoveAll(dir)
+
+	missingDir := filepath.Join(dir, "does-not-exist")
+
+	if !pathExists(dir) {
+		t.Errorf("%q exists", dir)
+	}
+	if pathExists(missingDir) {
+		t.Errorf("%q does not exist", missingDir)
+	}
+
+	if empty, err := pathEmpty(dir); err != nil || !empty {
+		t.Errorf("%q is empty and exists. empty=%v error=%v", dir, empty, err)
+	}
+	if empty, err := pathEmpty(missingDir); err == nil || empty {
+		t.Errorf("%q is missing. empty=%v error=%v", missingDir, empty, err)
+	}
+
+	filename := filepath.Join(dir, "just-some-file")
+	f, err := os.Create(filename)
+	if err != nil {
+		t.Fatalf("could not create %q: %v", filename, err)
+	}
+	f.Close()
+
+	if empty, err := pathEmpty(dir); err != nil || empty {
+		t.Errorf("%q is not empty. empty=%v error=%v", dir, empty, err)
+	}
+}

client/allocdir/fs_linux.go

@@ -29,9 +29,15 @@ func linkDir(src, dst string) error {
 }
 
 // unlinkDir unmounts a bind mounted directory as Linux doesn't support
-// hardlinking directories.
+// hardlinking directories. If the dir is already unmounted no error is
+// returned.
 func unlinkDir(dir string) error {
-	return syscall.Unmount(dir, 0)
+	if err := syscall.Unmount(dir, 0); err != nil {
+		if err != syscall.EINVAL {
+			return err
+		}
+	}
+	return nil
 }
 
 // createSecretDir creates the secrets dir folder at the given path using a
@@ -72,7 +78,7 @@ func createSecretDir(dir string) error {
 // createSecretDir removes the secrets dir folder
 func removeSecretDir(dir string) error {
 	if unix.Geteuid() == 0 {
-		if err := syscall.Unmount(dir, 0); err != nil {
+		if err := unlinkDir(dir); err != nil {
 			// Ignore invalid path errors
 			if err != syscall.ENOENT {
 				return os.NewSyscallError("unmount", err)

client/allocdir/fs_linux_test.go

@@ -54,19 +54,19 @@ func TestLinuxRootSecretDir(t *testing.T) {
 	}
 	tmpdir, err := ioutil.TempDir("", "nomadtest-rootsecretdir")
 	if err != nil {
-		t.Fatalf("unable to create tempdir for test: %s", err)
+		t.Fatalf("unable to create tempdir for test: %v", err)
 	}
 	defer os.RemoveAll(tmpdir)
 
 	secretsDir := filepath.Join(tmpdir, TaskSecrets)
 
-	// removing a non-existant secrets dir should NOT error
+	// removing a nonexistent secrets dir should NOT error
 	if err := removeSecretDir(secretsDir); err != nil {
-		t.Fatalf("error removing non-existant secrets dir %q: %v", secretsDir, err)
+		t.Fatalf("error removing nonexistent secrets dir %q: %v", secretsDir, err)
 	}
 	// run twice as it should be idemptotent
 	if err := removeSecretDir(secretsDir); err != nil {
-		t.Fatalf("error removing non-existant secrets dir %q: %v", secretsDir, err)
+		t.Fatalf("error removing nonexistent secrets dir %q: %v", secretsDir, err)
 	}
 
 	// creating a secrets dir should work
@@ -87,7 +87,7 @@ func TestLinuxRootSecretDir(t *testing.T) {
 		t.Fatalf("secrets dir %q is not a directory and should be", secretsDir)
 	}
 	if err := isMount(secretsDir); err != nil {
-		t.Fatalf("secrets dir %q is not a mount: %v", err)
+		t.Fatalf("secrets dir %q is not a mount: %v", secretsDir, err)
 	}
 
 	// now remove it
@@ -102,7 +102,7 @@ func TestLinuxRootSecretDir(t *testing.T) {
 
 	// removing again should be a noop
 	if err := removeSecretDir(secretsDir); err != nil {
-		t.Fatalf("error removing non-existant secrets dir %q: %v", secretsDir, err)
+		t.Fatalf("error removing nonexistent secrets dir %q: %v", secretsDir, err)
 	}
 }
 
@@ -120,13 +120,13 @@ func TestLinuxUnprivilegedSecretDir(t *testing.T) {
 
 	secretsDir := filepath.Join(tmpdir, TaskSecrets)
 
-	// removing a non-existant secrets dir should NOT error
+	// removing a nonexistent secrets dir should NOT error
 	if err := removeSecretDir(secretsDir); err != nil {
-		t.Fatalf("error removing non-existant secrets dir %q: %v", secretsDir, err)
+		t.Fatalf("error removing nonexistent secrets dir %q: %v", secretsDir, err)
 	}
 	// run twice as it should be idemptotent
 	if err := removeSecretDir(secretsDir); err != nil {
-		t.Fatalf("error removing non-existant secrets dir %q: %v", secretsDir, err)
+		t.Fatalf("error removing nonexistent secrets dir %q: %v", secretsDir, err)
 	}
 
 	// creating a secrets dir should work
@@ -162,6 +162,6 @@ func TestLinuxUnprivilegedSecretDir(t *testing.T) {
 
 	// removing again should be a noop
 	if err := removeSecretDir(secretsDir); err != nil {
-		t.Fatalf("error removing non-existant secrets dir %q: %v", secretsDir, err)
+		t.Fatalf("error removing nonexistent secrets dir %q: %v", secretsDir, err)
 	}
 }

client/allocdir/task_dir.go

@@ -57,8 +57,10 @@ func newTaskDir(logger *log.Logger, allocDir, taskName string) *TaskDir {
 	}
 }
 
-// Build default directories and permissions in a task directory.
-func (t *TaskDir) Build(chroot map[string]string, fsi cstructs.FSIsolation) error {
+// Build default directories and permissions in a task directory. chrootCreated
+// allows skipping chroot creation if the caller knows it has already been
+// done.
+func (t *TaskDir) Build(chrootCreated bool, chroot map[string]string, fsi cstructs.FSIsolation) error {
 	if err := os.MkdirAll(t.Dir, 0777); err != nil {
 		return err
 	}
@@ -94,8 +96,12 @@ func (t *TaskDir) Build(chroot map[string]string, fsi cstructs.FSIsolation) erro
 	// If there's no isolation the task will use the host path to the
 	// shared alloc dir.
 	if fsi == cstructs.FSIsolationChroot {
-		if err := linkDir(t.SharedAllocDir, t.SharedTaskDir); err != nil {
-			return fmt.Errorf("Failed to mount shared directory for task: %v", err)
+		// If the path doesn't exist OR it exists and is empty, link it
+		empty, _ := pathEmpty(t.SharedTaskDir)
+		if !pathExists(t.SharedTaskDir) || empty {
+			if err := linkDir(t.SharedAllocDir, t.SharedTaskDir); err != nil {
+				return fmt.Errorf("Failed to mount shared directory for task: %v", err)
+			}
 		}
 	}
 
@@ -110,7 +116,7 @@ func (t *TaskDir) Build(chroot map[string]string, fsi cstructs.FSIsolation) erro
 
 	// Build chroot if chroot filesystem isolation is going to be used
 	if fsi == cstructs.FSIsolationChroot {
-		if err := t.buildChroot(chroot); err != nil {
+		if err := t.buildChroot(chrootCreated, chroot); err != nil {
 			return err
 		}
 	}
@@ -121,11 +127,15 @@ func (t *TaskDir) Build(chroot map[string]string, fsi cstructs.FSIsolation) erro
 // buildChroot takes a mapping of absolute directory or file paths on the host
 // to their intended, relative location within the task directory. This
 // attempts hardlink and then defaults to copying. If the path exists on the
-// host and can't be embedded an error is returned.
-func (t *TaskDir) buildChroot(entries map[string]string) error {
-	// Link/copy chroot entries
-	if err := t.embedDirs(entries); err != nil {
-		return err
+// host and can't be embedded an error is returned. If chrootCreated is true
+// skip expensive embedding operations and only ephemeral operations (eg
+// mounting /dev) are done.
+func (t *TaskDir) buildChroot(chrootCreated bool, entries map[string]string) error {
+	if !chrootCreated {
+		// Link/copy chroot entries
+		if err := t.embedDirs(entries); err != nil {
+			return err
+		}
 	}
 
 	// Mount special dirs

client/allocdir/task_dir_linux.go

@@ -15,10 +15,15 @@ func (t *TaskDir) mountSpecialDirs() error {
 	// Mount dev
 	dev := filepath.Join(t.Dir, "dev")
 	if !pathExists(dev) {
-		if err := os.MkdirAll(dev, 0777); err != nil {
+		if err := os.Mkdir(dev, 0777); err != nil {
 			return fmt.Errorf("Mkdir(%v) failed: %v", dev, err)
 		}
-
+	}
+	devEmpty, err := pathEmpty(dev)
+	if err != nil {
+		return fmt.Errorf("error listing %q: %v", dev, err)
+	}
+	if devEmpty {
 		if err := syscall.Mount("none", dev, "devtmpfs", syscall.MS_RDONLY, ""); err != nil {
 			return fmt.Errorf("Couldn't mount /dev to %v: %v", dev, err)
 		}
@@ -27,10 +32,15 @@ func (t *TaskDir) mountSpecialDirs() error {
 	// Mount proc
 	proc := filepath.Join(t.Dir, "proc")
 	if !pathExists(proc) {
-		if err := os.MkdirAll(proc, 0777); err != nil {
+		if err := os.Mkdir(proc, 0777); err != nil {
 			return fmt.Errorf("Mkdir(%v) failed: %v", proc, err)
 		}
-
+	}
+	procEmpty, err := pathEmpty(proc)
+	if err != nil {
+		return fmt.Errorf("error listing %q: %v", proc, err)
+	}
+	if procEmpty {
 		if err := syscall.Mount("none", proc, "proc", syscall.MS_RDONLY, ""); err != nil {
 			return fmt.Errorf("Couldn't mount /proc to %v: %v", proc, err)
 		}
@@ -39,25 +49,27 @@ func (t *TaskDir) mountSpecialDirs() error {
 	return nil
 }
 
-// unmountSpecialDirs unmounts the dev and proc file system from the chroot
+// unmountSpecialDirs unmounts the dev and proc file system from the chroot. No
+// error is returned if the directories do not exist or have already been
+// unmounted.
 func (t *TaskDir) unmountSpecialDirs() error {
 	errs := new(multierror.Error)
 	dev := filepath.Join(t.Dir, "dev")
 	if pathExists(dev) {
-		if err := syscall.Unmount(dev, 0); err != nil {
-			errs = multierror.Append(errs, fmt.Errorf("Failed to unmount dev (%v): %v", dev, err))
+		if err := unlinkDir(dev); err != nil {
+			errs = multierror.Append(errs, fmt.Errorf("Failed to unmount dev %q: %v", dev, err))
 		} else if err := os.RemoveAll(dev); err != nil {
-			errs = multierror.Append(errs, fmt.Errorf("Failed to delete dev directory (%v): %v", dev, err))
+			errs = multierror.Append(errs, fmt.Errorf("Failed to delete dev directory %q: %v", dev, err))
 		}
 	}
 
 	// Unmount proc.
 	proc := filepath.Join(t.Dir, "proc")
 	if pathExists(proc) {
-		if err := syscall.Unmount(proc, 0); err != nil {
-			errs = multierror.Append(errs, fmt.Errorf("Failed to unmount proc (%v): %v", proc, err))
+		if err := unlinkDir(proc); err != nil {
+			errs = multierror.Append(errs, fmt.Errorf("Failed to unmount proc %q: %v", proc, err))
 		} else if err := os.RemoveAll(proc); err != nil {
-			errs = multierror.Append(errs, fmt.Errorf("Failed to delete proc directory (%v): %v", dev, err))
+			errs = multierror.Append(errs, fmt.Errorf("Failed to delete proc directory %q: %v", dev, err))
 		}
 	}