From 73697eba816f44c8207b9fe5bfe835bfc492bbc4 Mon Sep 17 00:00:00 2001 From: Saikat Roychowdhury Date: Wed, 22 Jul 2020 19:39:59 +0000 Subject: [PATCH] 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. --- pkg/common-controller/framework_test.go | 8 ++++ pkg/common-controller/snapshot_controller.go | 36 ++++++++-------- pkg/common-controller/snapshot_update_test.go | 42 +++++++++++++++++++ 3 files changed, 68 insertions(+), 18 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 254092aa..67696411 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -1145,6 +1145,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_controller.go b/pkg/common-controller/snapshot_controller.go index bbbc4bc2..ee05e061 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -453,25 +453,25 @@ 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) - } - 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 - klog.V(5).Infof("syncUnreadySnapshot [%s]: trying to update snapshot status", utils.SnapshotKey(snapshot)) - if _, err = ctrl.updateSnapshotStatus(snapshot, content); err != nil { - // update snapshot status failed - ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotStatusUpdateFailed", fmt.Sprintf("Snapshot status update failed, %v", err)) - return err - } + // If we reach here, it is a dynamically provisioned snapshot, and the volumeSnapshotContent object is not yet created. + 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) + } + 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 + klog.V(5).Infof("syncUnreadySnapshot [%s]: trying to update snapshot status", utils.SnapshotKey(snapshot)) + if _, err = ctrl.updateSnapshotStatus(snapshot, content); err != nil { + // update snapshot status failed + ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotStatusUpdateFailed", fmt.Sprintf("Snapshot status update failed, %v", err)) + return err } return nil } diff --git a/pkg/common-controller/snapshot_update_test.go b/pkg/common-controller/snapshot_update_test.go index 77fbf564..3aa8f2ee 100644 --- a/pkg/common-controller/snapshot_update_test.go +++ b/pkg/common-controller/snapshot_update_test.go @@ -510,6 +510,48 @@ func TestSync(t *testing.T) { expectSuccess: true, test: testUpdateSnapshotErrorStatus, }, + { + // Snapshot status nil, no initial content, new content should be created. + 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, + }, + { + // Snapshot status with nil error, no initial content, new content should be created. + 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, + }, + { + // Snapshot status with error, no initial content, new content should be created, snapshot error should be cleared. + name: "8-3 - Snapshot status with error, no initial content, new content should be created, snapshot error should 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, snapshotErr, false, false, nil), + expectedSnapshots: newSnapshotArray("snap8-3", "snapuid8-3", "claim8-3", "", validSecretClass, "snapcontent-snapuid8-3", &False, nil, nil, nil, 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)