fix: recognize VirtualMachineBackup's renamed Complete condition by kaovilai · Pull Request #208 · migtools/kubevirt-datamover-controller

kaovilai · 2026-08-27T21:23:56Z

Summary

Reported cross-session from a live nightly-KubeVirt (v1.20.0) e2e run: a fourth, distinct issue found while testing oadp-operator#2404, unrelated to CNV-85377 or the #207 annotation race. Fixes #209.

Root cause

evaluateVMBackupStatus/isVMBTerminal only recognize kubevirtbackupv1alpha1.ConditionDone ("Done"), but virt-controller's CBT backup path renamed that condition type to "Complete" starting with nightly v1.20.0. doneCond stays nil forever against that version, so the controller loops "VirtualMachineBackup in progress, requeuing" until Spec.OperationTimeout -- observed live: 243 repeats of that log line, then a 20-minute context deadline exceeded -- even though vmb.Status.Conditions already has Complete=True, reason=CompletedWithWarning. The backup itself completed fine; the controller just never recognized it.

Fix

Our vendored kubevirt.io/api (v1.8.0-alpha.0) predates the rename and has no ConditionComplete constant, so add it locally as the literal string virt-controller actually emits ("Complete"), and recognize it alongside the existing ConditionDone in both places that scan for it (evaluateVMBackupStatus and isVMBTerminal).

Bumping kubevirt.io/api instead was considered and rejected for this fix: v1.8.0-alpha.0 to whatever version introduced ConditionComplete is a large jump with its own unrelated API-surface risk, whereas accepting both condition names is a minimal, backward- and forward-compatible change.

Test plan (TDD)

New table cases in TestHandleAccepted_VMBStatusDetection and TestIsVMBTerminal pin the exact scenario reported (Complete=True, Reason=CompletedWithWarning) transitioning to Prepared/terminal instead of stalling.

go build ./...
go vet ./...
go test ./...
make lint

Note

Responses generated with Claude

Reported cross-session from a live nightly-KubeVirt (v1.20.0) e2e run: evaluateVMBackupStatus/isVMBTerminal only recognize kubevirtbackupv1alpha1.ConditionDone ("Done"), but virt-controller's CBT backup path renamed that condition type to "Complete" starting with nightly v1.20.0. doneCond stays nil forever against that version, so the controller loops "VirtualMachineBackup in progress, requeuing" until Spec.OperationTimeout, even though vmb.Status.Conditions already has Complete=True (observed live: 243 repeats of that log line, then a 20-minute context deadline exceeded) -- the VMB itself completed fine. Our vendored kubevirt.io/api (v1.8.0-alpha.0) predates the rename and has no ConditionComplete constant, so add it locally as the literal string virt-controller actually emits, and recognize it alongside the existing ConditionDone in both places that scan for it. Tests: new table cases in TestHandleAccepted_VMBStatusDetection and TestIsVMBTerminal pin the exact scenario reported (Complete=True, Reason=CompletedWithWarning) transitioning to Prepared/terminal instead of stalling. Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>

coderabbitai · 2026-08-27T21:24:15Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3c68410f-3b3e-4a92-aea0-27356806c85f

📥 Commits

Reviewing files that changed from the base of the PR and between e9fa72d and dd82d57.

📒 Files selected for processing (2)

internal/controller/kubevirt_dataupload_controller.go
internal/controller/kubevirt_dataupload_controller_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

The controller now recognizes KubeVirt nightly Complete VMB conditions alongside ConditionDone. Regression tests cover DataUpload preparation and terminal-state detection.

Changes

VMB Complete condition support

Layer / File(s)	Summary
Detect Complete during VMB status evaluation `internal/controller/kubevirt_dataupload_controller.go`, `internal/controller/kubevirt_dataupload_controller_test.go`	Defines `conditionComplete`, matches it during status evaluation, and verifies `Complete=True` transitions the DataUpload to `Prepared`.
Detect Complete as terminal `internal/controller/kubevirt_dataupload_controller.go`, `internal/controller/kubevirt_dataupload_controller_test.go`	Matches `conditionComplete` in terminal-state detection and verifies successful completion.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to dd82d

The controller now recognizes both old and renamed VirtualMachineBackup completion conditions, restoring expected completion handling without changing interfaces or dependencies. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: sseago, weshayutin, shubham-pampattiwar

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: recognizing the renamed "Complete" condition for VirtualMachineBackup while implying a bug fix.

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Joeavaikath · 2026-08-28T21:05:33Z

Bumping kubevirt.io/api instead was considered and rejected for this fix: v1.8.0-alpha.0 to whatever version introduced ConditionComplete is a large jump with its own unrelated API-surface risk, whereas accepting both condition names is a minimal, backward- and forward-compatible change.

