12183: Improve error handling if unable to delete symlinks marked for deletion.

Add unit test for EvalSymlinks.

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>

diff --git a/services/crunch-run/crunchrun.go b/services/crunch-run/crunchrun.go
index c50f799e8c43ccadf112ab24f980f46ffbe207a9..83c4ca39761f9c2ad5f2469e49ec863ea6d69160 100644 (file)
--- a/services/crunch-run/crunchrun.go
+++ b/services/crunch-run/crunchrun.go
@@ -942,7 +942,6 @@ func (runner *ContainerRunner) EvalSymlinks(path string, binds []string) (manife
                        return
                }
 
-               var tgt string
                if info.Mode()&os.ModeSymlink == 0 {
                        // Not a symlink, nothing to do.
                        return
@@ -951,7 +950,9 @@ func (runner *ContainerRunner) EvalSymlinks(path string, binds []string) (manife
                // Remember symlink for cleanup later
                links = append(links, path)
 
-               tgt, err = os.Readlink(path)
+               var readlinktgt string
+               readlinktgt, err = os.Readlink(path)
+               tgt := readlinktgt
                if err != nil {
                        return
                }
@@ -999,13 +1000,22 @@ func (runner *ContainerRunner) EvalSymlinks(path string, binds []string) (manife
                // directory, otherwise it is an error.
                if !strings.HasPrefix(tgt, runner.HostOutputDir+"/") {
                        err = fmt.Errorf("Output directory symlink %q points to invalid location %q, must point to mount or output directory.",
-                               path[len(runner.HostOutputDir):], tgt)
+                               path[len(runner.HostOutputDir):], readlinktgt)
                        return
                }
 
                // Update symlink to host FS
-               os.Remove(path)
-               os.Symlink(tgt, path)
+               err = os.Remove(path)
+               if err != nil {
+                       err = fmt.Errorf("Error removing symlink %q: %v", path, err)
+                       return
+               }
+
+               err = os.Symlink(tgt, path)
+               if err != nil {
+                       err = fmt.Errorf("Error updating symlink %q: %v", path, err)
+                       return
+               }
 
                // Target is within the output directory, so loop and check if
                // it is also a symlink.
@@ -1062,7 +1072,7 @@ func (runner *ContainerRunner) CaptureOutput() error {
        if err != nil {
                // Regular directory
 
-               var symlinksToRemove []string
+               symlinksToRemove := make(map[string]bool)
                var m string
                var srm []string
                // Find symlinks to arv-mounted files & dirs.
@@ -1071,14 +1081,24 @@ func (runner *ContainerRunner) CaptureOutput() error {
                                return err
                        }
                        m, srm, err = runner.EvalSymlinks(path, binds)
-                       symlinksToRemove = append(symlinksToRemove, srm...)
+                       for _, r := range srm {
+                               symlinksToRemove[r] = true
+                       }
                        if err == nil {
                                manifestText = manifestText + m
                        }
                        return err
                })
-               for _, l := range symlinksToRemove {
-                       os.Remove(l)
+               for l, _ := range symlinksToRemove {
+                       err2 := os.Remove(l)
+                       if err2 != nil {
+                               if err == nil {
+                                       err = fmt.Errorf("Error removing symlink %q: %v", err2)
+                               } else {
+                                       err = fmt.Errorf("%v\nError removing symlink %q: %v",
+                                               err, err2)
+                               }
+                       }
                }
                if err != nil {
                        return fmt.Errorf("While checking output symlinks: %v", err)

diff --git a/services/crunch-run/crunchrun_test.go b/services/crunch-run/crunchrun_test.go
index 9081783bdd59fd1bcb71722306742f8f9705e9ec..d3606ec6cb9c0ed9e02fbd52f445cfc75252388b 100644 (file)
--- a/services/crunch-run/crunchrun_test.go
+++ b/services/crunch-run/crunchrun_test.go
@@ -1685,3 +1685,41 @@ func (s *TestSuite) TestNumberRoundTrip(c *C) {
        c.Check(err, IsNil)
        c.Check(string(jsondata), Equals, `{"number":123456789123456789}`)
 }
+
+func (s *TestSuite) TestEvalSymlinks(c *C) {
+       cr := NewContainerRunner(&ArvTestClient{callraw: true}, &KeepTestClient{}, nil, "zzzzz-zzzzz-zzzzzzzzzzzzzzz")
+
+       realTemp, err := ioutil.TempDir("", "crunchrun_test-")
+       c.Assert(err, IsNil)
+       defer os.RemoveAll(realTemp)
+
+       cr.HostOutputDir = realTemp
+
+       // Absolute path outside output dir
+       os.Symlink("/etc/passwd", realTemp+"/p1")
+
+       // Relative outside output dir
+       os.Symlink("../..", realTemp+"/p2")
+
+       // Circular references
+       os.Symlink("p4", realTemp+"/p3")
+       os.Symlink("p5", realTemp+"/p4")
+       os.Symlink("p3", realTemp+"/p5")
+
+       symlinksToRemove := make(map[string]bool)
+       for _, v := range []string{"p1", "p2", "p3", "p4", "p5"} {
+               var srm []string
+               _, srm, err = cr.EvalSymlinks(realTemp+"/"+v, []string{})
+               c.Assert(err, NotNil)
+               for _, r := range srm {
+                       symlinksToRemove[r] = true
+               }
+       }
+       c.Assert(len(symlinksToRemove), Equals, 5)
+
+       c.Assert(map[string]bool{realTemp + "/" + "p1": true,
+               realTemp + "/" + "p2": true,
+               realTemp + "/" + "p3": true,
+               realTemp + "/" + "p4": true,
+               realTemp + "/" + "p5": true}, DeepEquals, symlinksToRemove)
+}

diff --git a/services/crunch-run/upload.go b/services/crunch-run/upload.go
index faf20b4f8b4568076b17151655927885a6d038de..7b13109533dbba3e0de1c073c95eb31afc26424d 100644 (file)
--- a/services/crunch-run/upload.go
+++ b/services/crunch-run/upload.go
@@ -283,7 +283,7 @@ func (m *WalkUpload) WalkFunc(path string, info os.FileInfo, err error) error {
                if err != nil {
                        return fmt.Errorf("stat symlink %q target %q: %s", path, targetPath, err)
                }
-               if targetInfo.Mode()&os.ModeDir != 0 {
+               if targetInfo.IsDir() {
                        // Symlinks to directories don't get walked, so do it
                        // here.  We've previously checked that they stay in
                        // the output directory and don't result in an endless