Add e2e coverage for kubevirt-datamover VM backup/restore: multi-PVC, incremental sequence, and CBT restore

[kaovilai]

Important

Draft — waiting on companion PRs in other repos to be reviewed and merged first:

• migtools/kubevirt-datamover-plugin#44 — VM halt-at-restore + stash logic
• migtools/kubevirt-datamover-controller#124 — flip-back-on-DataDownload-completion logic

Both are required for this PR's "restore from a full kubevirt-datamover CBT backup" scenario (see Design below). This PR currently pins their pre-merge images by digest via OADP_E2E_KDM_PREMERGE_IMAGES=true purely for testing — those overrides get removed once both merge and their fixes ship in the default images. Re-marking ready for review once that lands.

Summary

Adds e2e coverage for three kubevirt-datamover VM backup/restore scenarios: multi-PVC VMs, an incremental backup sequence, and restoring a VM from a full CBT backup. Closes coverage gaps tracked in migtools/kubevirt-datamover-controller#99, openshift/oadp-operator#2252, and migtools/kubevirt-datamover-controller#73 (phases 0-3; phases 4/5 are out of scope, see Known limitations).

Design

Multi-PVC VM backup/restore

• New VM template tests/e2e/sample-applications/virtual-machines/cirros-test/cirros-test-multipvc.yaml: a CirrOS VM with two disks — the existing rootdisk plus a second blank 100Mi data disk, so no extra image fetch is needed.
• New ginkgo.Entry("no-application CSI datamover backup and restore, multi-PVC CirrOS VM", ...), following the existing "no-application" CirrOS pattern with BackupRestoreType: lib.CSIDataMover.
• vmPvcsBound() verifies both PVCs exist and are Bound after restore.

Incremental backup sequence