Would we be handling periodic bumps + code changes as a long term fix?

kaovilai · 2026-08-28T21:34:09Z

Bumping kubevirt.io/api instead was considered and rejected for this fix: v1.8.0-alpha.0 to whatever version introduced ConditionComplete is a large jump with its own unrelated API-surface risk, whereas accepting both condition names is a minimal, backward- and forward-compatible change.

Would we be handling periodic bumps + code changes as a long term fix?

Yes, always.

…penshift#212 fix build Lets the incremental-sequence spec actually validate migtools/kubevirt-datamover-controller#208 and openshift#212 (both unmerged upstream) instead of self-skipping on the known flake pattern every run. Uses DPA's spec.unsupportedOverrides (kubevirtDatamoverControllerImageFqin), not a direct Deployment/manifest patch, so it's a pure e2e-test-time override with no manifest churn. Remove once openshift#208/openshift#212 merge upstream and a release picks them up. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

…two unmerged upstream PRs Lets the incremental-sequence spec actually validate migtools/kubevirt-datamover-controller#208 and migtools/kubevirt-datamover-controller#212 (both unmerged upstream) instead of self-skipping on the known flake pattern every run. Uses DPA's spec.unsupportedOverrides (kubevirtDatamoverControllerImageFqin), not a direct Deployment/manifest patch, so it's a pure e2e-test-time override with no manifest churn. Remove once those two PRs merge upstream and a release picks them up. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

…d confirmed live Verified against a real cluster (2026-08-29, AWS amd64) with the combined-208-212-test override image: the incremental-sequence spec ran to a genuine PASS in 7m20s with zero occurrences of either pattern's string in the whole run's logs -- previously this spec reliably flake-skipped within ~20 minutes on one or both patterns. See migtools/kubevirt-datamover-controller#208 and migtools/kubevirt-datamover-controller#212. Drops the two now-dead FlakePattern entries and the CheckIfFlakeOccurred- level tests that specifically exercised them; keeps the FilterLogLinesContaining-level tests and their real captured fixture data, which remain valid regardless of the pattern registry's contents. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

kaovilai · 2026-08-29T07:09:16Z

