18600: Test more snapshot/splice variations and error cases.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

diff --git a/sdk/go/arvados/fs_base.go b/sdk/go/arvados/fs_base.go
index 680a8431c525a948ebdd30d9408e3a251e3820a4..719e0e08ac38aa6deeca3148b28ff698d2ba7f89 100644 (file)
--- a/sdk/go/arvados/fs_base.go
+++ b/sdk/go/arvados/fs_base.go
@@ -588,7 +588,7 @@ func (fs *fileSystem) Rename(oldname, newname string) error {
                // supported. Locking inodes from different
                // filesystems could deadlock, so we must error out
                // now.
-               return ErrInvalidArgument
+               return ErrInvalidOperation
        }
 
        // To ensure we can test reliably whether we're about to move

diff --git a/sdk/go/arvados/fs_getternode.go b/sdk/go/arvados/fs_getternode.go
index 966fe9d5c04187829ebab9f065ea765355386fff..ddff0c52e709b50ba00db31225c5bd549866ea9c 100644 (file)
--- a/sdk/go/arvados/fs_getternode.go
+++ b/sdk/go/arvados/fs_getternode.go
@@ -24,7 +24,7 @@ func (*getternode) IsDir() bool {
 }
 
 func (*getternode) Child(string, func(inode) (inode, error)) (inode, error) {
-       return nil, ErrInvalidArgument
+       return nil, ErrInvalidOperation
 }
 
 func (gn *getternode) get() error {

diff --git a/sdk/go/arvados/fs_lookup.go b/sdk/go/arvados/fs_lookup.go
index 021e8241cfeda647334781499ac60396e0961ac1..471dc69c82f75318d290eaccfa7b8d4a542540f1 100644 (file)
--- a/sdk/go/arvados/fs_lookup.go
+++ b/sdk/go/arvados/fs_lookup.go
@@ -68,7 +68,7 @@ func (ln *lookupnode) Readdir() ([]os.FileInfo, error) {
        return ln.treenode.Readdir()
 }
 
-// Child rejects (with ErrInvalidArgument) calls to add/replace
+// Child rejects (with ErrInvalidOperation) calls to add/replace
 // children, instead calling loadOne when a non-existing child is
 // looked up.
 func (ln *lookupnode) Child(name string, replace func(inode) (inode, error)) (inode, error) {
@@ -97,12 +97,12 @@ func (ln *lookupnode) Child(name string, replace func(inode) (inode, error)) (in
        if replace != nil {
                // Let the callback try to delete or replace the
                // existing node; if it does, return
-               // ErrInvalidArgument.
+               // ErrInvalidOperation.
                if tryRepl, err := replace(existing); err != nil {
                        // Propagate error from callback
                        return existing, err
                } else if tryRepl != existing {
-                       return existing, ErrInvalidArgument
+                       return existing, ErrInvalidOperation
                }
        }
        // Return original error from ln.treenode.Child() (it might be

diff --git a/sdk/go/arvados/fs_project_test.go b/sdk/go/arvados/fs_project_test.go
index 89435132713b0a14a8438d1ebe07390beb4a8893..8e7f58815629478f243396472fb92b258fe3ac98 100644 (file)
--- a/sdk/go/arvados/fs_project_test.go
+++ b/sdk/go/arvados/fs_project_test.go
@@ -7,6 +7,7 @@ package arvados
 import (
        "bytes"
        "encoding/json"
+       "errors"
        "io"
        "os"
        "path/filepath"
@@ -311,17 +312,37 @@ func (s *SiteFSSuite) TestProjectUnsupportedOperations(c *check.C) {
        s.fs.MountProject("home", "")
 
        _, err := s.fs.OpenFile("/home/A Project/newfilename", os.O_CREATE|os.O_RDWR, 0)
-       c.Check(err, check.ErrorMatches, "invalid argument")
+       c.Check(err, ErrorIs, ErrInvalidOperation)
 
        err = s.fs.Mkdir("/home/A Project/newdirname", 0)
-       c.Check(err, check.ErrorMatches, "invalid argument")
+       c.Check(err, ErrorIs, ErrInvalidOperation)
 
        err = s.fs.Mkdir("/by_id/newdirname", 0)
-       c.Check(err, check.ErrorMatches, "invalid argument")
+       c.Check(err, ErrorIs, ErrInvalidOperation)
 
        err = s.fs.Mkdir("/by_id/"+fixtureAProjectUUID+"/newdirname", 0)
-       c.Check(err, check.ErrorMatches, "invalid argument")
+       c.Check(err, ErrorIs, ErrInvalidOperation)
 
        _, err = s.fs.OpenFile("/home/A Project", 0, 0)
        c.Check(err, check.IsNil)
 }
+
+type errorIsChecker struct {
+       *check.CheckerInfo
+}
+
+var ErrorIs check.Checker = errorIsChecker{
+       &check.CheckerInfo{Name: "ErrorIs", Params: []string{"value", "target"}},
+}
+
+func (checker errorIsChecker) Check(params []interface{}, names []string) (result bool, errStr string) {
+       err, ok := params[0].(error)
+       if !ok {
+               return false, ""
+       }
+       target, ok := params[1].(error)
+       if !ok {
+               return false, ""
+       }
+       return errors.Is(err, target), ""
+}

diff --git a/sdk/go/arvados/fs_site.go b/sdk/go/arvados/fs_site.go
index 5225df59ee58ec4c2017054f59ca68ee7936e41e..3892be1e9a97610522a1fc219d5c0fb807788e4c 100644 (file)
--- a/sdk/go/arvados/fs_site.go
+++ b/sdk/go/arvados/fs_site.go
@@ -133,7 +133,7 @@ func (fs *customFileSystem) Stale(t time.Time) bool {
 }
 
 func (fs *customFileSystem) newNode(name string, perm os.FileMode, modTime time.Time) (node inode, err error) {
-       return nil, ErrInvalidArgument
+       return nil, ErrInvalidOperation
 }
 
 func (fs *customFileSystem) mountByID(parent inode, id string) inode {
@@ -179,7 +179,7 @@ func (fs *customFileSystem) newProjectNode(root inode, name, uuid string) inode
        }
 }
 
-// vdirnode wraps an inode by rejecting (with ErrInvalidArgument)
+// vdirnode wraps an inode by rejecting (with ErrInvalidOperation)
 // calls that add/replace children directly, instead calling a
 // create() func when a non-existing child is looked up.
 //
@@ -204,7 +204,7 @@ func (vn *vdirnode) Child(name string, replace func(inode) (inode, error)) (inod
                } else if tryRepl, err := replace(existing); err != nil {
                        return existing, err
                } else if tryRepl != existing {
-                       return existing, ErrInvalidArgument
+                       return existing, ErrInvalidOperation
                } else {
                        return existing, nil
                }

diff --git a/sdk/go/arvados/fs_site_test.go b/sdk/go/arvados/fs_site_test.go
index 9d763118004c53e917572364b7d9b71ca2ae667b..59fa5fc17616f692e29a7565972c63b4cbfb88cc 100644 (file)
--- a/sdk/go/arvados/fs_site_test.go
+++ b/sdk/go/arvados/fs_site_test.go
@@ -135,18 +135,18 @@ func (s *SiteFSSuite) TestByUUIDAndPDH(c *check.C) {
        c.Check(names, check.DeepEquals, []string{"baz"})
 
        _, err = s.fs.OpenFile("/by_id/"+fixtureNonexistentCollection, os.O_RDWR|os.O_CREATE, 0755)
-       c.Check(err, check.Equals, ErrInvalidArgument)
+       c.Check(err, ErrorIs, ErrInvalidOperation)
        err = s.fs.Rename("/by_id/"+fixtureFooCollection, "/by_id/beep")
-       c.Check(err, check.Equals, ErrInvalidArgument)
+       c.Check(err, ErrorIs, ErrInvalidOperation)
        err = s.fs.Rename("/by_id/"+fixtureFooCollection+"/foo", "/by_id/beep")
-       c.Check(err, check.Equals, ErrInvalidArgument)
+       c.Check(err, ErrorIs, ErrInvalidOperation)
        _, err = s.fs.Stat("/by_id/beep")
        c.Check(err, check.Equals, os.ErrNotExist)
        err = s.fs.Rename("/by_id/"+fixtureFooCollection+"/foo", "/by_id/"+fixtureFooCollection+"/bar")
        c.Check(err, check.IsNil)
 
        err = s.fs.Rename("/by_id", "/beep")
-       c.Check(err, check.Equals, ErrInvalidArgument)
+       c.Check(err, ErrorIs, ErrInvalidOperation)
 }
 
 // Copy subtree from OS src to dst path inside fs. If src is a
@@ -211,9 +211,11 @@ func copyFromOS(fs FileSystem, dst, src string) error {
 
 func (s *SiteFSSuite) TestSnapshotSplice(c *check.C) {
        s.fs.MountProject("home", "")
+       thisfile, err := ioutil.ReadFile("fs_site_test.go")
+       c.Assert(err, check.IsNil)
 
        var src1 Collection
-       err := s.client.RequestAndDecode(&src1, "POST", "arvados/v1/collections", nil, map[string]interface{}{
+       err = s.client.RequestAndDecode(&src1, "POST", "arvados/v1/collections", nil, map[string]interface{}{
                "collection": map[string]string{
                        "name":       "TestSnapshotSplice src1",
                        "owner_uuid": fixtureAProjectUUID,
@@ -251,39 +253,118 @@ func (s *SiteFSSuite) TestSnapshotSplice(c *check.C) {
        defer s.client.RequestAndDecode(nil, "DELETE", "arvados/v1/collections/"+dst.UUID, nil, nil)
        err = s.fs.Sync()
        c.Assert(err, check.IsNil)
-       err = copyFromOS(s.fs, "/home/A Project/TestSnapshotSplice dst", "..") // arvados.git/sdk/go
+
+       dstPath := "/home/A Project/TestSnapshotSplice dst"
+       err = copyFromOS(s.fs, dstPath, "..") // arvados.git/sdk/go
        c.Assert(err, check.IsNil)
 
+       // Snapshot directory
        snap1, err := Snapshot(s.fs, "/home/A Project/TestSnapshotSplice src1/ctxlog")
-       c.Assert(err, check.IsNil)
-       err = Splice(s.fs, "/home/A Project/TestSnapshotSplice dst/ctxlog-copy", snap1)
-       c.Assert(err, check.IsNil)
-       err = Splice(s.fs, "/home/A Project/TestSnapshotSplice dst/ctxlog-copy2", snap1)
-       c.Assert(err, check.IsNil)
+       c.Check(err, check.IsNil)
+       // Attach same snapshot twice, at paths that didn't exist before
+       err = Splice(s.fs, dstPath+"/ctxlog-copy", snap1)
+       c.Check(err, check.IsNil)
+       err = Splice(s.fs, dstPath+"/ctxlog-copy2", snap1)
+       c.Check(err, check.IsNil)
+       // Splicing a snapshot twice results in two independent copies
+       err = s.fs.Rename(dstPath+"/ctxlog-copy2/log.go", dstPath+"/ctxlog-copy/log2.go")
+       c.Check(err, check.IsNil)
+       _, err = s.fs.Open(dstPath + "/ctxlog-copy2/log.go")
+       c.Check(err, check.Equals, os.ErrNotExist)
+       f, err := s.fs.Open(dstPath + "/ctxlog-copy/log.go")
+       if c.Check(err, check.IsNil) {
+               buf, err := ioutil.ReadAll(f)
+               c.Check(err, check.IsNil)
+               c.Check(string(buf), check.Not(check.Equals), "")
+               f.Close()
+       }
 
-       snap2, err := Snapshot(s.fs, "/home/A Project/TestSnapshotSplice dst/ctxlog-copy")
-       c.Assert(err, check.IsNil)
-       err = Splice(s.fs, "/home/A Project/TestSnapshotSplice dst/ctxlog-copy-copy", snap2)
-       c.Assert(err, check.IsNil)
+       // Snapshot regular file
+       snapFile, err := Snapshot(s.fs, "/home/A Project/TestSnapshotSplice src1/arvados/fs_site_test.go")
+       c.Check(err, check.IsNil)
+       // Replace dir with file
+       err = Splice(s.fs, dstPath+"/ctxlog-copy2", snapFile)
+       c.Check(err, check.IsNil)
+       if f, err := s.fs.Open(dstPath + "/ctxlog-copy2"); c.Check(err, check.IsNil) {
+               buf, err := ioutil.ReadAll(f)
+               c.Check(err, check.IsNil)
+               c.Check(string(buf), check.Equals, string(thisfile))
+       }
 
-       snapDst, err := Snapshot(s.fs, "/home/A Project/TestSnapshotSplice dst")
-       c.Assert(err, check.IsNil)
-       err = Splice(s.fs, "/home/A Project/TestSnapshotSplice dst", snapDst)
-       c.Assert(err, check.IsNil)
-       err = Splice(s.fs, "/home/A Project/TestSnapshotSplice dst/copy1", snapDst)
-       c.Assert(err, check.IsNil)
-       err = Splice(s.fs, "/home/A Project/TestSnapshotSplice dst/copy2", snapDst)
-       c.Assert(err, check.IsNil)
-       err = s.fs.RemoveAll("/home/A Project/TestSnapshotSplice dst/arvados")
-       c.Assert(err, check.IsNil)
-       _, err = s.fs.Open("/home/A Project/TestSnapshotSplice dst/arvados/fs_site_test.go")
-       c.Assert(err, check.Equals, os.ErrNotExist)
-       f, err := s.fs.Open("/home/A Project/TestSnapshotSplice dst/copy2/arvados/fs_site_test.go")
-       c.Assert(err, check.IsNil)
+       // Cannot splice a file onto a collection root, or anywhere
+       // outside a collection
+       for _, badpath := range []string{
+               dstPath,
+               "/home/A Project/newnodename",
+               "/home/A Project",
+               "/home/newnodename",
+               "/home",
+               "/newnodename",
+       } {
+               err = Splice(s.fs, badpath, snapFile)
+               c.Check(err, check.NotNil)
+               c.Check(err, ErrorIs, ErrInvalidOperation, check.Commentf("badpath %s"))
+               if badpath == dstPath {
+                       c.Check(err, check.ErrorMatches, `cannot use Splice to attach a file at top level of \*arvados.collectionFileSystem: invalid operation`, check.Commentf("badpath: %s", badpath))
+                       continue
+               }
+               err = Splice(s.fs, badpath, snap1)
+               c.Check(err, ErrorIs, ErrInvalidOperation, check.Commentf("badpath %s"))
+       }
+
+       // Destination cannot have trailing slash
+       for _, badpath := range []string{
+               dstPath + "/ctxlog/",
+               dstPath + "/",
+               "/home/A Project/",
+               "/home/",
+               "/",
+               "",
+       } {
+               err = Splice(s.fs, badpath, snap1)
+               c.Check(err, ErrorIs, ErrInvalidArgument, check.Commentf("badpath %s", badpath))
+               err = Splice(s.fs, badpath, snapFile)
+               c.Check(err, ErrorIs, ErrInvalidArgument, check.Commentf("badpath %s", badpath))
+       }
+
+       // Destination's parent must already exist
+       for _, badpath := range []string{
+               dstPath + "/newdirname/",
+               dstPath + "/newdirname/foobar",
+               "/foo/bar",
+       } {
+               err = Splice(s.fs, badpath, snap1)
+               c.Check(err, ErrorIs, os.ErrNotExist, check.Commentf("badpath %s", badpath))
+               err = Splice(s.fs, badpath, snapFile)
+               c.Check(err, ErrorIs, os.ErrNotExist, check.Commentf("badpath %s", badpath))
+       }
+
+       snap2, err := Snapshot(s.fs, dstPath+"/ctxlog-copy")
+       c.Check(err, check.IsNil)
+       err = Splice(s.fs, dstPath+"/ctxlog-copy-copy", snap2)
+       c.Check(err, check.IsNil)
+
+       // Snapshot entire collection, splice into same collection at
+       // a new path, remove file from original location, verify
+       // spliced content survives
+       snapDst, err := Snapshot(s.fs, dstPath+"")
+       c.Check(err, check.IsNil)
+       err = Splice(s.fs, dstPath+"", snapDst)
+       c.Check(err, check.IsNil)
+       err = Splice(s.fs, dstPath+"/copy1", snapDst)
+       c.Check(err, check.IsNil)
+       err = Splice(s.fs, dstPath+"/copy2", snapDst)
+       c.Check(err, check.IsNil)
+       err = s.fs.RemoveAll(dstPath + "/arvados/fs_site_test.go")
+       c.Check(err, check.IsNil)
+       err = s.fs.RemoveAll(dstPath + "/arvados")
+       c.Check(err, check.IsNil)
+       _, err = s.fs.Open(dstPath + "/arvados/fs_site_test.go")
+       c.Check(err, check.Equals, os.ErrNotExist)
+       f, err = s.fs.Open(dstPath + "/copy2/arvados/fs_site_test.go")
+       c.Check(err, check.IsNil)
        defer f.Close()
        buf, err := ioutil.ReadAll(f)
        c.Check(err, check.IsNil)
-       c.Check(string(buf), check.Not(check.Equals), "")
-       err = f.Close()
-       c.Assert(err, check.IsNil)
+       c.Check(string(buf), check.Equals, string(thisfile))
 }