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

nielash · 2024-04-22T23:40:16Z

What is the purpose of this change?

See #7743 (comment) and feel free to cherry-pick as you see fit if some of these warrant more discussion.

Was the change discussed in an issue or in the forum before?

bisync: more fixes for integration tests #7743 (comment)

Checklist

I have read the contribution guidelines.
I have added tests for all changes in this PR if appropriate.
I have added documentation for the changes if appropriate.
All commit messages are in house style.
I'm done, this Pull Request is ready for review :-)

nielash · 2024-04-23T00:51:34Z

I wonder if the TestSFTPRclone: issue is related to the one identified on #7791?

And fix vfs cache encoding described in #7760
The problem is when we create and write cache, we use the "OS" encoded ("OS" encoding is hardcoded for different operating systems, not specified by users).
But when we copy it to the remote (backend), we use local.Fs, which accept a name already encoded in "Standard" way.
So we need to send a "Standard" encoded name.

I've reverted it for now.

nielash · 2024-04-28T13:08:54Z

This was a real tricky one, but I think I finally figured out the TestSFTPRclone: issue. 15ff8ab

ncw

Thank you @nielash there are some great fixes in there and apologies for taking ages to review them.

I'll cherry pick the ones that don't need discussion and we can discuss the rest!

See comments inline.

Thank you

ncw · 2024-05-13T17:11:53Z

cmd/bisync/bisync_test.go

@@ -939,6 +939,9 @@ func (b *bisyncTest) checkPreReqs(ctx context.Context, opt *bisync.Options) (con
 		b.fs2.Features().Disable("Copy") // API has longstanding bug for conflictBehavior=replace https://github.com/rclone/rclone/issues/4590
 		b.fs2.Features().Disable("Move")
 	}
