VMOwnedVolume- Attach Detach Implementation#4097
Open
deepakkinni wants to merge 8 commits into
Open
Conversation
kubernetes-prow
Bot
added
the
cncf-cla: no
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
size/XXL
deepakkinni
force-pushed
the
topic/dk016388/vmown-att-det-v1
branch
from
June 22, 2026 06:40
c6b35ac to
2d9e905
Compare
kubernetes-prow
Bot
added
cncf-cla: yes
…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
… 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.
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.
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.
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.
…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.
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
force-pushed
the
topic/dk016388/vmown-att-det-v1
branch
from
June 22, 2026 21:26
2d9e905 to
abfb049
Compare
kubernetes-prow
Bot
removed
the
needs-rebase
deepakkinni
force-pushed
the
topic/dk016388/vmown-att-det-v1
branch
5 times, most recently
from
June 23, 2026 15:52
8cfbeec to
01513b3
Compare
- 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
force-pushed
the
topic/dk016388/vmown-att-det-v1
branch
from
June 23, 2026 18:31
01513b3 to
c251c50
Compare
|
PR needs rebase. 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. |
kubernetes-prow
Bot
added
the
needs-rebase
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
1 participant
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.
Description
Implements the CSI (vSphere CSI driver / syncer) side of VM-owned volume attach/detach (VGL-62908) for greenfield VMs on supervisor clusters, behind the cluster-wide
VMOwnedVolumesfeature gate.Disk ownership is transferred based on attachment state: a detached volume is a CNS-managed FCD; once attached in dependent-persistent mode it becomes a plain VMDK owned by the VM (FCD unregistered, volume leaves CNS). CSI owns only the catalog side of that transfer — unregister the FCD on first attach, re-register on last detach — driven declaratively off a new per-volume CR,
CsiVolumeInfo(CVI). vm-operator owns all VM reconfiguration and snapshot correlation; the two never cross boundaries.What this adds
CsiVolumeInfoCRD (cns.vmware.com/v1alpha1, namespaced tovmware-system-csi, deterministic namecns-volume-<volumeID>, PV ownerRef, status subresource,cvishort name) + service layer (idempotent CRUD, finalizer add/remove, conflict-only patch retry) + RBAC.VMOwnedVolumesFSS gate — all new behavior is gated; FSS-off creates no CVIs and uses legacy paths.volume.Manager:UnregisterVolumeEx,QueryPendingUnregisters,AckUnregister(wired to the vetted govmomi fork pinned atbccc2167).len(spec.vms)>0 ∧ CSIManaged → Unregister;len(spec.vms)==0 ∧ VMManaged → Register; otherwise idle. Handles brownfield-lazy CVIs (absentstatus.ownershiptreated asCSIManaged), advancesobservedGeneration/phaseas the green signal, and surfaces CNS faults tostatus.Retainrebind), explicit CVI delete onDeleteVolume, and startupPENDING_UNREGISTERrecovery for crash-interrupted unregisters.VMManaged, andVolumeSnapshot-create rejection whileVMManaged.Bug fixes included
observedGenerationnow reflects the post-patch generation (the reconciler's ownspec.diskPath/diskUUIDwrite advancesmetadata.generation), so vm-operator'sobservedGeneration >= generationgate can be satisfied.CnsAlreadyRegisteredFault(andCnsVolumeAlreadyExistsFault) treated as success.volume.Managertest mocks for the new interface methods so all packages compile.Closes: #(issue-number)
How Has This Been Tested?
pkg/apis/cnsoperator/csivolumeinfo/...,pkg/syncer/cnsoperator/controller/csivolumeinfo/...,pkg/common/cns-lib/volume/...,pkg/syncer/admissionhandler/...,pkg/syncer/, andpkg/csi/service/wcp/...— table-driven over the reconcile state matrix (unregister/register/idle, brownfield-lazy, multi-VM RWM, fault surfacing,observedGenerationadvancement, startup recovery incl. double-ACK and orphan records).observedGenerationtracks the controller's own spec write (the generation-bump the API server applies but the fake client does not).go test ./...— all packages pass, no regressions.gofmt,go vet ./...,staticcheck, andgolangci-lintall clean.