• Automates: full → incremental → a VM restart doesn't invalidate the checkpoint chain → hitting maxIncrementalBackups forces a full backup (via the per-VM kubevirt-datamover.io/max-incremental-backups annotation override, which takes effect immediately with no controller rollout to wait for).
• New tests/e2e/lib helpers: VirtualMachineBackup GVR + GetVMBBackupType (reads .status.type/.status.checkpointName), GetDataUploadForBackup (reads the controller's expected-backup-type annotation), SetVMAnnotation, IsDeploymentRolledOut (generation/updatedReplicas-aware, unlike the existing IsDeploymentReady).
• A fourth scenario (deleting libvirt checkpoints with maxIncrementalBackups=0) depends on an unfixed upstream bug (CNV-85377: virt-controller never falls back to full, the VMB hangs Initializing forever) and is scaffolded as a real, compiling ginkgo.PIt, ready to flip to ginkgo.It once that's fixed.

Restore from a full kubevirt-datamover CBT backup

Restoring a VM from a kubevirt-datamover backup has to hold the VM inactive until its DataDownload(s) finish: otherwise virt-launcher spawns immediately on the restored VM object, and WaitForFirstConsumer binds the target PVC to a scratch volume before the DataDownload can rebind it. That race is closed by two companion PRs, both required together:

• migtools/kubevirt-datamover-plugin#44: a new VM RestoreItemActionV2 halts the VM's run strategy at restore time and stashes the original value in an annotation.
• migtools/kubevirt-datamover-controller#124: the DataDownload controller restores the VM's stashed run state once every sibling DataDownload for that VM reaches Completed — driven by a watch on the phase transition itself rather than a single completion call site, so a controller-restart-mid-restore resume path can't skip it.
• The controller's ServiceAccount additionally needs update on kubevirt.io/virtualmachines to perform that flip; this PR syncs that verb into config/kubevirt-datamover-controller_rbac/role.yaml and the bundle CSV.
• RunVirshCommand's virt-launcher pod lookup now skips pods that are terminating or not yet Running: a VM restart (hit by the incremental-sequence test above) can briefly have both the old and new launcher pod sharing the same domain annotation.
• ClearStuckVMBFinalizers (workaround for kubevirt/kubevirt#18724: a deleted VirtualMachineBackup never has its vmbackup-protection finalizer released once its backing tracker is gone) now clears only that specific finalizer, rather than the VMB's entire finalizer list.
• Block volumeMode is explicitly asserted, not just incidentally exercised: GetDataDownloadForRestore also surfaces the kubevirt-datamover.io/restore-block-mode annotation the plugin stamps on the DataDownload, and the test cross-checks it against the restored PVC's actual spec.volumeMode.

The plugin and controller images are pinned by digest, not by tag, in tests/e2e/virt_backup_restore_suite_test.go's UnsupportedOverrides — gated behind OADP_E2E_KDM_PREMERGE_IMAGES=true since these are personal pre-merge builds; the override and its digests are removed once both companion PRs merge and the fixes ship in the default images.

Known limitations

• Two restore scenarios are scaffolded as ginkgo.PIt and are explicitly out of scope here: multi-disk restore hardening (kubevirt-datamover-controller#73 phase 4) and incremental-chain restore validation (phase 5) — neither is implemented upstream yet.
• The maxIncrementalBackups=0 checkpoint-delete scenario stays pending on CNV-85377.

Testing

All three in-scope scenarios pass end-to-end on a real OpenShift cluster (AWS, community HCO/KubeVirt) with the companion plugin and controller images deployed:

• multi-PVC backup/restore: pass
• incremental backup sequence (full → incremental → restart → max-limit): pass
• restore from a full CBT backup: pass — controller logs confirm the VM halts at restore and flips back to its stashed run state (Always) once its DataDownload reaches Completed; Block volumeMode confirmed via both the DataDownload annotation and the restored PVC itself.

go build, go vet, and golangci-lint all pass.

Test plan

TEST_VIRT=true OADP_E2E_KDM_PREMERGE_IMAGES=true GINKGO_ARGS="--focus='multi-PVC CirrOS VM'" make test-e2e
TEST_VIRT=true GINKGO_ARGS="--focus='Kubevirt datamover incremental backup sequence'" make test-e2e
TEST_VIRT=true OADP_E2E_KDM_PREMERGE_IMAGES=true GINKGO_ARGS="--focus='restore a VM from a full kubevirt-datamover CBT backup'" make test-e2e

Note

Responses generated with Claude

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The PR adds a multi-PVC CirrOS VM fixture, Velero and KubeVirt inspection helpers, readiness checks, and CSI datamover tests for restore and incremental backup behavior.

Changes

KubeVirt datamover validation

Layer / File(s)	Summary
VM fixture and API scheme tests/e2e/sample-applications/virtual-machines/cirros-test/cirros-test-multipvc.yaml, tests/e2e/lib/scheme.go	Defines a CirrOS VM with sourced root and blank data disks. Registers Velero v2alpha1 resources in the shared scheme.
Backup and VM inspection helpers tests/e2e/lib/apps.go, tests/e2e/lib/backup.go, tests/e2e/lib/virt_helpers.go	Adds Deployment rollout checks, DataUpload and VirtualMachineBackup lookup, virt-launcher discovery, virsh execution, and VM annotation updates.
Multi-PVC restore validation tests/e2e/virt_backup_restore_suite_test.go	Adds PVC binding polling, a datamover image override, and validation for both restored CirrOS disk PVCs.
Incremental backup sequence tests/e2e/virt_backup_restore_suite_test.go	Adds ordered full and incremental backups across VM restart, maximum-incremental fallback, cleanup, and checkpoint-deletion behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant BackupRestoreTest
  participant KubernetesAPI
  participant CSIDataMover
  participant VirtLauncherPod
  BackupRestoreTest->>KubernetesAPI: Wait for VM PVCs to become Bound
  BackupRestoreTest->>CSIDataMover: Create full backup
  CSIDataMover->>KubernetesAPI: Record DataUpload and checkpoint
  BackupRestoreTest->>VirtLauncherPod: Run virsh and restart VM
  BackupRestoreTest->>CSIDataMover: Create incremental backup
  CSIDataMover->>KubernetesAPI: Record backup type and checkpoint
  BackupRestoreTest->>CSIDataMover: Restore both VM PVCs

Loading

Possibly related PRs

• openshift/oadp-operator#2238: Extends the same KubeVirt datamover E2E tests and helper code.
• openshift/oadp-operator#2255: Covers related KubeVirt datamover restore behavior in a different code path.

Suggested reviewers: sseago, shubham-pampattiwar

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (2 errors, 3 warnings)

Check name	Status	Explanation	Resolution
Stable And Deterministic Test Names	❌ Error	The new PIt title includes the transient failure state “hangs forever” and issue ID CNV-85377, so it is likely to require renaming when the bug is fixed.	Use a stable desired-behavior title such as “falls back to a full backup after libvirt checkpoint deletion”; keep the blocker reference in a comment.
No-Sensitive-Data-In-Logs	❌ Error	RunVirshCommand uses ExecuteCommandInPodsSh, which logs the full Kubernetes exec URL and raw virsh output; this may expose an internal API hostname and VM checkpoint data.	Use an exec helper that does not log the full URL or command streams, or redact the host and output before logging.
Test Structure And Quality	⚠️ Warning	The new It at virt_backup_restore_suite_test.go:676 asserts four behaviors, while its shared BeforeAll setup is not rerun for the pending scenario after the active It deletes the VM and namespace.	Split the incremental scenarios into independently named Its or containers, use per-spec BeforeEach/AfterEach cleanup, and add bounded contexts to direct pod-exec/API calls.
Microshift Test Compatibility	⚠️ Warning	The new multi-PVC and incremental specs lack MicroShift guards and inherit setup that creates OLM Subscriptions and waits for ClusterServiceVersions, which MicroShift does not provide.	MicroShift compatibility notice: add [Skipped:MicroShift] or an IsMicroShiftCluster()+g.Skip guard; otherwise run /payload-job periodic-ci-openshift-microshift-release-4.22-periodics-e2e-aws-ovn-ocp-conformance.
Ipv6 And Disconnected Network Test Compatibility	⚠️ Warning	New specs use shared setup with http.Get to download.cirros-cloud.net and pin the datamover plugin to quay.io; neither new test has a disconnected skip marker.	Mirror the CirrOS and plugin images in an internal registry, or add [Skipped:Disconnected]; verify with the required IPv6/disconnected CI job.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The added multi-PVC entry and incremental tests use single VMs and ordinary PVCs; manifests and templates contain no node, affinity, topology, drain, failover, or multi-node assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	The PR changes only e2e helpers/tests and a VM manifest. No added affinity, topology spread, node selectors, tolerations, replica, or disruption constraints were found.
Ote Binary Stdout Contract	✅ Passed	No process-level stdout writes found. The only fmt.Printf calls are in test execution helpers; setup logs use Go's standard logger, which writes to stderr.
No-Weak-Crypto	✅ Passed	The full PR diff adds no MD5, SHA1, DES, 3DES, RC4, Blowfish, ECB, custom crypto, or secret/token comparisons; changed Go files add no crypto imports or crypto API calls.
Container-Privileges	✅ Passed	The full PR patch adds no privileged settings, host namespace flags, SYS_ADMIN, or root container configuration; the new VM manifest has no securityContext.
Title check	✅ Passed	The title clearly summarizes the primary change: adding end-to-end coverage for kubevirt-datamover VM backup and restore scenarios.
Description check	✅ Passed	The description explains the changes, design, limitations, testing results, and test commands, although it does not use the template headings exactly.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/e2e/virt_backup_restore_suite_test.go`:
- Line 96: Update the Eventually assertion for allBound to include a failure
message containing the relevant namespace and PVC names, so timeout reports
identify which restore resources failed.
- Line 84: Update the PVC lookup in the suite polling flow to create a bounded
timeout context for each Kubernetes PersistentVolumeClaims.Get call instead of
using context.Background(). Pass that context to Get and ensure the context is
canceled after the request, while preserving the existing ten-minute polling
behavior.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5c2eeb1f-c30f-4c5e-8abd-dcc7735ac335

📥 Commits

Reviewing files that changed from the base of the PR and between 789130a and 3967eba.

📒 Files selected for processing (2)

• tests/e2e/sample-applications/virtual-machines/cirros-test/cirros-test-multipvc.yaml
• tests/e2e/virt_backup_restore_suite_test.go

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (2)

tests/e2e/virt_backup_restore_suite_test.go (1)

664-672: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The spec depends on a counter value produced by earlier specs.

runSequenceBackup("full") here is correct only when backupCount is 3. ginkgo.Ordered preserves order, but it does not guarantee that the two earlier specs ran. A focused run, a label filter, or a --focus on this single spec leaves backupCount at 0, and the assertion then checks the wrong expected type. Derive the number of preceding incremental backups explicitly, or set the annotation value from backupCount so the expectation stays consistent.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/virt_backup_restore_suite_test.go` around lines 664 - 672, Update
the “hitting maxIncrementalBackups forces a full backup” spec so its expected
backup type does not depend on earlier specs having run. Derive the required
max-incremental-backups annotation or expected result from the current
backupCount, ensuring focused or label-filtered execution still validates the
intended full-backup behavior.

tests/e2e/lib/virt_helpers.go (1)

1386-1394: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Handle an unpopulated VMB status explicitly.

unstructured.NestedString returns found=false when status.type is absent. The code discards that bool, so a VirtualMachineBackup that is still initializing returns ("", "", nil). The caller then fails with Expected: "full", Actual: "", which does not show that the status was not yet written. Return a distinct error when the field is missing, so the failure identifies the cause.

♻️ Proposed change

-		backupType, _, err = unstructured.NestedString(vmb.Object, "status", "type")
-		if err != nil {
+		backupType, found, err := unstructured.NestedString(vmb.Object, "status", "type")
+		if err != nil {
 			return "", "", fmt.Errorf("failed to read status.type from VirtualMachineBackup %s/%s: %w", namespace, vmb.GetName(), err)
 		}
+		if !found {
+			return "", "", fmt.Errorf("VirtualMachineBackup %s/%s has no status.type yet", namespace, vmb.GetName())
+		}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/lib/virt_helpers.go` around lines 1386 - 1394, Update the status
extraction in the VirtualMachineBackup helper to retain and validate the found
flags returned by unstructured.NestedString for status.type and
status.checkpointName. Return a distinct, descriptive error when either required
field is absent, while preserving the existing wrapped errors for extraction
failures and the successful return path when both fields are present.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/e2e/lib/backup.go`:
- Around line 149-160: Update lib.Scheme initialization to register
velerov2alpha1 via AddToScheme alongside the existing velerov1 registration, so
GetDataUploadForBackup can list DataUploadList without an unregistered-kind
error.

In `@tests/e2e/virt_backup_restore_suite_test.go`:
- Around line 622-651: Add meaningful failure messages to every Gomega assertion
in the BeforeAll and AfterAll hooks, including namespace deletion/creation,
application installation, VM status polling, VM readiness, VM removal, and
namespace cleanup. Each message should identify the failed action and include
the relevant incSeqNamespace and incSeqVMName values where applicable.
- Around line 344-349: Remove the temporary KubeVirt datamover plugin override
from the DPA setup in the test, including the UnsupportedOverrides
initialization and assignment for KubeVirtDatamoverPluginImageKey. If the plugin
fix is not yet available, replace the personal mutable image with a
project-owned digest-pinned image and retain explicit tracking for removing the
override.
- Around line 589-597: Prevent the outer AfterEach teardown from running between
specs in the “Kubevirt datamover incremental backup sequence” Describe. Scope or
condition tearDownBackupAndRestore so the ordered sequence remains deployed,
then add cleanup in this Describe’s AfterAll to remove the sequence resources
and restore the expected final state.

---

Nitpick comments:
In `@tests/e2e/lib/virt_helpers.go`:
- Around line 1386-1394: Update the status extraction in the
VirtualMachineBackup helper to retain and validate the found flags returned by
unstructured.NestedString for status.type and status.checkpointName. Return a
distinct, descriptive error when either required field is absent, while
preserving the existing wrapped errors for extraction failures and the
successful return path when both fields are present.

In `@tests/e2e/virt_backup_restore_suite_test.go`:
- Around line 664-672: Update the “hitting maxIncrementalBackups forces a full
backup” spec so its expected backup type does not depend on earlier specs having
run. Derive the required max-incremental-backups annotation or expected result
from the current backupCount, ensuring focused or label-filtered execution still
validates the intended full-backup behavior.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: be76a5c0-d96a-49fb-a7cd-9b6173f7f69f

📥 Commits

Reviewing files that changed from the base of the PR and between 3967eba and 683a49f.

📒 Files selected for processing (4)

• tests/e2e/lib/apps.go
• tests/e2e/lib/backup.go
• tests/e2e/lib/virt_helpers.go
• tests/e2e/virt_backup_restore_suite_test.go

[coderabbitai]

🧹 Nitpick comments (1)

tests/e2e/lib/scheme.go (1)

31-31: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Handle scheme registration errors consistently.

velerov2alpha1.AddToScheme(Scheme) currently returns nil unconditionally, so this line cannot cause the stated DataUploadList failure. Do not discard the error return; use utilruntime.Must(...) for all scheme registrations to fail fast if their behavior changes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/lib/scheme.go` at line 31, Update the scheme initialization around
velerov2alpha1.AddToScheme(Scheme) to pass its return value through
utilruntime.Must instead of discarding it, and apply the same fail-fast wrapper
consistently to every scheme registration in that initialization block.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/e2e/lib/scheme.go`:
- Line 31: Update the scheme initialization around
velerov2alpha1.AddToScheme(Scheme) to pass its return value through
utilruntime.Must instead of discarding it, and apply the same fail-fast wrapper
consistently to every scheme registration in that initialization block.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: afb294c1-b581-4b68-b0c5-c16a7bd1f4a9

📥 Commits

Reviewing files that changed from the base of the PR and between 683a49f and 7d42f4d.

📒 Files selected for processing (2)

• tests/e2e/lib/scheme.go
• tests/e2e/virt_backup_restore_suite_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/e2e/virt_backup_restore_suite_test.go

[kaovilai]

Re review 4859513816 and the two remaining nitpicks from 4859450952:

• scheme.go:31 (utilruntime.Must): fixed in 5321f15 — all 15 scheme registrations now use utilruntime.Must(...), matching cmd/main.go's existing convention, instead of discarding errors via _ =. Verified none of them actually panic (ran TestOADPE2E without a cluster; it got past scheme init and failed later on an unrelated connectivity assertion).
• virt_helpers.go:1386-1394 (unpopulated VMB status): fixed in 0d0b911 — GetVMBBackupType now checks the found bool from unstructured.NestedString(status, "type") and returns a distinct error ("has no status.type yet") instead of silently returning ("", "", nil).
• virt_backup_restore_suite_test.go:664-672 (backupCount depends on earlier specs): no longer applies — this finding was against the pre-fix version with 3 separate ginkgo.Its (commit 683a49f). After the AfterEach-teardown fix in 7d42f4d, all 4 steps are sequential statements inside a single ginkgo.It, not separate specs, so there's no longer a focus/label-filter path that can run step 4 while skipping steps 1-3.

Note

Responses generated with Claude

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai

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]

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

[kaovilai]

/test all

[kaovilai]

/retest-required

[openshift-ci]

@kaovilai: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/5.1-e2e-test-aws	91931ff	link	true	/test 5.1-e2e-test-aws
ci/prow/5.1-ci-index	91931ff	link	true	/test 5.1-ci-index
ci/prow/5.1-e2e-test-cli-aws	91931ff	link	true	/test 5.1-e2e-test-cli-aws
ci/prow/5.1-images	91931ff	link	true	/test 5.1-images

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.