[VMOwnedVolumes][Draft] Completion gaps- UnregisterVolumeEx and additional tests#4060
Conversation
Signed-off-by: Deepak Kinni <deepak.kinni@broadcom.com>
Signed-off-by: Deepak Kinni <deepak.kinni@broadcom.com>
…pshot Deletion, Revert Re-adoption Signed-off-by: Deepak Kinni <deepak.kinni@broadcom.com>
…ndling Signed-off-by: Deepak Kinni <deepak.kinni@broadcom.com>
Signed-off-by: Deepak Kinni <deepak.kinni@broadcom.com>
…ional tests Signed-off-by: Deepak Kinni <deepak.kinni@broadcom.com>
|
[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 |
Signed-off-by: Deepak Kinni <deepak.kinni@broadcom.com>
|
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. |
Signed-off-by: Deepak Kinni <deepak.kinni@broadcom.com>
|
@deepakkinni: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
What this PR does / why we need it:
Critical Functional Gap
The greenfield attach path uses
UnregisterVolume(old single-phase API) instead ofUnregisterVolumeEx(new two-phase API with PENDING_UNREGISTER + ACK). The spec (Section 7.3, A.3/A.4) requires the two-phase commit model where:CnsUnregisterVolumeExmarks the CNS DB row asPENDING_UNREGISTER, preservingbackingDiskPathanddiskUUIDin the row, and returns them to CSI.AckUnregister(volumeID)to delete thePENDING_UNREGISTERrow.PENDING_UNREGISTERrow survives, enabling crash recovery viaQueryPendingUnregisters.Currently in
vm_owned_attach.goline 85, the code callsvolumeManager.UnregisterVolume(ctx, volumeID, true)which:backingDiskPath/diskUUIDfrom the unregister result (returns only a fault type string)PENDING_UNREGISTERrow -- the CNS DB row is deleted immediatelyThe govmomi fork already has
UnregisterVolumeEx,AcknowledgeUnregister, andQueryPendingUnregistersclient methods. The CSI driver'sManagerinterface already hasQueryPendingUnregistersandAckUnregisterwrappers. What is missing is:UnregisterVolumeExmethod on theManagerinterface that returns(backingDiskPath, diskUUID, error)UnregisterVolumeTest Coverage Gaps
Multiple areas lack unit tests:
cvi_pv_bind.go) -- no tests for ownerRef patching, crash recovery, cross-namespace rebindMaybeLazilyCreateCVIend-to-end flow -- no testVMHasVCenterSnapshotshelper -- no testReregisterVolumeAsFCD-- no unit testsreconcileSnapshotDeletion/processDiskAfterSnapshotDeletion-- no unit testsGetRetainedDiskUUIDs/IsVolumeRetainedByVCenterSnapshot-- no unit testscheckCVISnapshotRetainedadmission webhook -- no dedicated named test casereconcileStaleCVIs-- limited test coverage (only noop paths tested)Minor Issues
9e2bb78eincludes an unrelated.cursor/rules/pith.mdcfile that should be excludedWhich issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Testing done:
A PR must be marked "[WIP]", if no test result is provided. A WIP PR won't be reviewed, nor merged.
The requester can determine a sufficient test, e.g. build for a cosmetic change, E2E test in a predeployed setup, etc.
For new features, new tests should be done, in addition to regression tests.
If jtest is used to trigger precheckin tests, paste the result after jtest completes and remove [WIP] in the PR subject.
The review cycle will start, only after "[WIP]" is removed from the PR subject.
Special notes for your reviewer:
Release note: