disk_manager: batch snapshot transfer and retry only failed chunks#6403
disk_manager: batch snapshot transfer and retry only failed chunks#6403BarkovBG wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d9fa28a63
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Retriable: leave the chunk unwritten, keep going. | ||
| return false |
There was a problem hiding this comment.
When batched transfer is enabled, any Source.Read or Target.Write error that is not wrapped as NonRetriableError falls through here and is treated as a retryable chunk failure. In this task framework only RetriableError is retryable; a plain/permanent error from a source or target would have cancelled the streaming transfer, but this path masks it, saves partial batch state, and reschedules until the retry limit is exhausted. Please distinguish RetriableError explicitly and return other errors immediately.
Useful? React with 👍 / 👎.
| return errors.NewRetriableErrorf( | ||
| "batch at chunk %v is not fully written", | ||
| batchStart, | ||
| ) |
There was a problem hiding this comment.
This synthetic RetriableError is used as normal control flow after saving a partially written batch, but the task runner increments RetriableErrorCount for every such return and enforces the configured retry cap (default 100). With BatchSize enabled on a large degraded-S3 snapshot, more than 100 batches that each need an extra pass can fail the whole task even though the bitmap is making forward progress; use an ignore-limit retry or reset the count when persisted progress advances.
Useful? React with 👍 / 👎.
| return false | ||
| } | ||
|
|
||
| workerCount := t.WriterCount |
There was a problem hiding this comment.
In batched mode each worker performs both source.Read and target.Write, but the worker count comes from WriterCount and ReaderCount is otherwise unused. When operators configure WriterCount higher than ReaderCount to allow many snapshot writes while throttling NBS reads, enabling BatchSize will issue WriterCount concurrent reads instead of the configured read limit used by the streaming path, which can overload the source.
Useful? React with 👍 / 👎.
| // Reuse the persisted bits only if they match the current batch size, | ||
| // otherwise start clean and redo the whole batch (writes are idempotent). | ||
| if len(data) == size { |
There was a problem hiding this comment.
When a task resumes a partially written batch after BatchSize changes so the saved bitmap length no longer matches, this path deliberately drops the bitmap and redoes the whole batch. The saved TransferredChunkCount still includes the chunks that were already marked successful, so runBatchPass increments them again and the final transferred byte count can be overstated; either preserve/translate the saved successes or reset the count consistently when discarding the bitmap.
Useful? React with 👍 / 👎.
When S3 is degraded (serves only a fraction of requests), createSnapshotFromDisk could stall. A single retriable write error cancels the whole Transferer worker pool, the task restarts from the coarse contiguous-prefix Milestone, and under a high failure rate that prefix barely advances, so the transfer makes almost no progress. The cascade of context.Canceled errors from the cancelled pool also inflates the s3_client "errors" metric and pages the on-call (manager_dataplane_s3_client_errors). Add a batched transfer mode to Transferer, gated behind the new BatchSize config (0 keeps the current streaming behavior): - chunks are processed in fixed windows of BatchSize chunk indices; - within a window every not-yet-written chunk is written once and successes are recorded in a bitmap; - a non-retriable error still fails fast; if some chunks remain unwritten (for example S3 is degraded), the bitmap is persisted and a retriable error is returned, so the task reschedules and redoes only the unwritten chunks; - progress is persisted as Milestone.ChunkIndex (batch start) plus the new CurrentBatchBitmap field in CreateSnapshotFromDiskTaskState. Already-written chunks are never redone, so the transfer makes monotonic progress even against a heavily degraded target, and without pool-wide cancellation there is no context.Canceled amplification. For now batched mode is used by createSnapshotFromDisk for full (non-incremental) snapshots only (ShallowSource == nil); the shallow-copy path is unchanged.
BarkovBG
force-pushed
the
users/barkovbg/dataplane-batch-transfer
branch
from
0d9fa28 to
4b8d7c3
Compare
1 participant
Problem
When S3 is degraded and serves only a fraction of requests,
createSnapshotFromDiskcan stall:Transfererworker pool (fail-fast).Milestone, which is a contiguous-prefix watermark, so one failed chunk (a "hole") stops it from advancing.context.Canceledfrom the cancelled pool also inflates thes3_clienterrorsmetric, which pages the on-call (manager_dataplane_s3_client_errors).Change
A batched transfer mode in
Transferer, gated behind a newBatchSizeconfig option (0= current streaming behavior, unchanged):BatchSizechunk indices.Milestone.ChunkIndex(batch start) plus a newCurrentBatchBitmapfield inCreateSnapshotFromDiskTaskState.Already-written chunks are never redone, so the transfer makes monotonic progress even against a heavily degraded target, and without pool-wide cancellation there is no
context.Canceledamplification. The "retry" is the existing task reschedule mechanism (with its own backoff and limits) — there is no bespoke in-process retry loop or attempt budget.Scope / rollout
BatchSize = 0).TransfererandMilestoneare shared by all dataplane tasks; onlycreateSnapshotFromDiskreads the new config, and only for full (non-incremental) snapshots (ShallowSource == nil). The shallow-copy / incremental path is unchanged for now.*.pb.gois not committed (generated at build time).Testing
TestTransferBatched— batched transfer completes and transfers all chunks.TestTransferBatchedResumesOnlyUnwrittenChunks— when some writes fail, the batch is left incomplete, the bitmap is persisted, and a resumed transfer rewrites only the previously-failed chunks.TestCreateSnapshotFromDiskTransferMilestoneRoundTrip— the task persists the milestone (includingCurrentBatchBitmap) into its state and restores it unchanged for a resumed transfer.ya make --build relwithdebinfo -tA cloud/disk_manager/internal/pkg/dataplane/common— all tests pass.ya make --build relwithdebinfo -t cloud/disk_manager/internal/pkg/dataplane -F "*TransferMilestoneRoundTrip*"— task test passes.ya make --build relwithdebinfo cloud/disk_manager— full build passes (allTransferer/Milestoneconsumers plus regenerated proto/config).Integration testing against a real degraded S3 is recommended before enabling
BatchSizein production.