From b747de45c5b0d8eb8b41339f8688806ab48fe206 Mon Sep 17 00:00:00 2001 From: xiangqian Date: Fri, 13 Dec 2019 21:09:44 -0800 Subject: [PATCH] sidecar controller update, fix potential snapshot leaking resolve comments --- pkg/sidecar-controller/content_create_test.go | 7 +- pkg/sidecar-controller/framework_test.go | 4 - pkg/sidecar-controller/snapshot_controller.go | 132 +++++---- .../snapshot_delete_test.go | 271 ++++++------------ pkg/utils/util.go | 16 +- 5 files changed, 162 insertions(+), 268 deletions(-) diff --git a/pkg/sidecar-controller/content_create_test.go b/pkg/sidecar-controller/content_create_test.go index 435fa44fc..c2dd23139 100644 --- a/pkg/sidecar-controller/content_create_test.go +++ b/pkg/sidecar-controller/content_create_test.go @@ -25,12 +25,13 @@ func TestSyncContent(t *testing.T) { var tests []controllerTest tests = append(tests, controllerTest{ - name: "Basic content create ready to use", - initialContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "", retainPolicy, nil, &defaultSize, &False, true), - expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "", retainPolicy, nil, &defaultSize, &True, true), + name: "Basic content update ready to use", + initialContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true), + expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true), expectedEvents: noevents, expectedCreateCalls: []createCall{ { + volumeHandle: "volume-handle-1-1", snapshotName: "snapshot-snapuid1-1", driverName: mockDriverName, snapshotId: "snapuid1-1", diff --git a/pkg/sidecar-controller/framework_test.go b/pkg/sidecar-controller/framework_test.go index 73b8ea357..009fea6e8 100644 --- a/pkg/sidecar-controller/framework_test.go +++ b/pkg/sidecar-controller/framework_test.go @@ -553,10 +553,6 @@ func newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHa Spec: crdv1.VolumeSnapshotContentSpec{ Driver: mockDriverName, DeletionPolicy: deletionPolicy, - Source: crdv1.VolumeSnapshotContentSource{ - SnapshotHandle: &snapshotHandle, - VolumeHandle: &volumeHandle, - }, }, Status: &crdv1.VolumeSnapshotContentStatus{ CreationTime: creationTime, diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 54574a9bf..87c1d0ab8 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-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" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog" "k8s.io/kubernetes/pkg/util/goroutinemap" @@ -34,20 +34,20 @@ import ( // Design: // // This is the sidecar controller that is responsible for creating and deleting a -// snapshot on the storage infrastructure through a csi volume driver. It watches -// the VolumeSnapshotContent object which is either created/deleted by the +// snapshot on the storage system through a csi volume driver. It watches +// VolumeSnapshotContent objects which have been either created/deleted by the // common snapshot controller in the case of dynamic provisioning or by the admin // in the case of pre-provisioned snapshots. -// The snapshot creation through csi volume driver should return a snapshot after -// it is created successfully (however, the snapshot might not be ready to use yet if -// there is an uploading phase). The creationTime will be updated accordingly +// The snapshot creation through csi driver should return a snapshot after +// it is created successfully(however, the snapshot might not be ready to use yet +// if there is an uploading phase). The creationTime will be updated accordingly // on the status of VolumeSnapshotContent. // After that, the sidecar controller will keep checking the snapshot status // through csi snapshot calls. When the snapshot is ready to use, the sidecar // controller set the status "ReadyToUse" to true on the VolumeSnapshotContent object -// to indicate the snapshot is ready to use. If the creation failed for any reason, -// the Error status is set accordingly. +// to indicate the snapshot is ready to be used to restore a volume. +// If the creation failed for any reason, the Error status is set accordingly. const controllerUpdateFailMsg = "snapshot controller failed to update" @@ -57,73 +57,50 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps var err error if ctrl.shouldDelete(content) { - switch content.Spec.DeletionPolicy { - case crdv1.VolumeSnapshotContentRetain: - klog.V(4).Infof("VolumeSnapshotContent[%s]: policy is Retain. Keep physical snapshot and remove content finalizer", content.Name) - // It is a deletion candidate if DeletionTimestamp is not nil and - // VolumeSnapshotContentFinalizer is set. - if utils.IsContentDeletionCandidate(content) { - // Volume snapshot content is a deletion candidate. - // Remove the content finalizer. - klog.V(5).Infof("syncContent: Content [%s] is a deletion candidate. Remove finalizer.", content.Name) - return ctrl.removeContentFinalizer(content) - } - - case crdv1.VolumeSnapshotContentDelete: - klog.V(4).Infof("VolumeSnapshotContent[%s]: policy is Delete. Delete physical snapshot", content.Name) - err = ctrl.deleteCSISnapshot(content) - if err != nil { - return err - } - klog.V(5).Infof("syncContent: check if we should remove Finalizer for VolumeSnapshotContent[%s]", content.Name) - // It is a deletion candidate if DeletionTimestamp is not nil and - // VolumeSnapshotContentFinalizer is set. - if utils.IsContentDeletionCandidate(content) { - // Volume snapshot content is a deletion candidate. - // Remove the content finalizer. - klog.V(5).Infof("syncContent: Content [%s] is a deletion candidate. Remove finalizer.", content.Name) - return ctrl.removeContentFinalizer(content) - } - - default: - // Unknown VolumeSnapshotDeletionPolicy - ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotUnknownDeletionPolicy", "Volume Snapshot Content has unrecognized deletion policy") - } klog.V(4).Infof("VolumeSnapshotContent[%s]: the policy is %s", content.Name, content.Spec.DeletionPolicy) + if content.Spec.DeletionPolicy == crdv1.VolumeSnapshotContentDelete && + content.Status != nil && content.Status.SnapshotHandle != nil { + // issue a CSI deletion call if the snapshot has not been deleted yet from + // underlying storage system. Note that the deletion snapshot operation will + // update content SnapshotHandle to nil upon a successful deletion. At this + // point, the finalizer on content should NOT be removed to avoid leaking. + return ctrl.deleteCSISnapshot(content) + } else { + // otherwise, either the snapshot has been deleted from the underlying + // storage system, or the deletion policy is Retain, remove the finalizer + // if there is one so that API server could delete the object if there is + // no other finalizer. + return ctrl.removeContentFinalizer(content) + } } else { - var err error - klog.V(5).Infof("syncContent: Call CreateSnapshot for content %s", content.Name) if content.Spec.Source.VolumeHandle != nil && content.Status == nil { + klog.V(5).Infof("syncContent: Call CreateSnapshot for content %s", content.Name) if err = ctrl.createSnapshot(content); err != nil { ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot with error %v", err)) return err } } else { - if err = ctrl.checkandUpdateContentStatus(content); err != nil { ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotContentStatusUpdateFailed", fmt.Sprintf("Failed to update snapshot content status with error %v", err)) return err } } - - return nil } - return nil } // deleteCSISnapshot starts delete action. func (ctrl *csiSnapshotSideCarController) deleteCSISnapshot(content *crdv1.VolumeSnapshotContent) error { operationName := fmt.Sprintf("delete-%s", content.Name) - klog.V(5).Infof("Snapshotter is about to delete volume snapshot content and the operation named %s", operationName) + klog.V(5).Infof("schedule to delete snapshot, operation named %s", operationName) ctrl.scheduleOperation(operationName, func() error { return ctrl.deleteCSISnapshotOperation(content) }) return nil } -// scheduleOperation starts given asynchronous operation on given volume. It -// makes sure the operation is already not running. +// scheduleOperation starts given asynchronous operation on given snapshot. It +// makes sure there is no running operation with the same operationName func (ctrl *csiSnapshotSideCarController) scheduleOperation(operationName string, operation func() error) { klog.V(5).Infof("scheduleOperation[%s]", operationName) @@ -199,17 +176,17 @@ func (ctrl *csiSnapshotSideCarController) updateContentErrorStatusWithEvent(cont if content.Status != nil && content.Status.Error != nil && *content.Status.Error.Message == message { klog.V(4).Infof("updateContentStatusWithEvent[%s]: the same error %v is already set", content.Name, content.Status.Error) return nil - } else if content.Status == nil { - content.Status = &crdv1.VolumeSnapshotContentStatus{} } contentClone := content.DeepCopy() - statusError := &crdv1.VolumeSnapshotError{ + if contentClone.Status == nil { + contentClone.Status = &crdv1.VolumeSnapshotContentStatus{} + } + contentClone.Status.Error = &crdv1.VolumeSnapshotError{ Time: &metav1.Time{ Time: time.Now(), }, Message: &message, } - contentClone.Status.Error = statusError ready := false contentClone.Status.ReadyToUse = &ready newContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().UpdateStatus(contentClone) @@ -233,7 +210,7 @@ func (ctrl *csiSnapshotSideCarController) updateContentErrorStatusWithEvent(cont func (ctrl *csiSnapshotSideCarController) getCSISnapshotInput(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotClass, map[string]string, error) { className := content.Spec.VolumeSnapshotClassName - klog.V(5).Infof("getCSISnapshotInput for content [%s]: VolumeSnapshotClassName [%s]", content.Name, *className) + klog.V(5).Infof("getCSISnapshotInput for content [%s]", content.Name) var class *crdv1.VolumeSnapshotClass var err error if className != nil { @@ -358,6 +335,7 @@ func (ctrl *csiSnapshotSideCarController) deleteCSISnapshotOperation(content *cr _, snapshotterCredentials, err := ctrl.getCSISnapshotInput(content) if err != nil { + ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotDeleteError", "Failed to get snapshot class or credentials") return fmt.Errorf("failed to get input parameters to delete snapshot for content %s: %q", content.Name, err) } @@ -366,10 +344,42 @@ func (ctrl *csiSnapshotSideCarController) deleteCSISnapshotOperation(content *cr ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotDeleteError", "Failed to delete snapshot") return fmt.Errorf("failed to delete snapshot %#v, err: %v", content.Name, err) } - + // the snapshot has been deleted from the underlying storage system, update + // content status to remove snapshot handle etc. + newContent, err := ctrl.clearVolumeContentStatus(content.Name) + if err != nil { + ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotDeleteError", "Failed to clear content status") + return err + } + // update local cache + ctrl.updateContentInCacheStore(newContent) return nil } +// clearVolumeContentStatus resets all fields to nil related to a snapshot in +// content.Status. On success, the latest version of the content object will be +// returned. +func (ctrl *csiSnapshotSideCarController) clearVolumeContentStatus( + contentName string) (*crdv1.VolumeSnapshotContent, error) { + klog.V(5).Infof("cleanVolumeSnapshotStatus content [%s]", contentName) + // get the latest version from API server + content, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Get(contentName, metav1.GetOptions{}) + if err != nil { + return nil, fmt.Errorf("error get snapshot content %s from api server: %v", contentName, err) + } + if content.Status != nil { + content.Status.SnapshotHandle = nil + content.Status.ReadyToUse = nil + content.Status.CreationTime = nil + content.Status.RestoreSize = nil + } + newContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().UpdateStatus(content) + if err != nil { + return nil, newControllerUpdateError(contentName, err.Error()) + } + return newContent, nil +} + func (ctrl *csiSnapshotSideCarController) updateSnapshotContentStatus( content *crdv1.VolumeSnapshotContent, snapshotHandle string, @@ -497,8 +507,13 @@ func (ctrl *csiSnapshotSideCarController) GetCredentialsFromAnnotation(content * return snapshotterCredentials, nil } -// removeContentFinalizer removes a Finalizer for VolumeSnapshotContent. +// removeContentFinalizer removes the VolumeSnapshotContentFinalizer from a +// content if there exists one. func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.VolumeSnapshotContent) error { + if !slice.ContainsString(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer, nil) { + // the finalizer does not exit, return directly + return nil + } contentClone := content.DeepCopy() contentClone.ObjectMeta.Finalizers = slice.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer, nil) @@ -507,12 +522,11 @@ func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.V return newControllerUpdateError(content.Name, err.Error()) } + klog.V(5).Infof("Removed protection finalizer from volume snapshot content %s", content.Name) _, err = ctrl.storeContentUpdate(contentClone) if err != nil { klog.Errorf("failed to update content store %v", err) } - - klog.V(5).Infof("Removed protection finalizer from volume snapshot content %s", content.Name) return nil } @@ -524,7 +538,7 @@ func (ctrl *csiSnapshotSideCarController) shouldDelete(content *crdv1.VolumeSnap if content.ObjectMeta.DeletionTimestamp == nil { return false } - // 1) shouldDelete returns true if content is not bound + // 1) shouldDelete returns true if a content is not bound // (VolumeSnapshotRef.UID == "") for pre-provisioned snapshot if content.Spec.Source.SnapshotHandle != nil && content.Spec.VolumeSnapshotRef.UID == "" { return true diff --git a/pkg/sidecar-controller/snapshot_delete_test.go b/pkg/sidecar-controller/snapshot_delete_test.go index 593b71c2a..8a8bb607d 100644 --- a/pkg/sidecar-controller/snapshot_delete_test.go +++ b/pkg/sidecar-controller/snapshot_delete_test.go @@ -25,7 +25,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" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -140,7 +140,7 @@ func TestDeleteSync(t *testing.T) { { name: "1-1 - content non-nil DeletionTimestamp with delete policy will delete snapshot", initialContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1), expectedEvents: noevents, errors: noerrors, initialSecrets: []*v1.Secret{secret()}, @@ -183,9 +183,9 @@ func TestDeleteSync(t *testing.T) { test: testSyncContent, }, { - name: "1-3 - delete snapshot error should result in an event", + name: "1-3 - delete snapshot error should result in an event, bound finalizer should remain", initialContents: newContentArrayWithDeletionTimestamp("content1-3", "snapuid1-3", "snap1-3", "sid1-3", validSecretClass, "", "snap1-3-volumehandle", deletePolicy, nil, nil, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-3", "snapuid1-3", "snap1-3", "sid1-3", validSecretClass, "", "snap1-3-volumehandle", deletePolicy, nil, nil, false, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-3", "snapuid1-3", "snap1-3", "sid1-3", validSecretClass, "", "snap1-3-volumehandle", deletePolicy, nil, nil, true, &timeNowMetav1), errors: noerrors, expectedCreateCalls: []createCall{ { @@ -204,235 +204,124 @@ func TestDeleteSync(t *testing.T) { expectedListCalls: []listCall{{"sid1-3", true, time.Now(), 1, nil}}, test: testSyncContent, }, - /*{ - name: "1-4 - create snapshot error should result in an event", - initialContents: newContentArrayWithDeletionTimestamp("content1-4", "snapuid1-4", "snap1-4", "sid1-4", classGold, "", "snap1-4-volumehandle", deletePolicy, nil, nil, true, nil), - //expectedContents: newContentArrayWithDeletionTimestamp("content1-4", "snapuid1-4", "snap1-4", "sid1-4", classGold, "", "snap1-4-volumehandle", deletePolicy, nil, nil, true, nil), - errors: []reactorError{}, - expectedCreateCalls: []createCall{ - { - snapshotName: "snapshot-snapuid1-4", - volumeHandle: "snap1-4-volumehandle", - parameters: map[string]string{"param1": "value1"}, - err: fmt.Errorf("Create failed"), - }, - }, - //expectedDeleteCalls: []deleteCall{{"sid1-4", nil, nil}}, - expectedListCalls: []listCall{{"sid1-4", true, time.Now(), 1, nil}}, - expectedEvents: []string{"Warning SnapshotContentCheckandUpdateFailed Failed to check and update snapshot content: Create failed"}, - test: testSyncContent, - },*/ - /*{ - name: "2-1 - content with empty snapshot class will not be deleted if it is bound to a non-exist snapshot but it does not have a snapshot uid specified", - initialContents: newContentArray("content2-1", "", "snap2-1", "sid2-1", "", "", "", deletionPolicy, nil, nil, true), - expectedContents: newContentArray("content2-1", "", "snap2-1", "sid2-1", "", "", "", deletionPolicy, nil, nil, true), - expectedEvents: noevents, - errors: noerrors, - expectedDeleteCalls: []deleteCall{{"sid2-1", nil, nil}}, - test: testSyncContent, - },*/ - /*{ - name: "1-2 - successful delete with snapshot class that has empty secret parameter", - initialContents: newContentArray("content1-2", "sid1-2", "snap1-2", "sid1-2", emptySecretClass, "", "volumeHandle", deletionPolicy, nil, nil, true), - expectedContents: nocontents, - initialSnapshots: nosnapshots, - expectedSnapshots: nosnapshots, - initialSecrets: []*v1.Secret{emptySecret()}, - expectedEvents: noevents, - errors: noerrors, - expectedDeleteCalls: []deleteCall{{"sid1-2", map[string]string{}, nil}}, - test: testSyncContent, - },*/ - /*{ - name: "1-3 - content non-nil DeletionTimestamp with delete policy will delete snapshot", - initialContents: newContentArrayWithDeletionTimestamp("content1-3", "snapuid1-1", "snap1-1", "sid1-3", classGold, "", "snap1-3-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-3", "snapuid1-1", "snap1-1", "sid1-3", classGold, "", "snap1-3-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1), - initialSnapshots: newSnapshotArray("snap1-3", "snapuid1-3", "claim1-3", "", validSecretClass, "", &False, nil, nil, nil), - expectedSnapshots: newSnapshotArray("snap1-3", "snapuid1-3", "claim1-3", "", validSecretClass, "", &False, nil, nil, nil), - expectedEvents: noevents, - errors: noerrors, - initialSecrets: []*v1.Secret{secret()}, - expectedCreateCalls: []createCall{ - { - snapshotName: "snapshot-snapuid1-1", - volumeHandle: "snap1-1-volumehandle", - parameters: map[string]string{"param1": "value1"}, - driverName: mockDriverName, - size: defaultSize, - snapshotId: "snapuid1-1-deleted", - creationTime: timeNow, - readyToUse: true, - }, - }, - expectedListCalls: []listCall{{"sid1-1", true, time.Now(), 1, nil}}, - expectedDeleteCalls: []deleteCall{{"sid1-1", nil, nil}}, - test: testSyncContent, - },*/ - - /* name: "1-3 - successful delete with snapshot class that has valid secret parameter", - initialContents: newContentArray("content1-3", "sid1-3", "snap1-3", "sid1-3", validSecretClass, "", "snap1-3-volumehandle", deletionPolicy, nil, nil, true), - expectedContents: newContentArray("content1-3", "sid1-3", "snap1-3", "sid1-3", validSecretClass, "", "snap1-3-volumehandle", deletionPolicy, nil, nil, false), - initialSnapshots: newSnapshotArray("snapshot-snapuid1-3", "snapuid1-3", "claim1-3", "", validSecretClass, "", &False, nil, nil, nil), - expectedSnapshots: newSnapshotArray("snapshot-snapuid1-3", "snapuid1-3", "claim1-3", "", validSecretClass, "", &False, nil, nil, nil), - expectedEvents: noevents, - errors: noerrors, - initialSecrets: []*v1.Secret{secret()}, - expectedCreateCalls: []createCall{ - { - snapshotName: "snap1-3", - volumeHandle: "snap1-3-volumehandle", - parameters: map[string]string{"param1": "value1"}, - driverName: mockDriverName, - size: defaultSize, - snapshotId: "sid1-3", - creationTime: timeNow, - }, - }, - expectedListCalls: []listCall{{"sid1-3", true, time.Now(), 1, nil}}, - expectedDeleteCalls: []deleteCall{{"sid1-3", map[string]string{"param1": "value1"}, nil}}, - test: testSyncContent, - }, - */ { - name: "1-4 - fail delete with snapshot class that has invalid secret parameter", - initialContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-1-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1), + name: "1-4 - fail to delete with a snapshot class which has invalid secret parameter, bound finalizer should remain", + initialContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-4-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-4-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1), expectedEvents: noevents, errors: noerrors, test: testSyncContent, }, { - name: "1-5 - csi driver delete snapshot returns error", - initialContents: newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "", deletionPolicy, nil, &defaultSize, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "", deletionPolicy, nil, &defaultSize, false, &timeNowMetav1), + name: "1-5 - csi driver delete snapshot returns error, bound finalizer should remain", + initialContents: newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "snap1-5-volumehandle", deletionPolicy, nil, &defaultSize, true, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "snap1-5-volumehandle", deletionPolicy, nil, &defaultSize, true, &timeNowMetav1), expectedListCalls: []listCall{{"sid1-5", true, time.Now(), 1000, nil}}, expectedDeleteCalls: []deleteCall{{"sid1-5", nil, errors.New("mock csi driver delete error")}}, expectedEvents: []string{"Warning SnapshotDeleteError"}, errors: noerrors, test: testSyncContent, }, - /*{ - name: "1-6 - api server delete content returns error", - initialContents: newContentArray("content1-6", "sid1-6", "snap1-6", "sid1-6", classGold, "", "", deletionPolicy, nil, nil, true), - //expectedContents: newContentArray("content1-6", "sid1-6", "snap1-6", "sid1-6", classGold, "", "", deletionPolicy, nil, nil, true), - expectedDeleteCalls: []deleteCall{{"sid1-6", map[string]string{"foo": "bar"}, nil}}, - expectedListCalls: []listCall{{"sid1-6", false, time.Now(), 0, nil}}, - expectedEvents: []string{"Warning SnapshotContentObjectDeleteError"}, - errors: []reactorError{ - // Inject error to the first client.VolumesnapshotV1beta1().VolumeSnapshotContents().Delete call. - // All other calls will succeed. - {"delete", "volumesnapshotcontents", errors.New("mock delete error")}, - }, - test: testSyncContent, - },*/ - /* - { - // delete success - snapshot that the content was pointing to was deleted, and another - // with the same name created. - name: "1-7 - prebound content is deleted while the snapshot exists", - initialContents: newContentArray("content1-7", "sid1-7", "snap1-7", "sid1-7", emptySecretClass, "", "", deletionPolicy, nil, nil, true), - expectedContents: nocontents, - initialSecrets: []*v1.Secret{secret()}, - expectedDeleteCalls: []deleteCall{{"sid1-7", map[string]string{"foo": "bar"}, nil}}, - expectedEvents: noevents, - errors: noerrors, - test: testSyncContent, - },*/ { - // delete success(?) - content is deleted before doDelete() starts - name: "1-8 - content is deleted before deleting", - initialContents: newContentArray("content1-8", "sid1-8", "snap1-8", "sid1-8", classGold, "", "", deletionPolicy, nil, nil, true), + // delete success(?) - content is deleted before deleteSnapshot starts + name: "1-6 - content is deleted before deleting", + initialContents: newContentArray("content1-6", "sid1-6", "snap1-6", "sid1-6", classGold, "sid1-6", "", deletionPolicy, nil, nil, true), expectedContents: nocontents, - expectedListCalls: []listCall{{"sid1-8", false, time.Now(), 0, nil}}, - expectedDeleteCalls: []deleteCall{{"sid1-8", map[string]string{"foo": "bar"}, nil}}, + expectedListCalls: []listCall{{"sid1-6", false, time.Now(), 0, nil}}, + expectedDeleteCalls: []deleteCall{{"sid1-6", map[string]string{"foo": "bar"}, nil}}, expectedEvents: noevents, errors: noerrors, test: wrapTestWithInjectedOperation(testSyncContent, func(ctrl *csiSnapshotSideCarController, reactor *snapshotReactor) { - // Delete the volume before delete operation starts + // Delete the content before delete operation starts reactor.lock.Lock() - delete(reactor.contents, "content1-8") + delete(reactor.contents, "content1-6") reactor.lock.Unlock() }), }, - /*{ - name: "1-9 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is specified", - expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "snapuid1-9", "snap1-9", "sid1-9", classGold, "", "snap1-9-volumehandle", retainPolicy, nil, nil, false, &timeNowMetav1), - initialContents: newContentArrayWithDeletionTimestamp("content1-9", "snapuid1-9", "snap1-9", "sid1-9", classGold, "", "snap1-9-volumehandle", retainPolicy, nil, nil, false, &timeNowMetav1), - expectedEvents: noevents, - expectedListCalls: []listCall{{"sid1-9", true, time.Now(), 0, nil}}, - initialSecrets: []*v1.Secret{secret()}, - errors: noerrors, - test: testSyncContent, - },*/ - /* - { - name: "1-10 - will not delete content with retain policy set which is bound to a snapshot incorrectly", - initialContents: newContentArray("content1-10", "snapuid1-10-x", "snap1-10", "sid1-10", validSecretClass, "", "", retainPolicy, nil, nil, true), - expectedContents: newContentArray("content1-10", "snapuid1-10-x", "snap1-10", "sid1-10", validSecretClass, "", "", retainPolicy, nil, nil, true), - initialSnapshots: newSnapshotArray("snap1-10", "snapuid1-10", "claim1-10", "", validSecretClass, "content1-10", &False, nil, nil, nil), - expectedSnapshots: newSnapshotArray("snap1-10", "snapuid1-10", "claim1-10", "", validSecretClass, "content1-10", &False, nil, nil, nil), - expectedEvents: noevents, - initialSecrets: []*v1.Secret{secret()}, - errors: noerrors, - test: testSyncContent, - },*/ { - name: "1-11 - content will not be deleted if it is bound to a snapshot correctly, snapsht uid is not specified", - initialContents: newContentArrayWithReadyToUse("content1-11", "", "snap1-11", "sid1-11", validSecretClass, "", "", deletePolicy, nil, &defaultSize, &True, true), - expectedContents: newContentArrayWithReadyToUse("content1-11", "", "snap1-11", "sid1-11", validSecretClass, "", "", deletePolicy, nil, &defaultSize, &True, true), - expectedEvents: noevents, - expectedCreateCalls: []createCall{ - { - snapshotName: "snap1-11", - volumeHandle: "snap1-11", - parameters: map[string]string{"param1": "value1"}, - driverName: mockDriverName, - size: defaultSize, - snapshotId: "snapuid1-1-deleted", - creationTime: timeNow, - readyToUse: true, - }, - }, - expectedListCalls: []listCall{{"sid1-11", true, time.Now(), 1000, nil}}, + name: "1-7 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is not specified", + initialContents: newContentArrayWithReadyToUse("content1-7", "", "snap1-7", "sid1-7", validSecretClass, "sid1-7", "", deletePolicy, nil, &defaultSize, &True, true), + expectedContents: newContentArrayWithReadyToUse("content1-7", "", "snap1-7", "sid1-7", validSecretClass, "sid1-7", "", deletePolicy, nil, &defaultSize, &True, true), + expectedEvents: noevents, + expectedListCalls: []listCall{{"sid1-7", true, time.Now(), 1000, nil}}, initialSecrets: []*v1.Secret{secret()}, errors: noerrors, test: testSyncContent, }, { - name: "1-12 - content with retain policy will not be deleted if it is bound to a non-exist snapshot and also has a snapshot uid specified", - initialContents: newContentArrayWithReadyToUse("content1-12", "sid1-12", "snap1-11", "sid1-11", validSecretClass, "", "", retainPolicy, nil, &defaultSize, &True, true), - expectedContents: newContentArrayWithReadyToUse("content1-12", "sid1-12", "snap1-11", "sid1-11", validSecretClass, "", "", retainPolicy, nil, &defaultSize, &True, true), + name: "1-8 - content with retain policy will not be deleted if it is bound to a non-exist snapshot and also has a snapshot uid specified", + initialContents: newContentArrayWithReadyToUse("content1-8", "sid1-8", "none-existed-snapshot", "sid1-8", validSecretClass, "sid1-8", "", retainPolicy, nil, &defaultSize, &True, true), + expectedContents: newContentArrayWithReadyToUse("content1-8", "sid1-8", "none-existed-snapshot", "sid1-8", validSecretClass, "sid1-8", "", retainPolicy, nil, &defaultSize, &True, true), expectedEvents: noevents, - expectedListCalls: []listCall{{"sid1-11", true, time.Now(), 0, nil}}, + expectedListCalls: []listCall{{"sid1-8", true, time.Now(), 0, nil}}, errors: noerrors, test: testSyncContent, }, - /*{ - name: "1-13 - content with empty snapshot class is not deleted when Deletion policy is not set even if it is bound to a non-exist snapshot and also has a snapshot uid specified", - initialContents: newContentArray("content1-13", "sid1-13", "snap1-13", "sid1-13", validSecretClass, "", "", retainPolicy, nil, nil, true), - expectedContents: newContentArray("content1-13", "sid1-13", "snap1-13", "sid1-13", validSecretClass, "", "", retainPolicy, nil, nil, true), + { + name: "1-9 - continue deletion with snapshot class that has nonexistent secret, bound finalizer removed", + initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1), + expectedEvents: noevents, + expectedListCalls: []listCall{{"sid1-9", true, time.Now(), 0, nil}}, + errors: noerrors, + initialSecrets: []*v1.Secret{}, // secret does not exist + expectedDeleteCalls: []deleteCall{{"sid1-9", nil, nil}}, + test: testSyncContent, + }, + { + name: "1-10 - (dynamic)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer", + initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &timeNowMetav1), expectedEvents: noevents, + expectedListCalls: []listCall{{"sid1-10", true, time.Now(), 0, nil}}, errors: noerrors, + initialSecrets: []*v1.Secret{}, test: testSyncContent, }, { - name: "1-14 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is specified", - initialContents: newContentArray("content1-14", "snapuid1-14", "snap1-14", "sid1-14", validSecretClass, "", "", retainPolicy, nil, nil, true), - expectedContents: newContentArray("content1-14", "snapuid1-14", "snap1-14", "sid1-14", validSecretClass, "", "", retainPolicy, nil, nil, true), + name: "1-11 - (dynamic)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.", + initialContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "sid1-11", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &timeNowMetav1), + expectedEvents: noevents, + errors: noerrors, + expectedDeleteCalls: []deleteCall{{"sid1-11", nil, nil}}, + test: testSyncContent, + }, + { + name: "1-12 - (pre-provision)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer", + initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &timeNowMetav1), expectedEvents: noevents, - initialSecrets: []*v1.Secret{secret()}, + expectedListCalls: []listCall{{"sid1-12", true, time.Now(), 0, nil}}, errors: noerrors, + initialSecrets: []*v1.Secret{}, test: testSyncContent, - },*/ + }, { - name: "1-16 - continue delete with snapshot class that has nonexistent secret", - initialContents: newContentArrayWithDeletionTimestamp("content1-16", "sid1-16", "snap1-16", "sid1-16", emptySecretClass, "", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), - expectedContents: newContentArrayWithDeletionTimestamp("content1-16", "sid1-16", "snap1-16", "sid1-16", emptySecretClass, "", "", deletePolicy, nil, &defaultSize, false, &timeNowMetav1), + name: "1-13 - (pre-provision)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.", + initialContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "sid1-13", emptySecretClass, "sid1-13", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &timeNowMetav1), expectedEvents: noevents, - expectedListCalls: []listCall{{"sid1-16", true, time.Now(), 0, nil}}, errors: noerrors, - initialSecrets: []*v1.Secret{}, // secret does not exist - expectedDeleteCalls: []deleteCall{{"sid1-16", nil, nil}}, + expectedDeleteCalls: []deleteCall{{"sid1-13", nil, nil}}, + test: testSyncContent, + }, + { + name: "1-14 - (pre-provision)deletion of content with deletion policy and no snapshotclass should trigger CSI call, update status, and remove bound finalizer removed.", + initialContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "sid1-14", "", "sid1-14", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &timeNowMetav1), + expectedEvents: noevents, + errors: noerrors, + expectedDeleteCalls: []deleteCall{{"sid1-14", nil, nil}}, + test: testSyncContent, + }, + { + name: "1-15 - (dynamic)deletion of content with no snapshotclass should produce error", + initialContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), + expectedContents: newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1), + expectedEvents: []string{"Warning SnapshotDeleteError"}, + errors: noerrors, + expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}}, test: testSyncContent, }, } diff --git a/pkg/utils/util.go b/pkg/utils/util.go index b246efc4b..404902fcf 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -20,8 +20,12 @@ import ( "fmt" "strings" + "os" + "strconv" + "time" + crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1beta1" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -30,9 +34,6 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/klog" "k8s.io/kubernetes/pkg/util/slice" - "os" - "strconv" - "time" ) var ( @@ -318,13 +319,6 @@ func NoResyncPeriodFunc() time.Duration { return 0 } -// isContentDeletionCandidate checks if a volume snapshot content is a deletion candidate. -// It is a deletion candidate if DeletionTimestamp is not nil and -// VolumeSnapshotContentFinalizer is set. -func IsContentDeletionCandidate(content *crdv1.VolumeSnapshotContent) bool { - return content.ObjectMeta.DeletionTimestamp != nil && slice.ContainsString(content.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer, nil) -} - // needToAddContentFinalizer checks if a Finalizer needs to be added for the volume snapshot content. func NeedToAddContentFinalizer(content *crdv1.VolumeSnapshotContent) bool { return content.ObjectMeta.DeletionTimestamp == nil && !slice.ContainsString(content.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer, nil)