-
-
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
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.
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.
Thanks. I've cherry-picked this now.
@@ -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.
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. I've cherry picked this :-)
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), | |
} |
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 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 :-)
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
@@ -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 { |
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 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.
@@ -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 { |
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 fixed this one in 76798d5 - I'd forgotten I hadn't merged your fix - sorry!
I've merged all these (or equivalents) now, so I shall close this PR - thank you :-) Let's see how the integration tests do tomorrow! |
Much better! 36 test failures in total
|
Getting closer! I think a lot of the remaining ones are issues I made guesses about here. I think they are all backend-specific issues now, as opposed to bisync issues. |
I removed the ones that are now fixed from your list and added one more, protondrive. It looks like there are a few low hanging fruits there. General comments
Current failures
|
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