Skip to content

Commit 4afb6dc

Browse files
authored
Enhancement - Use CNS Unregister Volume API for volume unregistration (#3488)
Added UnregisterVolume method on Manager interface and it's implementations Updated CNSUnregisterVolume reconciler to invoke the above method for unregistration
1 parent 7878e6f commit 4afb6dc

File tree

7 files changed

+164
-12
lines changed

7 files changed

+164
-12
lines changed

pkg/common/cns-lib/volume/manager.go

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,9 @@ type Manager interface {
156156
// BatchAttachVolumes attaches multiple volumes to a virtual machine.
157157
BatchAttachVolumes(ctx context.Context,
158158
vm *cnsvsphere.VirtualMachine, batchAttachRequest []BatchAttachRequest) ([]BatchAttachResult, string, error)
159+
// UnregisterVolume unregisters a volume from CNS.
160+
// If unregisterDisk is true, it will also unregister the disk from FCD.
161+
UnregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) *Error
159162
}
160163

161164
// CnsVolumeInfo hold information related to volume created by CNS.
@@ -215,6 +218,22 @@ type ExpandVolumeExtraParams struct {
215218
IsPodVMOnStretchSupervisorFSSEnabled bool
216219
}
217220

221+
// Error is a custom error type that wraps an error and adds a transient flag.
222+
// This is used to indicate whether the error is transient or not, which can be
223+
// useful for retry logic in the context of volume operations.
224+
// More fields can be added to this struct in the future if needed.
225+
type Error struct {
226+
error
227+
IsTransient bool
228+
}
229+
230+
func newError(err error, isTransient bool) *Error {
231+
return &Error{
232+
error: err,
233+
IsTransient: isTransient,
234+
}
235+
}
236+
218237
var (
219238
// managerInstance is a Manager singleton.
220239
managerInstance *defaultManager
@@ -3615,3 +3634,103 @@ func (m *defaultManager) SetListViewNotReady(ctx context.Context) {
36153634
m.listViewIf.SetListViewNotReady(ctx)
36163635
}
36173636
}
3637+
3638+
// UnregisterVolume unregisters a volume from CNS.
3639+
// If unregisterDisk is true, it will also unregister the disk from FCD.
3640+
func (m *defaultManager) UnregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) *Error {
3641+
ctx, cancelFunc := ensureOperationContextHasATimeout(ctx)
3642+
defer cancelFunc()
3643+
start := time.Now()
3644+
e := m.unregisterVolume(ctx, volumeID, unregisterDisk)
3645+
if e != nil {
3646+
prometheus.CnsControlOpsHistVec.WithLabelValues(prometheus.PrometheusUnregisterVolumeOpType,
3647+
prometheus.PrometheusFailStatus).Observe(time.Since(start).Seconds())
3648+
return e
3649+
}
3650+
3651+
prometheus.CnsControlOpsHistVec.WithLabelValues(prometheus.PrometheusUnregisterVolumeOpType,
3652+
prometheus.PrometheusPassStatus).Observe(time.Since(start).Seconds())
3653+
return nil
3654+
}
3655+
3656+
func (m *defaultManager) unregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) *Error {
3657+
log := logger.GetLogger(ctx)
3658+
3659+
if m.virtualCenter == nil {
3660+
return newError(errors.New("invalid manager instance"), true)
3661+
}
3662+
3663+
err := m.virtualCenter.ConnectCns(ctx)
3664+
if err != nil {
3665+
log.Errorf("ConnectCns failed with err: %+v", err)
3666+
return newError(err, true)
3667+
}
3668+
3669+
targetVolumeType := "FCD"
3670+
if unregisterDisk {
3671+
targetVolumeType = "LEGACY_DISK"
3672+
}
3673+
spec := []cnstypes.CnsUnregisterVolumeSpec{
3674+
{
3675+
VolumeId: cnstypes.CnsVolumeId{Id: volumeID},
3676+
TargetVolumeType: targetVolumeType,
3677+
},
3678+
}
3679+
task, err := m.virtualCenter.CnsClient.UnregisterVolume(ctx, spec)
3680+
if err != nil {
3681+
log.Errorf("CNS UnregisterVolume failed from the vCenter %q with err: %v", m.virtualCenter.Config.Host, err)
3682+
return handleUnregisterError(ctx, err)
3683+
}
3684+
3685+
taskInfo, err := m.waitOnTask(ctx, task.Reference())
3686+
if err != nil {
3687+
log.Errorf("failed to get UnregisterVolume taskInfo from vCenter %q with err: %v",
3688+
m.virtualCenter.Config.Host, err)
3689+
// TODO: check if there could be non-transient errors here.
3690+
return newError(err, true)
3691+
}
3692+
3693+
if taskInfo == nil {
3694+
log.Errorf("failed to get UnregisterVolume taskInfo from vCenter %q",
3695+
m.virtualCenter.Config.Host)
3696+
return newError(errors.New("taskInfo is empty for UnregisterVolume task"), true)
3697+
}
3698+
3699+
log.Infof("UnregisterVolume: volumeID: %q, opId: %q", volumeID, taskInfo.ActivationId)
3700+
res, err := getTaskResultFromTaskInfo(ctx, taskInfo)
3701+
if err != nil {
3702+
log.Errorf("failed to get UnregisterVolume task result with error: %v", err)
3703+
return newError(err, true)
3704+
}
3705+
3706+
if res == nil {
3707+
log.Errorf("failed to get UnregisterVolume task result from vCenter %q. taskID: %q, opId: %q",
3708+
m.virtualCenter.Config.Host, taskInfo.Task.Value, taskInfo.ActivationId)
3709+
return newError(errors.New("task result is empty for UnregisterVolume task"), true)
3710+
}
3711+
3712+
volOpRes := res.GetCnsVolumeOperationResult()
3713+
if volOpRes.Fault != nil {
3714+
log.Errorf("failed to unregister volume: %q, fault: %q, opID: %q",
3715+
volumeID, ExtractFaultTypeFromVolumeResponseResult(ctx, volOpRes), taskInfo.ActivationId)
3716+
return newError(errors.New("observed a fault in UnregisterVolume result"), true)
3717+
}
3718+
3719+
log.Infof("UnregisterVolume: volume unregistered successfully. volumeID: %q, opId: %q",
3720+
volumeID, taskInfo.ActivationId)
3721+
return nil
3722+
}
3723+
3724+
func handleUnregisterError(ctx context.Context, err error) *Error {
3725+
faultType := ExtractFaultTypeFromErr(ctx, err)
3726+
e := Error{err, true}
3727+
if faultType == csifault.VimFaultNotFound ||
3728+
faultType == csifault.VimFaultInvalidState ||
3729+
faultType == csifault.VimFaultInvalidDatastore ||
3730+
faultType == csifault.VimFaultInvalidArgument {
3731+
// If the fault type is NotFound, InvalidState, InvalidDatastore, or InvalidArgument,
3732+
// retry won't help.
3733+
e.IsTransient = false
3734+
}
3735+
return &e
3736+
}

