diff --git a/CHANGELOG/CHANGELOG-3.0.md b/CHANGELOG/CHANGELOG-3.0.md index 54469cacf..8d4e91bef 100644 --- a/CHANGELOG/CHANGELOG-3.0.md +++ b/CHANGELOG/CHANGELOG-3.0.md @@ -10,6 +10,8 @@ - Cherry-pick from https://github.com/kubernetes-csi/external-snapshotter/pull/413. Bug fix to allow creation of snapshot content if pvc finalizer exists, even if pvc is marked for deletion. ([#414](https://github.com/kubernetes-csi/external-snapshotter/pull/414), [@RaunakShah](https://github.com/RaunakShah)) +- Cherry-pick from https://github.com/kubernetes-csi/external-snapshotter/pull/381. Fixed crash of snapshot-controller when source PVC of a snapshot to take does not exist. ([#415](https://github.com/kubernetes-csi/external-snapshotter/pull/415), [@jsafrane](https://github.com/jsafrane)) + ## Dependencies ### Added diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 2a73fe60a..bc481c91f 100644 --- a/pkg/common-controller/framework_test.go +++ b/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) } diff --git a/pkg/common-controller/snapshot_controller_base.go b/pkg/common-controller/snapshot_controller_base.go index 0f30b8d48..2046b24c8 100644 --- a/pkg/common-controller/snapshot_controller_base.go +++ b/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 } } diff --git a/pkg/common-controller/snapshotclass_test.go b/pkg/common-controller/snapshotclass_test.go index be3ff812b..1930c6c84 100644 --- a/pkg/common-controller/snapshotclass_test.go +++ b/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)