+	if strings.Contains(strings.ToLower(fs.ConfigString(b.fs1)), "mailru") || strings.Contains(strings.ToLower(fs.ConfigString(b.fs2)), "mailru") {


At some point in the future when we can specify global config in backend config we should move this config from here into the TestMailru remote. Let's see how well it works here.

ncw · 2024-05-13T17:14:16Z

fstest/fstest.go

@@ -47,7 +47,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}$`)


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!

Made this change.

ncw · 2024-05-13T17:17:00Z

vfs/dir.go

@@ -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))


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!

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

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

I added a test.

ncw · 2024-05-13T17:18:01Z

vfs/dir.go

+func name(p string) string {
+	p = path.Base(p)
+	if p == "." {
+		p = "/"


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

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

}

ncw · 2024-05-13T17:26:41Z

cmd/serve/s3/backend.go

+// 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


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!

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),

}

ncw · 2024-05-13T17:29:24Z

backend/azureblob/azureblob.go

@@ -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) {


directory should already be in Standard encoding here since it ultimately came from the user rather than the backend, so I'm not sure what is going on here.

Same for the other backends.

I'm not sure about that. For example here:

rclone/backend/s3/s3.go

Line 4178 in e9e9feb

bucket, directory := f.split(dir)

directory is returned from f.split, which does FromStandardPath on it:

rclone/backend/s3/s3.go

Lines 2823 to 2826 in e9e9feb

func (f *Fs) split(rootRelativePath string) (bucketName, bucketPath string) {

bucketName, bucketPath = bucket.Split(bucket.Join(f.root, rootRelativePath))

return f.opt.Enc.FromStandardName(bucketName), f.opt.Enc.FromStandardPath(bucketPath)

}

Here's a test to reproduce the issue:

go test -timeout 3h -run '^TestBisyncRemoteRemote/extended_filenames$' github.com/rclone/rclone/cmd/bisync -remote TestS3,directory_markers: -v

Use 12 random characters instead of 24, to avoid trouble on the bisync tests

Before this change, renaming a directory d failed to rename its key in d.parent.items, which caused trouble later when doing Dir.Stat on a subdirectory. This change fixes the issue.

Before this change, serve s3 did not consistently save the correct modtime value in memory after putting or copying an object, which could sometimes cause an incorrect modtime to be returned. This change fixes the issue by ensuring that both "mtime" and "X-Amz-Meta-Mtime" are updated in b.meta when we have fresh data. The issue was discovered on the TestBisyncRemoteRemote/ext_paths test.

…omparison `remote` has been converted ToStandardPath a few lines above, so `directory` needs to be converted the same way in order to be compared properly. This was spotted on `TestBisyncRemoteRemote/extended_filenames` for `TestS3,directory_markers:` and `TestGoogleCloudStorage,directory_markers:` which tripped over a directory name containing a Line Feed symbol.

golangci-lint was complaining about this. `entry` can never be nil because itemToDirEntry never returns a nil interface value

rclone@10eb474 introduced a bug that caused operations.CopyDirMetadata to sometimes be called even when the dest Fs lacks metadata support. This resulted in errors such as `expecting X to have MkdirMetadata method: optional feature not implemented` and `expecting X to have SetMetadata method`. This change fixes the issue by instead using operations.SetDirModTime when s.setDirMetadata is false.

nielash · 2024-05-19T02:40:48Z

@ncw heads up -- potentially important bug fix here (1e8d42f) for a regression introduced by our recent --create-empty-src-dirs change (10eb474)

(See this log for many examples of the problem)

Previously these failed with: directory "empty metadata sub dir" not found in "": directory not found

nielash added bug Remote: SFTP Remote: Mailru integration test issue bisync labels Apr 22, 2024

nielash added this to the v1.67 milestone Apr 22, 2024

nielash added this to In progress in bisync via automation Apr 22, 2024

nielash force-pushed the bisync-integration-tests branch from ca8b380 to 2b7b725 Compare April 23, 2024 00:23

nielash marked this pull request as ready for review April 23, 2024 00:51

nielash force-pushed the bisync-integration-tests branch from 2b7b725 to 15ff8ab Compare April 28, 2024 12:55

nielash requested a review from ncw April 29, 2024 08:07

ncw reviewed May 13, 2024

View reviewed changes

nielash added 4 commits May 18, 2024 20:04

fstest: make RandomRemoteName shorter

7ecc1e7

Use 12 random characters instead of 24, to avoid trouble on the bisync tests

vfs: fix renaming a directory

a0ea81d

Before this change, renaming a directory d failed to rename its key in d.parent.items, which caused trouble later when doing Dir.Stat on a subdirectory. This change fixes the issue.

nielash force-pushed the bisync-integration-tests branch from b56d330 to 39eb229 Compare May 19, 2024 01:47

nielash added 2 commits May 18, 2024 22:05

googlephotos: remove unnecessary nil check

47690fb

golangci-lint was complaining about this. `entry` can never be nil because itemToDirEntry never returns a nil interface value

sync: fix testCopyMetadata when Fs doesn't support empty directories

0e12c78

Previously these failed with: directory "empty metadata sub dir" not found in "": directory not found

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nielash commented Apr 22, 2024

nielash commented Apr 23, 2024

nielash commented Apr 28, 2024

ncw left a comment

ncw May 13, 2024

ncw May 13, 2024

nielash May 19, 2024

ncw May 13, 2024

nielash May 14, 2024

nielash May 14, 2024

nielash May 19, 2024

ncw May 13, 2024

nielash May 14, 2024

ncw May 13, 2024

nielash May 14, 2024

ncw May 13, 2024

nielash May 14, 2024

nielash commented May 19, 2024

	// 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
	}

	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
	}

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

	func (f *Fs) split(rootRelativePath string) (bucketName, bucketPath string) {
	bucketName, bucketPath = bucket.Split(bucket.Join(f.root, rootRelativePath))
	return f.opt.Enc.FromStandardName(bucketName), f.opt.Enc.FromStandardPath(bucketPath)
	}

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

Are you sure you want to change the base?

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

Conversation

nielash commented Apr 22, 2024

What is the purpose of this change?

Was the change discussed in an issue or in the forum before?

Checklist

nielash commented Apr 23, 2024

nielash commented Apr 28, 2024

ncw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielash commented May 19, 2024