Suspected bug in S3 (or compatible) sync logic to Glacier

[tekkiwi]

When syncing from a S3 bucket to a S3 bucket if rclone detects that the source object timestamp has changed and the file is the same size and etag is matching in source and destination it attempts to update the metadata that stores the original modtime. For S3 standard tiers this update succeeds (and results in a server side copy) but for objects that are in Glacier or Glacier Deep Archive tier on the target side updating the modtime updates fails since you can't do a server side copy of an object that is in Glacier tier.
The error message is:
Failed to set modification time: InvalidObjectState: Operation is not valid for the source object's storage class

I believe the behavior should be that if the object is in Glacier or Glacier Deep Archive that it should upload the object with the new modtime rather than attempt to do the server side copy/modtime update.

I am using rclone 1.47 and have tested with the latest beta, rclone-v1.47.0-074-g81fad0f0-beta-windows-amd64. Tested on Windows and Linux.
Rclone command line:
rclone sync ecs:bucket1 aws:bucket1 --bwlimit 95M --transfers 64 --s3-upload-concurrency 10 --checkers 500 --s3-chunk-size 100M --log-level INFO

rclone.conf for sync target:
[aws]
type = s3
provider = AWS
env_auth = false
access_key_id =XXX
secret_access_key = XXX
region = us-west-2
location_constraint = us-west-2
acl = private
storage_class = DEEP_ARCHIVE

[tekkiwi]

Missed that the return code is a 403, so rclone could catch the 403 and upload the object if a 403 is returned trying to update the modtime tag

[ncw (Nick Craig-Wood)]

I believe the behavior should be that if the object is in Glacier or Glacier Deep Archive that it should upload the object with the new modtime rather than attempt to do the server side copy/modtime update.

That should be relatively easy to implement....

The SetModTime() call in the s3 backend needs to end up returning an error fs.ErrorCantSetModTime which will cause rclone to re-upload the file.

It looks like there is a precise error we can check InvalidObjectState (or maybe we can tell in advance the object is in Glacier, I'm not sure).

Do you fancy having a go at changing this?

[tekkiwi]

Thanks for the reply ncw.
I have no knowledge of Go (or development in general) and am struggling to figure out how to fix this.
Would the SetModTime function need to return nil in order to cause the calling function to copy rather than update mod time? Can we detect if the object is in Glacier with something like if o.fs.opt.StorageClass == "GLACIER", and does this account for Glacier Deep Archive tier?

[ncw (Nick Craig-Wood)]

I have no knowledge of Go (or development in general) and am struggling to figure out how to fix this.
Would the SetModTime function need to return nil in order to cause the calling function to copy rather than update mod time?

You need to return the specific error fs.ErrorCantSetModTime then higher layers will do a copy. If you return nil, then you are saying that the operation was successful and the higher layers will do nothing.

Can we detect if the object is in Glacier with something like if o.fs.opt.StorageClass == "GLACIER", and does this account for Glacier Deep Archive tier?

I guess that will be enough since opt.StorageClass indicates the desired storage class.

[tekkiwi]

Honestly no idea if I've done the pull/merge request correctly since I'm new to both Go and Git, but I did make a proposed code fix and tested against the original issue.

Fixes 3224

Let me know if I've missed something that needs to be done before this can be considered for a merge.
Thanks!

[tekkiwi]

Not sure how to resolve this issue:
File is not goimports -ed
return fs.ErrorCantSetModTime

[ncw (Nick Craig-Wood)]

Run go fmt on the file.

[tekkiwi]

Sorry, not following. I ended up with no changes to the file after running the formatting tool. Do you think the error is due to invalid formatting?
Thanks,
Philip

[ncw (Nick Craig-Wood)]

Here is what happens when I try it. It looks like spaces vs tabs or something like that.

$ go fmt backend/s3/s3.go 
backend/s3/s3.go
$ git diff
diff --git a/backend/s3/s3.go b/backend/s3/s3.go
index 953b7abea..65525ec13 100644
--- a/backend/s3/s3.go
+++ b/backend/s3/s3.go
@@ -1737,8 +1737,8 @@ func (o *Object) SetModTime(modTime time.Time) error {
                req.SSEKMSKeyId = &o.fs.opt.SSEKMSKeyID
        }
        if o.fs.opt.StorageClass == "GLACIER" || o.fs.opt.StorageClass == "DEEP_ARCHIVE" {
-               return fs.ErrorCantSetModTime 
-       }    
+               return fs.ErrorCantSetModTime
+       }
        if o.fs.opt.StorageClass != "" {
                req.StorageClass = &o.fs.opt.StorageClass
        }

[tekkiwi]

backend/s3/s3.go

Well, not sure why but there was a stray space, removing that fixed the issue.
Anything else I need to do to get this accepted for a merge?

Thanks,
Philip

[ncw (Nick Craig-Wood)]

I replied on the pull request

[tekkiwi]

I added to the documentation, anything else I should do to get this accepted?
Thanks,
Philip