Skip to content

VMOwnedVolume- CnsRegisterVolume deferFcdRegistration implementation#4103

Open
deepakkinni wants to merge 9 commits into
kubernetes-sigs:masterfrom
deepakkinni:topic/dk016388/vmown-vm-import-v1
Open

VMOwnedVolume- CnsRegisterVolume deferFcdRegistration implementation#4103
deepakkinni wants to merge 9 commits into
kubernetes-sigs:masterfrom
deepakkinni:topic/dk016388/vmown-vm-import-v1

Conversation

@deepakkinni

@deepakkinni deepakkinni commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

What this PR does / why we need it:

Implements CSI-driver processing of CnsRegisterVolume CRs with
deferFcdRegistration=true (VGL-62908 step I.5 — VM disk import, no-FCD path).

When vm-operator imports a VM that already has plain VMDKs attached, it backfills
one PVC per disk and creates a CnsRegisterVolume with deferFcdRegistration=true.
This PR adds the controller logic to handle that CR without touching vCenter:

  1. Reads the pre-created PVC; derives volumeID = string(pvc.UID).
  2. Creates a PV with volumeHandle = volumeID, pre-bound to the PVC.
  3. Waits for PV↔PVC binding.
  4. Creates a CsiVolumeInfo directly in VMManaged state (finalizer + PV ownerRef
    • spec.vms entry + disk metadata). No FCD, no CNS DB row, no vCenter API call.
  5. Marks the CnsRegisterVolume as registered=true with volumeID and pvName
    set in status.

The FCD is registered lazily at detach time by the existing CVI reconciler's
register path (Workflow B, already landed).

Changes:

  • cnsregistervolume_types.go — new spec fields: deferFcdRegistration, vmName,
    vmInstanceUUID, diskUUID; new status fields: volumeID, pvName.
  • cnsregistervolume_crd.yaml — new spec/status properties; broaden diskURLPath
    pattern to also accept the datastore-path form [ds] dir/file.vmdk.
  • cnsregistervolume_controller.go — wire CsiVolumeInfoService; add FSS-gated
    early branch before GetVirtualCenterInstance.
  • cnsregistervolume_import.go (new) — reconcileDeferFcdRegistration + helpers.
  • cnsregistervolume_import_test.go (new) — unit tests (6 cases).

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Testing done:

Unit tests added in cnsregistervolume_import_test.go (6 cases):

  • Happy path: PV created with correct volumeHandle, CVI in VMManaged/Succeeded
    with ImportSucceeded condition, CRV status registered=true with volumeID/pvName.
  • Idempotent re-run: no error when PV and CVI already exist.
  • PVC not found: instance error set, requeue, no PV or CVI created.
  • FSS VMOwnedVolumes disabled: instance error set, no import.
  • Disk metadata propagation: diskUUID and diskPath correctly set in CVI spec.
  • No-FCD assertion: volumeManager.CreateVolume is never called.

make check passes clean (staticcheck + golangci-lint zero issues).

