Merge pull request #1250 from sunpa93/unpublish-bug-fix

[V2] bug: UnpublishVolume should not delete the CRI without detaching
kubernetes-sigs · Mar 30, 2022 · 3f9c306 · 3f9c306
2 parents 1cef636 + f358c69
commit 3f9c306
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 15 deletions.
diff --git a/pkg/controller/common.go b/pkg/controller/common.go
@@ -1938,7 +1938,7 @@ func (c *SharedState) garbageCollectReplicas(ctx context.Context, volumeName str
 		volumeName,
 		replica,
 		func() error {
-			_, err := c.cleanUpAzVolumeAttachmentByVolume(context.Background(), volumeName, requester, all, detachAndDeleteCRI)
+			_, err := c.cleanUpAzVolumeAttachmentByVolume(context.Background(), volumeName, requester, replicaOnly, detachAndDeleteCRI)
 			if err != nil {
 				return err
 			}

diff --git a/pkg/provisioner/crdprovisioner.go b/pkg/provisioner/crdprovisioner.go
@@ -658,20 +658,30 @@ func (c *CrdProvisioner) detachVolume(ctx context.Context, azVAClient v1beta1.Az
 		return err
 	}
 
-	if azVolumeAttachment.Status.Detail != nil {
+	updateFunc := func(obj interface{}) error {
+		updateInstance := obj.(*diskv1beta1.AzVolumeAttachment)
+		if updateInstance.Annotations == nil {
+			updateInstance.Annotations = map[string]string{}
+		}
+		updateInstance.Annotations[consts.VolumeDetachRequestAnnotation] = "crdProvisioner"
+		return nil
+	}
+	switch azVolumeAttachment.Status.State {
+	case diskv1beta1.Detaching:
 		// if detachment is pending, return to prevent duplicate request
-		if azVolumeAttachment.Status.State == diskv1beta1.Detaching {
-			return status.Errorf(codes.Aborted, "detachment still in process for volume (%s) and node (%s)", volumeName, nodeName)
+		return status.Errorf(codes.Aborted, "detachment still in process for volume (%s) and node (%s)", volumeName, nodeName)
+	case diskv1beta1.Attaching:
+		// if attachment is still happening in the background async, wait until attachment is complete
+		klog.Infof("Attachment for volume (%s) to node (%s) is in process... Waiting for the attachment to complete.", volumeName, nodeName)
+		if azVolumeAttachment, err = c.WaitForAttach(ctx, volumeID, nodeName); err != nil {
+			return err
 		}
-
-		klog.Infof("Requesting AzVolumeAttachment (%s) deletion", attachmentName)
-
-		updateFunc := func(obj interface{}) error {
+	case diskv1beta1.DetachmentFailed:
+		innerFunc := updateFunc
+		// if detachment failed, reset error and state to retrigger operation
+		updateFunc = func(obj interface{}) error {
+			_ = innerFunc(obj)
 			updateInstance := obj.(*diskv1beta1.AzVolumeAttachment)
-			if updateInstance.Annotations == nil {
-				updateInstance.Annotations = map[string]string{}
-			}
-			updateInstance.Annotations[consts.VolumeDetachRequestAnnotation] = "crdProvisioner"
 
 			// remove detachment failure error from AzVolumeAttachment CRI to retrigger detachment
 			updateInstance.Status.Error = nil
@@ -680,10 +690,11 @@ func (c *CrdProvisioner) detachVolume(ctx context.Context, azVAClient v1beta1.Az
 
 			return nil
 		}
+	}
 
-		if err = azureutils.UpdateCRIWithRetry(ctx, c.conditionWatcher.informerFactory, nil, c.azDiskClient, azVolumeAttachment, updateFunc, consts.NormalUpdateMaxNetRetry); err != nil {
-			return err
-		}
+	klog.Infof("Requesting AzVolumeAttachment (%s) deletion", attachmentName)
+	if err = azureutils.UpdateCRIWithRetry(ctx, c.conditionWatcher.informerFactory, nil, c.azDiskClient, azVolumeAttachment, updateFunc, consts.NormalUpdateMaxNetRetry); err != nil {
+		return err
 	}
 
 	// only make delete request if deletionTimestamp is not set