Repository: kubernetes-csi/external-snapshotter
Pull request #478 (2021-02-26 02:58:27)

Cherrypick #335 - "Call dynamic VS content creation unconditionally"
Irrespective of any error on the Volume Snapshot
object, initiate dynamic VolumeSnapshotContent object
creation. If any required parameters are not found, (e.g. missing
VS class), the VS content object creation would fail gracefully and retried

<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line:
>
> /kind api-change
> /kind bug
> /kind cleanup
> /kind design
> /kind documentation
/kind failing-test
> /kind feature
> /kind flake

**What this PR does / why we need it**:
Cherrypick #335  allows snapshot workflow to recover gracefully from missing snapshot class issues.
**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
Call dynamic VS content creation unconditionally
```



Patch:
diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go
index 8425b5414..85e5207cf 100644
--- a/pkg/common-controller/snapshot_controller.go
+++ b/pkg/common-controller/snapshot_controller.go
@@ -459,34 +459,34 @@ func (ctrl *csiSnapshotCommonController) syncUnreadySnapshot(snapshot *crdv1.Vol
 		}
 		klog.V(5).Infof("bindandUpdateVolumeSnapshot %v", newSnapshot)
 		return nil
-	} else if snapshot.Status == nil || snapshot.Status.Error == nil || isControllerUpdateFailError(snapshot.Status.Error) {
-		if snapshot.Spec.Source.PersistentVolumeClaimName == nil {
-			ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotPVCSourceMissing", fmt.Sprintf("PVC source for snapshot %s is missing", uniqueSnapshotName))
-			return fmt.Errorf("expected PVC source for snapshot %s but got nil", uniqueSnapshotName)
-		} else {
-			var err error
-			var content *crdv1.VolumeSnapshotContent
-			if content, err = ctrl.createSnapshotContent(snapshot); err != nil {
-				ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotContentCreationFailed", fmt.Sprintf("Failed to create snapshot content with error %v", err))
-				return err
-			}
+	}
 
-			// Update snapshot status with BoundVolumeSnapshotContentName
-			for i := 0; i < ctrl.createSnapshotContentRetryCount; i++ {
-				klog.V(5).Infof("syncUnreadySnapshot [%s]: trying to update snapshot status", utils.SnapshotKey(snapshot))
-				_, err = ctrl.updateSnapshotStatus(snapshot, content)
-				if err == nil {
-					break
-				}
-				klog.V(4).Infof("failed to update snapshot %s status: %v", utils.SnapshotKey(snapshot), err)
-				time.Sleep(ctrl.createSnapshotContentInterval)
-			}
+	if snapshot.Spec.Source.PersistentVolumeClaimName == nil {
+		ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotPVCSourceMissing", fmt.Sprintf("PVC source for snapshot %s is missing", uniqueSnapshotName))
+		return fmt.Errorf("expected PVC source for snapshot %s but got nil", uniqueSnapshotName)
+	} else {
+		var err error
+		var content *crdv1.VolumeSnapshotContent
+		if content, err = ctrl.createSnapshotContent(snapshot); err != nil {
+			ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotContentCreationFailed", fmt.Sprintf("Failed to create snapshot content with error %v", err))
+			return err
+		}
 
-			if err != nil {
-				// update snapshot status failed
-				ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotStatusUpdateFailed", fmt.Sprintf("Snapshot status update failed, %v", err))
-				return err
+		// Update snapshot status with BoundVolumeSnapshotContentName
+		for i := 0; i < ctrl.createSnapshotContentRetryCount; i++ {
+			klog.V(5).Infof("syncUnreadySnapshot [%s]: trying to update snapshot status", utils.SnapshotKey(snapshot))
+			_, err = ctrl.updateSnapshotStatus(snapshot, content)
+			if err == nil {
+				break
 			}
+			klog.V(4).Infof("failed to update snapshot %s status: %v", utils.SnapshotKey(snapshot), err)
+			time.Sleep(ctrl.createSnapshotContentInterval)
+		}
+
+		if err != nil {
+			// update snapshot status failed
+			ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotStatusUpdateFailed", fmt.Sprintf("Snapshot status update failed, %v", err))
+			return err
 		}
 	}
 	return nil


Test patch:
diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go
index 42a5b55dd..495af698b 100644
--- a/pkg/common-controller/framework_test.go
+++ b/pkg/common-controller/framework_test.go
@@ -1134,6 +1134,14 @@ func testUpdateSnapshotClass(ctrl *csiSnapshotCommonController, reactor *snapsho
 	return err
 }
 
+func testNewSnapshotContentCreation(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
+	if err := ctrl.syncUnreadySnapshot(test.initialSnapshots[0]); err != nil {
+		return fmt.Errorf("syncUnreadySnapshot failed: %v", err)
+	}
+
+	return nil
+}
+
 var (
 	classEmpty         string
 	classGold          = "gold"
diff --git a/pkg/common-controller/snapshot_update_test.go b/pkg/common-controller/snapshot_update_test.go
index 64295e771..03b2ef662 100644
--- a/pkg/common-controller/snapshot_update_test.go
+++ b/pkg/common-controller/snapshot_update_test.go
@@ -459,6 +459,45 @@ func TestSync(t *testing.T) {
 			expectSuccess:     false,
 			test:              testSyncContentError,
 		},
+		{
+			name:              "8-1 - Snapshot status nil, no initial snapshot content, new content should be created",
+			initialContents:   nocontents,
+			expectedContents:  withContentAnnotations(newContentArrayNoStatus("snapcontent-snapuid8-1", "snapuid8-1", "snap8-1", "sid8-1", validSecretClass, "", "pv-handle8-1", deletionPolicy, nil, nil, false, false), map[string]string{utils.AnnDeletionSecretRefName: "secret", utils.AnnDeletionSecretRefNamespace: "default"}),
+			initialSnapshots:  newSnapshotArray("snap8-1", "snapuid8-1", "claim8-1", "", validSecretClass, "", nil, nil, nil, nil, true, false, nil),
+			expectedSnapshots: newSnapshotArray("snap8-1", "snapuid8-1", "claim8-1", "", validSecretClass, "snapcontent-snapuid8-1", &False, nil, nil, nil, false, false, nil),
+			initialClaims:     newClaimArray("claim8-1", "pvc-uid8-1", "1Gi", "volume8-1", v1.ClaimBound, &classEmpty),
+			initialVolumes:    newVolumeArray("volume8-1", "pv-uid8-1", "pv-handle8-1", "1Gi", "pvc-uid8-1", "claim8-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			initialSecrets:    []*v1.Secret{secret()},
+			errors:            noerrors,
+			expectSuccess:     true,
+			test:              testNewSnapshotContentCreation,
+		},
+		{
+			name:              "8-2 - Snapshot status with nil error, no initial snapshot content, new content should be created",
+			initialContents:   nocontents,
+			expectedContents:  withContentAnnotations(newContentArrayNoStatus("snapcontent-snapuid8-2", "snapuid8-2", "snap8-2", "sid8-2", validSecretClass, "", "pv-handle8-2", deletionPolicy, nil, nil, false, false), map[string]string{utils.AnnDeletionSecretRefName: "secret", utils.AnnDeletionSecretRefNamespace: "default"}),
+			initialSnapshots:  newSnapshotArray("snap8-2", "snapuid8-2", "claim8-2", "", validSecretClass, "", nil, nil, nil, nil, false, false, nil),
+			expectedSnapshots: newSnapshotArray("snap8-2", "snapuid8-2", "claim8-2", "", validSecretClass, "snapcontent-snapuid8-2", &False, nil, nil, nil, false, false, nil),
+			initialClaims:     newClaimArray("claim8-2", "pvc-uid8-2", "1Gi", "volume8-2", v1.ClaimBound, &classEmpty),
+			initialVolumes:    newVolumeArray("volume8-2", "pv-uid8-2", "pv-handle8-2", "1Gi", "pvc-uid8-2", "claim8-2", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			initialSecrets:    []*v1.Secret{secret()},
+			errors:            noerrors,
+			expectSuccess:     true,
+			test:              testNewSnapshotContentCreation,
+		},
+		{
+			name:              "8-3 - Snapshot status with error, no initial content, new content created with no status, snapshot updates bound content and readytoUse=False state, error will not be cleared",
+			initialContents:   nocontents,
+			expectedContents:  withContentAnnotations(newContentArrayNoStatus("snapcontent-snapuid8-3", "snapuid8-3", "snap8-3", "sid8-3", validSecretClass, "", "pv-handle8-3", deletionPolicy, nil, nil, false, false), map[string]string{utils.AnnDeletionSecretRefName: "secret", utils.AnnDeletionSecretRefNamespace: "default"}),
+			initialSnapshots:  newSnapshotArray("snap8-3", "snapuid8-3", "claim8-3", "", validSecretClass, "", nil, nil, nil, newVolumeError("Mock content error"), false, false, nil),
+			expectedSnapshots: newSnapshotArray("snap8-3", "snapuid8-3", "claim8-3", "", validSecretClass, "snapcontent-snapuid8-3", &False, nil, nil, newVolumeError("Mock content error"), false, false, nil),
+			initialClaims:     newClaimArray("claim8-3", "pvc-uid8-3", "1Gi", "volume8-3", v1.ClaimBound, &classEmpty),
+			initialVolumes:    newVolumeArray("volume8-3", "pv-uid8-3", "pv-handle8-3", "1Gi", "pvc-uid8-3", "claim8-3", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			initialSecrets:    []*v1.Secret{secret()},
+			errors:            noerrors,
+			expectSuccess:     true,
+			test:              testNewSnapshotContentCreation,
+		},
 	}
 
 	runSyncTests(t, tests, snapshotClasses)