[kaovilai]

Summary

Supersedes #195 (closed). Per discussion with @sseago: instead of requiring restore-target StorageClasses to use volumeBindingMode: WaitForFirstConsumer, this teaches the controller to honor the same selector-based technique Velero's own built-in CSI DataDownload restore path already uses (velero.io/dynamic-pv-restore, see vendored pkg/restore/actions/csi/pvc_action.go's restoreFromDataUploadResult): setting a matchLabels-only Spec.Selector on the target PVC makes dynamic provisioners skip it entirely, regardless of volumeBindingMode.

What changed

• handleAccepted's early-phase gate no longer rejects any target PVC with Spec.Selector set -- only matchExpressions (which we have no general way to satisfy) still fails fast.
• validateExistingPVCForBind now reconciles the rebound PV's labels to satisfy a matchLabels selector instead of requiring them to already match. The PV is one this controller fully owns, so it can just patch the labels the plugin's selector names, rather than needing the plugin and controller to coordinate a label value in advance.
• Unit tests updated/added in both files to pin: matchExpressions still rejected, matchLabels now succeeds end-to-end (Accepted -> Prepared, and the PV label reconciliation itself).

Scope

This is the controller-side half. The companion kubevirt-datamover-plugin change landed in migtools/kubevirt-datamover-plugin#50 (draft), using Velero's own exported velerov1.DynamicPVRestoreLabel constant with an operationID-derived, per-(restore,PVC)-unique value.

Live end-to-end validation

Validated on a real GCP OCP cluster (combined image quay.io/tkaovila/kubevirt-datamover-controller:combined-197-199-test + plugin PR#50's image), cross-session with a peer session driving the plugin side. Confirmed all three links of the chain, not just "no error":

1. Plugin sets the selector: restored PVC sat Pending with spec.selector.matchLabels = {"velero.io/dynamic-pv-restore": "<operationID-derived value>"}.
2. Controller reconciles the PV: once the DataDownload reached Completed, the bound PV carried that same velero.io/dynamic-pv-restore label (alongside velero.io/datadownload-uid).
3. Rebind completed: target PVC transitioned Pending -> Bound; the Restore CR showed restoreItemOperationsAttempted=2, Completed=2.

Unrelated findings from that same run (VM didn't fully reach Ready) are out of scope for this PR: a KubeVirt admission-webhook rejection of reserved kubevirt.io/ labels on direct VMI restore (known non-fatal Velero+KubeVirt behavior), and a libvirt SyncVMI/CBT permission-denied error unrelated to PVC rebinding.

Test plan

• go build ./...
• go vet ./...
• go test ./...
• make lint
• End-to-end validation with the plugin-side change (see above)

Note

Responses generated with Claude

[coderabbitai]

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: 46cde981-f5bd-4a5e-800d-ca7ad4a17168

📥 Commits

Reviewing files that changed from the base of the PR and between 1cf3fa2 and 4ac3829.

📒 Files selected for processing (4)

• internal/controller/kubevirt_datadownload_controller.go
• internal/controller/kubevirt_datadownload_controller_test.go
• internal/controller/pv_rebind.go
• internal/controller/pv_rebind_test.go

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

---

📝 Walkthrough

Walkthrough

The change updates PVC rebinding to reconcile matchLabels onto PVs and reject matchExpressions. DataDownload validation and tests now cover both selector forms, including scratch PVC creation and Prepared status.

Changes

PVC selector reconciliation

Layer / File(s)	Summary
PV selector validation and reconciliation internal/controller/pv_rebind.go, internal/controller/pv_rebind_test.go	PV rebinding rejects matchExpressions selectors and patches missing matchLabels onto the PV. Tests verify the persisted labels.
DataDownload target validation internal/controller/kubevirt_datadownload_controller.go, internal/controller/kubevirt_datadownload_controller_test.go	DataDownload validation rejects unsupported selectors before scratch PVC creation and accepts matchLabels selectors, resulting in Prepared status and scratch PVC creation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 4ac38

The controller now supports matchLabels-based restore targets and reconciles labels on owned rebound PVs; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: mpryc, shubham-pampattiwar

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: honoring restore-target PVC matchLabels selectors.
Docstring Coverage	✅ Passed	Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
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.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[kaovilai]

race.. fixed in openshift/oadp-operator#2404 (comment)

[kaovilai]

/test virt-kdm-e2e-test-aws

[shubham-pampattiwar]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

[kaovilai]

Live end-to-end validation done (cross-session, with the companion plugin fix in migtools/kubevirt-datamover-plugin#50). Ran on a real GCP OCP cluster using a combined image built from this branch + #197 (quay.io/tkaovila/kubevirt-datamover-controller:combined-197-199-test) paired with the plugin's PR#50 image.

Confirmed all three links of the chain, not just "no error":

1. Plugin sets the selector: restored PVC sat Pending with spec.selector.matchLabels = {"velero.io/dynamic-pv-restore": "<operationID-derived value>"}.
2. Controller reconciles the PV: once the DataDownload reached Completed, the bound PV carried that same velero.io/dynamic-pv-restore label (alongside velero.io/datadownload-uid).
3. Rebind completed: target PVC transitioned Pending -> Bound; the Restore CR showed restoreItemOperationsAttempted=2, Completed=2.

Unrelated findings from that same run (VM didn't fully reach Ready) are out of scope here: a KubeVirt admission-webhook rejection of reserved kubevirt.io/ labels on direct VMI restore (known non-fatal Velero+KubeVirt behavior), and a libvirt SyncVMI/CBT permission-denied error unrelated to PVC rebinding.

Note

Responses generated with Claude

[kaovilai]

Closing the loop on your third pointer (whether BindTargetExisting's existing-PVC pre-checks need updating for a deliberately-unbound-with-selector PVC): verified, no change needed. handleAccepted's conflict check only fires on targetPVC.Spec.VolumeName != "" || targetPVC.Status.Phase == corev1.ClaimBound. A selector-bearing-but-not-yet-labeled PVC stays Pending with an empty VolumeName by construction -- nothing can bind to it until this controller labels a PV with the selector's unique value -- so it never trips that check as-is.

Confirmed two ways:

• TestHandleAcceptedDataDownload_MatchLabelsSelectorSucceeds fixtures exactly this shape (target PVC with a matchLabels selector, unbound) and reaches Prepared.
• The live e2e run (see the comment above) observed the restored PVC sitting Pending with the selector set, proceeding normally through the phase machine to Completed.

Your other two pointers (plugin setting selector+empty volumeName ahead of creation; pv_rebind.go stamping the label alongside the claimRef patch) are both implemented as described and live-validated -- see the comment above and the companion migtools/kubevirt-datamover-plugin#50.

Note

Responses generated with Claude

[kaovilai]

/cherry-pick oadp-1.6

[openshift-cherrypick-robot]

@kaovilai: new pull request created: #204

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.