From da3e34710c61b4520d9d4bf7f8ef85f2f01f8a6a Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 21 Feb 2017 17:22:10 -0800 Subject: [PATCH] Fix allocdir Move test and make code more defensive A change in the behavior of `os.Rename` in Go 1.8 brought to light a difference in the logic between `{Alloc,Task}Runner` and this test: AllocRunner builds the alloc dir, moves dirs if necessary, and then lets TaskRunner call TaskDir.Build(). This test called `TaskDir.Build` *before* `AllocDir.Move`, so in Go 1.8 it failed to `os.Rename over` the empty {data,local} dirs. I updated the test to behave like the real code, but I defensively added `os.Remove` calls as a subtle change in call order shouldn't break this code. `os.Remove` won't remove a non-empty directory, so it's still safe. --- client/allocdir/alloc_dir.go | 2 ++ client/allocdir/alloc_dir_test.go | 7 +++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/client/allocdir/alloc_dir.go b/client/allocdir/alloc_dir.go index 7f622f558..faa8e2362 100644 --- a/client/allocdir/alloc_dir.go +++ b/client/allocdir/alloc_dir.go @@ -161,6 +161,7 @@ func (d *AllocDir) Move(other *AllocDir, tasks []*structs.Task) error { otherDataDir := filepath.Join(other.SharedDir, SharedDataDir) dataDir := filepath.Join(d.SharedDir, SharedDataDir) if fileInfo, err := os.Stat(otherDataDir); fileInfo != nil && err == nil { + os.Remove(dataDir) // remove an empty data dir if it exists if err := os.Rename(otherDataDir, dataDir); err != nil { return fmt.Errorf("error moving data dir: %v", err) } @@ -179,6 +180,7 @@ func (d *AllocDir) Move(other *AllocDir, tasks []*structs.Task) error { return fmt.Errorf("error creating task %q dir: %v", task.Name, err) } localDir := filepath.Join(newTaskDir, TaskLocal) + os.Remove(localDir) // remove an empty local dir if it exists if err := os.Rename(otherTaskLocal, localDir); err != nil { return fmt.Errorf("error moving task %q local dir: %v", task.Name, err) } diff --git a/client/allocdir/alloc_dir_test.go b/client/allocdir/alloc_dir_test.go index cc3d48c66..d09263104 100644 --- a/client/allocdir/alloc_dir_test.go +++ b/client/allocdir/alloc_dir_test.go @@ -229,10 +229,9 @@ func TestAllocDir_Move(t *testing.T) { t.Fatalf("TaskDir.Build() faild: %v", err) } - td2 := d2.NewTaskDir(t1.Name) - if err := td2.Build(nil, cstructs.FSIsolationImage); err != nil { - t.Fatalf("TaskDir.Build() faild: %v", err) - } + // Create but don't build second task dir to mimic alloc/task runner + // behavior (AllocDir.Move() is called pre-TaskDir.Build). + d2.NewTaskDir(t1.Name) dataDir := filepath.Join(d1.SharedDir, SharedDataDir)