pkg/common/fault/constants.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,4 +78,20 @@ const (
7878
VimFaultInvalidHostState = VimFaultPrefix + "InvalidHostState"
7979
// VimFaultHostNotConnected is the fault returned from CNS when host is not connected.
8080
VimFaultHostNotConnected = VimFaultPrefix + "HostNotConnected"
81+
// VimFaultNotFound is the fault returned from CNS when the object is not found.
82+
VimFaultNotFound = VimFaultPrefix + "NotFound"
83+
// VimFaultInvalidState is the fault returned from CNS
84+
// when the operation is not valid in the current state of the object.
85+
VimFaultInvalidState = VimFaultPrefix + "InvalidState"
86+
// VimFaultInvalidDatastore is the fault returned from CNS
87+
// when the datastore is not valid for the operation being performed.
88+
VimFaultInvalidDatastore = VimFaultPrefix + "InvalidDatastore"
89+
// VimFaultTaskInProgress is the fault returned from CNS
90+
// when the task is already in progress for the object.
91+
VimFaultTaskInProgress = VimFaultPrefix + "TaskInProgress"
92+
// VimFaultInvalidArgument is the fault returned from CNS
93+
// when the argument provided is not valid for the operation being performed.
94+
VimFaultInvalidArgument = VimFaultPrefix + "InvalidArgument"
95+
// VimFaultCNSFault is the fault returned from CNS when a generic CNS fault occurs.
96+
VimFaultCNSFault = VimFaultPrefix + "CnsFault"
8197
)

pkg/common/prometheus/metrics.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,8 @@ const (
8888
PrometheusAccessibleVolumes = "accessible-volumes"
8989
// PrometheusInaccessibleVolumes represents inaccessible volumes.
9090
PrometheusInaccessibleVolumes = "inaccessible-volumes"
91+
// PrometheusUnregisterVolumeOpType represents UnregisterVolume operation.
92+
PrometheusUnregisterVolumeOpType = "unregister-volume"
9193

9294
// PrometheusPassStatus represents a successful API run.
9395
PrometheusPassStatus = "pass"

pkg/common/unittestcommon/types.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,12 @@ type MockVolumeManager struct {
106106
extraParams interface{}) (*cnsvolume.CnsVolumeInfo, string, error)
107107
}
108108

