From 5c594fea90429ccc35328a3c387b9d326167cfa9 Mon Sep 17 00:00:00 2001 From: Janne Hellsten Date: Tue, 4 Apr 2023 18:19:16 +0300 Subject: [PATCH] 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 | 57 +++++++++++++++++++++++---- fs/operations/operations_test.go | 58 ++++++++++++++++++++++++++++ 5 files changed, 153 insertions(+), 7 deletions(-) diff --git a/docs/content/docs.md b/docs/content/docs.md index 22e8c8c93..fe68b65b7 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 d243d794a..a6de70b85 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 63f107fd0..60047227e 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 2752b06c3..ecdf4cef4 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,13 +457,16 @@ 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 { - actionTaken = "Copied (replaced existing)" + if doUpdate && inplace { err = dst.Update(ctx, in, wrappedSrc, options...) } else { - actionTaken = "Copied (new)" dst, err = f.Put(ctx, in, wrappedSrc, options...) } + if doUpdate { + actionTaken = "Copied (replaced existing)" + } else { + actionTaken = "Copied (new)" + } closeErr := in.Close() if err == nil { newDst = dst @@ -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 5125e5865..a77646610 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