Degrade CBT bitmap failures to allocated blocks instead of whole-device - #10306
Open
kaovilai wants to merge 6 commits into
Open
Degrade CBT bitmap failures to allocated blocks instead of whole-device#10306kaovilai wants to merge 6 commits into
kaovilai wants to merge 6 commits into
Conversation
getParentBackupInfo's parent-selection log lines interpolated the parentSnapshot parameter. On the discovery branch (no explicit parent passed by the caller) that parameter is empty by definition, so every message about which parent was chosen, or why a run fell back to full, printed no identifier at all -- e.g. "Using parent snapshot , start time ...". This is the normal path for scheduled/incremental backups, so the omission hit the common case, not an edge one. Bind a parentID local that starts as the parameter but is overwritten once a parent is actually resolved (explicit or discovered), and log that instead. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
SetBitmapOrFull had two tiers and its error tier was the worst possible
outcome: any failure of GetChangedBlocks marked every block dirty, so a
failed incremental transferred more than an explicit full backup of the
same volume would have. Measured on a 3 GiB Ceph RBD volume with an
8 MiB delta, an unreachable CBT service moved the entire device -- a
384x amplification -- while reporting no error above the node agent.
Walk a ladder of changed -> allocated -> full instead, returning a Tier
so the caller can tell the three apart and log them distinctly. The
tiers are additive and need no new Bitmap method: the interface has Set
and SetFull but no Clear, and a partial delta unioned with the allocated
set is still a safe superset.
Parent handling becomes per tier, and the distinction is a correctness
one rather than an optimisation:
TierChanged keep the parent (normal incremental).
TierFull keep the parent. Every block is dirty so every block is
written and nothing can be inherited; the parent is pure
dedup benefit. The old code cleared it here, costing the
run its dedup base on top of forcing a whole-device read.
TierAllocated drop the parent. The uploader writes only bitmap blocks
and lets every other offset resolve to the parent object,
so a block written in the parent and discarded since
would be inherited back instead of reading as a hole.
Clearing the parent selects full mode, where unwritten
ranges are holes.
Measured after the change, same scenario: 273,678,336 B moved on the
3 GiB volume instead of 3,221,225,472 B, which is byte-for-byte what an
explicit --backup-type Full transfers for the same allocated set.
Tests: set_test.go gains degraded-to-allocated (asserting SetFull is NOT
called) and both-tiers-fail; snapshot_test.go gains parent-retained-on-
TierFull and parent-dropped-on-TierAllocated.
Fixes velero-io#10295
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
✅ Deploy Preview for velero canceled.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
3 tasks
golangci-lint flagged two issues on this diff: - misspell: "behaviour" -> "behavior" in a comment. - unparam: snapshotSource's description parameter always receives "Block Uploader" from its one call site. Drop the parameter and hardcode the value inside the function instead. No behavior change. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Lyndon-Li
reviewed
Aug 18, 2026
|
|
||
| func getParentBackupInfo(ctx context.Context, rep udmrepo.BackupRepo, forceFull bool, parentSnapshot string, volumeID string, realSource string, snapshotTags map[string]string, log logrus.FieldLogger) parentBackupInfo { | ||
| var previous *udmrepo.Snapshot | ||
|
|
Contributor
There was a problem hiding this comment.
The changes in this function is duplicated with another PR, let's just remove them in the current PR, as it is for another topic.
Lyndon-Li
reviewed
Aug 18, 2026
| } | ||
|
|
||
| snap.Description = description | ||
| snap.Description = "Block Uploader" |
Contributor
There was a problem hiding this comment.
I would prefer to have the description passed by the caller, which makes the responsibility for this function clear.
Lyndon-Li
reviewed
Aug 18, 2026
|
|
||
| err := cbt.SetBitmapOrFull(ctx, cbtService, bitmap) | ||
| if err != nil { | ||
| tier, err := cbt.SetBitmapOrFull(ctx, cbtService, bitmap) |
Contributor
There was a problem hiding this comment.
It we need so complex logics just for printing the correct logs, probably, the right choice is to compose the errors inside SetBitmapOrFull which takes the essential of the full (allocated vs. real full). Outside here, we just say fallback to full with the error.
Outside is just driven by CBT bitmap, it doesn't care about the type of full/incremental/real full.
Per Lyndon-Li's review on velero-io#10306: - Restore snapshotSource's description parameter and pass "Block Uploader" from the caller, rather than hardcoding it inside the function -- keeps the caller responsible for what it's naming the snapshot, unrelated to this PR's tier-ladder change. - Collapse the per-tier warning into a single message when SetBitmapOrFull returns an error. SetBitmapOrFull's own error text already distinguishes an allocated-blocks degradation from a real whole-device fallback; snapshotSource doesn't need to re-derive and print that distinction itself. The tier is still used to decide whether to drop the parent object -- only the logging collapsed. The parentID/getParentBackupInfo diff noise against main is velero-io#10305's commit (this branch is stacked on it, not yet merged) -- left untouched; it resolves on its own once velero-io#10306 rebases past velero-io#10305. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Lyndon-Li's review on velero-io#10306 asked for the description parameter to be restored as part of the function's caller-facing contract, but it's currently only ever called with "Block Uploader" -- which unparam flags as always-constant. Suppress with an explanation rather than removing the parameter again. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.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
2 participants
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.
Note
Responses generated with Claude
Stacked on #10305 (same file,
pkg/uploader/block/snapshot.go— GitHub will show #10305's commit until that one merges; this PR's own diff is only the second commit). Rebase onto main once #10305 lands.Does your change fix a particular issue?
Fixes #10295
Summary
SetBitmapOrFullhad two tiers, and the error tier was the worst possible outcome: any failure ofGetChangedBlocksmarked every block dirty via the deferredSetFull(), so a failed incremental transferred more data than an explicit full backup of the same volume would have — even though a separate, still-functioningGetAllocatedBlockscall could have supplied a far cheaper degraded result.Measured on a 3 GiB Ceph RBD volume with an 8 MiB delta: an unreachable CBT service moved the entire device — a 384x amplification — while reporting no error above the node-agent logs.
Fix
Walk a ladder of changed → allocated → full instead of changed → full, returning a
Tierso the caller can distinguish and log the three outcomes distinctly. No newBitmapmethod needed — the interface hasSetandSetFullbut noClear, and a partial delta unioned with the allocated set remains a safe superset.Parent handling becomes per-tier, and this distinction is a correctness one, not an optimization:
TierChanged— keep the parent (normal incremental).TierFull— keep the parent. Every block is dirty, so every block is written and nothing can be inherited; the parent is pure dedup benefit. The old code cleared it here, costing the run its dedup base on top of forcing a whole-device read.TierAllocated— drop the parent. The uploader writes only bitmap blocks and lets every other offset resolve to the parent object, so a block written in the parent and discarded since would be inherited back instead of reading as a hole. Clearing the parent selects full mode, where unwritten ranges are holes.Measured before/after
Same 3 GiB volume, same 8 MiB delta, CBT service made unreachable to force the error path:
273,678,336 B is byte-for-byte what an explicit
--backup-type Fulltransfers for the same allocated set on this volume — confirming the degraded path now costs exactly what a full costs, not the whole device.What was deliberately not changed
TotalBytessemantics are untouched — this PR only changes which blocks get marked in the bitmap and whether the parent is kept, not how transfer size is reported downstream.Live validation
Developed and validated live on a real Ceph/ODF cluster as part of a combined branch carrying all five fixes from this campaign together — full diff: main...kaovilai:velero:ceph-changeid
The 3,221,225,472 → 273,678,336 numbers above are from that live run: CBT service made unreachable via the SnapshotMetadataService, incremental backup taken, DataUpload
.status.progress.totalBytes/actual transferred bytes compared against an explicit--backup-type Fullon the same volume state to confirm the degraded tier costs exactly what a full costs. Restore from the degraded-tier backup was also verified byte-for-byte against source.Tests
set_test.go: new cases for "changed blocks error degrades to allocated blocks" (asserts the resulting tier and thatSetFullis not called) and "changed and allocated blocks both fail" (both queries fail →TierFull, combined error).snapshot_test.go:TestSnapshotSourceKeepsParentOnCBTFailure(both tiers fail → parent retained) andTestSnapshotSourceDropsParentOnAllocatedTier(delta fails, allocated succeeds → parent dropped).All new and existing tests in
pkg/uploader/cbtandpkg/uploader/blockpass locally (go test ./pkg/uploader/cbt/... ./pkg/uploader/block/...).Please indicate you have done the following:
make new-changelog) — will run once this PR is open (its filename derives fromgh pr view).site/content/docs/main— n/a, no user-facing CLI/API surface changes.