From ad4559d019f4d942852dbb6bb0121cde33623e44 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 2 Mar 2017 13:21:34 -0800 Subject: [PATCH] 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 faa8e2362..bec1d279a 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 d09263104..bc4b24284 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 043df578c..4cf061d8d 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 35fc209bc..d8e62f49c 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 000000000..43faf3c85 --- /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 4d4b47338..6521b6b87 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 a18769503..ad0b48cc5 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 }