[oadp-1.6] test: unpend two kdm restore PIts, fixing bugs found via live e2e validation - #2427
Open
openshift-cherrypick-robot wants to merge 15 commits into
Conversation
…idation Upstream blockers (kubevirt-datamover-controller#169, openshift#73 phase 4) have landed, so this un-pends 'restore run-state flip is not blocked by a stale sibling DataDownload from a different restore attempt' and 'restore a multi-PVC VM from a kubevirt-datamover CBT backup'. Live validation against real AWS/GCP/Azure clusters surfaced and fixed several bugs along the way: - the decoy DataDownload used in the run-state-flip test correlated by restore-uid instead of restore-name (the actual key the real fix uses), and used Status().Update() against a CRD version with no status subresource, which unconditionally 404s - GetDataUploadForBackup could return before kubevirt_dataupload_controller had stamped the expected-backup-type annotation, racing the caller - virt-controller's VirtualMachineBackup status can permanently stop advancing after successfully attaching the backup target PVC (its attach branch returns without writing a status condition or requeuing, see kubevirt/kubevirt pkg/storage/cbt/backup.go startBackup()), tracked as CNV-85377/CNV-89684 and reported upstream with a fix at kubevirt/kubevirt#18949. Until that merges, runKubevirtDMBackup polls manually and nudges the VMI (a harmless annotation patch forcing a fresh watch event) whenever kdm-controller's logs show the frozen pattern, giving the stuck reconcile a real chance to recover instead of waiting out or retrying the whole timeout. Confirmed working across dozens of live hits on both GCP and Azure. If the nudge doesn't unstick it before the timeout, the spec marks pending (no ginkgo-level retry) rather than failing on a known, tracked upstream bug. - a second, distinct upstream bug found testing against kubevirt nightly: reconcileStart() (same file) can mark an already-successfully-completed VirtualMachineBackup Failed with reason SourceLost -- vmi.Status.ChangedBlockTracking.BackupStatus being nil is treated unconditionally as "status lost mid-flight", but virt-handler also clears that same field as part of normal post-completion cleanup. Introduced by kubevirt's June 2026 "observation-driven dispatch" restructure; distinct from the attach-freeze bug above. This only ever surfaces as a Kubernetes Event (never in any pod's log), so runKubevirtDMBackup's poll now also checks the VM namespace's own events, not just kdm-controller's log, for known-flake patterns. - EnsureCommunityHcoCatalog/GetVirtOperator derived the OLM channel by guessing from the HCO index tag's numeric shape, which breaks for a moving tag like 'nightly' (channel has no relationship to the tag string). Channel is now discovered from the live PackageManifest instead (filtered by its catalog= label, since more than one CatalogSource can publish a manifest under the same package name -- getCsvFromPackageManifest's own separate, unfiltered lookup had the exact same bug, fixed the same way), so hco_index_tag=nightly works through the existing community-HCO path. - HCO_INDEX_TAG's Makefile default is now "nightly" instead of a pinned "1.18.0", so every virt/kdm e2e run (local and CI) picks up kubevirt/kubevirt fixes like #18949 automatically as soon as they land in a nightly build, with no version bump needed on our side. Override to a pinned release for a reproducible/stable run instead. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…al kdm-controller log data Uses real captured lines from a live CI run (/tmp/kdm-mgr-log2.txt) for the openshift#212 pattern and a genuinely healthy sibling DataUpload, plus a source-verified fixture for the openshift#208 pattern (no per-CI-artifact raw capture exists for it, since it now produces a Skip rather than a failure -- verified instead by reading kubevirt_dataupload_controller.go directly and confirming its log.FromContext(ctx) logger is shared, unmodified, with the openshift#212 call site). Reproduces the actual misattribution bug live (unfiltered combined log matches the openshift#212 pattern even for a spec whose own backup is healthy), proves scoping by backup/DataUpload name fixes it without losing real detections for either pattern. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…keOccurred Per second-opinion review: name-scoping a log before flake-checking risks silently disabling detection for any pattern whose known-bug string doesn't appear on a line naming the object. Verified each currently-tracked pattern against kdm-controller's actual source -- all reachable via kdm-controller's own pod log share the same context-injected logger (name-bearing even with no inline args), the one event-only pattern is already excluded from filtering, and the remaining three patterns are velero/snapshot-controller strings never reachable via kdm-controller's log regardless of filtering. Documented so a future added pattern can be checked the same way. 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>
…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>
Adds a second fix on top of openshift#208/openshift#212: handleAccepted was trusting the informer cache's VirtualMachineBackup.Status, which can be stale after a missed/delayed watch event, instead of re-reading it via APIReader before concluding terminal state. Found live via a Prow failure (ci/prow/5.0-e2e-test-kubevirt-aws) where virt-controller had already written Done=True but kdm-controller's reconcile loop never observed it, confirmed via a live repro capturing the real VirtualMachineBackup object's status.conditions directly against the API server. migtools/kubevirt-datamover-controller#212 Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Adds debug logging to the uncached-status-refresh's own success/NotFound/ error branches, to settle -- on the next recurrence of the identical "in progress, requeuing" x243/0-completed signature -- whether the live API server itself never had Done=True (a virt-controller/kubevirt-level stall, not kdm-controller's bug) or the "uncached" read has some subtler issue, without needing another live-debug session. migtools/kubevirt-datamover-controller#212 Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
… flake A third manifestation of the same upstream bug, confirmed by kubevirt-fixer to trace to the identical root cause as kubevirt/kubevirt#18949: the VirtualMachineBackup can sit with zero status.conditions for the whole backup timeout, not just a frozen "is being attached to VMI" message. Confirmed live via 244 consecutive uncached reads across 20 minutes, all nil. Has no distinguishing log text for lib.CheckIfFlakeOccurred to match, so lib.VirtOperator.VMBHasNoConditions checks the VMB object directly instead, gated on the condition persisting for a few checks (not just the first sighting) so a freshly-created VMB's normal brief pre-condition window isn't misdetected. Feeds the same existing nudge-then-skip path as the other CNV-85377 manifestation. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
EnableCBTFeatureGate was writing {"incrementalBackup": true} -- an object
with a bool field -- but HCO's actual HyperConvergedFeatureGates type
(api/v1/featuregates/feature_gates.go) is []FeatureGate{Name, State}, an
array of {name, state} objects with State one of "Enabled"/"Disabled".
Confirmed identical shape at both HEAD and the v1.18.0 tag, so this isn't
a version skew -- it was simply wrong the whole time.
HCO's v1beta1 write path is permissive enough to accept the wrong shape,
but the v1 conversion webhook then fails to unmarshal it on the next read:
"conversion webhook for hco.kubevirt.io/v1, Kind=HyperConverged failed:
json: cannot unmarshal object into Go struct field HyperConvergedSpec.
spec.featureGates of type featuregates.HyperConvergedFeatureGates" --
confirmed live as the actual cause of an unrelated-looking suite-wide
BeforeAll timeout on this PR, previously misattributed to a separate
upstream issue (kubevirt/hyperconverged-cluster-operator#4549).
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…an object"
That fix was wrong: it was based on HCO's v1 API type
(api/v1/featuregates/feature_gates.go, an array of {name, state}), but
EnableCBTFeatureGate writes to hyperConvergedGvr's v1beta1, not v1.
Confirmed by reading v1beta1's actual type directly
(api/v1beta1/hyperconverged_types.go): HyperConvergedFeatureGates has a
plain IncrementalBackup *bool field with json tag "incrementalBackup" --
the original object-with-bool-field shape was correct all along. The
array-shape rewrite got rejected outright by v1beta1's own mutating
admission webhook ("unknown field spec.featureGates[0].name/.state"),
confirmed live on the very next CI run.
The real root cause of the original "conversion webhook for hco.kubevirt.io/v1
... cannot unmarshal object" error remains genuinely unclear -- it may be a
real, transient upstream HCO nightly-catalog issue after all (as originally
suspected and tracked at kubevirt/hyperconverged-cluster-operator#4549),
not a shape bug in this repo's own code. Reverting to the known-correct
shape while that gets investigated properly instead of guessing again.
This reverts commit 5e4d980.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
hco.kubevirt.io's HyperConverged CRD serves both v1 (storage:true, the hub version) and v1beta1 (storage:false). Every v1beta1 read/write round-trips through HCO's own conversion webhook, which has a live bug (kubevirt/hyperconverged-cluster-operator#4549) that has rejected valid spec.featureGates writes/reads in this suite's CI runs. Reading/writing v1 directly needs zero conversion, sidestepping that webhook entirely. hyperConvergedGVR() discovers via the Discovery API whether the cluster serves hco.kubevirt.io/v1 and caches the choice per VirtOperator, falling back to v1beta1 for older HCO releases that don't yet serve v1. EnableCBTFeatureGate's spec.featureGates write now branches on the resolved version, since v1's shape is a HyperConvergedFeatureGates array of {name, state} objects, not v1beta1's bool-field object. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…irt's own path 3/3 kubevirt-aws runs hit the identical failure: the "restore run-state flip..." spec's known-bug skip path (lib.CheckIfFlakeOccurred) unwinds via ginkgo.Skip before reaching its own namespace cleanup in virt_backup_restore_suite_test.go, which already knew to clear stuck VirtualMachineBackup finalizers (IsNamespaceDeletedClearingStuckVMBFinalizers, the kubevirt#18724 workaround) before waiting for termination. The shared AfterEach's plain deleteNamespace doesn't, so it hangs 5m waiting on a namespace whose VMB never got a real completed status (same still-open kubevirt/kubevirt#18949 disease as VMBHasNoConditions), failing the whole run and contributing to hitting the openshift#2413 timeout ceiling. deleteNamespace now clears stuck VMB finalizers unconditionally via a throwaway VirtOperator wrapping the suite's existing dynamic client -- a harmless no-op for namespaces with no VMBs, so non-virt specs are unaffected. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
migtools/kubevirt-datamover-controller#207, openshift#208, and openshift#212 all merged (16:48, 19:06, 21:38 UTC). Confirmed the default image has caught up too: quay.io/konveyor/kubevirt-datamover-controller:latest's mirror refreshed at 22:01:45 UTC, after the last merge; and openshift/release#82762 wires this image directly into oadp-dev's ci-operator base_images/ operator.substitutions, so Prow e2e picks up a freshly-built image immediately regardless of mirror cadence. The custom quay.io/tkaovila/kubevirt-datamover-controller:combined-208-212v3-test override this suite carried since validating those PRs pre-merge is no longer needed -- the settings.json-driven UnsupportedOverrides path (unaffected by this change) is the normal, permanent path going forward. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
item 1) The restore-side "hard" data-integrity checksum in the kdm restore specs was silently skipped on effectively every run: it only trusted the read if the VM was still Halted immediately before and after, but the restored VM was already Running by the very first status read after restore, every time observed. The core assertion these specs exist to run had likely never actually executed. Adds VirtOperator.EnsureVmHaltedForExclusivePVCAccess: deterministically stops the VM (v.StopVm) and waits for its virt-launcher pod to actually disappear, rather than hoping to catch a naturally-occurring halted window. Unconditional by design -- a bypass keyed on "no pod right now" would have the same race shape as the bug this closes, since the restored VM's spec.running stays true and KubeVirt could create a new launcher pod moments later. Both call sites now always restart the VM afterward (StartVm), including when EnsureVmHaltedForExclusivePVCAccess itself errors out, since StopVm was still called either way. Validated live end-to-end on a real bare-metal KVM cluster (tkaovila-260901-amd64, us-west-2), not just locally-reasoned: - First attempt caught two real bugs CodeRabbit flagged that a synthetic run alone wouldn't have exercised: (1) trusting the VM's printableStatus string instead of the virt-launcher pod's actual presence -- Paused/Starting/Stopping all still have an attached pod just like Running; (2) the restart-on-error path never firing because the code short-circuited past it whenever the halt itself failed, which would have left the VM permanently stopped for the rest of the spec. - After fixing both, a live run still hit a 5-minute timeout waiting for the virt-launcher pod to disappear. Root cause: GetAllPodsWithLabel returns an error on a genuinely empty list ("no Pod found") instead of a clean empty result, so every poll tick misread "the pod is actually gone" as a transient failure worth retrying rather than success -- fixed by calling the Pods().List() client directly instead of routing through that helper. - Final live run: both kdm restore specs passed, hard assertions genuinely executed (real matching checksums via the exclusive helper pod, not skipped), stop-to-pod-gone taking ~5-36s in practice. go build/vet/gofmt clean. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Contributor
|
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: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
shubham-pampattiwar
approved these changes
Sep 2, 2026
openshift-ci
Bot
added
the
approved
Member
|
/test 5.0-e2e-test-kubevirt-aws |
sseago
approved these changes
Sep 2, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: openshift-cherrypick-robot, shubham-pampattiwar, sseago 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 |
Member
|
/lgtm |
Contributor
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
Member
|
/hold for new test post openshift/release pr coming up to split kdm test out to avoid 2h timeout |
openshift-ci
Bot
added
the
do-not-merge/hold
kaovilai
added a commit
to kaovilai/release
that referenced
this pull request
Sep 2, 2026
…p-1.6) Same fix as the earlier oadp-dev/5.0 split (openshift#84337), now backported to oadp-1.6's own e2e-test-kubevirt-aws presubmit across all three variants (4.22, 4.23, 5.0). openshift/oadp-operator#2424 (cherry-pick of openshift#2423 to oadp-1.6) gave this branch's Makefile the same TEST_VIRT_KDM support, so the split is safe here too. - virt-nokdm-e2e-test-aws: TEST_VIRT=true TEST_VIRT_KDM=false - virt-kdm-e2e-test-aws: TEST_VIRT_KDM=true Prompted by openshift/oadp-operator#2427 (unpends two more kdm restore specs on oadp-1.6), which risks pushing the still-unsplit job back over the 2h step timeout the same way oadp-dev/5.0 hit it originally. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Member
|
/override ci/prow/5.0-e2e-test-kubevirt-aws |
|
@kaovilai: Overrode contexts on behalf of kaovilai: ci/prow/5.0-e2e-test-kubevirt-aws DetailsIn response to this:
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-cherrypick-robot: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
openshift-merge-bot Bot
pushed a commit
to openshift/release
that referenced
this pull request
Sep 2, 2026
…p-1.6) (#84490) Same fix as the earlier oadp-dev/5.0 split (#84337), now backported to oadp-1.6's own e2e-test-kubevirt-aws presubmit across all three variants (4.22, 4.23, 5.0). openshift/oadp-operator#2424 (cherry-pick of #2423 to oadp-1.6) gave this branch's Makefile the same TEST_VIRT_KDM support, so the split is safe here too. - virt-nokdm-e2e-test-aws: TEST_VIRT=true TEST_VIRT_KDM=false - virt-kdm-e2e-test-aws: TEST_VIRT_KDM=true Prompted by openshift/oadp-operator#2427 (unpends two more kdm restore specs on oadp-1.6), which risks pushing the still-unsplit job back over the 2h step timeout the same way oadp-dev/5.0 hit it originally. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Member
|
/override "ci/prow/5.0-e2e-test-kubevirt-aws" |
|
@kaovilai: Overrode contexts on behalf of kaovilai: ci/prow/5.0-e2e-test-kubevirt-aws DetailsIn response to this:
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. |
Member
|
/test 4.22-virt-kdm-e2e-test-aws |
Member
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
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
4 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 #2404
/assign kaovilai