Special notes for your reviewer:

  • The deferred-FCD path is gated behind the VMOwnedVolumes FSS; no behaviour
    change on clusters where that feature is off.
  • The diskURLPath CRD pattern change is a strict superset — all existing
    folder-URL CRVs (https://host/folder/...) still validate unchanged.
  • StorageClassName is read from *pvc.Spec.StorageClassName rather than derived
    from a storage-policy ID (which would require an FCD QueryVolumeByID). vm-operator
    populates it when creating the PVC, and PV↔PVC binding requires the SC to match,
    so this is both authoritative and the only available source in the no-FCD path.
  • zz_generated.deepcopy.go required no regeneration: all new fields are scalars
    and the existing *out = *in assignment covers them.

Release note:

Add CnsRegisterVolume deferred-FCD import support for VM-owned volumes.
When deferFcdRegistration=true, the CSI controller registers an imported VM disk
by creating a PV and CsiVolumeInfo keyed on the PVC UID, without FCD or vCenter
API calls. The FCD is created lazily at first detach.

@deepakkinni
Phase 0: govmomi wiring, CsiVolumeInfo CRD types, FSS constant, servi…
bfa4123 
…ce layer, RBAC

- Wire go.mod replace directive to deepakkinni/govmomi fork (UnregisterVolumeEx APIs)
- Add VMOwnedVolumes FSS constant to WCPFeatureStates and WCPFeatureStatesSupportsLateEnablement
- New pkg/apis/cnsoperator/csivolumeinfo/v1alpha1: CsiVolumeInfo types with new schema
  (vmware-system-csi namespace, cns-volume- prefix, spec.vms[], CSIManaged/VMManaged states)
- New csivolumeinfoservice.go: CRUD service layer with AddVolumeProtectionFinalizer/Remove
- New config/: CRD YAML manifest + embed wrapper
- Register CsiVolumeInfo types in pkg/apis/cnsoperator/register.go
- Add csivolumeinfos RBAC to controller and webhook roles in all 4 supervisor manifests
- Unit tests (6 passing): deepcopy, CRUD idempotency, finalizer add/remove, not-found
@deepakkinni
Phase 1: CNS unregister wrappers, CsiVolumeInfo provisioning, PV-bind…
bf2ca6e 
… reconciler

- Add UnregisterVolumeEx, QueryPendingUnregisters, AckUnregister to the
  Manager interface with defaultManager and MockManager implementations.
- Create CsiVolumeInfo CR at block-volume provisioning in wcp/controller.go
  when the VMOwnedVolumes capability is enabled; missing CR is a non-fatal
  warning so provisioning is never blocked.
- Add pkg/syncer/cvi_pv_bind.go: ReconcileCsiVolumeInfoOnPVBind patches the
  CsiVolumeInfo spec (pvName, pvcName, pvcNamespace) on PV Available→Bound.
- Unit tests: manager_unreg_test.go (nil-VC guards, mock success/error paths,
  type regression guard) and cvi_pv_bind_test.go (no-transition, non-vSphere,
  missing CR, stale pvName patch, up-to-date no-op, empty volumeHandle).
- Fix MockVolumeManager in unittestcommon/types.go and
  mockVolumeManagerForFullSync in fullsync_test.go to satisfy expanded interface.
@deepakkinni
Phase 1 fix: use PendingUnregisterRecord in QueryPendingUnregisters
711a4cf 
Replace raw []cnstypes.CnsUnregisterVolumeResult return type with the
[]PendingUnregisterRecord wrapper struct in the Manager interface,
defaultManager implementation, MockManager, MockVolumeManager, and
mockVolumeManagerForFullSync. Also add the PendingUnregisterRecord
struct definition before the Manager interface declaration.
@deepakkinni
Phase 2: Declarative CsiVolumeInfo reconciler (unregister/register/idle)
ed56993 
Add a new standalone controller that watches CsiVolumeInfo CRs and drives
the volume ownership lifecycle based on spec.vms and status.ownership.
Two-state machine: UnregisterVolumeEx on first-attach (CSIManaged→VMManaged),
CreateVolume re-register on last-detach (VMManaged→CSIManaged). Includes
backoff, fault surfacing, brownfield-lazy handling, and table-driven unit tests.
@deepakkinni
Phase 3: Startup recovery for in-flight two-phase unregister operations
d7a84a1 
Add RecoverPendingUnregisters to the csivolumeinfo service package. On
syncer startup (when VMOwnedVolumes is enabled), query CNS for any
PENDING_UNREGISTER rows left from a crash, write disk coordinates to the
CsiVolumeInfo spec, advance status to VMManaged, then acknowledge each
row. Double-ACK and orphan rows are handled safely. Per-record errors are
logged and do not abort recovery for subsequent records.
@deepakkinni
Phase 4: Cross-cutting guards — PVC-delete webhook and snapshot-creat…
89c61e2 
…e block

Add ValidatingAdmissionWebhook guard that rejects PVC deletion when the
volume is VM-managed (fail-open on CVI lookup errors). Add snapshot-create
rejection in CreateSnapshot when the volume is VM-managed. Both guards are
no-ops when VMOwnedVolumes FSS is disabled.
@deepakkinni
Phase 5: CVI deletion on FCD removal, integration hardening
8c0a951 
After DeleteVolume succeeds, explicitly delete the CsiVolumeInfo CR for
the volume so it does not become an orphan when the PV ownerReference GC
hasn't run yet. NotFound is treated as success (idempotent). This covers
the permanent-removal path; the normal GC cascade via PV ownerRef still
handles the typical case.
@deepakkinni
Bug fixes for VM-owned volume attach/detach (CsiVolumeInfo)
c251c50 
- Fix green-signal stall: the unregister reconcile wrote observedGeneration
  from the generation observed at reconcile entry, but its own diskPath/diskUUID
  spec write advances metadata.generation. PatchCsiVolumeInfo now returns the
  post-patch generation and the reconciler and startup recovery record that
  value, so observedGeneration >= generation can be satisfied.
- Treat CnsAlreadyRegisteredFault as success on re-register (add
  IsCnsAlreadyRegisteredFault helper; check it alongside CnsVolumeAlreadyExistsFault).
- Drop the backoff map entry when a CsiVolumeInfo is gone so deleted volumes
  do not leak entries.
- Recovery now writes the full status shape (cleared error + Ready condition),
  matching the steady-state unregister status.
- Retry CsiVolumeInfo patches only on conflict; surface other errors immediately.
- PVC-deletion webhook rejection message updated to name the PVC and the action
  to take.
- Remove the invalid integer print column over the spec.vms array.
- Move the pending-unregister recovery test alongside the recovery code.
- Add the new two-phase unregister methods to the remaining volume.Manager
  test mocks so all packages compile.
- Add PV OwnerRef on CsiVolumeInfo.
- Updated webhook

Signed-off-by: Deepak Kinni <deepak.kinni@broadcom.com>
@deepakkinni
VM import: CnsRegisterVolume deferred-FCD import support
86d4497 
Extend CnsRegisterVolume to support VM disk import without FCD registration.

New spec fields: deferFcdRegistration (bool), vmName, vmInstanceUUID, diskUUID.
New status fields: volumeID, pvName.

When deferFcdRegistration=true the controller skips all FCD/CNS operations and
instead derives the volume identity from the PVC UID, creates a PV pre-bound to
that PVC (volumeHandle=PVC UID), and creates a CsiVolumeInfo directly in
VMManaged state with the VM entry and disk metadata. The FCD is registered lazily
at detach time by the existing CsiVolumeInfo register path.

The CRD diskURLPath pattern is broadened to also accept the datastore path form
('[datastore] dir/file.vmdk') used by vm-operator for imported disks.

Includes unit tests covering: happy path, idempotency, missing PVC, FSS disabled,
disk metadata propagation, and no-FCD-call assertion.
@kubernetes-prow
kubernetes-prow Bot requested a review from akankshapanse June 24, 2026 06:48
@kubernetes-prow

kubernetes-prow Bot commented Jun 24, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deepakkinni

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:

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

@kubernetes-prow
kubernetes-prow Bot requested a review from kolluria June 24, 2026 06:48
@kubernetes-prow kubernetes-prow Bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 24, 2026
@kubernetes-prow

kubernetes-prow Bot commented Jun 26, 2026

Copy link
Copy Markdown

PR needs rebase.

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.

@kubernetes-prow kubernetes-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akankshapanse akankshapanse Awaiting requested review from akankshapanse
@kolluria kolluria Awaiting requested review from kolluria

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@deepakkinni