Live end-to-end validation confirmed (cross-session): the combined test image (this fix + #212) ran the incremental-sequence spec against a real cluster on nightly KubeVirt -- passed clean in 7m20s, with zero occurrences of the previously-triggering log pattern (this spec used to hit the stuck "VirtualMachineBackup in progress, requeuing" loop and skip within 20 minutes every time before this fix). A separate, unrelated known issue (CNV-85377) surfaced once mid-run but didn't block success.

The now-obsolete flake-pattern workaround for this bug has been removed from oadp-operator's e2e suite (openshift/oadp-operator#2404).

Note

Responses generated with Claude

…s warning RunMustGather's check treated ANY content in the must-gather summary's "## Errors" section as a hard failure. The summary generator flags any DPA using spec.unsupportedOverrides at all as a warning, regardless of key or reason, since that field is inherently "unsupported" -- purely informational, not an actual problem. This broke live: adding a single test-time kdm-controller image override to the shared dpaCR (for validating migtools/kubevirt-datamover-controller#208 and migtools/kubevirt-datamover-controller#212 before they merge) made every e2e job's must-gather check fail, including completely unrelated CLI suites -- confirmed on ci/prow/5.0-e2e-test-cli-aws and ci/prow/5.1-e2e-test-cli-aws. Now tolerates that one specific, expected warning line while still failing on any other content in the Errors section. Verified against the real captured summary text from the failing 5.1-e2e-test-cli-aws run. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

kaovilai · 2026-08-31T16:44:21Z

/cherry-pick oadp-1.6

openshift-cherrypick-robot · 2026-08-31T16:44:25Z

@kaovilai: once the present PR merges, I will cherry-pick it on top of oadp-1.6 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick oadp-1.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci · 2026-08-31T16:48:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Joeavaikath, kaovilai, shubham-pampattiwar, sseago

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [kaovilai,shubham-pampattiwar,sseago]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sseago · 2026-08-31T16:48:27Z

/lgtm

openshift-cherrypick-robot · 2026-08-31T19:07:08Z

@kaovilai: new pull request created: #215

Details

In response to this:

/cherry-pick oadp-1.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

…two unmerged upstream PRs Lets the incremental-sequence spec actually validate migtools/kubevirt-datamover-controller#208 and migtools/kubevirt-datamover-controller#212 (both unmerged upstream) instead of self-skipping on the known flake pattern every run. Uses DPA's spec.unsupportedOverrides (kubevirtDatamoverControllerImageFqin), not a direct Deployment/manifest patch, so it's a pure e2e-test-time override with no manifest churn. Remove once those two PRs merge upstream and a release picks them up. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Verified against a real cluster (2026-08-29, AWS amd64) with the combined-208-212-test override image: the incremental-sequence spec ran to a genuine PASS in 7m20s with zero occurrences of either pattern's string in the whole run's logs -- previously this spec reliably flake-skipped within ~20 minutes on one or both patterns. See migtools/kubevirt-datamover-controller#208 and migtools/kubevirt-datamover-controller#212. Drops the two now-dead FlakePattern entries and the CheckIfFlakeOccurred- level tests that specifically exercised them; keeps the FilterLogLinesContaining-level tests and their real captured fixture data, which remain valid regardless of the pattern registry's contents. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

…s warning RunMustGather's check treated ANY content in the must-gather summary's "## Errors" section as a hard failure. The summary generator flags any DPA using spec.unsupportedOverrides at all as a warning, regardless of key or reason, since that field is inherently "unsupported" -- purely informational, not an actual problem. This broke live: adding a single test-time kdm-controller image override to the shared dpaCR (for validating migtools/kubevirt-datamover-controller#208 and migtools/kubevirt-datamover-controller#212 before they merge) made every e2e job's must-gather check fail, including completely unrelated CLI suites -- confirmed on ci/prow/5.0-e2e-test-cli-aws and ci/prow/5.1-e2e-test-cli-aws. Now tolerates that one specific, expected warning line while still failing on any other content in the Errors section. Verified against the real captured summary text from the failing 5.1-e2e-test-cli-aws run. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

openshift-ci Bot requested review from sseago and weshayutin August 27, 2026 21:24

openshift-ci Bot added the approved label Aug 27, 2026

kaovilai mentioned this pull request Aug 27, 2026

test: unpend two kdm restore PIts, fixing bugs found via live e2e validation openshift/oadp-operator#2404

Merged

Joeavaikath approved these changes Aug 28, 2026

View reviewed changes

kaovilai mentioned this pull request Aug 29, 2026

fix: two DataUpload livelocks in VMB handling (stuck retry guard + stale cached status) #212

Merged

5 tasks done

shubham-pampattiwar approved these changes Aug 31, 2026

View reviewed changes

sseago approved these changes Aug 31, 2026

View reviewed changes

openshift-ci Bot assigned sseago Aug 31, 2026

openshift-ci Bot added the lgtm label Aug 31, 2026

kaovilai mentioned this pull request Aug 31, 2026

Remove kubevirt/kubevirt e2e workarounds once their upstream PRs merge openshift/oadp-operator#2420

Open

openshift-merge-bot Bot merged commit 88155e5 into migtools:oadp-dev Aug 31, 2026
7 checks passed

openshift-cherrypick-robot mentioned this pull request Aug 31, 2026

[oadp-1.6] fix: recognize VirtualMachineBackup's renamed Complete condition #215

Merged

Navigation Menu

Repository navigation

fix: recognize VirtualMachineBackup's renamed Complete condition - #208

fix: recognize VirtualMachineBackup's renamed Complete condition#208
openshift-merge-bot[bot] merged 1 commit into
migtools:oadp-devfrom
kaovilai:fix-vmb-condition-complete-rename

kaovilai commented Aug 27, 2026 •

edited by openshift-ci Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 27, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Joeavaikath commented Aug 28, 2026

Uh oh!

kaovilai commented Aug 28, 2026

Uh oh!

kaovilai commented Aug 29, 2026 •

edited by openshift-ci Bot

Loading

Uh oh!

kaovilai commented Aug 31, 2026

Uh oh!

openshift-cherrypick-robot commented Aug 31, 2026

Uh oh!

openshift-ci Bot commented Aug 31, 2026

Uh oh!

sseago commented Aug 31, 2026

Uh oh!

Uh oh!

openshift-cherrypick-robot commented Aug 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

kaovilai commented Aug 27, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause

Fix

Test plan (TDD)

Uh oh!

coderabbitai Bot commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Joeavaikath commented Aug 28, 2026

Uh oh!

kaovilai commented Aug 28, 2026

Uh oh!

kaovilai commented Aug 29, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaovilai commented Aug 31, 2026

Uh oh!

openshift-cherrypick-robot commented Aug 31, 2026

Uh oh!

openshift-ci Bot commented Aug 31, 2026

Uh oh!

sseago commented Aug 31, 2026

Uh oh!

Uh oh!

openshift-cherrypick-robot commented Aug 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kaovilai commented Aug 27, 2026 •

edited by openshift-ci Bot

Loading

coderabbitai Bot commented Aug 27, 2026 •

edited

Loading

kaovilai commented Aug 29, 2026 •

edited by openshift-ci Bot

Loading