[oadp-1.6] fix: two DataUpload livelocks in VMB handling (stuck retry guard + stale cached status) - #216
Merged
openshift-merge-bot[bot] merged 3 commits intoSep 1, 2026
Conversation
Fixes migtools#211. handleAccepted had a guard: whenever AnnotationVMBTName was already set but findVMBForDataUpload found no VMB, it assumed a VMB from a prior reconcile just hadn't reached the informer cache yet, and requeued briefly to "let the cache catch up". That assumption no longer holds: 1. findVMBForDataUpload already falls back to an uncached APIReader read when the cached client finds nothing -- a genuine cache-lag false negative is resolved inside that call, before this guard. Reaching here with vmb == nil means no VMB exists even by a live read, not merely an invisible one. 2. prepareVMBackupTracker (Step 2, skipped by this guard) no longer deletes an existing VMBT -- it reuses one by VM-name-hash label if found, only creating fresh when none exists. The guard's own rationale ("prepareVMBackupTracker would delete the VMBT that the (not-yet-visible) VMB references") describes behavior the function no longer has. The real failure mode (reproduced live): Step 4 can legitimately defer VMB creation (KubeVirt's one-active-VMB-per-VM admission webhook, "in progress for source", already handled via RequeueAfter: RequeueAfterLong) after Step 2 has already persisted AnnotationVMBTName. No VMB gets created that reconcile. Every following reconcile then hit this guard and took the short-requeue-and-wait path forever, since Step 4 -- the only code path that can ever create the VMB -- was never reached again. The DataUpload looped "VMBT already prepared but VMB not yet visible in cache, requeuing" every 5s indefinitely (bounded only by Spec.OperationTimeout, e.g. 4h). With only one active DataUpload allowed per VM (hasOlderActiveDUForVM), a single stuck DataUpload also blocked every later backup attempt for that VM, unrelated to whatever originally got stuck. Fix: remove the guard and let handleAccepted fall through to the normal Steps 2-4 on every reconcile where no VMB is found. Both are already idempotent (Step 2 reuses-or-creates by label; Step 4 itself re-checks findVMBForDataUpload before creating), so retrying is safe and lets the deferred-creation case resolve once the conflicting VMB clears. TestHandleAccepted_RetriesVMBCreationAfterAnnotationPersisted reproduces the exact scenario via two handleAccepted calls: first hits a simulated admission-webhook conflict and defers (RequeueAfterLong, no VMB created, VMBT preserved); second, once the conflict clears, retries and successfully creates the VMB -- confirmed failing against the old guard (which never re-attempts Step 4 at all) before this fix. Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
Reported live from a nightly-KubeVirt e2e run: a second, independent livelock from the same-shaped symptom as migtools#211/migtools#212, but a different root cause. virt-controller had already written status.conditions[type=Done,status=True] to the API server -- the backup genuinely finished -- but this controller's cached copy of the same VirtualMachineBackup never reflected it. The reconciler found the VMB fine (logged "already exists, skipping VMBT preparation"), just with a stale Status, and looped "VirtualMachineBackup in progress, requeuing" until Spec.OperationTimeout even though nothing would ever change on its own. Confirmed intermittent by live reproduction (a missed/delayed informer watch event, not a deterministic logic bug) -- distinct from migtools#211/migtools#212, which is about the VMB not existing in the cache at all. Fix: right before deciding whether the VMB is done, refresh its Status via r.APIReader (an uncached, direct read) rather than trusting whatever the informer cache last delivered. A single-object Get is cheap enough to do on every reconcile of this phase. Best-effort: if the read fails, or r.APIReader isn't configured (as in some existing test fixtures), falls back to the cached copy already in hand. TestHandleAccepted_RefreshesVMBStatusFromLiveRead reproduces the exact scenario: two separate fake clients (r.Client vs r.APIReader) seeded with different Status for the same VMB -- one still Progressing, one Done=True -- confirmed failing without this fix (stuck on Accepted), passing with it (transitions to Prepared using the live status). Folded into this PR rather than opened separately, per reviewer request, since it touches the same handleAccepted VMB-status-handling code migtools#211/migtools#212 already do. Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
Requested cross-session while investigating a live repro of migtools#212's fix that still showed the stuck symptom despite the refresh logic verified correct end-to-end (image freshness, wiring, code placement all confirmed). The refresh block itself was silent on both branches, leaving no way to tell apart: (a) the uncached read genuinely got Done=False/absent -- meaning virt-controller itself hadn't written Done:True yet this run (a KubeVirt-side timing issue, not this controller's bug), vs. (b) the uncached read itself is somehow not seeing the live state (e.g. an unexpected caching layer between APIReader and the API server). Logs the live conditions on a successful refresh, and the NotFound / error cases separately, so the next occurrence (live or in a targeted repro) is self-diagnosing without needing to add logging and rebuild again. Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
5 tasks
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kaovilai
approved these changes
Aug 31, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, openshift-cherrypick-robot, shubham-pampattiwar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
3 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.
This is an automated cherry-pick of #212
/assign kaovilai