BE-817: atlas: push fit generations to S3 - #9681
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Dependency ReviewThe following issues were found:
VulnerabilitiesCargo.lock
OpenSSF Scorecard
Scanned Files
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Merging this PR will not alter performance
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
as_constant |
< 1 ns | < 1 ns | N/A | |
constant_equal |
< 1 ns | < 1 ns | N/A | |
constant_not_equal |
< 1 ns | < 1 ns | N/A | |
access |
< 1 ns | < 1 ns | N/A | |
runtime_equal |
< 1 ns | < 1 ns | N/A | |
runtime_not_equal |
< 1 ns | < 1 ns | N/A |
Comparing bm/be-817-atlas-push-fit-generations-to-s3 (eef86dc) with bm/be-815-atlas-serve-more-than-one-generation-at-a-time (f455d38)1
Footnotes
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## bm/be-815-atlas-serve-more-than-one-generation-at-a-time #9681 +/- ##
============================================================================================
- Coverage 66.76% 66.66% -0.11%
============================================================================================
Files 1821 1838 +17
Lines 192552 193639 +1087
Branches 7879 7919 +40
============================================================================================
+ Hits 128553 129083 +530
- Misses 62489 63041 +552
- Partials 1510 1515 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cbb76f2 to
5cd676b
Compare
5cd676b to
c8e1087
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b32a808. Configure here.
`tokio::io::copy` already flushes the destination writer, so the manual flush call is redundant. Update the test assertion to verify flushing occurred without asserting an exact count.
- Expose Storage and FilePath types; restructure initialization - Add CloneToUninit implementations for S3Path, Legend, Label, and DenseBitSlice with detailed safety rationale - Simplify StorageError by removing ByteStreamError variant - Update file path tests to verify async reading with actual I/O
- Rename `scratch.rs`, `error.rs`, `metadata.rs`, and `parts.rs` to `mod.rs` within their respective directories - Add explicit `drop` calls in storage path and multipart tests to suppress unused variable warnings - Update multipart backend trait methods to return `impl Future` instead of using `async fn` - Fix ETag documentation formatting in parts module
- Consolidate sync/async task failure handling in upload and storage - Extract `FileContents` to its own module - Simplify local storage tests to async, remove foreign revision test - Improve code comments for clarity - Fix `BucketPath::append` to preserve separator position
- Remove unnecessary clippy expect attribute - Normalize GitHub issue reference format - Simplify nested conditional in test helper
- Rewrote fit command and method documentation to be more concise - Extracted storage initialization into `fit_storage` helper - Made `From<uuid::Uuid>` impls const for `ArchivedEntityUuid` and `ArchivedWebId` - Added `Sha256Digest::BYTES` constant and improved related docs
- Move `ScratchStorage` logic into `ScratchDirectory` - Store `Storage` with owned `ScratchDirectory` instead of tuple - Add clippy exceptions for false positives on drop analysis

🌟 What is the purpose of this PR?
Publish a finished fit to S3 so serving hosts can pull it. We must do this because fitting and serving are distinct: fitting runs occasionally, while serving must serve (pun intended) that data. To coordinate that, we use remote storage, and to support future deployments, we keep the backend generic (for now, only S3-compatible storage and local storage are supported) to push and pull generations.
This PR handles the upload half and infrastructure; BE-816 handles the pull (note that S3-based files are already supported via this PR).
The primary challenge in this PR is coordination, especially since S3 doesn't yet support multi-object commits. We therefore upload all files first, then
metadata.jsonto complete the upload. Once done, we copy toactive, then setcurrentand cycleprevious.Directory layout:
For review, the properties we rely on are the following:
currentmoves under the precondition captured atUpload::prepare, we hard error ifcurrenthas been modified in any way, as it indicates either corruption or another process having written content.S3::single_attempt): a retry after a lost response could report a false conflict or start a second multipart upload after losing the first one's identifier.finish_object→verify_destination); this makes sure that no partial uploads pollute the system (or someone else has written to the file in the meantime).🔍 What does this change?
flowchart LR F[fit] -->|seal| G[local generation] G -->|artifacts, then metadata.json| R["repository/<id>/"] R -->|copy after verify, metadata last| A["active/<id>/"] A -->|If-Match on captured ETag| C[current] C -.->|advisory| P[previous]file/storage/:FilePath(a filesystem path or ans3://location),Storage(the optional S3 client and the scratch directory),WriteConditionandRevision, with one operation set over both backends.local/is the lock-and-rename implementation,s3/the SDK one, withmultipart/for objects over the single-request bound andpath/for bucket locations that keep the caller's literal key text.file/generation/upload/:Upload::preparecaptures the current pointer and its revision,uploadcompletes the repository prefix,promotecopies intoactive/and swapscurrent.GenerationUploadBackendis the trait the tests implement with a fault-injecting fake, andStorageimplements it for real.file/generation/document/:GenerationDocumentkeeps the verified original bytes beside the parsedSaltRepository.S3Args(--s3,--s3-region,--s3-endpoint,--s3-force-path-style, an explicit key pair and session token, each with itsHASH_GRAPH_ATLAS_S3_*variable and the rest from the SDK's ownAWS_*chain) and--uploadonfit.FitCommand::newtakes theStorageand resolves remote inputs before the run.previouspointer, as it's advisory.repository/. S3 bucket policies must be used to clean upactivegenerations, tracked in BE-852❓ How to test this?
yarn compose up -d minio, then create a bucket (the compose credentials aredev-s3-access-key-id/dev-s3-secret-access-key).--s3 --s3-endpoint http://localhost:9000 --s3-force-path-style --s3-region us-east-1 --s3-access-key-id dev-s3-access-key-id --s3-secret-access-key dev-s3-secret-access-key --upload s3://<bucket>/atlas.s3://<bucket>/atlas/generations/:repository/<id>/holds the artifacts withmetadata.jsonlast by modification time,active/<id>/exists only if the verdict saysactivated true, andcurrentreads the id.currentmoves to the new id under the ETag the first run left, andpreviousreads the old one.