Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bisync: another day, another set of integration test fixes #7795

Closed
wants to merge 7 commits into from
+54 −13
Closed

bisync: another day, another set of integration test fixes #7795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7ecc1e7
fstest: make RandomRemoteName shorter
nielash Apr 22, 2024
a0ea81d
vfs: fix renaming a directory
nielash Apr 28, 2024
7d61df0
serve s3: fix in-memory metadata storing wrong modtime
nielash Apr 29, 2024
39eb229
s3, googlecloudstorage, azureblob: fix encoding issue with dir path c…
nielash Apr 29, 2024
47690fb
googlephotos: remove unnecessary nil check
nielash May 19, 2024
1e8d42f
sync: fix optional feature not implemented errors
nielash May 19, 2024
0e12c78
sync: fix testCopyMetadata when Fs doesn't support empty directories
nielash May 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion backend/azureblob/azureblob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1088,7 +1088,7 @@ func (f *Fs) list(ctx context.Context, containerName, directory, prefix string,
isDirectory := isDirectoryMarker(*file.Properties.ContentLength, file.Metadata, remote)
if isDirectory {
// Don't insert the root directory
if remote == directory {
if remote == f.opt.Enc.ToStandardPath(directory) {
ncw marked this conversation as resolved.
Show resolved Hide resolved
continue
}
// process directory markers as directories
Expand Down
2 changes: 1 addition & 1 deletion backend/googlecloudstorage/googlecloudstorage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ func (f *Fs) list(ctx context.Context, bucket, directory, prefix string, addBuck
// is this a directory marker?
if isDirectory {
// Don't insert the root directory
if remote == directory {
if remote == f.opt.Enc.ToStandardPath(directory) {
continue
}
// process directory markers as directories
Expand Down
4 changes: 1 addition & 3 deletions backend/googlephotos/googlephotos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,9 +620,7 @@ func (f *Fs) listDir(ctx context.Context, prefix string, filter api.SearchFilter
if err != nil {
return err
}
if entry != nil {
entries = append(entries, entry)
}
entries = append(entries, entry)
return nil
})
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion backend/s3/s3.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4026,7 +4026,7 @@ func (f *Fs) list(ctx context.Context, opt listOpt, fn listFn) error {
isDirectory = false
} else {
// Don't insert the root directory
if remote == opt.directory {
if remote == f.opt.Enc.ToStandardPath(opt.directory) {
continue
}
}
Expand Down
13 changes: 13 additions & 0 deletions cmd/serve/s3/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,14 @@ func (b *s3Backend) GetObject(ctx context.Context, bucketName, objectName string
}, nil
}

// storeModtime sets both "mtime" and "X-Amz-Meta-Mtime" to val in b.meta.
// Call this whenever modtime is updated.
func (b *s3Backend) storeModtime(fp string, meta map[string]string, val string) {
meta["X-Amz-Meta-Mtime"] = val
meta["mtime"] = val
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should only be necessary to set X-Amz-Meta-Mtime as the s3 backend turns it into the mtime metadata.

So I'm not 100% sure what this is needed for.

I think the serve s3 code is a bit confused about exactly which parameter to use so we should probably unconfuse it rather than make it worse!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a test to reproduce:

go run ./fstest/test_all  -run '^TestBisyncRemoteRemote/ext_paths.*$' -timeout 30s -verbose -maxtries 1 -remotes TestS3Rclone:

If I remember correctly, I think the problem is that there are blocks like this which try X-Amz-Meta-Mtime first but fall back to using mtime:

rclone/cmd/serve/s3/backend.go

Lines 307 to 321 in e9e9feb

if val, ok := meta["X-Amz-Meta-Mtime"]; ok {
ti, err := swift.FloatStringToTime(val)
if err == nil {
return result, b.vfs.Chtimes(fp, ti, ti)
}
// ignore error since the file is successfully created
}
if val, ok := meta["mtime"]; ok {
ti, err := swift.FloatStringToTime(val)
if err == nil {
return result, b.vfs.Chtimes(fp, ti, ti)
}
// ignore error since the file is successfully created
}

And previously it was not always consistently saving the one it ended up using in memory.

And then there's also Last-Modified just to make things even more fun!

rclone/cmd/serve/s3/backend.go

Lines 194 to 197 in e9e9feb

meta := map[string]string{
"Last-Modified": node.ModTime().Format(timeFormat),
"Content-Type": fs.MimeType(context.Background(), fobj),
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably fix serve s3 to be consistent and use just one of these values, but I decided to cherry-pick your patch for v1.67 - thank you :-)

b.meta.Store(fp, meta)
}

// TouchObject creates or updates meta on specified object.
func (b *s3Backend) TouchObject(ctx context.Context, fp string, meta map[string]string) (result gofakes3.PutObjectResult, err error) {
_, err = b.vfs.Stat(fp)
Expand All @@ -237,6 +245,7 @@ func (b *s3Backend) TouchObject(ctx context.Context, fp string, meta map[string]
if val, ok := meta["X-Amz-Meta-Mtime"]; ok {
ti, err := swift.FloatStringToTime(val)
if err == nil {
b.storeModtime(fp, meta, val)
return result, b.vfs.Chtimes(fp, ti, ti)
}
// ignore error since the file is successfully created
Expand All @@ -245,6 +254,7 @@ func (b *s3Backend) TouchObject(ctx context.Context, fp string, meta map[string]
if val, ok := meta["mtime"]; ok {
ti, err := swift.FloatStringToTime(val)
if err == nil {
b.storeModtime(fp, meta, val)
return result, b.vfs.Chtimes(fp, ti, ti)
}
// ignore error since the file is successfully created
Expand Down Expand Up @@ -307,6 +317,7 @@ func (b *s3Backend) PutObject(
if val, ok := meta["X-Amz-Meta-Mtime"]; ok {
ti, err := swift.FloatStringToTime(val)
if err == nil {
b.storeModtime(fp, meta, val)
return result, b.vfs.Chtimes(fp, ti, ti)
}
// ignore error since the file is successfully created
Expand All @@ -315,6 +326,7 @@ func (b *s3Backend) PutObject(
if val, ok := meta["mtime"]; ok {
ti, err := swift.FloatStringToTime(val)
if err == nil {
b.storeModtime(fp, meta, val)
return result, b.vfs.Chtimes(fp, ti, ti)
}
// ignore error since the file is successfully created
Expand Down Expand Up @@ -425,6 +437,7 @@ func (b *s3Backend) CopyObject(ctx context.Context, srcBucket, srcKey, dstBucket
if err != nil {
return result, nil
}
b.storeModtime(fp, meta, val)

return result, b.vfs.Chtimes(fp, ti, ti)
}
Expand Down
2 changes: 1 addition & 1 deletion fs/sync/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,7 @@ func (s *syncCopyMove) setDelayedDirModTimes(ctx context.Context) error {
g.Go(func() error {
var err error
// if item.src is set must copy full metadata
if item.src != nil {
if item.src != nil && s.setDirMetadata {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now fixed this is a slightly different way in 39f8d03 to make it clearer what is going on rather than using the state of item.src == nil to signify metadata.

_, err = operations.CopyDirMetadata(gCtx, s.fdst, item.dst, item.dir, item.src)
} else {
_, err = operations.SetDirModTime(gCtx, s.fdst, item.dst, item.dir, item.modTime)
Expand Down
8 changes: 6 additions & 2 deletions fs/sync/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ func testCopyMetadata(t *testing.T, createEmptySrcDirs bool) {
if features.ReadDirMetadata {
fstest.CheckEntryMetadata(ctx, t, r.Fremote, fstest.NewDirectory(ctx, t, r.Fremote, dirPath), dirMetadata)
}
if !createEmptySrcDirs {
if !createEmptySrcDirs || !features.CanHaveEmptyDirectories {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this one in 76798d5 - I'd forgotten I hadn't merged your fix - sorry!

// dir must not exist
_, err := fstest.NewDirectoryRetries(ctx, t, r.Fremote, emptyDirPath, 1)
assert.Error(t, err, "Not expecting to find empty directory")
Expand Down Expand Up @@ -2310,15 +2310,19 @@ func testSyncBackupDir(t *testing.T, backupDir string, suffix string, suffixKeep

r.CheckRemoteItems(t, file1b, file2, file3a, file1a)
}

func TestSyncBackupDir(t *testing.T) {
testSyncBackupDir(t, "backup", "", false)
}

func TestSyncBackupDirWithSuffix(t *testing.T) {
testSyncBackupDir(t, "backup", ".bak", false)
}

func TestSyncBackupDirWithSuffixKeepExtension(t *testing.T) {
testSyncBackupDir(t, "backup", "-2019-01-01", true)
}

func TestSyncBackupDirSuffixOnly(t *testing.T) {
testSyncBackupDir(t, "", ".bak", false)
}
Expand Down Expand Up @@ -2788,7 +2792,7 @@ func predictDstFromLogger(ctx context.Context) context.Context {
}

func DstLsf(ctx context.Context, Fremote fs.Fs) *bytes.Buffer {
var opt = operations.ListJSONOpt{
opt := operations.ListJSONOpt{
NoModTime: false,
NoMimeType: true,
DirsOnly: false,
Expand Down
6 changes: 3 additions & 3 deletions fstest/fstest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ var (
// ListRetries is the number of times to retry a listing to overcome eventual consistency
ListRetries = flag.Int("list-retries", 3, "Number or times to retry listing")
// MatchTestRemote matches the remote names used for testing
MatchTestRemote = regexp.MustCompile(`^rclone-test-[abcdefghijklmnopqrstuvwxyz0123456789]{24}$`)
MatchTestRemote = regexp.MustCompile(`^rclone-test-[abcdefghijklmnopqrstuvwxyz0123456789]{12}$`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to change

rclone/fstest/test_all/clean.go

Line 20 in 0735f44

var MatchTestRemote = regexp.MustCompile(`^rclone-test-[abcdefghijklmnopqrstuvwxyz0123456789]{24}(_segments)?$`)

Though maybe that one should say {12,24} to clean the old and the new!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I've cherry-picked this now.

)

// Initialise rclone for testing
Expand Down Expand Up @@ -266,7 +266,7 @@ func CheckListingWithRoot(t *testing.T, f fs.Fs, dir string, items []Item, expec
var objs []fs.Object
var dirs []fs.Directory
var err error
var retries = *ListRetries
retries := *ListRetries
sleep := time.Second / 2
wantListing := makeListingFromItems(items)
gotListing := "<unset>"
Expand Down Expand Up @@ -431,7 +431,7 @@ func RandomRemoteName(remoteName string) (string, string, error) {
if !strings.HasSuffix(remoteName, ":") {
remoteName += "/"
}
leafName = "rclone-test-" + random.String(24)
leafName = "rclone-test-" + random.String(12)
if !MatchTestRemote.MatchString(leafName) {
log.Fatalf("%q didn't match the test remote name regexp", leafName)
}
Expand Down
2 changes: 1 addition & 1 deletion fstest/test_all/clean.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
// MatchTestRemote matches the remote names used for testing (copied
// from fstest/fstest.go so we don't have to import that and get all
// its flags)
var MatchTestRemote = regexp.MustCompile(`^rclone-test-[abcdefghijklmnopqrstuvwxyz0123456789]{24}(_segments)?$`)
var MatchTestRemote = regexp.MustCompile(`^rclone-test-[abcdefghijklmnopqrstuvwxyz0123456789]{12,24}(_segments)?$`)

// cleanFs runs a single clean fs for left over directories
func cleanFs(ctx context.Context, remote string, cleanup bool) error {
Expand Down
13 changes: 13 additions & 0 deletions vfs/dir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,9 @@ func (d *Dir) renameTree(dirPath string) {
// Make sure the path is correct for each node
if d.path != dirPath {
fs.Debugf(d.path, "Renaming to %q", dirPath)
delete(d.parent.items, name(d.path))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good fix, but it really needs a test. I try to keep the VFS coverage up as all these corner cases are really difficult to track down!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll try to add one! (Although FWIW, I found this because it was causing TestSFTPRclone: to fail!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also btw, I am now seeing a bunch of expecting X to have MkdirMetadata method: optional feature not implemented and expecting X to have SetMetadata method errors that I'm pretty sure were not there before...maybe related to the recent --create-empty-src-dirs change?

https://pub.rclone.org/integration-tests/2024-05-13-010010/sftp-cmd.bisync-TestSFTPRclone-5.txt

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I've cherry picked this :-)

d.path = dirPath
d.parent.items[name(d.path)] = d
d.entry = fs.NewDirCopy(context.TODO(), d.entry).SetRemote(dirPath)
}

Expand Down Expand Up @@ -404,6 +406,8 @@ func (d *Dir) rename(newParent *Dir, fsDir fs.Directory) {
d.entry = fsDir
d.path = fsDir.Remote()
newPath := d.path
delete(d.parent.items, name(oldPath))
d.parent.items[name(d.path)] = d
d.read = time.Time{}
d.mu.Unlock()

Expand All @@ -418,6 +422,15 @@ func (d *Dir) rename(newParent *Dir, fsDir fs.Directory) {
}
}

// convert path to name
func name(p string) string {
p = path.Base(p)
if p == "." {
p = "/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should probably be p = "" to fit with rclone conventions, not 100% sure!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I copied this from here:

rclone/vfs/dir.go

Lines 163 to 172 in e9e9feb

// Name (base) of the directory - satisfies Node interface
func (d *Dir) Name() (name string) {
d.mu.RLock()
name = path.Base(d.path)
d.mu.RUnlock()
if name == "." {
name = "/"
}
return name
}

}
return p
}

// addObject adds a new object or directory to the directory
//
// The name passed in is marked as virtual as it hasn't been read from a remote
Expand Down
13 changes: 13 additions & 0 deletions vfs/dir_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,19 @@ func TestDirRename(t *testing.T) {
vfs.Opt.ReadOnly = true
err = dir.Rename("potato", "tuba", dir)
assert.Equal(t, EROFS, err)

// Rename a dir, check that key was correctly renamed in dir.parent.items
vfs.Opt.ReadOnly = false
_, ok := dir.parent.items["dir2"]
assert.True(t, ok, "dir.parent.items should have 'dir2' key before rename")
_, ok = dir.parent.items["dir3"]
assert.False(t, ok, "dir.parent.items should not have 'dir3' key before rename")
dir.renameTree("dir3") // rename dir2 to dir3
_, ok = dir.parent.items["dir2"]
assert.False(t, ok, "dir.parent.items should not have 'dir2' key after rename")
d, ok := dir.parent.items["dir3"]
assert.True(t, ok, fmt.Sprintf("expected to find 'dir3' key in dir.parent.items after rename, got %v", dir.parent.items))
assert.Equal(t, dir, d, `expected renamed dir to match value of dir.parent.items["dir3"]`)
}

func TestDirStructSize(t *testing.T) {
Expand Down