-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
bisync: another day, another set of integration test fixes #7795
base: master
Are you sure you want to change the base?
Conversation
ca8b380
to
2b7b725
Compare
I wonder if the
I've reverted it for now. |
2b7b725
to
15ff8ab
Compare
This was a real tricky one, but I think I finally figured out the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
cmd/bisync/bisync_test.go
Outdated
@@ -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") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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}$`) |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this change.
@@ -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)) |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test.
func name(p string) string { | ||
p = path.Base(p) | ||
if p == "." { | ||
p = "/" |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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:
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 | |
} |
// 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 |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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), | |
} |
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about that. For example here:
Line 4178 in e9e9feb
bucket, directory := f.split(dir) |
directory
is returned from f.split
, which does FromStandardPath
on it:
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.
b56d330
to
39eb229
Compare
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.
Previously these failed with: directory "empty metadata sub dir" not found in "": directory not found
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?
Checklist