From: Tom Clegg Date: Thu, 10 Feb 2022 18:06:32 +0000 (-0500) Subject: 18600: Test more snapshot/splice variations and error cases. X-Git-Tag: 2.4.0~85^2~4 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/23bad0a705b1809a73ffbd5f6866e14dde5dd52e 18600: Test more snapshot/splice variations and error cases. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- diff --git a/sdk/go/arvados/fs_base.go b/sdk/go/arvados/fs_base.go index 680a8431c5..719e0e08ac 100644 --- 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 966fe9d5c0..ddff0c52e7 100644 --- 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 021e8241cf..471dc69c82 100644 --- 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 8943513271..8e7f588156 100644 --- 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 5225df59ee..3892be1e9a 100644 --- 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 9d76311800..59fa5fc176 100644 --- 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)) }