[snapshot] Resume a broken chunk stream instead of losing the whole volume - #465
Open
kkozoriz wants to merge 13 commits into
Open
[snapshot] Resume a broken chunk stream instead of losing the whole volume#465kkozoriz wants to merge 13 commits into
kkozoriz wants to merge 13 commits into
Conversation
… client Add ValidateHTTPSURL (transport) and NewSafeClientForConfig (safe client) as the foundational pieces needed to build an HTTPS client against a published (ingress) DataImport endpoint. Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Add spec.publish to the DataImport built by d8 snapshot upload, align it alongside spec.ttl on reuse, wait on status.publicURL when publish is enabled, and switch to a merged TLS trust pool with 401/403 diagnostics on that path. Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Wire the flag through to the DataImport importer, auto-detecting the upload mode when unset, and document the bearer-token requirement for the published path. Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
SetTLSCAData merged a trust pool but never reset the caller's inherited insecure-skip-tls-verify/tls-server-name, so Go skipped certificate verification entirely on that path — any endpoint could receive the real Kubernetes bearer token. Force verification on unconditionally, in both the rest.Config and the cloned transport, since client-go may already have baked the insecure flag into a base transport before this wrapper runs. Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Add spec.publish to the DataExport built by d8 snapshot download, upgrade it (one-way, optimistic-locked) on an adopted CR, wait on status.publicURL when publish is enabled, and switch to a merged TLS trust pool with 401/403 diagnostics on that path. Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…es in one place Introduce exporter.IsTransientDataPlaneError as the single place that decides whether a chunk/file transport error is worth retrying. It fails closed: anything not on the allow-list (io.ErrUnexpectedEOF, io.EOF, ErrDataPlaneIdle, ECONNRESET/ECONNABORTED/EPIPE/ETIMEDOUT, and a net.Error reporting Timeout()) is treated as fatal, so a misclassification costs a loud failure rather than a silent retry loop masking a real defect. Cancellation is checked before the net.Error timeout branch: context.DeadlineExceeded itself satisfies net.Error with Timeout() == true, so checking timeouts first would misclassify an intentional cancellation/deadline as retryable. ErrExportUnauthorized and ErrContentRangeMismatch are checked next and are never transient: they describe a request the server actively rejected or a response that cannot be trusted, not a broken transport worth re-issuing. syscall.ECONNREFUSED is deliberately excluded: it means the export never accepted the connection at all, not an abrupted mid-stream transport. HTTP 5xx statuses are excluded too: RangeGet turns a non-206 status into an ordinary status error, and none has ever been observed in production ingress logs. Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
A single transient transport failure anywhere inside a chunk's Range GET used to fail the whole block/file volume, even though an exact byte-durable resume checkpoint (fsync'd .part + offset sidecar) already existed on disk. Reuse that same mechanism as the retry unit: on a transient error, re-issue the Range GET from the durable offset the interrupted attempt persisted, with bounded exponential backoff, instead of surfacing the failure to the caller immediately. chunkRetrier wraps fetchChunkRaw (unchanged: it stays a single attempt, preserving the resume contract several existing tests pin down) with a wait.ExponentialBackoffWithContext loop. Cancellation is checked before error classification so an aborted request never gets mistaken for a retryable transport error. A no-progress guard stops attempts that keep advancing zero bytes well before the backoff budget would otherwise be spent on a link that is never going to deliver. chunkProgressLedger de-duplicates onProgress credits across attempts: each fetchChunkRaw attempt re-credits its own resume prefix, so without the ledger a retried chunk would over-report progress past the volume's true size. downloadBlockChunks creates one chunkRetrier per volume, shared by every chunk goroutine, and logs a single aggregate WARN if any retries were absorbed. stageChunkedFile downloads every non-empty file through the same downloadBlockChunks path, so the filesystem volume path gets this fix for free. Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…ky links --per-volume-concurrency default stays 4; the help text now hints that lowering it to 1 helps on a distant/flaky link where long-lived chunk streams keep breaking, now that a broken stream retries in place rather than failing the whole volume. Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
TestDownloadBlockChunks_RetryIsPerChunk only ever flakes one chunk out of several clean ones, which never actually exercises concurrent increments to chunkRetrier.recovered or concurrent chunkProgressLedgers feeding the shared onProgress sink at the same time. Add two internal tests that drive several chunks through fetchChunk truly concurrently (real goroutines, one shared retrier and onProgress sink): - TestChunkRetrier_ConcurrentChunksIndependentRecoveredCount flakes every chunk exactly once at the same time and asserts recovered lands on the exact expected count, part-file contents are not cross-chunk corrupted, and the shared progress sink sums to exactly the total raw bytes with no loss or double-count. - TestChunkRetrier_ConcurrentContextCancelStopsAllRetries cancels a context shared by several chunks that are all mid-backoff at once and asserts every goroutine stops promptly instead of riding out its sleep. Both pass under -race -count=5. Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…unk retry logging wait.Backoff.Cap forces chunkRetrier.fetchChunk's retry loop to stop one or more attempts short of the policy's declared Steps budget, but the code compared against Steps in two places: the WARN guard (which never actually suppressed the last attempt's log line once Cap cut the loop short) and the exhausted-budget error message (which always reported the declared budget, never the real attempt count). Remove the guard entirely — the closure has no way to observe Cap's early cutoff since wait.ExponentialBackoffWithContext mutates its own copy of backoff — and report the real attempt count in the error message instead. Also unwrap the %w chain before taking %T in the non-retryable-error diagnostic, since fetchChunkRaw always wraps its errors and the previous code only ever logged *fmt.wrapError. Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…t, not wall clock
TestChunkRetrier_ConcurrentContextCancelStopsAllRetries and
TestChunkRetrier_ContextCancelStopsRetryImmediately raced a
time.AfterFunc-scheduled cancel() against goroutines that had not
necessarily issued their first HTTP request yet, so the retry loop's very
first backoff iteration could already observe a cancelled ctx and make zero
requests instead of the expected one per chunk (~60% flake rate on the
concurrent test under -race -count=5). Both tests now signal over a channel
once each doer has actually seen its first request, and cancel only after
every goroutine has reached that point; elapsed-time measurement starts
right before cancel() so it actually reflects time-to-stop.
Also add table-driven coverage for rootCause (nil, unwrapped, single- and
double-wrapped errors), note its known limitation on multi-wrap
(Unwrap() []error) chains, and reword the per-attempt WARN log to state a
plain fact ("chunk transfer interrupted by...") instead of "retrying",
since it fires on the terminal attempt too, where no retry follows.
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
kkozoriz
changed the title
kkozoriz
marked this pull request as ready for review
August 31, 2026 18:11
kkozoriz
added a commit
that referenced
this pull request
Aug 31, 2026
…463 The team merges with "Squash and merge", which drops the #462 -> #463 -> #464 ancestry. Git then falls back to origin/main as the merge base and sees #462's changes -- already present here -- as competing additions, producing two conflicts that a plain merge chain never hits: CONFLICT (content): internal/snapshot/transport/http.go CONFLICT (add/add): pkg/libsaferequest/client/http_test.go Both were purely structural, so reshape them instead of changing behaviour: - http_test.go is created by #462 too, so an add/add conflicts unless both sides match byte for byte. Restore it to #462's exact content and move the 7 SetTLSCAData tests and their 3 helpers to a new http_tls_test.go, which exists on neither side of main and merges as a clean single-side add. - The "keep in sync" note sat at the end of a doc comment block #462 had just added, so the two additions were adjacent and conflicted. Move it just inside SetTLSCAData, a region #462 does not touch. No production code changes: pkg/libsaferequest/client/http.go is untouched and internal/snapshot/transport/http.go now matches #463 apart from the relocated comment. All 8 tests are preserved and still pass under -race. Verified from origin/main: squash #462, squash #463, merge #464, merge #465 -- all four clean, then build, vet and go test -race clean over ./internal/snapshot/... and ./pkg/libsaferequest/... Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A single transient transport failure inside a chunk's Range GET failed the whole block or filesystem volume, even though a byte-exact durable resume checkpoint (fsync'd
.partplus offset sidecar) already existed on disk. On the publish path a flaky link therefore threw away an in-progress multi-gigabyte volume transfer.Stacked on #463.
Fix
exporter.IsTransientDataPlaneError- one allow-list deciding what is worth retrying (unexpected EOF,EOF, idle data plane,ECONNRESET/ECONNABORTED/EPIPE/ETIMEDOUT,net.Errortimeouts). It fails closed; cancellation is classified first, so a deadline is never mistaken for a retryable error.chunkRetrierre-issues the Range GET from the durable offset the interrupted attempt persisted, with bounded exponential backoff and a no-progress guard.fetchChunkRawstays a single attempt, so the existing resume contract is untouched.chunkProgressLedgerde-duplicates progress credits across attempts, so a retried chunk cannot over-report past the volume's real size.Before / After
Probe: an 8 MiB block volume in 1 MiB chunks over 2 workers, against a server that cuts off the first Range GET of every chunk mid-body (
unexpected EOF).BEFORE (
f9063c791)AFTER (
aa3be4307)(Source payload sha256 is
a591762ba183f90b...in both runs.)Validation
Beyond the probe: during a real
download --publish=trueof a 4-volume snapshot from a live cluster, 8 genuine transient interruptions (unexpected EOF) hit the two block volumes' chunked transfer, 4 per volume. Every one recovered on the first retry (attempt=1), the per-volume summary loggedretries=4, and the final archive's sha256 matched two independently obtained copies of the same data. Progress reporting across ~458 log lines showed no double-counted bytes.Tests
internal/snapshot/volume/chunk_retry_internal_test.go- retry, backoff, no-progress guard, concurrent retry, context cancellation, attempt-count logginginternal/snapshot/volume/block_test.go- retry is per chunk and resumes from the durable offsetinternal/snapshot/exporter/retry_test.go- transient classification allow-list, including what must stay fatal