Merge pull request #381 from jsafrane/fix-nil-default-class

Fix panic when source PVC does not exist

pkg/common-controller/framework_test.go

@@ -163,6 +163,21 @@ type reactorError struct {
 	error    error
 }
 
+// testError is an error returned from a test that marks a test as failed even
+// though the test case itself expected a common error (such as API error)
+type testError string
+
+func (t testError) Error() string {
+	return string(t)
+}
+
+var _ error = testError("foo")
+
+func isTestError(err error) bool {
+	_, ok := err.(testError)
+	return ok
+}
+
 func withSnapshotFinalizers(snapshots []*crdv1.VolumeSnapshot, finalizers ...string) []*crdv1.VolumeSnapshot {
 	for i := range snapshots {
 		for _, f := range finalizers {
@@ -1159,7 +1174,11 @@ func testRemoveSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *sna
 }
 
 func testUpdateSnapshotClass(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
-	_, err := ctrl.checkAndUpdateSnapshotClass(test.initialSnapshots[0])
+	snap, err := ctrl.checkAndUpdateSnapshotClass(test.initialSnapshots[0])
+	// syncSnapshotByKey expects that checkAndUpdateSnapshotClass always returns a snapshot
+	if snap == nil {
+		return testError(fmt.Sprintf("checkAndUpdateSnapshotClass returned nil snapshot on error: %v", err))
+	}
 	return err
 }
 
@@ -1497,6 +1516,9 @@ func runUpdateSnapshotClassTests(t *testing.T, tests []controllerTest, snapshotC
 
 		// Run the tested functions
 		err = test.test(ctrl, reactor, test)
+		if err != nil && isTestError(err) {
+			t.Errorf("Test %q failed: %v", test.name, err)
+		}
 		if test.expectSuccess && err != nil {
 			t.Errorf("Test %q failed: %v", test.name, err)
 		}

pkg/common-controller/snapshot_controller_base.go

@@ -317,6 +317,8 @@ func (ctrl *csiSnapshotCommonController) syncContentByKey(key string) error {
 
 // checkAndUpdateSnapshotClass gets the VolumeSnapshotClass from VolumeSnapshot. If it is not set,
 // gets it from default VolumeSnapshotClass and sets it.
+// On error, it must return the original snapshot, not nil, because the caller syncContentByKey
+// needs to check snapshot's timestamp.
 func (ctrl *csiSnapshotCommonController) checkAndUpdateSnapshotClass(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshot, error) {
 	className := snapshot.Spec.VolumeSnapshotClassName
 	var class *crdv1.VolumeSnapshotClass
@@ -337,7 +339,7 @@ func (ctrl *csiSnapshotCommonController) checkAndUpdateSnapshotClass(snapshot *c
 		if err != nil {
 			klog.Errorf("checkAndUpdateSnapshotClass failed to setDefaultClass %v", err)
 			ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SetDefaultSnapshotClassFailed", fmt.Sprintf("Failed to set default snapshot class with error %v", err))
-			return nil, err
+			return snapshot, err
 		}
 	}
 

pkg/common-controller/snapshotclass_test.go

@@ -86,6 +86,19 @@ func TestUpdateSnapshotClass(t *testing.T) {
 			},
 			test: testUpdateSnapshotClass,
 		},
+		{
+			// PVC does not exist
+			name:                  "1-5 - snapshot update with default class name failed because PVC was not found",
+			initialContents:       nocontents,
+			initialSnapshots:      newSnapshotArray("snap1-5", "snapuid1-5", "claim1-5", "", "", "", &True, nil, nil, nil, false, true, nil),
+			expectedSnapshots:     newSnapshotArray("snap1-5", "snapuid1-5", "claim1-5", "", "", "", &False, nil, nil, newVolumeError("Failed to set default snapshot class with error failed to retrieve PVC claim1-5 from the lister: \"persistentvolumeclaim \\\"claim1-5\\\" not found\""), false, true, nil),
+			initialClaims:         nil,
+			initialVolumes:        nil,
+			initialStorageClasses: []*storage.StorageClass{},
+			expectedEvents:        []string{"Warning SetDefaultSnapshotClassFailed"},
+			errors:                noerrors,
+			test:                  testUpdateSnapshotClass,
+		},
 	}
 
 	runUpdateSnapshotClassTests(t, tests, snapshotClasses)