109+
func (m *MockVolumeManager) UnregisterVolume(ctx context.Context, volumeID string,
110+
unregisterDisk bool) *cnsvolume.Error {
111+
//TODO implement me
112+
return nil
113+
}
114+
109115
func (m *MockVolumeManager) CreateVolume(ctx context.Context, spec *cnstypes.CnsVolumeCreateSpec,
110116
extraParams interface{}) (*cnsvolume.CnsVolumeInfo, string, error) {
111117
if m.createVolumeFunc != nil {

pkg/csi/service/common/vsphereutil_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@ type mockVolumeManager struct {
2525
extraParams interface{}) (*cnsvolume.CnsVolumeInfo, string, error)
2626
}
2727

28+
func (m *mockVolumeManager) UnregisterVolume(ctx context.Context, volumeID string,
29+
unregisterDisk bool) *cnsvolume.Error {
30+
//TODO implement me
31+
return nil
32+
}
33+
2834
func (m *mockVolumeManager) CreateVolume(ctx context.Context, spec *cnstypes.CnsVolumeCreateSpec,
2935
extraParams interface{}) (*cnsvolume.CnsVolumeInfo, string, error) {
3036
if m.createVolumeFunc != nil {

pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,12 @@ type mockVolumeManager struct {
5656
ctxParams interface{}) (*cnsvolume.CnsVolumeInfo, string, error)
5757
}
5858

59+
func (m *mockVolumeManager) UnregisterVolume(ctx context.Context, volumeID string,
60+
unregisterDisk bool) *cnsvolume.Error {
61+
//TODO implement me
62+
return nil
63+
}
64+
5965
func (m *mockVolumeManager) AttachVolume(ctx context.Context, vm *cnsvsphere.VirtualMachine,
6066
volumeID string, checkNVMeController bool) (string, string, error) {
6167
//TODO implement me

pkg/syncer/cnsoperator/controller/cnsunregistervolume/cnsunregistervolume_controller.go

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@ import (
4545
apis "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator"
4646
cnsunregistervolumev1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsunregistervolume/v1alpha1"
4747
volumes "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume"
48-
cnsvsphere "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/vsphere"
4948
commonconfig "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/config"
5049
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/utils"
5150
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common"
@@ -213,10 +212,11 @@ func (r *ReconcileCnsUnregisterVolume) Reconcile(ctx context.Context,
213212
// 6. Set the CnsUnregisterVolumeStatus.Unregistered to true.
214213

215214
var pvName, pvcName, pvcNamespace string
215+
var pvcFound bool
216216
pvName, pvfound := commonco.ContainerOrchestratorUtility.GetPVNameFromCSIVolumeID(instance.Spec.VolumeID)
217217
if pvfound {
218218
log.Infof("found PV: %q for the volumd Id: %q", pvName, instance.Spec.VolumeID)
219-
pvcName, pvcNamespace, pvcFound := commonco.ContainerOrchestratorUtility.
219+
pvcName, pvcNamespace, pvcFound = commonco.ContainerOrchestratorUtility.
220220
GetPVCNameFromCSIVolumeID(instance.Spec.VolumeID)
221221
if pvcFound {
222222
log.Infof("found PVC: %q in the namespace:%q for the volumd Id: %q", pvcName, pvcNamespace,
@@ -304,16 +304,13 @@ func (r *ReconcileCnsUnregisterVolume) Reconcile(ctx context.Context,
304304
}
305305

306306
// Invoke CNS DeleteVolume API with deleteDisk flag set to false.
307-
_, err = r.volumeManager.DeleteVolume(ctx, instance.Spec.VolumeID, false)
308-
if err != nil {
309-
if cnsvsphere.IsNotFoundError(err) {
310-
log.Infof("VolumeID %q not found in CNS. It may have already been deleted."+
311-
"Marking the operation as success.", instance.Spec.VolumeID)
312-
} else {
313-
log.Errorf("Failed to delete volume %q in CNS with error %+v.",
314-
instance.Spec.VolumeID, err)
315-
return reconcile.Result{}, err
316-
}
307+
// TODO: Handle non-transient errors from CNS DeleteVolume API.
308+
e := r.volumeManager.UnregisterVolume(ctx, instance.Spec.VolumeID, false)
309+
if e != nil {
310+
msg := fmt.Sprintf("Failed to unregister volume %q with error: %+v", instance.Spec.VolumeID, e)
311+
log.Error(msg)
312+
setInstanceError(ctx, r, instance, msg)
313+
return reconcile.Result{RequeueAfter: timeout}, nil
317314
} else {
318315
log.Infof("Deleted CNS volume %q with deleteDisk set to false", instance.Spec.VolumeID)
319316
}

0 commit comments

Comments
 (0)