[ARVADOS] updated: 941502322ccea097e1944972d3647182cd1b6268

[Git user]

Summary of changes:
 services/crunch-run/crunchrun.go      | 38 ++++++++++++++++++++++++++---------
 services/crunch-run/crunchrun_test.go | 38 +++++++++++++++++++++++++++++++++++
 services/crunch-run/upload.go         |  2 +-
 3 files changed, 68 insertions(+), 10 deletions(-)

       via  941502322ccea097e1944972d3647182cd1b6268 (commit)
      from  3afc0c377d2859d0aba622d1883af771b1b62594 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 941502322ccea097e1944972d3647182cd1b6268
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Mon Oct 2 15:34:38 2017 -0400

    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 at veritasgenetics.com>

diff --git a/services/crunch-run/crunchrun.go b/services/crunch-run/crunchrun.go
index b467c4f..53ca1b9 100644
--- a/services/crunch-run/crunchrun.go
+++ b/services/crunch-run/crunchrun.go
@@ -931,7 +931,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
@@ -940,7 +939,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
 		}
@@ -988,13 +989,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.
@@ -1051,7 +1061,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.
@@ -1060,14 +1070,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 4a94648..2aaac25 100644
--- a/services/crunch-run/crunchrun_test.go
+++ b/services/crunch-run/crunchrun_test.go
@@ -1676,3 +1676,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 faf20b4..7b13109 100644
--- 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

-----------------------------------------------------------------------


hooks/post-receive
--