From 5555998f4461f2cfd15b942519d25d7b4750e573 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Thu, 4 May 2023 17:35:06 +0100 Subject: [PATCH 1/5] fs: fix String() method on fs.OverrideRemote Before this fix it was returning the base objects string rather than the overridden remote. --- fs/override.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fs/override.go b/fs/override.go index 014acb8ae4ed1..b06c0d0700610 100644 --- a/fs/override.go +++ b/fs/override.go @@ -23,6 +23,11 @@ func (o *OverrideRemote) Remote() string { return o.remote } +// String returns the overridden remote name +func (o *OverrideRemote) String() string { + return o.remote +} + // MimeType returns the mime type of the underlying object or "" if it // can't be worked out func (o *OverrideRemote) MimeType(ctx context.Context) string { From f9945b7f80d2a28ba04154966e5ac0c3f43db8b1 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Thu, 4 May 2023 17:49:39 +0100 Subject: [PATCH 2/5] fs: when creating new fs.OverrideRemotes don't layer overrides if not needed --- fs/override.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/fs/override.go b/fs/override.go index b06c0d0700610..9ba8c4aeffba1 100644 --- a/fs/override.go +++ b/fs/override.go @@ -12,6 +12,13 @@ type OverrideRemote struct { // NewOverrideRemote returns an OverrideRemoteObject which will // return the remote specified func NewOverrideRemote(oi ObjectInfo, remote string) *OverrideRemote { + // re-wrap an OverrideRemote + if or, ok := oi.(*OverrideRemote); ok { + return &OverrideRemote{ + ObjectInfo: or.ObjectInfo, + remote: remote, + } + } return &OverrideRemote{ ObjectInfo: oi, remote: remote, From d4cb92e457e80b9d8b99b173567294a355296e31 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Tue, 9 May 2023 15:29:55 +0100 Subject: [PATCH 3/5] sftp: fix move to allow overwriting existing files Before this change rclone used a normal SFTP rename if present to implement Move. However the normal SFTP rename won't overwrite existing files. This fixes it to either use the POSIX rename extension ("posix-rename@openssh.com") or to delete the source first before renaming using the normal SFTP rename. This isn't normally a problem as rclone always removes any existing objects first, however to implement non --inplace operations we do require overwriting an existing file. --- backend/sftp/sftp.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/backend/sftp/sftp.go b/backend/sftp/sftp.go index 3554343b82113..6338534bf958c 100644 --- a/backend/sftp/sftp.go +++ b/backend/sftp/sftp.go @@ -1329,10 +1329,17 @@ func (f *Fs) Move(ctx context.Context, src fs.Object, remote string) (fs.Object, if err != nil { return nil, fmt.Errorf("Move: %w", err) } - err = c.sftpClient.Rename( - srcObj.path(), - path.Join(f.absRoot, remote), - ) + srcPath, dstPath := srcObj.path(), path.Join(f.absRoot, remote) + if _, ok := c.sftpClient.HasExtension("posix-rename@openssh.com"); ok { + err = c.sftpClient.PosixRename(srcPath, dstPath) + } else { + // If haven't got PosixRename then remove source first before renaming + err = c.sftpClient.Remove(dstPath) + if err != nil && !errors.Is(err, iofs.ErrNotExist) { + fs.Errorf(f, "Move: Failed to remove existing file %q: %v", dstPath, err) + } + err = c.sftpClient.Rename(srcPath, dstPath) + } f.putSftpConnection(&c, err) if err != nil { return nil, fmt.Errorf("Move Rename failed: %w", err) From 5f880dceb359a8189ceeb57269b49a51577fe470 Mon Sep 17 00:00:00 2001 From: Janne Hellsten Date: Tue, 4 Apr 2023 18:19:16 +0300 Subject: [PATCH 4/5] fs: Implement PartialUploads feature flag Implement a Partialuploads feature flag to mark backends for which uploads are not atomic. This is set for the following backends - local - ftp - sftp See #3770 --- backend/ftp/ftp.go | 1 + backend/local/local.go | 1 + backend/sftp/sftp.go | 1 + fs/features.go | 1 + 4 files changed, 4 insertions(+) diff --git a/backend/ftp/ftp.go b/backend/ftp/ftp.go index 7aa9935c5e5b4..7db6437e3991c 100644 --- a/backend/ftp/ftp.go +++ b/backend/ftp/ftp.go @@ -580,6 +580,7 @@ func NewFs(ctx context.Context, name, root string, m configmap.Mapper) (ff fs.Fs } f.features = (&fs.Features{ CanHaveEmptyDirectories: true, + PartialUploads: true, }).Fill(ctx, f) // set the pool drainer timer going if f.opt.IdleTimeout > 0 { diff --git a/backend/local/local.go b/backend/local/local.go index 4acb1b7883418..a83a3c4551497 100644 --- a/backend/local/local.go +++ b/backend/local/local.go @@ -303,6 +303,7 @@ func NewFs(ctx context.Context, name, root string, m configmap.Mapper) (fs.Fs, e WriteMetadata: true, UserMetadata: xattrSupported, // can only R/W general purpose metadata if xattrs are supported FilterAware: true, + PartialUploads: true, }).Fill(ctx, f) if opt.FollowSymlinks { f.lstat = os.Stat diff --git a/backend/sftp/sftp.go b/backend/sftp/sftp.go index 6338534bf958c..6127a23339f92 100644 --- a/backend/sftp/sftp.go +++ b/backend/sftp/sftp.go @@ -994,6 +994,7 @@ func NewFsWithConnection(ctx context.Context, f *Fs, name string, root string, m f.features = (&fs.Features{ CanHaveEmptyDirectories: true, SlowHash: true, + PartialUploads: true, }).Fill(ctx, f) // Make a connection and pool it to return errors early c, err := f.getSftpConnection(ctx) diff --git a/fs/features.go b/fs/features.go index 8a85c02269225..5820b117802e3 100644 --- a/fs/features.go +++ b/fs/features.go @@ -30,6 +30,7 @@ type Features struct { WriteMetadata bool // can write metadata to objects UserMetadata bool // can read/write general purpose metadata FilterAware bool // can make use of filters if provided for listing + PartialUploads bool // uploaded file can appear incomplete on the fs while it's being uploaded // Purge all files in the directory specified // From a5e81476738ab0342c6c1539c62fa2e5742c6038 Mon Sep 17 00:00:00 2001 From: Janne Hellsten Date: Tue, 4 Apr 2023 18:19:16 +0300 Subject: [PATCH 5/5] operations: implement uploads to temp name with --inplace to disable When copying to a backend which has the PartialUploads feature flag set and can Move files the file is copied into a temporary name first. Once the copy is complete, the file is renamed to the real destination. This prevents other processes from seeing partially downloaded copies of files being downloaded and prevents overwriting the old file until the new one is complete. This also adds --inplace flag that can be used to disable the partial file copy/rename feature. See #3770 Co-authored-by: Nick Craig-Wood --- docs/content/docs.md | 43 +++++++++++++++++++++ fs/config.go | 1 + fs/config/configflags/configflags.go | 1 + fs/operations/operations.go | 55 +++++++++++++++++++++++--- fs/operations/operations_test.go | 58 ++++++++++++++++++++++++++++ 5 files changed, 152 insertions(+), 6 deletions(-) diff --git a/docs/content/docs.md b/docs/content/docs.md index 22e8c8c93694b..fe68b65b7f628 100644 --- a/docs/content/docs.md +++ b/docs/content/docs.md @@ -1257,6 +1257,49 @@ This can be useful as an additional layer of protection for immutable or append-only data sets (notably backup archives), where modification implies corruption and should not be propagated. +### --inplace {#inplace} + +The `--inplace` flag changes the behaviour of rclone when uploading +files to some backends (backends with the `PartialUploads` feature +flag set) such as: + +- local +- ftp +- sftp + +Without `--inplace` (the default) rclone will first upload to a +temporary file with an extension like this where `XXXXXX` represents a +random string. + + original-file-name.XXXXXX.partial + +(rclone will make sure the final name is no longer than 100 characters +by truncating the `original-file-name` part if necessary). + +When the upload is complete, rclone will rename the `.partial` file to +the correct name, overwriting any existing file at that point. If the +upload fails then the `.partial` file will be deleted. + +This prevents other users of the backend from seeing partially +uploaded files in their new names and prevents overwriting the old +file until the new one is completely uploaded. + +If the `--inplace` flag is supplied, rclone will upload directly to +the final name without creating a `.partial` file. + +This means that an incomplete file will be visible in the directory +listings while the upload is in progress and any existing files will +be overwritten as soon as the upload starts. If the transfer fails +then the file will be deleted. This can cause data loss of the +existing file if the transfer fails. + +Note that on the local file system if you don't use `--inplace` hard +links (Unix only) will be broken. And if you do use `--inplace` you +won't be able to update in use executables. + +Note also that versions of rclone prior to v1.63.0 behave as if the +`--inplace` flag is always supplied. + ### -i, --interactive {#interactive} This flag can be used to tell rclone that you wish a manual diff --git a/fs/config.go b/fs/config.go index d243d794a5f32..a6de70b85f7a4 100644 --- a/fs/config.go +++ b/fs/config.go @@ -145,6 +145,7 @@ type ConfigInfo struct { ServerSideAcrossConfigs bool TerminalColorMode TerminalColorMode DefaultTime Time // time that directories with no time should display + Inplace bool // Download directly to destination file instead of atomic download to temp/rename } // NewConfig creates a new config with everything set to the default diff --git a/fs/config/configflags/configflags.go b/fs/config/configflags/configflags.go index 63f107fd07a5a..60047227e0926 100644 --- a/fs/config/configflags/configflags.go +++ b/fs/config/configflags/configflags.go @@ -145,6 +145,7 @@ func AddFlags(ci *fs.ConfigInfo, flagSet *pflag.FlagSet) { flags.BoolVarP(flagSet, &ci.ServerSideAcrossConfigs, "server-side-across-configs", "", ci.ServerSideAcrossConfigs, "Allow server-side operations (e.g. copy) to work across different configs") flags.FVarP(flagSet, &ci.TerminalColorMode, "color", "", "When to show colors (and other ANSI codes) AUTO|NEVER|ALWAYS") flags.FVarP(flagSet, &ci.DefaultTime, "default-time", "", "Time to show if modtime is unknown for files and directories") + flags.BoolVarP(flagSet, &ci.Inplace, "inplace", "", ci.Inplace, "Download directly to destination file instead of atomic download to temp/rename") } // ParseHeaders converts the strings passed in via the header flags into HTTPOptions diff --git a/fs/operations/operations.go b/fs/operations/operations.go index 2752b06c3c577..ecdf4cef4b1e5 100644 --- a/fs/operations/operations.go +++ b/fs/operations/operations.go @@ -336,6 +336,27 @@ func Copy(ctx context.Context, f fs.Fs, dst fs.Object, remote string, src fs.Obj doUpdate := dst != nil hashType, hashOption := CommonHash(ctx, f, src.Fs()) + if dst != nil { + remote = dst.Remote() + } + + var ( + inplace = true + remotePartial = remote + ) + if !ci.Inplace && f.Features().Move != nil && f.Features().PartialUploads { + // Avoid making the leaf name longer if it's already lengthy to avoid + // trouble with file name length limits. + suffix := "." + random.String(8) + ".partial" + base := path.Base(remotePartial) + if len(base) > 100 { + remotePartial = remotePartial[:len(remotePartial)-len(suffix)] + suffix + } else { + remotePartial += suffix + } + inplace = false + } + var actionTaken string for { // Try server-side copy first - if has optional interface and @@ -363,6 +384,7 @@ func Copy(ctx context.Context, f fs.Fs, dst fs.Object, remote string, src fs.Obj dst = newDst in.ServerSideCopyEnd(dst.Size()) // account the bytes for the server-side transfer _ = in.Close() + inplace = true } else { _ = in.Close() } @@ -384,7 +406,10 @@ func Copy(ctx context.Context, f fs.Fs, dst fs.Object, remote string, src fs.Obj if streams < 2 { streams = 2 } - dst, err = multiThreadCopy(ctx, f, remote, src, int(streams), tr) + dst, err = multiThreadCopy(ctx, f, remotePartial, src, int(streams), tr) + if err == nil { + newDst = dst + } if doUpdate { actionTaken = "Multi-thread Copied (replaced existing)" } else { @@ -416,14 +441,14 @@ func Copy(ctx context.Context, f fs.Fs, dst fs.Object, remote string, src fs.Obj } } // NB Rcat closes in0 - dst, err = Rcat(ctx, f, remote, in0, src.ModTime(ctx), meta) + dst, err = Rcat(ctx, f, remotePartial, in0, src.ModTime(ctx), meta) newDst = dst } else { in := tr.Account(ctx, in0).WithBuffer() // account and buffer the transfer var wrappedSrc fs.ObjectInfo = src // We try to pass the original object if possible - if src.Remote() != remote { - wrappedSrc = fs.NewOverrideRemote(src, remote) + if src.Remote() != remotePartial { + wrappedSrc = fs.NewOverrideRemote(src, remotePartial) } options := []fs.OpenOption{hashOption} for _, option := range ci.UploadHeaders { @@ -432,12 +457,15 @@ func Copy(ctx context.Context, f fs.Fs, dst fs.Object, remote string, src fs.Obj if ci.MetadataSet != nil { options = append(options, fs.MetadataOption(ci.MetadataSet)) } + if doUpdate && inplace { + err = dst.Update(ctx, in, wrappedSrc, options...) + } else { + dst, err = f.Put(ctx, in, wrappedSrc, options...) + } if doUpdate { actionTaken = "Copied (replaced existing)" - err = dst.Update(ctx, in, wrappedSrc, options...) } else { actionTaken = "Copied (new)" - dst, err = f.Put(ctx, in, wrappedSrc, options...) } closeErr := in.Close() if err == nil { @@ -499,6 +527,21 @@ func Copy(ctx context.Context, f fs.Fs, dst fs.Object, remote string, src fs.Obj return newDst, err } } + + // Move the copied file to its real destination. + if err == nil && !inplace && remotePartial != remote { + dst, err = f.Features().Move(ctx, newDst, remote) + if err == nil { + fs.Debugf(newDst, "renamed to: %s", remote) + newDst = dst + } else { + fs.Errorf(newDst, "partial file rename failed: %v", err) + err = fs.CountError(err) + removeFailedCopy(ctx, newDst) + return newDst, err + } + } + if newDst != nil && src.String() != newDst.String() { actionTaken = fmt.Sprintf("%s to: %s", actionTaken, newDst.String()) } diff --git a/fs/operations/operations_test.go b/fs/operations/operations_test.go index 5125e5865048e..a77646610a728 100644 --- a/fs/operations/operations_test.go +++ b/fs/operations/operations_test.go @@ -1228,6 +1228,64 @@ func TestCopyFileCopyDest(t *testing.T) { r.CheckRemoteItems(t, file2, file2dst, file3, file4, file4dst, file6, file7dst) } +func TestCopyInplace(t *testing.T) { + ctx := context.Background() + ctx, ci := fs.AddConfig(ctx) + r := fstest.NewRun(t) + + ci.Inplace = true + + file1 := r.WriteFile("file1", "file1 contents", t1) + r.CheckLocalItems(t, file1) + + file2 := file1 + file2.Path = "sub/file2" + + err := operations.CopyFile(ctx, r.Fremote, r.Flocal, file2.Path, file1.Path) + require.NoError(t, err) + r.CheckLocalItems(t, file1) + r.CheckRemoteItems(t, file2) + + err = operations.CopyFile(ctx, r.Fremote, r.Flocal, file2.Path, file1.Path) + require.NoError(t, err) + r.CheckLocalItems(t, file1) + r.CheckRemoteItems(t, file2) + + err = operations.CopyFile(ctx, r.Fremote, r.Fremote, file2.Path, file2.Path) + require.NoError(t, err) + r.CheckLocalItems(t, file1) + r.CheckRemoteItems(t, file2) +} + +func TestCopyLongFileName(t *testing.T) { + ctx := context.Background() + ctx, ci := fs.AddConfig(ctx) + r := fstest.NewRun(t) + + ci.Inplace = false // the default + + file1 := r.WriteFile("file1", "file1 contents", t1) + r.CheckLocalItems(t, file1) + + file2 := file1 + file2.Path = "sub/" + strings.Repeat("file2", 30) + + err := operations.CopyFile(ctx, r.Fremote, r.Flocal, file2.Path, file1.Path) + require.NoError(t, err) + r.CheckLocalItems(t, file1) + r.CheckRemoteItems(t, file2) + + err = operations.CopyFile(ctx, r.Fremote, r.Flocal, file2.Path, file1.Path) + require.NoError(t, err) + r.CheckLocalItems(t, file1) + r.CheckRemoteItems(t, file2) + + err = operations.CopyFile(ctx, r.Fremote, r.Fremote, file2.Path, file2.Path) + require.NoError(t, err) + r.CheckLocalItems(t, file1) + r.CheckRemoteItems(t, file2) +} + // testFsInfo is for unit testing fs.Info type testFsInfo struct { name string