Skip to content
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

fix: sync glacier objects overwrite #734

Merged
merged 23 commits into from
Nov 27, 2024
Merged

Conversation

4o4x
Copy link
Member

@4o4x 4o4x commented Jul 4, 2024

This pull request addresses a bug (#712) in the sync command where objects in Glacier storage at the destination were being overwritten during synchronization. closes #712

Bug Details:

During the sync process, if an object exists in both the source and destination, and the destination object is in Glacier storage, it was incorrectly being overwritten by the source object.

Fixes:

  • Modified the function that checks which objects to compare.

Previously, if either the source or destination object was in Glacier storage, it was skipped. In the new version, only the source object is skipped, but the destination object is not. If the destination object is skipped, it will be considered as only in the source during the comparison phase, leading to it being overwritten by the source object.

  • Added tests for this specific case to ensure correctness.

@4o4x 4o4x requested a review from a team as a code owner July 4, 2024 13:22
@4o4x 4o4x requested review from igungor and sonmezonur and removed request for a team July 4, 2024 13:22
command/sync.go Outdated Show resolved Hide resolved
command/sync_strategy.go Outdated Show resolved Hide resolved
e2e/util_test.go Outdated Show resolved Hide resolved
e2e/sync_test.go Outdated Show resolved Hide resolved
test-dir/myfile.txt Outdated Show resolved Hide resolved
e2e/sync_test.go Outdated Show resolved Hide resolved
command/sync.go Outdated Show resolved Hide resolved
@4o4x
Copy link
Member Author

4o4x commented Jul 8, 2024

I wrote a script to observe how the sync command behaves with Glacier objects in AWS CLI. I used these flags in every possible combination:

  • --delete
  • --ignore-glacier-warnings
  • --force-glacier-transfer
  • --storage-class=STANDARD | GLACIER

The most important result obtained:

If the object in the source bucket is a Glacier object, no matter which flags are used, we cannot do anything to the object.
Here are the test results:

delete.md
sync.md

e2e/sync_test.go Outdated Show resolved Hide resolved
@ilkinulas
Copy link
Member

I wrote a script to observe how the sync command behaves with Glacier objects in AWS CLI. I used these flags in every possible combination:

  • --delete
  • --ignore-glacier-warnings
  • --force-glacier-transfer
  • --storage-class=STANDARD | GLACIER

The most important result obtained:

If the object in the source bucket is a Glacier object, no matter which flags are used, we cannot do anything to the object. Here are the test results:

delete.md sync.md

Thank you for your efforts on this. What happens if the src or dst object's storage class is glacier deep archive? Does GLACIER also covers DEEP ARCHIVE?

e2e/sync_test.go Outdated Show resolved Hide resolved
e2e/sync_test.go Outdated Show resolved Hide resolved
command/sync_strategy_test.go Outdated Show resolved Hide resolved
e2e/run_test.go Outdated Show resolved Hide resolved
@4o4x 4o4x marked this pull request as draft July 29, 2024 09:18
@4o4x 4o4x marked this pull request as ready for review July 31, 2024 07:57
@igungor
Copy link
Member

igungor commented Oct 18, 2024

@4o4x could you take a look at the PR description if it's still correct after the review changes please? thanks.

@4o4x
Copy link
Member Author

4o4x commented Oct 18, 2024

@4o4x could you take a look at the PR description if it's still correct after the review changes please? thanks.

I have removed compare strategy part 👍

@igungor
Copy link
Member

igungor commented Oct 18, 2024

@sonmezonur @ilkinulas PTAL

@4o4x
Copy link
Member Author

4o4x commented Nov 1, 2024

@ilkinulas @sonmezonur PTAL

@4o4x
Copy link
Member Author

4o4x commented Nov 27, 2024

Merge ?

@ilkinulas ilkinulas merged commit d72716a into peak:master Nov 27, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sync command copies a new object everytime when the s3 version is on GLACIER
4 participants