Merge pull request kubernetes-csi#17 from huffmanca/BZ1808123

Bug 1808123: UPSTREAM 275: Allows VolumeSnapshot to be deleted if the class isn't found

Author: openshift-merge-robot

pkg/common-controller/snapshot_controller.go

@@ -23,7 +23,7 @@ import (
 
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1beta1"
 	"github.com/kubernetes-csi/external-snapshotter/pkg/utils"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	storagev1 "k8s.io/api/storage/v1"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -1092,7 +1092,7 @@ func (ctrl *csiSnapshotCommonController) getSnapshotClass(className string) (*cr
 	class, err := ctrl.classLister.Get(className)
 	if err != nil {
 		klog.Errorf("failed to retrieve snapshot class %s from the informer: %q", className, err)
-		return nil, fmt.Errorf("failed to retrieve snapshot class %s from the informer: %q", className, err)
+		return nil, err
 	}
 
 	return class, nil

pkg/common-controller/snapshot_controller_base.go

@@ -26,7 +26,7 @@ import (
 	storagelisters "github.com/kubernetes-csi/external-snapshotter/pkg/client/listers/volumesnapshot/v1beta1"
 	"github.com/kubernetes-csi/external-snapshotter/pkg/utils"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -215,8 +215,11 @@ func (ctrl *csiSnapshotCommonController) snapshotWorker() {
 			// The volume snapshot still exists in informer cache, the event must have
 			// been add/update/sync
 			newSnapshot, err := ctrl.checkAndUpdateSnapshotClass(snapshot)
-			if err == nil {
-				klog.V(5).Infof("passed checkAndUpdateSnapshotClass for snapshot %q", key)
+			if err == nil || (newSnapshot.ObjectMeta.DeletionTimestamp != nil && errors.IsNotFound(err)) {
+				// If the VolumeSnapshotClass is not found, we still need to process an update
+				// so that syncSnapshot can delete the snapshot, should it still exist in the
+				// cluster after it's been removed from the informer cache
+				klog.V(5).Infof("updating snapshot %q; snapshotClass may have already been removed", key)
 				ctrl.updateSnapshot(newSnapshot)
 			}
 			return false
@@ -243,7 +246,10 @@ func (ctrl *csiSnapshotCommonController) snapshotWorker() {
 			return false
 		}
 		newSnapshot, err := ctrl.checkAndUpdateSnapshotClass(snapshot)
-		if err == nil {
+		if err == nil || errors.IsNotFound(err) {
+			// We should still handle deletion events even if the VolumeSnapshotClass
+			// is not found in the cluster
+			klog.V(5).Infof("deleting snapshot %q; snapshotClass may have already been removed", key)
 			ctrl.deleteSnapshot(newSnapshot)
 		}
 		return false
@@ -329,7 +335,8 @@ func (ctrl *csiSnapshotCommonController) checkAndUpdateSnapshotClass(snapshot *c
 		if err != nil {
 			klog.Errorf("checkAndUpdateSnapshotClass failed to getSnapshotClass %v", err)
 			ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "GetSnapshotClassFailed", fmt.Sprintf("Failed to get snapshot class with error %v", err))
-			return nil, err
+			// we need to return the original snapshot even if the class isn't found, as it may need to be deleted
+			return newSnapshot, err
 		}
 	} else {
 		klog.V(5).Infof("checkAndUpdateSnapshotClass [%s]: SetDefaultSnapshotClass", snapshot.Name)

pkg/common-controller/snapshot_create_test.go

@@ -94,7 +94,7 @@ func TestCreateSnapshotSync(t *testing.T) {
 			initialContents:   nocontents,
 			expectedContents:  nocontents,
 			initialSnapshots:  newSnapshotArray("snap7-1", "snapuid7-1", "claim7-1", "", classNonExisting, "", &False, nil, nil, nil, false, true, nil),
-			expectedSnapshots: newSnapshotArray("snap7-1", "snapuid7-1", "claim7-1", "", classNonExisting, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-1: \"failed to retrieve snapshot class non-existing from the informer: \\\"volumesnapshotclass.snapshot.storage.k8s.io \\\\\\\"non-existing\\\\\\\" not found\\\"\""), false, true, nil),
+			expectedSnapshots: newSnapshotArray("snap7-1", "snapuid7-1", "claim7-1", "", classNonExisting, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-1: \"volumesnapshotclass.snapshot.storage.k8s.io \\\"non-existing\\\" not found\""), false, true, nil),
 			initialClaims:     newClaimArray("claim7-1", "pvc-uid7-1", "1Gi", "volume7-1", v1.ClaimBound, &classEmpty),
 			initialVolumes:    newVolumeArray("volume7-1", "pv-uid7-1", "pv-handle7-1", "1Gi", "pvc-uid7-1", "claim7-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
 			expectedEvents:    []string{"Warning SnapshotContentCreationFailed"},

pkg/common-controller/snapshotclass_test.go

@@ -63,7 +63,7 @@ func TestUpdateSnapshotClass(t *testing.T) {
 			name:                  "1-3 - snapshot class name not found",
 			initialContents:       nocontents,
 			initialSnapshots:      newSnapshotArray("snap1-1", "snapuid1-1", "claim1-1", "content1-1", "missing-class", "content1-1", &True, nil, nil, nil, false, true, nil),
-			expectedSnapshots:     newSnapshotArray("snap1-1", "snapuid1-1", "claim1-1", "content1-1", "missing-class", "content1-1", &False, nil, nil, newVolumeError("Failed to get snapshot class with error failed to retrieve snapshot class missing-class from the informer: \"volumesnapshotclass.snapshot.storage.k8s.io \\\"missing-class\\\" not found\""), false, true, nil),
+			expectedSnapshots:     newSnapshotArray("snap1-1", "snapuid1-1", "claim1-1", "content1-1", "missing-class", "content1-1", &False, nil, nil, newVolumeError("Failed to get snapshot class with error volumesnapshotclass.snapshot.storage.k8s.io \"missing-class\" not found"), false, true, nil),
 			initialClaims:         newClaimArray("claim1-1", "pvc-uid1-1", "1Gi", "volume1-1", v1.ClaimBound, &sameDriver),
 			initialVolumes:        newVolumeArray("volume1-1", "pv-uid1-1", "pv-handle1-1", "1Gi", "pvc-uid1-1", "claim1-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
 			initialStorageClasses: []*storage.StorageClass{sameDriverStorageClass},