fix (cache): Migrate cache S3 manager to AWS transfermanager - #4118
fix (cache): Migrate cache S3 manager to AWS transfermanager#4118ss1909 wants to merge 6 commits into
Conversation
ss1909
added
change
ss1909
added
internal
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
The upload migration needs a bounded-memory adjustment before it is safe for the existing transfer settings.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 7312, then answer my questions about the findings.
There was a problem hiding this comment.
The upload-memory blocker is still open; I’ve added the remaining size-driven case to the existing thread.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 7455, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Previous concerns have been addressed.
There was a problem hiding this comment.
The upload-memory concern is addressed, and I found no new issues. I’m leaving this comment-only because this changes the performance-critical S3 cache transfer path and needs a human sanity-check.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 7722, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
ss1909
changed the title
zhming0
left a comment
zhming0
left a comment
There was a problem hiding this comment.
I have left some questions mainly for myself to understand this better 🙏
| // pool. It holds for all normal object sizes; multi-TiB objects bottom out at | ||
| // concurrency 1, where one part buffer (size/uploadMaxParts) is unavoidable | ||
| // and may exceed this. | ||
| uploadMemoryBudget = 256 * 1024 * 1024 |
There was a problem hiding this comment.
Just a question for my understanding, what was this memory budget before this change?
There was a problem hiding this comment.
So there was no explicit "budget" before - manager was inherently bounded as it was streaming rather than buffering (which is waht transferManager does).
There was a problem hiding this comment.
I see what this is trying to achieve now. But I am not sure if hard coding to 256MB is the right path:
- We should at least detect the available RAM.
- But even then, we don't know how many concurrent upload is happening at the same time.
- We don't seem to clearly understand the implication of this knob → e.g. by turning it up and down, are we expecting a speed up/down? It feels to me that we are trying to use this knob to serve as a workaround for a pitfall in AWS transfer manager.
At this point, I think it'd be fair to pause to consider if transfer manager is mature enough to justify this change.
| // resolveTransferSettings turns parsed Options into concrete transfer settings. | ||
| // An explicit concurrency or part_size_mb override applies to both uploads and | ||
| // downloads; otherwise uploads use the SDK's upload defaults and downloads use | ||
| // the download-tuned defaults. | ||
| // The concurrency and part_size_mb URL overrides restore | ||
| // path only; save always uses the fixed upload defaults. | ||
| func resolveTransferSettings(opts *Options) transferSettings { | ||
| uploadConcurrency := manager.DefaultUploadConcurrency | ||
| // Upload settings are fixed and intentionally not derived from the URL | ||
| uploadConcurrency := defaultUploadConcurrency | ||
| uploadPartSize := int64(defaultUploadPartSizeBytes) | ||
| downloadConcurrency := defaultDownloadConcurrency | ||
| downloadPartSize := int64(defaultDownloadPartSizeMB) * 1024 * 1024 | ||
| if opts.Concurrency > 0 { | ||
| uploadConcurrency = opts.Concurrency | ||
| downloadConcurrency = opts.Concurrency | ||
| } | ||
|
|
||
| uploadPartSize := manager.DefaultUploadPartSize | ||
| downloadPartSize := int64(defaultDownloadPartSizeMB) * 1024 * 1024 | ||
| if opts.PartSizeMB > 0 { | ||
| uploadPartSize = int64(opts.PartSizeMB) * 1024 * 1024 |
There was a problem hiding this comment.
Sorry, can you help me understand again (using your words) why we need to set different concurrency and part size for upload and download? I did read the buildsworth thread, that doesn't quite make sense to my brain :g_thinking:.
The thing is, our current url scheme allows a generic currency and part size setting, if we were to make this change, our URL params will become misleading.
There was a problem hiding this comment.
save and restore have differing memory behaviours:
- Save buffers each in-flight part fully in RAM
- Restore streams each range through a small (32 KiB) buffer
Save/Upload buffers (memory grows with part size); Restore/download streams (memory doesn't). That mismatch is why this need for different concurrency/part-size handling arises.
I very much agree with the generic names for concurrency/part_size_mb, are misleading, so I propose we could do one of these:
- Document them as restore/download only tuning
- Rename to download_concurrency / download_part_size_mb
There was a problem hiding this comment.
Save buffers each in-flight part fully in RAM
I can see that the buffer behave a bit differently, but why does the the configuration need to differ I wonder? I read the other thread it's a bit avoiding excessive memory? But does the upload memory budget setting cap the memory?
There was a problem hiding this comment.
the reason why the configs need to be different is because the numbers that make restore fast are exactly the ones that make save allocate big chunks (gbs), so a single shared value can't serve both! it's either too small (slow restore) or too large (save OOM).
Restore streams, so memory is flat regardless of part size/concurrency - the best config is high concurrency + large parts (c32/p32) purely for speed. Save buffers, so memory = (concurrency+1) × partSize - the best config is low concurrency + small parts to stay bounded. More on this here - https://docs.aws.amazon.com/AmazonS3/latest/userguide/qfacts.html and https://github.com/aws/aws-sdk-go-v2/tree/50fd8e78e0780bc66f360bc50d59aa314d164d54/feature/s3/transfermanager
does that make sense?
|
Sorry, my review failed. Check the build for details: https://buildkite.com/buildkite/buildsworth-review-pull-request/builds/8963 |
There was a problem hiding this comment.
The upload-memory fix still misses the transfer manager's separately held first multipart chunk, so the intended bound is exceeded for large caches. I've reopened the existing thread with the concrete allocation bounds.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 8963, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Previous concerns have been addressed.
There was a problem hiding this comment.
The upload-memory concern is addressed, and I found no new issues. I’m leaving this comment-only because this changes the performance-critical S3 cache transfer path and needs a human sanity-check.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 9110, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
I think we should reconsider the upgrade. My quick read (I could be wrong), is that the upstream upload manager is quite flawed and we had to build extra finicky harness to prevent its causing OOM issue.
I am leaning towards not upgrading to transfer manager now (at least for the upload path) after seeing the efforts here. What do you think @ss1909?
| // uploadConcurrencyForSize picks how many part buffers UploadObject may hold in | ||
| // parallel for an object of the given size, so its peak allocation stays within | ||
| // uploadMemoryBudget. That peak is (concurrency+2) × partSize: the eager pool | ||
| // holds concurrency+1 buffers, and UploadObject reads the first chunk into a | ||
| // separate buffer outside the pool. | ||
| // | ||
| // transfermanager raises the part size to size/uploadMaxParts once an object | ||
| // would exceed S3's 10,000-part limit, so for large objects the part buffers | ||
| // grow with the object and we trade away concurrency to compensate. Multi-TiB | ||
| // objects bottom out at concurrency 1, where three part buffers are still | ||
| // unavoidable, so the budget is a ceiling for normal sizes, not a hard cap at | ||
| // every size. | ||
| func uploadConcurrencyForSize(size int64, maxConcurrency int) int { | ||
| partSize := int64(defaultUploadPartSizeBytes) | ||
| if forced := size/uploadMaxParts + 1; forced > partSize { | ||
| partSize = forced | ||
| } | ||
|
|
||
| // Peak = (concurrency+1) pool buffers + 1 separately-held first chunk, so | ||
| // solve (concurrency+2) × partSize <= budget. | ||
| concurrency := int(uploadMemoryBudget/partSize) - 2 | ||
| if concurrency > maxConcurrency { | ||
| concurrency = maxConcurrency | ||
| } | ||
| if concurrency < 1 { | ||
| concurrency = 1 | ||
| } | ||
| return concurrency | ||
| } |
There was a problem hiding this comment.
[blocking] I believe the arithmetic is based on some internal logic of transfer function, this isn't ideal because we don't want to be coupled to something that will change anytime.
|
Yeah I concur. I reckon we could move this to later, maybe after the next milestone even. |
|
yeah, I agree. Let's move it off the current milestone back to backlog. |
2 participants
Description
The agent cache S3 store used the deprecated aws-sdk-go-v2
feature/s3/manageruploader/downloader for cache save and restore. AWS has superseded that package withfeature/s3/transfermanager, which the agent already uses for artifact multipart downloads. This migrates cache save/restore ontotransfermanager, aligning cache transfers with the maintained API and letting us drop themanager-related//nolint:staticcheck(SA1019) suppressions from the cache package.Restore uses explicit range-based downloads (
GetObjectType = GetObjectRanges) instead of the SDK's default part-number fan-out. Part-number fan-out only parallelises objects uploaded as multipart — a singlePutObject(or aCopyObject, e.g. our TTL refresh) collapses to a single stream. Range mode fans out for any object, so restore parallelism is a property of our configured concurrency/part size rather than of how each object happened to be written. Save usestransfermanager.UploadObject.Unlike the old
manager, which streamed parts straight from the file,transfermanager.UploadObjectbuffers each in-flight part in memory, eagerly allocating(concurrency+1) × partSize. To keep this bounded, upload settings are decoupled from the S3 URL (concurrency/part_size_mbnow tune restore only), and upload concurrency is chosen per object from its size so the pool stays under a 256 MiB budget — full parallelism for normal caches, stepping down to 1 for multi-TiB objects. The residual floor is S3's 10,000-part limit: a buffering uploader must hold at least one part ofsize/10_000, which only a streaming uploader could avoid.Context
Linear - https://linear.app/buildkite/issue/A-1470/migrate-cache-s3-transfers-to-aws-transfermanager
Changes
internal/cache/store/s3.go: replacemanager.Uploader/manager.Downloaderwith twotransfermanager.Clients (upload- and download-tuned) for save and restore.transfermanager.DownloadObjectwithGetObjectType = GetObjectRanges.transfermanager.UploadObject.downloadWithRetry); update theobjectDownloaderinterface to theDownloadObjectsignature.PartCountermiddleware with a computed part count (ceil(bytes / partSize)); remove the now-unusedsync/atomicandsmithy-go/middlewareimports.concurrency/part_size_mbnow tune restore only; uploads use fixed defaults (manager.DefaultUploadConcurrency/DefaultUploadPartSizereplaced by local 5 × 5 MiB constants).uploadConcurrencyForSizepicks per-object concurrency so the eager part-buffer pool(concurrency+1) × partSizestays under a 256 MiB budget; pinMaxUploadPartsandMultipartUploadThresholdfor deterministic part sizing.feature/s3/managerimport and all//nolint:staticcheck // SA1019suppressions in the package.TestResolveTransferSettings,TestDownloadWithRetry, and thefakeDownloaderdouble to the new API; addTestNewS3Blob(custom-endpoint + path-style construction) andTestUploadConcurrencyForSize(size→concurrency bounding).Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)Affiliation (optional, external contributors)
Disclosures / Credits