Merge pull request #1303 from jsafrane/fix-finalizer-removal-requeue

Fix finalizer removal requeue

Author: k8s-ci-robot

pkg/sidecar-controller/snapshot_controller.go

@@ -67,22 +67,22 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
 			// Note that the deletion snapshot operation will update content SnapshotHandle
 			// to nil upon a successful deletion. At this
 			// point, the finalizer on content should NOT be removed to avoid leaking.
-			err := ctrl.deleteCSISnapshot(content)
+			var err error
+			content, err = ctrl.deleteCSISnapshot(content)
 			if err != nil {
 				return true, err
 			}
-			return false, nil
+			// Continue removing the finalizer
 		}
-		// otherwise, either the snapshot has been deleted from the underlying
-		// storage system, or it belongs to a volumegroupsnapshot, or the deletion policy is Retain,
+		// the snapshot has been deleted from the underlying storage system, or
+		// it belongs to a volumegroupsnapshot, or the deletion policy is Retain,
 		// remove the finalizer if there is one so that API server could delete
 		// the object if there is no other finalizer.
 		err := ctrl.removeContentFinalizer(content)
 		if err != nil {
 			return true, err
 		}
 		return false, nil
-
 	}
 
 	// Create snapshot calling the CSI driver only if it is a dynamic
@@ -109,7 +109,7 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
 }
 
 // deleteCSISnapshot starts delete action.
-func (ctrl *csiSnapshotSideCarController) deleteCSISnapshot(content *crdv1.VolumeSnapshotContent) error {
+func (ctrl *csiSnapshotSideCarController) deleteCSISnapshot(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) {
 	klog.V(5).Infof("Deleting snapshot for content: %s", content.Name)
 	return ctrl.deleteCSISnapshotOperation(content)
 }
@@ -406,31 +406,29 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V
 }
 
 // Delete a snapshot: Ask the backend to remove the snapshot device
-func (ctrl *csiSnapshotSideCarController) deleteCSISnapshotOperation(content *crdv1.VolumeSnapshotContent) error {
+func (ctrl *csiSnapshotSideCarController) deleteCSISnapshotOperation(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) {
 	klog.V(5).Infof("deleteCSISnapshotOperation [%s] started", content.Name)
 
 	snapshotterCredentials, err := ctrl.GetCredentialsFromAnnotation(content)
 	if err != nil {
 		ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotDeleteError", "Failed to get snapshot credentials")
-		return fmt.Errorf("failed to get input parameters to delete snapshot for content %s: %q", content.Name, err)
+		return content, fmt.Errorf("failed to get input parameters to delete snapshot for content %s: %q", content.Name, err)
 	}
 
 	err = ctrl.handler.DeleteSnapshot(content, snapshotterCredentials)
 	if err != nil {
 		ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotDeleteError", "Failed to delete snapshot")
-		return fmt.Errorf("failed to delete snapshot %#v, err: %v", content.Name, err)
+		return content, fmt.Errorf("failed to delete snapshot %#v, err: %v", content.Name, err)
 	}
 	// the snapshot has been deleted from the underlying storage system, update
 	// content status to remove snapshot handle etc.
+	// This triggers a re-sync of the content object, which will continue cleaning the object (e.g. finalizers)
 	newContent, err := ctrl.clearVolumeContentStatus(content.Name)
 	if err != nil {
 		ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotDeleteError", "Failed to clear content status")
-		return err
+		return content, err
 	}
-	// trigger syncContent
-	// TODO: just enqueue the content object instead of calling syncContent directly
-	ctrl.updateContentInInformerCache(newContent)
-	return nil
+	return newContent, nil
 }
 
 // clearVolumeContentStatus resets all fields to nil related to a snapshot in

pkg/sidecar-controller/snapshot_controller_base.go

@@ -260,11 +260,27 @@ func (ctrl *csiSnapshotSideCarController) syncContentByKey(key string) (requeue
 	// been add/update/sync
 	if err == nil {
 		if ctrl.isDriverMatch(content) {
-			requeue, err = ctrl.updateContentInInformerCache(content)
-		}
-		if err != nil {
-			// If error occurs we add this item back to the queue
-			return true, err
+			// Store the new content version in the cache and do not process it if this is
+			// an old version.
+			new, err := ctrl.storeContentUpdate(content)
+			if err != nil {
+				klog.Errorf("%v", err)
+			}
+			if !new {
+				return false, nil
+			}
+			requeue, err = ctrl.syncContent(content)
+			if err != nil {
+				if errors.IsConflict(err) {
+					// Version conflict error happens quite often and the controller
+					// recovers from it easily.
+					klog.V(3).Infof("could not sync content %q: %+v", content.Name, err)
+				} else {
+					klog.Errorf("could not sync content %q: %+v", content.Name, err)
+				}
+				// If error occurs we add this item back to the queue
+				return true, err
+			}
 		}
 		return requeue, nil
 	}
@@ -339,32 +355,6 @@ func (ctrl *csiSnapshotSideCarController) isDriverMatch(object interface{}) bool
 	return false
 }
 
-// updateContentInInformerCache runs in worker thread and handles "content added",
-// "content updated" and "periodic sync" events.
-func (ctrl *csiSnapshotSideCarController) updateContentInInformerCache(content *crdv1.VolumeSnapshotContent) (requeue bool, err error) {
-	// Store the new content version in the cache and do not process it if this is
-	// an old version.
-	new, err := ctrl.storeContentUpdate(content)
-	if err != nil {
-		klog.Errorf("%v", err)
-	}
-	if !new {
-		return false, nil
-	}
-	requeue, err = ctrl.syncContent(content)
-	if err != nil {
-		if errors.IsConflict(err) {
-			// Version conflict error happens quite often and the controller
-			// recovers from it easily.
-			klog.V(3).Infof("could not sync content %q: %+v", content.Name, err)
-		} else {
-			klog.Errorf("could not sync content %q: %+v", content.Name, err)
-		}
-		return requeue, err
-	}
-	return requeue, nil
-}
-
 // deleteContent runs in worker thread and handles "content deleted" event.
 func (ctrl *csiSnapshotSideCarController) deleteContentInCacheStore(content *crdv1.VolumeSnapshotContent) {
 	_ = ctrl.contentStore.Delete(content)