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

Package upload more atomic #10719

Merged
merged 2 commits into from
Apr 11, 2022
Merged

Package upload more atomic #10719

merged 2 commits into from
Apr 11, 2022

Conversation

Firobe
Copy link
Contributor

@Firobe Firobe commented Apr 7, 2022

Explain your changes
This PR uses a modified version of deb-s3 to achieve the following behavior (see the actual changes in deb-s3 here):

During package uploads, packages are now uploaded as temporary filenames
and then moved to their final names in bulk.

This prevents:

  1. corrupted files with visible paths if the transfer is interrupted
  2. a user downloading a corrupted file while it is being transferred

Ultimately, this improves (but cannot completely fix) the possibility of
race conditions between deb-s3 and other CI jobs downloading from the
repository.

An example of such a race is https://buildkite.com/o-1-labs-2/mina/builds/21806#19105d06-9d77-4dce-94de-69bb819d17db/116-197

Manifests are uploaded as before, since they have to be uploaded as soon
as possible, before the lock is released.

Explain how you tested your changes

  • Tested locally on a test bucket
  • Tested with the CI of this PR

@Firobe Firobe added the ci-build-me Add this label to trigger a circle+buildkite build for this branch label Apr 7, 2022
During package uploads, packages are now uploaded as temporary filenames
and then moved to their final names in bulk.

This prevents:
1. corrupted files with visible paths if the transfer is interrupted
2. a user downloading a corrupted file while it is being transferred

Ultimately, this improves (but cannot completely fix) the possibility of
race conditions between deb-s3 and other jobs downloading from the
repository.

Manifests are uploaded as before, since they have to be uploaded as soon
as possible, before the lock is released.
@Firobe Firobe changed the title [WIP] Package upload more atomic Package upload more atomic Apr 8, 2022
@Firobe Firobe marked this pull request as ready for review April 8, 2022 10:32
@Firobe Firobe requested a review from a team as a code owner April 8, 2022 10:32
@Firobe Firobe requested a review from lk86 April 8, 2022 10:32
@Firobe Firobe self-assigned this Apr 11, 2022
@yorickvP yorickvP added the Tweag label Apr 11, 2022
Copy link
Contributor

@lk86 lk86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes sound good to me in theory, but can you make this same update to the toolchain container in compatible so that both major branches see the benefits? You will need this PR + a new one for compatible otherwise you will hit "merge conflicts with develop"

Copy link
Member

@mrmr1993 mrmr1993 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deb-s3 changes lgtm

@Firobe Firobe merged commit cab2595 into develop Apr 11, 2022
@Firobe Firobe deleted the deb-s3-atomic-upload branch April 11, 2022 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants