From ba87cb8a7f6b0a04a8d7588b44de34e57651cc29 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 2 Mar 2017 13:20:05 -0800 Subject: [PATCH 1/9] Fix typos --- client/allocdir/fs_linux_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/client/allocdir/fs_linux_test.go b/client/allocdir/fs_linux_test.go index b2aa3c29e452..d328755e4810 100644 --- a/client/allocdir/fs_linux_test.go +++ b/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 @@ -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) } } From 94ce9d2146624d874b30bf49d9b83149f8af9205 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 2 Mar 2017 13:20:47 -0800 Subject: [PATCH 2/9] unlinkDir should not error if already unlinked --- client/allocdir/fs_linux.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/client/allocdir/fs_linux.go b/client/allocdir/fs_linux.go index 09e8ce54bdd7..542fe46acf44 100644 --- a/client/allocdir/fs_linux.go +++ b/client/allocdir/fs_linux.go @@ -30,8 +30,15 @@ func linkDir(src, dst string) error { // unlinkDir unmounts a bind mounted directory as Linux doesn't support // 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 +79,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) From ad4559d019f4d942852dbb6bb0121cde33623e44 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 2 Mar 2017 13:21:34 -0800 Subject: [PATCH 3/9] Safely ensure {dev,proc,alloc} are mounted If they're unmounted by a reboot they'll be properly remounted. --- client/allocdir/alloc_dir.go | 15 ++++++ client/allocdir/alloc_dir_test.go | 51 +++++++++++++++--- client/allocdir/task_dir.go | 31 +++++++---- client/allocdir/task_dir_linux.go | 34 ++++++++---- client/allocdir/task_dir_linux_test.go | 72 ++++++++++++++++++++++++++ client/allocdir/task_dir_test.go | 4 +- client/task_runner.go | 10 ++-- 7 files changed, 179 insertions(+), 38 deletions(-) create mode 100644 client/allocdir/task_dir_linux_test.go diff --git a/client/allocdir/alloc_dir.go b/client/allocdir/alloc_dir.go index faa8e2362f38..bec1d279ac25 100644 --- a/client/allocdir/alloc_dir.go +++ b/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. diff --git a/client/allocdir/alloc_dir_test.go b/client/allocdir/alloc_dir_test.go index d09263104f51..bc4b24284f31 100644 --- a/client/allocdir/alloc_dir_test.go +++ b/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", empty, err) + } + if empty, err := pathEmpty(missingDir); err == nil || empty { + t.Errorf("%q is missing. empty=%v error=%v", 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", empty, err) + } +} diff --git a/client/allocdir/task_dir.go b/client/allocdir/task_dir.go index 043df578c6a3..4cf061d8d357 100644 --- a/client/allocdir/task_dir.go +++ b/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. If the built +// boolean is true Build assumes the task dir has already been built and skips +// expensive operations like chroot creation. +func (t *TaskDir) Build(built bool, chroot map[string]string, fsi cstructs.FSIsolation) error { if err := os.MkdirAll(t.Dir, 0777); err != nil { return err } @@ -69,7 +71,7 @@ func (t *TaskDir) Build(chroot map[string]string, fsi cstructs.FSIsolation) erro } // Create a local directory that each task can use. - if err := os.MkdirAll(t.LocalDir, 0777); err != nil { + if err := os.Mkdir(t.LocalDir, 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(built, chroot); err != nil { return err } } @@ -122,10 +128,15 @@ func (t *TaskDir) Build(chroot map[string]string, fsi cstructs.FSIsolation) erro // 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 +// +// If built is true the chroot is assumed to have been built already and only +// ephemeral operations (eg mounting /dev) are done. +func (t *TaskDir) buildChroot(built bool, entries map[string]string) error { + if !built { + // Link/copy chroot entries + if err := t.embedDirs(entries); err != nil { + return err + } } // Mount special dirs diff --git a/client/allocdir/task_dir_linux.go b/client/allocdir/task_dir_linux.go index 35fc209bc0a8..d8e62f49cfff 100644 --- a/client/allocdir/task_dir_linux.go +++ b/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)) } } diff --git a/client/allocdir/task_dir_linux_test.go b/client/allocdir/task_dir_linux_test.go new file mode 100644 index 000000000000..43faf3c85a04 --- /dev/null +++ b/client/allocdir/task_dir_linux_test.go @@ -0,0 +1,72 @@ +package allocdir + +import ( + "io/ioutil" + "os" + "path/filepath" + "testing" + + "golang.org/x/sys/unix" +) + +// TestLinuxSpecialDirs ensures mounting /dev and /proc works. +func TestLinuxSpecialDirs(t *testing.T) { + if unix.Geteuid() != 0 { + t.Skip("Must be run as root") + } + + allocDir, err := ioutil.TempDir("", "nomadtest-specialdirs") + if err != nil { + t.Fatalf("unable to create tempdir for test: %v", err) + } + defer os.RemoveAll(allocDir) + + td := newTaskDir(testLogger(), allocDir, "test") + + // Despite the task dir not existing, unmountSpecialDirs should *not* + // return an error + if err := td.unmountSpecialDirs(); err != nil { + t.Fatalf("error removing nonexistent special dirs: %v", err) + } + + // Mounting special dirs in a nonexistent task dir *should* return an + // error + if err := td.mountSpecialDirs(); err == nil { + t.Fatalf("expected mounting in a nonexistent task dir %q to fail", td.Dir) + } + + // Create the task dir like TaskDir.Build would + if err := os.MkdirAll(td.Dir, 0777); err != nil { + t.Fatalf("error creating task dir %q: %v", td.Dir, err) + } + + // Mounting special dirs should now work and contain files + if err := td.mountSpecialDirs(); err != nil { + t.Fatalf("error mounting special dirs in %q: %v", td.Dir, err) + } + if empty, err := pathEmpty(filepath.Join(td.Dir, "dev")); empty || err != nil { + t.Fatalf("expected dev to be populated but found: empty=%v error=%v", empty, err) + } + if empty, err := pathEmpty(filepath.Join(td.Dir, "proc")); empty || err != nil { + t.Fatalf("expected proc to be populated but found: empty=%v error=%v", empty, err) + } + + // Remounting again should be fine + if err := td.mountSpecialDirs(); err != nil { + t.Fatalf("error remounting special dirs in %q: %v", td.Dir, err) + } + + // Now unmount + if err := td.unmountSpecialDirs(); err != nil { + t.Fatalf("error unmounting special dirs in %q: %v", td.Dir, err) + } + if pathExists(filepath.Join(td.Dir, "dev")) { + t.Fatalf("dev was not removed from %q", td.Dir) + } + if pathExists(filepath.Join(td.Dir, "proc")) { + t.Fatalf("proc was not removed from %q", td.Dir) + } + if err := td.unmountSpecialDirs(); err != nil { + t.Fatalf("error re-unmounting special dirs in %q: %v", td.Dir, err) + } +} diff --git a/client/allocdir/task_dir_test.go b/client/allocdir/task_dir_test.go index 4d4b47338fac..6521b6b870ae 100644 --- a/client/allocdir/task_dir_test.go +++ b/client/allocdir/task_dir_test.go @@ -103,7 +103,7 @@ func TestTaskDir_NonRoot_Image(t *testing.T) { t.Fatalf("Build() failed: %v", err) } - 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) } } @@ -126,7 +126,7 @@ func TestTaskDir_NonRoot(t *testing.T) { t.Fatalf("Build() failed: %v", err) } - if err := td.Build(nil, cstructs.FSIsolationNone); err != nil { + if err := td.Build(false, nil, cstructs.FSIsolationNone); err != nil { t.Fatalf("TaskDir.Build failed: %v", err) } diff --git a/client/task_runner.go b/client/task_runner.go index a18769503162..ad0b48cc568d 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -285,7 +285,7 @@ func (r *TaskRunner) RestoreState() error { // In the case it fails, we relaunch the task in the Run() method. if err != nil { - r.logger.Printf("[ERR] client: failed to open handle to task '%s' for alloc '%s': %v", + r.logger.Printf("[ERR] client: failed to open handle to task %q for alloc %q: %v", r.task.Name, r.alloc.ID, err) return nil } @@ -1208,11 +1208,7 @@ func (r *TaskRunner) startTask() error { // to call multiple times as its state is persisted. func (r *TaskRunner) buildTaskDir(fsi cstructs.FSIsolation) error { r.persistLock.Lock() - if r.taskDirBuilt { - // Already built! Nothing to do. - r.persistLock.Unlock() - return nil - } + built := r.taskDirBuilt r.persistLock.Unlock() r.setState(structs.TaskStatePending, structs.NewTaskEvent(structs.TaskSetup). @@ -1222,7 +1218,7 @@ func (r *TaskRunner) buildTaskDir(fsi cstructs.FSIsolation) error { if len(r.config.ChrootEnv) > 0 { chroot = r.config.ChrootEnv } - if err := r.taskDir.Build(chroot, fsi); err != nil { + if err := r.taskDir.Build(built, chroot, fsi); err != nil { return err } From 866600e980d9e278076af859a27a8b3c51c9692b Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 2 Mar 2017 15:44:52 -0800 Subject: [PATCH 4/9] Cleanup comments/names --- client/allocdir/fs_linux.go | 5 ++--- client/allocdir/task_dir.go | 21 ++++++++++----------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/client/allocdir/fs_linux.go b/client/allocdir/fs_linux.go index 542fe46acf44..7a76c5dd1a01 100644 --- a/client/allocdir/fs_linux.go +++ b/client/allocdir/fs_linux.go @@ -29,9 +29,8 @@ func linkDir(src, dst string) error { } // unlinkDir unmounts a bind mounted directory as Linux doesn't support -// hardlinking directories. -// -// If the dir is already unmounted no error is returned. +// hardlinking directories. If the dir is already unmounted no error is +// returned. func unlinkDir(dir string) error { if err := syscall.Unmount(dir, 0); err != nil { if err != syscall.EINVAL { diff --git a/client/allocdir/task_dir.go b/client/allocdir/task_dir.go index 4cf061d8d357..b4c3310e2955 100644 --- a/client/allocdir/task_dir.go +++ b/client/allocdir/task_dir.go @@ -57,10 +57,10 @@ func newTaskDir(logger *log.Logger, allocDir, taskName string) *TaskDir { } } -// Build default directories and permissions in a task directory. If the built -// boolean is true Build assumes the task dir has already been built and skips -// expensive operations like chroot creation. -func (t *TaskDir) Build(built bool, 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 } @@ -116,7 +116,7 @@ func (t *TaskDir) Build(built bool, chroot map[string]string, fsi cstructs.FSIso // Build chroot if chroot filesystem isolation is going to be used if fsi == cstructs.FSIsolationChroot { - if err := t.buildChroot(built, chroot); err != nil { + if err := t.buildChroot(chrootCreated, chroot); err != nil { return err } } @@ -127,12 +127,11 @@ func (t *TaskDir) Build(built bool, chroot map[string]string, fsi cstructs.FSIso // 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. -// -// If built is true the chroot is assumed to have been built already and only -// ephemeral operations (eg mounting /dev) are done. -func (t *TaskDir) buildChroot(built bool, entries map[string]string) error { - if !built { +// 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 From 6535aa8d95ffffaa1f7039ce7034efc9063a04b1 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 2 Mar 2017 15:50:18 -0800 Subject: [PATCH 5/9] Fix API breaks in tests --- client/driver/docker_test.go | 2 +- client/driver/driver_test.go | 2 +- client/task_runner_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/driver/docker_test.go b/client/driver/docker_test.go index e7920f949eb7..fe725252fc8f 100644 --- a/client/driver/docker_test.go +++ b/client/driver/docker_test.go @@ -1171,7 +1171,7 @@ func setupDockerVolumes(t *testing.T, cfg *config.Config, hostpath string) (*str t.Fatalf("failed to build alloc dir: %v", err) } taskDir := allocDir.NewTaskDir(task.Name) - if err := taskDir.Build(nil, cstructs.FSIsolationImage); err != nil { + if err := taskDir.Build(false, nil, cstructs.FSIsolationImage); err != nil { allocDir.Destroy() t.Fatalf("failed to build task dir: %v", err) } diff --git a/client/driver/driver_test.go b/client/driver/driver_test.go index 747aee283781..19f7eae38357 100644 --- a/client/driver/driver_test.go +++ b/client/driver/driver_test.go @@ -104,7 +104,7 @@ func testDriverContexts(t *testing.T, task *structs.Task) *testContext { // Build the task dir td := allocDir.NewTaskDir(task.Name) - if err := td.Build(config.DefaultChrootEnv, tmpdrv.FSIsolation()); err != nil { + if err := td.Build(false, config.DefaultChrootEnv, tmpdrv.FSIsolation()); err != nil { allocDir.Destroy() t.Fatalf("TaskDir.Build(%#v, %q) failed: %v", config.DefaultChrootEnv, tmpdrv.FSIsolation(), err) return nil diff --git a/client/task_runner_test.go b/client/task_runner_test.go index 1234919ef016..1718bff979d3 100644 --- a/client/task_runner_test.go +++ b/client/task_runner_test.go @@ -98,7 +98,7 @@ func testTaskRunnerFromAlloc(t *testing.T, restarts bool, alloc *structs.Allocat fsi = cstructs.FSIsolationChroot } taskDir := allocDir.NewTaskDir(task.Name) - if err := taskDir.Build(config.DefaultChrootEnv, fsi); err != nil { + if err := taskDir.Build(false, config.DefaultChrootEnv, fsi); err != nil { t.Fatalf("error building task dir %q: %v", task.Name, err) return nil } From 16fb45e4baab0e47bf78132784582b92ff70aac9 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 2 Mar 2017 15:54:12 -0800 Subject: [PATCH 6/9] Fix tests broken by API change --- client/driver/executor/executor_linux_test.go | 2 +- client/driver/executor/executor_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/client/driver/executor/executor_linux_test.go b/client/driver/executor/executor_linux_test.go index e60aa57586ad..ab5dc6a848df 100644 --- a/client/driver/executor/executor_linux_test.go +++ b/client/driver/executor/executor_linux_test.go @@ -45,7 +45,7 @@ func testExecutorContextWithChroot(t *testing.T) (*ExecutorContext, *allocdir.Al if err := allocDir.Build(); err != nil { log.Fatalf("AllocDir.Build() failed: %v", err) } - if err := allocDir.NewTaskDir(task.Name).Build(chrootEnv, cstructs.FSIsolationChroot); err != nil { + if err := allocDir.NewTaskDir(task.Name).Build(false, chrootEnv, cstructs.FSIsolationChroot); err != nil { allocDir.Destroy() log.Fatalf("allocDir.NewTaskDir(%q) failed: %v", task.Name, err) } diff --git a/client/driver/executor/executor_test.go b/client/driver/executor/executor_test.go index 4f74eabd3956..8508233d6c8b 100644 --- a/client/driver/executor/executor_test.go +++ b/client/driver/executor/executor_test.go @@ -49,7 +49,7 @@ func testExecutorContext(t *testing.T) (*ExecutorContext, *allocdir.AllocDir) { if err := allocDir.Build(); err != nil { log.Fatalf("AllocDir.Build() failed: %v", err) } - if err := allocDir.NewTaskDir(task.Name).Build(nil, cstructs.FSIsolationNone); err != nil { + if err := allocDir.NewTaskDir(task.Name).Build(false, nil, cstructs.FSIsolationNone); err != nil { allocDir.Destroy() log.Fatalf("allocDir.NewTaskDir(%q) failed: %v", task.Name, err) } From 94974152fdc486c4f129f9d330150e27b66ecac2 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Thu, 2 Mar 2017 19:35:31 -0800 Subject: [PATCH 7/9] Mkdir -> MkdirAll to avoid error when folder already exists --- client/allocdir/task_dir.go | 2 +- client/task_runner_test.go | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/client/allocdir/task_dir.go b/client/allocdir/task_dir.go index b4c3310e2955..7e7a479c95e8 100644 --- a/client/allocdir/task_dir.go +++ b/client/allocdir/task_dir.go @@ -71,7 +71,7 @@ func (t *TaskDir) Build(chrootCreated bool, chroot map[string]string, fsi cstruc } // Create a local directory that each task can use. - if err := os.Mkdir(t.LocalDir, 0777); err != nil { + if err := os.MkdirAll(t.LocalDir, 0777); err != nil { return err } diff --git a/client/task_runner_test.go b/client/task_runner_test.go index 1718bff979d3..2282f3921559 100644 --- a/client/task_runner_test.go +++ b/client/task_runner_test.go @@ -157,7 +157,7 @@ func TestTaskRunner_SimpleRun(t *testing.T) { } if len(ctx.upd.events) != 4 { - t.Fatalf("should have 3 ctx.upd.tes: %#v", ctx.upd.events) + t.Fatalf("should have 3 ctx.updates: %#v", ctx.upd.events) } if ctx.upd.state != structs.TaskStateDead { @@ -255,7 +255,7 @@ func TestTaskRunner_Destroy(t *testing.T) { } if len(ctx.upd.events) != 5 { - t.Fatalf("should have 5 ctx.upd.tes: %#v", ctx.upd.events) + t.Fatalf("should have 5 ctx.updates: %#v", ctx.upd.events) } if ctx.upd.state != structs.TaskStateDead { @@ -418,7 +418,7 @@ func TestTaskRunner_Download_List(t *testing.T) { } if len(ctx.upd.events) != 5 { - t.Fatalf("should have 5 ctx.upd.tes: %#v", ctx.upd.events) + t.Fatalf("should have 5 ctx.updates: %#v", ctx.upd.events) } if ctx.upd.state != structs.TaskStateDead { @@ -485,7 +485,7 @@ func TestTaskRunner_Download_Retries(t *testing.T) { } if len(ctx.upd.events) != 8 { - t.Fatalf("should have 8 ctx.upd.tes: %#v", ctx.upd.events) + t.Fatalf("should have 8 ctx.updates: %#v", ctx.upd.events) } if ctx.upd.state != structs.TaskStateDead { @@ -758,7 +758,7 @@ func TestTaskRunner_BlockForVault(t *testing.T) { } if len(ctx.upd.events) != 2 { - t.Fatalf("should have 2 ctx.upd.tes: %#v", ctx.upd.events) + t.Fatalf("should have 2 ctx.updates: %#v", ctx.upd.events) } if ctx.upd.state != structs.TaskStatePending { @@ -783,7 +783,7 @@ func TestTaskRunner_BlockForVault(t *testing.T) { } if len(ctx.upd.events) != 4 { - t.Fatalf("should have 4 ctx.upd.tes: %#v", ctx.upd.events) + t.Fatalf("should have 4 ctx.updates: %#v", ctx.upd.events) } if ctx.upd.state != structs.TaskStateDead { @@ -853,7 +853,7 @@ func TestTaskRunner_DeriveToken_Retry(t *testing.T) { } if len(ctx.upd.events) != 4 { - t.Fatalf("should have 4 ctx.upd.tes: %#v", ctx.upd.events) + t.Fatalf("should have 4 ctx.updates: %#v", ctx.upd.events) } if ctx.upd.state != structs.TaskStateDead { @@ -966,7 +966,7 @@ func TestTaskRunner_Template_Block(t *testing.T) { } if len(ctx.upd.events) != 2 { - t.Fatalf("should have 2 ctx.upd.tes: %#v", ctx.upd.events) + t.Fatalf("should have 2 ctx.updates: %#v", ctx.upd.events) } if ctx.upd.state != structs.TaskStatePending { @@ -991,7 +991,7 @@ func TestTaskRunner_Template_Block(t *testing.T) { } if len(ctx.upd.events) != 4 { - t.Fatalf("should have 4 ctx.upd.tes: %#v", ctx.upd.events) + t.Fatalf("should have 4 ctx.updates: %#v", ctx.upd.events) } if ctx.upd.state != structs.TaskStateDead { @@ -1058,7 +1058,7 @@ func TestTaskRunner_Template_Artifact(t *testing.T) { } if len(ctx.upd.events) != 5 { - t.Fatalf("should have 5 ctx.upd.tes: %#v", ctx.upd.events) + t.Fatalf("should have 5 ctx.updates: %#v", ctx.upd.events) } if ctx.upd.state != structs.TaskStateDead { From fbfeecb486ceded4de05e1bf17f99e59036de2b8 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Thu, 2 Mar 2017 20:45:46 -0800 Subject: [PATCH 8/9] Fix TestAllocRunner_SaveRestoreState --- client/alloc_runner_test.go | 5 +++-- client/task_runner.go | 9 +++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/client/alloc_runner_test.go b/client/alloc_runner_test.go index 025a18ab72f0..721a6e016b75 100644 --- a/client/alloc_runner_test.go +++ b/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 diff --git a/client/task_runner.go b/client/task_runner.go index 02e7211023be..36c0d7646172 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -1212,8 +1212,13 @@ func (r *TaskRunner) buildTaskDir(fsi cstructs.FSIsolation) error { built := r.taskDirBuilt r.persistLock.Unlock() - r.setState(structs.TaskStatePending, structs.NewTaskEvent(structs.TaskSetup). - SetMessage(structs.TaskBuildingTaskDir)) + // We do not set the state again since this only occurs during restoration + // and the task dir is already built. The reason we call Build again is to + // ensure that the task dir invariants are still held. + if !built { + r.setState(structs.TaskStatePending, structs.NewTaskEvent(structs.TaskSetup). + SetMessage(structs.TaskBuildingTaskDir)) + } chroot := config.DefaultChrootEnv if len(r.config.ChrootEnv) > 0 { From 50fafa2808aa993da70cb55069cace15bb72014d Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Thu, 2 Mar 2017 21:03:05 -0800 Subject: [PATCH 9/9] Fix lint errors --- GNUmakefile | 1 + client/allocdir/alloc_dir_test.go | 6 +++--- client/allocdir/fs_linux_test.go | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/GNUmakefile b/GNUmakefile index c44cec98ae55..1f849836c82b 100644 --- a/GNUmakefile +++ b/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 \ diff --git a/client/allocdir/alloc_dir_test.go b/client/allocdir/alloc_dir_test.go index bc4b24284f31..efd4805014d5 100644 --- a/client/allocdir/alloc_dir_test.go +++ b/client/allocdir/alloc_dir_test.go @@ -408,10 +408,10 @@ func TestPathFuncs(t *testing.T) { } if empty, err := pathEmpty(dir); err != nil || !empty { - t.Errorf("%q is empty and exists. empty=%v error=%v", empty, err) + 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", empty, err) + t.Errorf("%q is missing. empty=%v error=%v", missingDir, empty, err) } filename := filepath.Join(dir, "just-some-file") @@ -422,6 +422,6 @@ func TestPathFuncs(t *testing.T) { f.Close() if empty, err := pathEmpty(dir); err != nil || empty { - t.Errorf("%q is not empty. empty=%v error=%v", empty, err) + t.Errorf("%q is not empty. empty=%v error=%v", dir, empty, err) } } diff --git a/client/allocdir/fs_linux_test.go b/client/allocdir/fs_linux_test.go index d328755e4810..fd51400a4bc8 100644 --- a/client/allocdir/fs_linux_test.go +++ b/client/allocdir/fs_linux_test.go @@ -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