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

[kaovilai]

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.
• 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 now merged:

• 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. That sync (via make update-kubevirt-datamover-manifests) also picked up a pre-existing events: create,patch rule already present in the controller's own generated RBAC — used by internal/controller/kubevirt_datadownload_controller.go's Eventf calls, unrelated to this PR's own changes but now correctly reflected here since the manifests are resynced as a whole.
• 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.

Both companion PRs above are merged, so the suite exercises the default plugin/controller images — no image override needed.

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 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 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 main change: adding end-to-end kubevirt-datamover VM backup and restore coverage for the listed scenarios.
Description check	✅ Passed	The description explains the motivation, implementation, limitations, validation results, and concrete commands for testing the changes.

✨ 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]

Note

Responses generated with Claude

CI red but not real fail — e2e-test-aws test step pass clean (52min run, zero failures). Only gather-must-gather post-step die, transient DNS blip in CI cluster (curl: could not resolve host: github.com), not our code.

[kaovilai]

Note

Responses generated with Claude

/test all

[kaovilai]

Note

Responses generated with Claude

/test all

[kaovilai]

Note

Responses generated with Claude

/test all -- retesting for migtools/kubevirt-datamover-controller#124 rebase (tip ff1c1b8)

[kaovilai]

Note

Responses generated with Claude

/test all -- retesting for migtools/kubevirt-datamover-controller#124 real functional fix (tip 3b4b3d5): stale sibling DataDownload correlation bug that could permanently block VM run-state restore, not just a rebase

[kaovilai]

Note

Responses generated with Claude

/test all -- retesting for migtools/kubevirt-datamover-controller#124 tip e66317e: 2 more real bugs fixed (missing APIReader fallback in isRestoreAlreadyProvisioned; qemu-img convert missing -n corrupting already-provisioned block-mode restore targets; operation-timeout could mark an already-successful upload Failed). Note: the previous run (tip 3b4b3d5) gave no real signal -- 6/9 e2e jobs failed purely on CI infra (AWS session-token expiry, Route53 zone conflicts/missing-zone, a step-registry script bug), none reached actual test code.

[kaovilai]

Note

Responses generated with Claude

/test all -- new commit 307d369 updates the kubevirt-datamover-controller test image to quay.io/tkaovila/kubevirt-datamover-controller@sha256:6d3d80dc7f0096114f4b1da2379194bca100fc063f0fd26ee97e9c8d2e21856f (commit e66317e, now public and confirmed anonymously pullable)

[kaovilai]

Note

Responses generated with Claude

/test all -- commit cf64841 removes the kubevirt-datamover plugin/controller image overrides now that both fixes are merged upstream (migtools/kubevirt-datamover-plugin#41, #44 and migtools/kubevirt-datamover-controller#124). This validates the suite against the default images instead of pinned personal-registry builds.

[kaovilai]

/hold
may i redirect attention to merge openshift/release#83282 first then unhold here when release merges we should have images promoted to unblock openshift/release#83049

[shubham-pampattiwar]

Two exported helpers here have no callers in this PR: IsDeploymentRolledOut (this func) and ChecksumUploadPodQcow2Flattened in virt_helpers.go (its own comment says it's not called). tests/e2e/lib is shared code, so unused exports tend to rot. Can we drop both and add them back in the PR that actually calls them? If IsDeploymentRolledOut has a caller I missed, point me at it.

[shubham-pampattiwar]

This events: create,patch rule isn't mentioned in the description, unlike the update on virtualmachines. Two things:

1. Were role.yaml and the CSV regenerated via make update-kubevirt-datamover-manifests, or hand-edited? This file syncs from the controller repo, so hand edits drift on the next sync and can diverge from the CSV.
2. Please add a line to the description explaining the events rule so the CSV grant is traceable.

[shubham-pampattiwar]

This It runs two full restore cycles plus the backup in one spec, with timeouts summing past an hour. I get why it can't split: the AfterEach recreates the DPA, which gets a new random BSL prefix, so a second It can't reuse this backup. Two small asks:

• Move that "can't split" note to a comment above the It, so the next person sees it before trying to break it apart. It currently sits mid-spec near the rejected-restore block.
• If the rejection-path half can ever run off its own small backup, that's the natural spec to break out later.

[shubham-pampattiwar]

A persistent GetVmStatus error returns "" here, which passes ShouldNot(Equal("Running")) for the full minute. So a broken API makes the "VM stayed halted" check pass exactly when we can't see the VM at all. The eager-wait polls swallow the same error harmlessly (they time out loudly), but this negative check needs to tell "confirmed not Running" apart from "couldn't read status". Fail on a run of consecutive errors, or require at least one valid non-Running read before passing.

[shubham-pampattiwar]

vmPvcsBound ignores its ocClient and namespace params and uses the kubernetesClientForSuiteRun global plus pvcNamespace instead. Works, but the unused params are misleading. Either use ocClient, or add a one-line note on why the global clientset is used.

[shubham-pampattiwar]

On the CodeRabbit no-sensitive-data flag: fine to leave. The only caller is the CNV-85377 PIt, which doesn't run, and checkpoint output isn't sensitive. The live guest-exec path logs base64 dd-of-urandom, also harmless. Noting it so the thread closes with a reason.

[kaovilai]

thank you for your review kind sir.

[kaovilai]

/retest-required

[kaovilai]

/retest

[shubham-pampattiwar]

This retry loop exits 0 even when all three clones fail. git clone ... && break || sleep 5: on failure the && break is skipped and || sleep 5 returns 0, so after three misses the loop's status is the last sleep, and the RUN succeeds with no velero checkout. The build then continues and only degrades failure-analysis later, instead of failing here.

Every other fetch in this file uses curl --retry, which does fail the build after exhausting retries. Worth making this one consistent, e.g. assert the result after the loop:

RUN for i in 1 2 3; do \
      rm -rf /go/src/github.com/openshift/velero && \
      git clone --depth 1 --branch oadp-dev \
        https://github.com/openshift/velero.git \
        /go/src/github.com/openshift/velero && break || sleep 5; \
    done; \
    test -d /go/src/github.com/openshift/velero/.git

[shubham-pampattiwar]

checksum-helper- + pvcName can exceed the 63-char DNS label limit for longer PVC names and fail pod creation with an opaque error. Fine for the current short names, but worth truncating or hashing the suffix so a future longer-named PVC scenario doesn't trip on it.

[kaovilai]

This is our own e2e no customer input but will harden

[openshift-ci]

@kaovilai: The following test 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/4.23-e2e-test-aws	9beda53	link	false	/test 4.23-e2e-test-aws

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.