From d4350943e2aa991bc4db32ca8d7957b4fe012219 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Thu, 14 Nov 2019 18:14:44 +0000 Subject: [PATCH 1/9] Update snapshot controller --- pkg/common-controller/snapshot_controller.go | 196 +++++++++++++------ pkg/utils/util.go | 8 +- 2 files changed, 137 insertions(+), 67 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 80a41914..24644c6f 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -85,12 +85,6 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh snapshotName := utils.SnapshotRefKey(&content.Spec.VolumeSnapshotRef) - if utils.NeedToAddContentFinalizer(content) { - // Content is not being deleted -> it should have the finalizer. - klog.V(5).Infof("syncContent: Add Finalizer for VolumeSnapshotContent[%s]", content.Name) - return ctrl.addContentFinalizer(content) - } - klog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotName) // The VolumeSnapshotContent is reserved for a VolumeSnapshot; // that VolumeSnapshot has not yet been bound to this VolumeSnapshotContent; the VolumeSnapshot sync will handle it. @@ -98,23 +92,23 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh klog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is pre-bound to VolumeSnapshot %s", content.Name, snapshotName) return nil } - // Get the VolumeSnapshot by _name_ + + if utils.NeedToAddContentFinalizer(content) { + // Content is not being deleted -> it should have the finalizer. + klog.V(5).Infof("syncContent: Add Finalizer for VolumeSnapshotContent[%s]", content.Name) + return ctrl.addContentFinalizer(content) + } + + // Check if snapshot exists in cache store + // If snapshotExists returns (nil, nil), it means snapshot not found + // and it may have already been deleted, and it will fall into the + // snapshot == nil case below var snapshot *crdv1.VolumeSnapshot - obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName) + snapshot, err := ctrl.snapshotExists(snapshotName) if err != nil { return err } - if !found { - klog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s not found", content.Name, snapshotName) - // Fall through with snapshot = nil - } else { - var ok bool - snapshot, ok = obj.(*crdv1.VolumeSnapshot) - if !ok { - return fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", content.Name, obj) - } - klog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s found", content.Name, snapshotName) - } + if snapshot != nil && snapshot.UID != content.Spec.VolumeSnapshotRef.UID { // The snapshot that the content was pointing to was deleted, and another // with the same name created. @@ -122,6 +116,7 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh // Treat the content as bound to a missing snapshot. snapshot = nil } else { + // TODO(xyang): Write a function to check if snapshot.Status is different from content.Status and add snapshot to queue if there is a difference and it is worth triggering an snapshot status update // Check if content status is set to true and update snapshot status if so if snapshot != nil && content.Status != nil && content.Status.ReadyToUse != nil && *content.Status.ReadyToUse == true { klog.V(4).Infof("synchronizing VolumeSnapshotContent for snapshot [%s]: update snapshot status to true if needed.", snapshotName) @@ -131,8 +126,8 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh } } - // Trigger content deletion if snapshot has deletion - // timestamp or snapshot does not exist any more + // Trigger content deletion if snapshot is nil or snapshot has + // deletion timestamp. // If snapshot has deletion timestamp and finalizers, set // AnnVolumeSnapshotBeingDeleted annotation on the content. // This may trigger the deletion of the content in the @@ -141,22 +136,9 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh // Snapshot won't be deleted until content is deleted // due to the finalizer if snapshot == nil || utils.IsSnapshotDeletionCandidate(snapshot) { - // Set AnnVolumeSnapshotBeingDeleted if it is not set yet - if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted) { - klog.V(5).Infof("syncContent: set annotation [%s] on content [%s].", utils.AnnVolumeSnapshotBeingDeleted, content.Name) - metav1.SetMetaDataAnnotation(&content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted, "yes") - - updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(content) - if err != nil { - return newControllerUpdateError(content.Name, err.Error()) - } - - _, err = ctrl.storeContentUpdate(updateContent) - if err != nil { - klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status: cannot update internal cache %v", content.Name, err) - return err - } - klog.V(5).Infof("syncContent: volume snapshot content %+v", content) + _, err = ctrl.setAnnVolumeSnapshotBeingDeleted(content) + if err != nil { + return err } } @@ -171,28 +153,31 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnapshot) error { klog.V(5).Infof("synchronizing VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) - err := ctrl.processFinalizersAndCheckandDeleteContent(snapshot) - if err != nil { - return err + if snapshot.ObjectMeta.DeletionTimestamp != nil { + err := ctrl.processIfDeletionTimestampSet(snapshot) + if err != nil { + return err + } + } else { + klog.V(5).Infof("syncSnapshot[%s]: check if we should add finalizers on snapshot", utils.SnapshotKey(snapshot)) + ctrl.checkandAddSnapshotFinalizers(snapshot) + // Need to build or update snapshot.Status in following cases: + // 1) snapshot.Status is nil + // 2) snapshot.Status.ReadyToUse is false + // 3) snapshot.Status.BoundVolumeSnapshotContentName is not set + if !utils.IsSnapshotReady(snapshot) || !utils.IsBoundVolumeSnapshotContentNameSet(snapshot) { + return ctrl.syncUnreadySnapshot(snapshot) + } + return ctrl.syncReadySnapshot(snapshot) } - - if !utils.IsSnapshotReady(snapshot) { - return ctrl.syncUnreadySnapshot(snapshot) - } - return ctrl.syncReadySnapshot(snapshot) + return nil } -// processFinalizersAndCheckandDeleteContent processes finalizers and deletes the content when appropriate -// It checks if contents exists, it checks if snapshot has bi-directional binding, it checks if -// finalizers should be added or removed, and it checks if content should be deleted and deletes it -// if needed. -func (ctrl *csiSnapshotCommonController) processFinalizersAndCheckandDeleteContent(snapshot *crdv1.VolumeSnapshot) error { - klog.V(5).Infof("processFinalizersAndCheckandDeleteContent VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) - +func (ctrl *csiSnapshotCommonController) checkContentAndBoundStatus(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshotContent, bool, bool, error) { // If content is deleted already, remove SnapshotBound finalizer content, err := ctrl.contentExists(snapshot) if err != nil { - return err + return nil, false, false, err } deleteContent := false // It is possible for contentExists to return nil, nil @@ -206,18 +191,29 @@ func (ctrl *csiSnapshotCommonController) processFinalizersAndCheckandDeleteConte if content != nil && utils.IsSnapshotBound(snapshot, content) { klog.Infof("syncSnapshot: VolumeSnapshot %s is bound to volumeSnapshotContent [%s]", snapshot.Name, content.Name) snapshotBound = true + + } + return content, deleteContent, snapshotBound, nil +} + +// processIfDeletionTimestampSet processes finalizers and deletes the content when appropriate +// It checks if contents exists, it checks if snapshot has bi-directional binding, it checks if +// finalizers should be removed, and it checks if content should be deleted and deletes it if needed. +func (ctrl *csiSnapshotCommonController) processIfDeletionTimestampSet(snapshot *crdv1.VolumeSnapshot) error { + klog.V(5).Infof("processIfDeletionTimestampSet VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) + + content, deleteContent, _, err := ctrl.checkContentAndBoundStatus(snapshot) + if err != nil { + return err } - klog.V(5).Infof("processFinalizersAndCheckandDeleteContent[%s]: delete snapshot content and remove finalizer from snapshot if needed", utils.SnapshotKey(snapshot)) + klog.V(5).Infof("processIfDeletionTimestampSet[%s]: delete snapshot content and remove finalizer from snapshot if needed", utils.SnapshotKey(snapshot)) err = ctrl.checkandRemoveSnapshotFinalizersAndCheckandDeleteContent(snapshot, content, deleteContent) if err != nil { return err } - klog.V(5).Infof("processFinalizersAndCheckandDeleteContent[%s]: check if we should add finalizers on snapshot", utils.SnapshotKey(snapshot)) - ctrl.checkandAddSnapshotFinalizers(snapshot, snapshotBound, deleteContent) - - klog.V(5).Infof("processFinalizersAndCheckandDeleteContent[%s]: check if we should remove finalizer on snapshot source and remove it if we can", utils.SnapshotKey(snapshot)) + klog.V(5).Infof("processIfDeletionTimestampSet[%s]: check if we should remove finalizer on snapshot source and remove it if we can", utils.SnapshotKey(snapshot)) // Check if we should remove finalizer on PVC and remove it if we can. errFinalizer := ctrl.checkandRemovePVCFinalizer(snapshot) @@ -231,7 +227,7 @@ func (ctrl *csiSnapshotCommonController) processFinalizersAndCheckandDeleteConte // checkandRemoveSnapshotFinalizersAndCheckandDeleteContent deletes the content and removes snapshot finalizers if needed func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndCheckandDeleteContent(snapshot *crdv1.VolumeSnapshot, content *crdv1.VolumeSnapshotContent, deleteContent bool) error { - klog.V(5).Infof("deleteContentAndSnapshotFinalizers VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) + klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) var err error // Check is snapshot deletionTimestamp is set and any finalizer is on @@ -241,7 +237,7 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec // Check if a volume is being created from snapshot. inUse := ctrl.isVolumeBeingCreatedFromSnapshot(snapshot) - klog.V(5).Infof("syncSnapshot[%s]: set DeletionTimeStamp on content.", utils.SnapshotKey(snapshot)) + klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: set DeletionTimeStamp on content.", utils.SnapshotKey(snapshot)) // If content exists, set DeletionTimeStamp on the content; // content won't be deleted immediately due to the finalizer if content != nil && deleteContent && !inUse { @@ -254,7 +250,7 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec } if !inUse || (content == nil && err == nil) { - klog.V(5).Infof("syncSnapshot: Remove Finalizer for VolumeSnapshot[%s]", utils.SnapshotKey(snapshot)) + klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent: Remove Finalizer for VolumeSnapshot[%s]", utils.SnapshotKey(snapshot)) doesContentExist := false if content != nil { doesContentExist = true @@ -266,7 +262,12 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec } // checkandAddSnapshotFinalizers checks and adds snapshot finailzers when needed -func (ctrl *csiSnapshotCommonController) checkandAddSnapshotFinalizers(snapshot *crdv1.VolumeSnapshot, snapshotBound bool, deleteContent bool) { +func (ctrl *csiSnapshotCommonController) checkandAddSnapshotFinalizers(snapshot *crdv1.VolumeSnapshot) error { + _, deleteContent, snapshotBound, err := ctrl.checkContentAndBoundStatus(snapshot) + if err != nil { + return err + } + addSourceFinalizer := false addBoundFinalizer := false if utils.NeedToAddSnapshotAsSourceFinalizer(snapshot) { @@ -281,10 +282,11 @@ func (ctrl *csiSnapshotCommonController) checkandAddSnapshotFinalizers(snapshot klog.V(5).Infof("checkandAddSnapshotFinalizers: Add Finalizer for VolumeSnapshot[%s]", utils.SnapshotKey(snapshot)) ctrl.addSnapshotFinalizer(snapshot, addSourceFinalizer, addBoundFinalizer) } + return nil } // syncReadySnapshot checks the snapshot which has been bound to snapshot content successfully before. -// If there is any problem with the binding (e.g., snapshot points to a non-exist snapshot content), update the snapshot status and emit event. +// If there is any problem with the binding (e.g., snapshot points to a non-existent snapshot content), update the snapshot status and emit event. func (ctrl *csiSnapshotCommonController) syncReadySnapshot(snapshot *crdv1.VolumeSnapshot) error { if !utils.IsBoundVolumeSnapshotContentNameSet(snapshot) { return nil @@ -987,11 +989,34 @@ func (ctrl *csiSnapshotCommonController) getVolumeFromVolumeSnapshot(snapshot *c return nil, fmt.Errorf("failed to retrieve PV %s from the API server: %q", pvName, err) } + // Verify binding between PV/PVC is still valid + bound := ctrl.isVolumeBoundToClaim(pv, pvc) + if bound == false { + klog.Warningf("binding between PV %s and PVC %s is broken", pvName, pvc.Name) + return nil, fmt.Errorf("claim in dataSource not bound or invalid") + } + klog.V(5).Infof("getVolumeFromVolumeSnapshot: snapshot [%s] PV name [%s]", snapshot.Name, pvName) return pv, nil } +// isVolumeBoundToClaim returns true, if given volume is pre-bound or bound +// to specific claim. Both claim.Name and claim.Namespace must be equal. +// If claim.UID is present in volume.Spec.ClaimRef, it must be equal too. +func (ctrl *csiSnapshotCommonController) isVolumeBoundToClaim(volume *v1.PersistentVolume, claim *v1.PersistentVolumeClaim) bool { + if volume.Spec.ClaimRef == nil { + return false + } + if claim.Name != volume.Spec.ClaimRef.Name || claim.Namespace != volume.Spec.ClaimRef.Namespace { + return false + } + if volume.Spec.ClaimRef.UID != "" && claim.UID != volume.Spec.ClaimRef.UID { + return false + } + return true +} + func (ctrl *csiSnapshotCommonController) getStorageClassFromVolumeSnapshot(snapshot *crdv1.VolumeSnapshot) (*storagev1.StorageClass, error) { // Get storage class from PVC or PV pvc, err := ctrl.getClaimFromVolumeSnapshot(snapshot) @@ -1196,3 +1221,48 @@ func (ctrl *csiSnapshotCommonController) contentExists(snapshot *crdv1.VolumeSna // Found in content cache store and convert object is successful return content, nil } + +func (ctrl *csiSnapshotCommonController) snapshotExists(snapshotName string) (*crdv1.VolumeSnapshot, error) { + // Get the VolumeSnapshot by _name_ + var snapshot *crdv1.VolumeSnapshot + obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName) + if err != nil { + return nil, err + } + if !found { + klog.V(4).Infof("snapshotExists: snapshot %s not found", snapshotName) + // Fall through with snapshot = nil + return nil, nil + } else { + var ok bool + snapshot, ok = obj.(*crdv1.VolumeSnapshot) + if !ok { + return nil, fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", snapshotName, obj) + } + klog.V(4).Infof("snapshotExists: snapshot %s found", snapshotName) + } + return snapshot, nil +} + +func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) { + // Set AnnVolumeSnapshotBeingDeleted if it is not set yet + if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted) { + klog.V(5).Infof("setAnnVolumeSnapshotBeingDeleted: set annotation [%s] on content [%s].", utils.AnnVolumeSnapshotBeingDeleted, content.Name) + metav1.SetMetaDataAnnotation(&content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted, "yes") + + updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(content) + if err != nil { + return content, newControllerUpdateError(content.Name, err.Error()) + } + // update content if update is successful + content = updateContent + + _, err = ctrl.storeContentUpdate(content) + if err != nil { + klog.V(4).Infof("setAnnVolumeSnapshotBeingDeleted for content [%s]: cannot update internal cache %v", content.Name, err) + return content, err + } + klog.V(5).Infof("setAnnVolumeSnapshotBeingDeleted: volume snapshot content %+v", content) + } + return content, nil +} diff --git a/pkg/utils/util.go b/pkg/utils/util.go index 404902fc..34d8f54c 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -319,23 +319,23 @@ func NoResyncPeriodFunc() time.Duration { return 0 } -// needToAddContentFinalizer checks if a Finalizer needs to be added for the volume snapshot content. +// 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) } -// isSnapshotDeletionCandidate checks if a volume snapshot deletionTimestamp +// IsSnapshotDeletionCandidate checks if a volume snapshot deletionTimestamp // is set and any finalizer is on the snapshot. func IsSnapshotDeletionCandidate(snapshot *crdv1.VolumeSnapshot) bool { return snapshot.ObjectMeta.DeletionTimestamp != nil && (slice.ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotAsSourceFinalizer, nil) || slice.ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer, nil)) } -// needToAddSnapshotAsSourceFinalizer checks if a Finalizer needs to be added for the volume snapshot as a source for PVC. +// NeedToAddSnapshotAsSourceFinalizer checks if a Finalizer needs to be added for the volume snapshot as a source for PVC. func NeedToAddSnapshotAsSourceFinalizer(snapshot *crdv1.VolumeSnapshot) bool { return snapshot.ObjectMeta.DeletionTimestamp == nil && !slice.ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotAsSourceFinalizer, nil) } -// needToAddSnapshotBoundFinalizer checks if a Finalizer needs to be added for the bound volume snapshot. +// NeedToAddSnapshotBoundFinalizer checks if a Finalizer needs to be added for the bound volume snapshot. func NeedToAddSnapshotBoundFinalizer(snapshot *crdv1.VolumeSnapshot) bool { return snapshot.ObjectMeta.DeletionTimestamp == nil && !slice.ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer, nil) && snapshot.Status != nil && snapshot.Status.BoundVolumeSnapshotContentName != nil } From f2814e5a18f801c7bbd801866d954750794750aa Mon Sep 17 00:00:00 2001 From: xing-yang Date: Tue, 17 Dec 2019 02:29:13 +0000 Subject: [PATCH 2/9] Address review comments --- pkg/common-controller/snapshot_controller.go | 33 ++++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 24644c6f..9a32f46f 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -100,11 +100,11 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh } // Check if snapshot exists in cache store - // If snapshotExists returns (nil, nil), it means snapshot not found + // If getSnapshotFromStore returns (nil, nil), it means snapshot not found // and it may have already been deleted, and it will fall into the // snapshot == nil case below var snapshot *crdv1.VolumeSnapshot - snapshot, err := ctrl.snapshotExists(snapshotName) + snapshot, err := ctrl.getSnapshotFromStore(snapshotName) if err != nil { return err } @@ -136,7 +136,7 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh // Snapshot won't be deleted until content is deleted // due to the finalizer if snapshot == nil || utils.IsSnapshotDeletionCandidate(snapshot) { - _, err = ctrl.setAnnVolumeSnapshotBeingDeleted(content) + err = ctrl.setAnnVolumeSnapshotBeingDeleted(content) if err != nil { return err } @@ -175,12 +175,12 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap func (ctrl *csiSnapshotCommonController) checkContentAndBoundStatus(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshotContent, bool, bool, error) { // If content is deleted already, remove SnapshotBound finalizer - content, err := ctrl.contentExists(snapshot) + content, err := ctrl.getContentFromStore(snapshot) if err != nil { return nil, false, false, err } deleteContent := false - // It is possible for contentExists to return nil, nil + // It is possible for getContentFromStore to return nil, nil if content != nil && content.Spec.DeletionPolicy == crdv1.VolumeSnapshotContentDelete { klog.V(5).Infof("processFinalizersAndCheckandDeleteContent: Content [%s] deletion policy [%s] is delete.", content.Name, content.Spec.DeletionPolicy) deleteContent = true @@ -202,6 +202,7 @@ func (ctrl *csiSnapshotCommonController) checkContentAndBoundStatus(snapshot *cr func (ctrl *csiSnapshotCommonController) processIfDeletionTimestampSet(snapshot *crdv1.VolumeSnapshot) error { klog.V(5).Infof("processIfDeletionTimestampSet VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) + // If content is really not found in the cache store, err is nil content, deleteContent, _, err := ctrl.checkContentAndBoundStatus(snapshot) if err != nil { return err @@ -1198,7 +1199,10 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1 return nil } -func (ctrl *csiSnapshotCommonController) contentExists(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshotContent, error) { +// getContentFromStore finds content from the cache store. +// If getContentFromStore returns (nil, nil), it means content not found +// and it may have already been deleted. +func (ctrl *csiSnapshotCommonController) getContentFromStore(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshotContent, error) { var contentName string if snapshot.Status != nil && snapshot.Status.BoundVolumeSnapshotContentName != nil { contentName = *snapshot.Status.BoundVolumeSnapshotContentName @@ -1222,7 +1226,10 @@ func (ctrl *csiSnapshotCommonController) contentExists(snapshot *crdv1.VolumeSna return content, nil } -func (ctrl *csiSnapshotCommonController) snapshotExists(snapshotName string) (*crdv1.VolumeSnapshot, error) { +// getSnapshotFromStore finds snapshot from the cache store. +// If getSnapshotFromStore returns (nil, nil), it means snapshot not found +// and it may have already been deleted. +func (ctrl *csiSnapshotCommonController) getSnapshotFromStore(snapshotName string) (*crdv1.VolumeSnapshot, error) { // Get the VolumeSnapshot by _name_ var snapshot *crdv1.VolumeSnapshot obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName) @@ -1230,7 +1237,7 @@ func (ctrl *csiSnapshotCommonController) snapshotExists(snapshotName string) (*c return nil, err } if !found { - klog.V(4).Infof("snapshotExists: snapshot %s not found", snapshotName) + klog.V(4).Infof("getSnapshotFromStore: snapshot %s not found", snapshotName) // Fall through with snapshot = nil return nil, nil } else { @@ -1239,12 +1246,12 @@ func (ctrl *csiSnapshotCommonController) snapshotExists(snapshotName string) (*c if !ok { return nil, fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", snapshotName, obj) } - klog.V(4).Infof("snapshotExists: snapshot %s found", snapshotName) + klog.V(4).Infof("getSnapshotFromStore: snapshot %s found", snapshotName) } return snapshot, nil } -func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) { +func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(content *crdv1.VolumeSnapshotContent) error { // Set AnnVolumeSnapshotBeingDeleted if it is not set yet if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted) { klog.V(5).Infof("setAnnVolumeSnapshotBeingDeleted: set annotation [%s] on content [%s].", utils.AnnVolumeSnapshotBeingDeleted, content.Name) @@ -1252,7 +1259,7 @@ func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(conten updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(content) if err != nil { - return content, newControllerUpdateError(content.Name, err.Error()) + return newControllerUpdateError(content.Name, err.Error()) } // update content if update is successful content = updateContent @@ -1260,9 +1267,9 @@ func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(conten _, err = ctrl.storeContentUpdate(content) if err != nil { klog.V(4).Infof("setAnnVolumeSnapshotBeingDeleted for content [%s]: cannot update internal cache %v", content.Name, err) - return content, err + return err } klog.V(5).Infof("setAnnVolumeSnapshotBeingDeleted: volume snapshot content %+v", content) } - return content, nil + return nil } From 0ac299cf77e4a054593dcc52f31997570e8d4540 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Tue, 17 Dec 2019 03:50:25 +0000 Subject: [PATCH 3/9] Add a function needsUpdateSnapshotStatus --- pkg/common-controller/snapshot_controller.go | 39 ++++++++++++++++++-- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 9a32f46f..c5069e33 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -116,9 +116,9 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh // Treat the content as bound to a missing snapshot. snapshot = nil } else { - // TODO(xyang): Write a function to check if snapshot.Status is different from content.Status and add snapshot to queue if there is a difference and it is worth triggering an snapshot status update - // Check if content status is set to true and update snapshot status if so - if snapshot != nil && content.Status != nil && content.Status.ReadyToUse != nil && *content.Status.ReadyToUse == true { + // Check if snapshot.Status is different from content.Status and add snapshot to queue + // if there is a difference and it is worth triggering an snapshot status update. + if snapshot != nil && ctrl.needsUpdateSnapshotStatus(snapshot, content) { klog.V(4).Infof("synchronizing VolumeSnapshotContent for snapshot [%s]: update snapshot status to true if needed.", snapshotName) // Manually trigger a snapshot status update to happen // right away so that it is in-sync with the content status @@ -898,6 +898,39 @@ func (ctrl *csiSnapshotCommonController) bindandUpdateVolumeSnapshot(snapshotCon return snapshotCopy, nil } +// needsUpdateSnapshotStatus compares snapshot status with the content status and decide +// if snapshot status needs to be updated based on content status +func (ctrl *csiSnapshotCommonController) needsUpdateSnapshotStatus(snapshot *crdv1.VolumeSnapshot, content *crdv1.VolumeSnapshotContent) bool { + klog.V(5).Infof("needsUpdateSnapshotStatus[%s]", utils.SnapshotKey(snapshot)) + + if snapshot.Status == nil && content.Status != nil { + return true + } + if snapshot.Status != nil && content.Status == nil { + return false + } + if snapshot.Status == nil && content.Status == nil { + return false + } + if snapshot.Status.BoundVolumeSnapshotContentName == nil { + return true + } + if snapshot.Status.CreationTime == nil && content.Status.CreationTime != nil { + return true + } + if snapshot.Status.ReadyToUse == nil && content.Status.ReadyToUse != nil { + return true + } + if snapshot.Status.ReadyToUse != nil && content.Status.ReadyToUse != nil && snapshot.Status.ReadyToUse != content.Status.ReadyToUse { + return true + } + if (snapshot.Status.RestoreSize == nil && content.Status.RestoreSize != nil) || (snapshot.Status.RestoreSize != nil && snapshot.Status.RestoreSize.IsZero() && content.Status.RestoreSize != nil && *content.Status.RestoreSize > 0) { + return true + } + + return false +} + // UpdateSnapshotStatus updates snapshot status based on content status func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.VolumeSnapshot, content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshot, error) { klog.V(5).Infof("updateSnapshotStatus[%s]", utils.SnapshotKey(snapshot)) From 62d9ab547e1a89d0ae01faf32e67ce170ee24951 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Thu, 19 Dec 2019 15:24:01 +0000 Subject: [PATCH 4/9] Address review comments --- pkg/common-controller/snapshot_controller.go | 2 +- pkg/utils/util.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index c5069e33..bc9518dd 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -290,7 +290,7 @@ func (ctrl *csiSnapshotCommonController) checkandAddSnapshotFinalizers(snapshot // If there is any problem with the binding (e.g., snapshot points to a non-existent snapshot content), update the snapshot status and emit event. func (ctrl *csiSnapshotCommonController) syncReadySnapshot(snapshot *crdv1.VolumeSnapshot) error { if !utils.IsBoundVolumeSnapshotContentNameSet(snapshot) { - return nil + return fmt.Errorf("snapshot %s is not bound to a content.", utils.SnapshotKey(snapshot)) } obj, found, err := ctrl.contentStore.GetByKey(*snapshot.Status.BoundVolumeSnapshotContentName) if err != nil { diff --git a/pkg/utils/util.go b/pkg/utils/util.go index 34d8f54c..78866481 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -337,7 +337,7 @@ func NeedToAddSnapshotAsSourceFinalizer(snapshot *crdv1.VolumeSnapshot) bool { // NeedToAddSnapshotBoundFinalizer checks if a Finalizer needs to be added for the bound volume snapshot. func NeedToAddSnapshotBoundFinalizer(snapshot *crdv1.VolumeSnapshot) bool { - return snapshot.ObjectMeta.DeletionTimestamp == nil && !slice.ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer, nil) && snapshot.Status != nil && snapshot.Status.BoundVolumeSnapshotContentName != nil + return snapshot.ObjectMeta.DeletionTimestamp == nil && !slice.ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer, nil) && IsBoundVolumeSnapshotContentNameSet(snapshot) } func deprecationWarning(deprecatedParam, newParam, removalVersion string) string { From bf48d62ab5c869f7ab23315e4730d8009fb4e414 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Thu, 19 Dec 2019 15:55:18 +0000 Subject: [PATCH 5/9] Don't trigger content deletion if snapshot is nil --- pkg/common-controller/snapshot_controller.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index bc9518dd..7e476d84 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -126,16 +126,24 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh } } - // Trigger content deletion if snapshot is nil or snapshot has - // deletion timestamp. + // NOTE(xyang): Do not trigger content deletion if + // snapshot is nil. This is to avoid data loss if + // the user copied the yaml files and expect it to work + // in a different setup. In this case snapshot is nil. + // If we trigger content deletion, it will delete + // physical snapshot resource on the storage system + // and result in data loss! + // + // Trigger content deletion if snapshot is not nil + // and snapshot has deletion timestamp. // If snapshot has deletion timestamp and finalizers, set // AnnVolumeSnapshotBeingDeleted annotation on the content. // This may trigger the deletion of the content in the // sidecar controller depending on the deletion policy // on the content. // Snapshot won't be deleted until content is deleted - // due to the finalizer - if snapshot == nil || utils.IsSnapshotDeletionCandidate(snapshot) { + // due to the finalizer. + if snapshot != nil && utils.IsSnapshotDeletionCandidate(snapshot) { err = ctrl.setAnnVolumeSnapshotBeingDeleted(content) if err != nil { return err From 29e69f4c67ba3b83f2a3d39ac08f24229f78344d Mon Sep 17 00:00:00 2001 From: xing-yang Date: Fri, 20 Dec 2019 04:06:29 +0000 Subject: [PATCH 6/9] Try to find content from store by key before searching thru the list --- pkg/common-controller/snapshot_controller.go | 32 +++++++------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 7e476d84..97a640ac 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -367,10 +367,19 @@ func (ctrl *csiSnapshotCommonController) syncUnreadySnapshot(snapshot *crdv1.Vol return nil } else { // snapshot.Spec.Source.VolumeSnapshotContentName == nil - dynamically creating snapshot klog.V(5).Infof("before getMatchSnapshotContent for snapshot %s", uniqueSnapshotName) - if contentObj := ctrl.getMatchSnapshotContent(snapshot); contentObj != nil { + var contentObj *crdv1.VolumeSnapshotContent + contentObj, _ = ctrl.getContentFromStore(snapshot) + // Ignore err from getContentFromStore. If content not found + // in the cache store by the content name, try to search it from + // the ctrl.contentStore.List() + if contentObj == nil { + contentObj = ctrl.getMatchSnapshotContent(snapshot) + } + + if contentObj != nil { klog.V(5).Infof("Found VolumeSnapshotContent object %s for snapshot %s", contentObj.Name, uniqueSnapshotName) if contentObj.Spec.Source.SnapshotHandle != nil { - ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotHandleNotFound", fmt.Sprintf("Snapshot handle not found in content %s", contentObj.Name)) + ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotHandleSet", fmt.Sprintf("Snapshot handle should not be set in content %s for dynamic provisioning", uniqueSnapshotName)) return fmt.Errorf("snapshotHandle should not be set in the content for dynamic provisioning for snapshot %s", uniqueSnapshotName) } newSnapshot, err := ctrl.bindandUpdateVolumeSnapshot(contentObj, snapshot) @@ -379,25 +388,6 @@ func (ctrl *csiSnapshotCommonController) syncUnreadySnapshot(snapshot *crdv1.Vol } klog.V(5).Infof("bindandUpdateVolumeSnapshot %v", newSnapshot) return nil - } else if snapshot.Status != nil && snapshot.Status.BoundVolumeSnapshotContentName != nil { - contentObj, found, err := ctrl.contentStore.GetByKey(*snapshot.Status.BoundVolumeSnapshotContentName) - if err != nil { - return err - } - if !found { - if snapshot.ObjectMeta.DeletionTimestamp == nil { - ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotContentNotFound", fmt.Sprintf("Content for snapshot %s not found, but deletion timestamp not set on snapshot", uniqueSnapshotName)) - return fmt.Errorf("content for snapshot %s not found without deletion timestamp on snapshot", uniqueSnapshotName) - } - // NOTE: this is not an error now because we delete content before the snapshot - klog.V(5).Infof("Content for snapshot %s not found. It may be already deleted as expected.", uniqueSnapshotName) - } else { - klog.V(5).Infof("converting content object for snapshot %s", uniqueSnapshotName) - _, ok := contentObj.(*crdv1.VolumeSnapshotContent) - if !ok { - return fmt.Errorf("expected volume snapshot content, got %+v", contentObj) - } - } } 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)) From 37d5705019dd948ea655c81cb6069ee6ad975ec2 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Mon, 23 Dec 2019 21:13:21 +0000 Subject: [PATCH 7/9] Address review comments --- pkg/common-controller/snapshot_controller.go | 26 ++++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 97a640ac..e345a903 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -144,10 +144,7 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh // Snapshot won't be deleted until content is deleted // due to the finalizer. if snapshot != nil && utils.IsSnapshotDeletionCandidate(snapshot) { - err = ctrl.setAnnVolumeSnapshotBeingDeleted(content) - if err != nil { - return err - } + return ctrl.setAnnVolumeSnapshotBeingDeleted(content) } return nil @@ -184,19 +181,19 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap func (ctrl *csiSnapshotCommonController) checkContentAndBoundStatus(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshotContent, bool, bool, error) { // If content is deleted already, remove SnapshotBound finalizer content, err := ctrl.getContentFromStore(snapshot) - if err != nil { + if err != nil || content == nil { return nil, false, false, err } deleteContent := false // It is possible for getContentFromStore to return nil, nil - if content != nil && content.Spec.DeletionPolicy == crdv1.VolumeSnapshotContentDelete { + if content.Spec.DeletionPolicy == crdv1.VolumeSnapshotContentDelete { klog.V(5).Infof("processFinalizersAndCheckandDeleteContent: Content [%s] deletion policy [%s] is delete.", content.Name, content.Spec.DeletionPolicy) deleteContent = true } snapshotBound := false // Check if the snapshot content is bound to the snapshot - if content != nil && utils.IsSnapshotBound(snapshot, content) { + if utils.IsSnapshotBound(snapshot, content) { klog.Infof("syncSnapshot: VolumeSnapshot %s is bound to volumeSnapshotContent [%s]", snapshot.Name, content.Name) snapshotBound = true @@ -244,7 +241,10 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec // Volume snapshot should be deleted. Check if it's used // and remove finalizer if it's not. // Check if a volume is being created from snapshot. - inUse := ctrl.isVolumeBeingCreatedFromSnapshot(snapshot) + inUse := false + if content != nil { + inUse = ctrl.isVolumeBeingCreatedFromSnapshot(snapshot) + } klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: set DeletionTimeStamp on content.", utils.SnapshotKey(snapshot)) // If content exists, set DeletionTimeStamp on the content; @@ -904,10 +904,7 @@ func (ctrl *csiSnapshotCommonController) needsUpdateSnapshotStatus(snapshot *crd if snapshot.Status == nil && content.Status != nil { return true } - if snapshot.Status != nil && content.Status == nil { - return false - } - if snapshot.Status == nil && content.Status == nil { + if content.Status == nil { return false } if snapshot.Status.BoundVolumeSnapshotContentName == nil { @@ -922,7 +919,10 @@ func (ctrl *csiSnapshotCommonController) needsUpdateSnapshotStatus(snapshot *crd if snapshot.Status.ReadyToUse != nil && content.Status.ReadyToUse != nil && snapshot.Status.ReadyToUse != content.Status.ReadyToUse { return true } - if (snapshot.Status.RestoreSize == nil && content.Status.RestoreSize != nil) || (snapshot.Status.RestoreSize != nil && snapshot.Status.RestoreSize.IsZero() && content.Status.RestoreSize != nil && *content.Status.RestoreSize > 0) { + if snapshot.Status.RestoreSize == nil && content.Status.RestoreSize != nil { + return true + } + if snapshot.Status.RestoreSize != nil && snapshot.Status.RestoreSize.IsZero() && content.Status.RestoreSize != nil && *content.Status.RestoreSize > 0 { return true } From 23bba14ef3ca20183878bf7f3a5b358581f05b42 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Mon, 23 Dec 2019 21:23:19 +0000 Subject: [PATCH 8/9] Address one more comment --- pkg/common-controller/snapshot_controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index e345a903..8ba9b51c 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -235,7 +235,6 @@ func (ctrl *csiSnapshotCommonController) processIfDeletionTimestampSet(snapshot func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndCheckandDeleteContent(snapshot *crdv1.VolumeSnapshot, content *crdv1.VolumeSnapshotContent, deleteContent bool) error { klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) - var err error // Check is snapshot deletionTimestamp is set and any finalizer is on if utils.IsSnapshotDeletionCandidate(snapshot) { // Volume snapshot should be deleted. Check if it's used @@ -258,7 +257,7 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec } } - if !inUse || (content == nil && err == nil) { + if !inUse { klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent: Remove Finalizer for VolumeSnapshot[%s]", utils.SnapshotKey(snapshot)) doesContentExist := false if content != nil { From 6b179a5066de65ed34e75e66765094f7cb4e074f Mon Sep 17 00:00:00 2001 From: xing-yang Date: Tue, 24 Dec 2019 03:37:17 +0000 Subject: [PATCH 9/9] Address review comments --- pkg/common-controller/snapshot_controller.go | 31 +++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 8ba9b51c..5f58350a 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -81,8 +81,6 @@ const controllerUpdateFailMsg = "snapshot controller failed to update" // syncContent deals with one key off the queue. It returns false when it's time to quit. func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapshotContent) error { - klog.V(5).Infof("synchronizing VolumeSnapshotContent[%s]", content.Name) - snapshotName := utils.SnapshotRefKey(&content.Spec.VolumeSnapshotRef) klog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotName) @@ -159,7 +157,7 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap klog.V(5).Infof("synchronizing VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) if snapshot.ObjectMeta.DeletionTimestamp != nil { - err := ctrl.processIfDeletionTimestampSet(snapshot) + err := ctrl.processSnapshotWithDeletionTimestamp(snapshot) if err != nil { return err } @@ -178,6 +176,10 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap return nil } +// checkContentAndBoundStatus is a helper function that checks the following: +// - It checks if content exists and returns the content object if it exists and nil otherwise. +// - It checks the deletionPolicy and returns true if policy is Delete and false otherwise. +// - It checks if snapshot and content are bound and returns true if bound and false otherwise. func (ctrl *csiSnapshotCommonController) checkContentAndBoundStatus(snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshotContent, bool, bool, error) { // If content is deleted already, remove SnapshotBound finalizer content, err := ctrl.getContentFromStore(snapshot) @@ -201,11 +203,12 @@ func (ctrl *csiSnapshotCommonController) checkContentAndBoundStatus(snapshot *cr return content, deleteContent, snapshotBound, nil } -// processIfDeletionTimestampSet processes finalizers and deletes the content when appropriate -// It checks if contents exists, it checks if snapshot has bi-directional binding, it checks if -// finalizers should be removed, and it checks if content should be deleted and deletes it if needed. -func (ctrl *csiSnapshotCommonController) processIfDeletionTimestampSet(snapshot *crdv1.VolumeSnapshot) error { - klog.V(5).Infof("processIfDeletionTimestampSet VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) +// processSnapshotWithDeletionTimestamp processes finalizers and deletes the content when appropriate. It has the following steps: +// 1. Call a helper function checkContentAndBoundStatus() to check content and bound status. +// 2. Call checkandRemoveSnapshotFinalizersAndCheckandDeleteContent() with information obtained from step 1. This function name is very long but the name suggests what it does. It determines whether to remove finalizers on snapshot and whether to delete content. +// 3. Call checkandRemovePVCFinalizer() to determine whether to remove the finalizer on PVC. +func (ctrl *csiSnapshotCommonController) processSnapshotWithDeletionTimestamp(snapshot *crdv1.VolumeSnapshot) error { + klog.V(5).Infof("processSnapshotWithDeletionTimestamp VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) // If content is really not found in the cache store, err is nil content, deleteContent, _, err := ctrl.checkContentAndBoundStatus(snapshot) @@ -213,13 +216,13 @@ func (ctrl *csiSnapshotCommonController) processIfDeletionTimestampSet(snapshot return err } - klog.V(5).Infof("processIfDeletionTimestampSet[%s]: delete snapshot content and remove finalizer from snapshot if needed", utils.SnapshotKey(snapshot)) + klog.V(5).Infof("processSnapshotWithDeletionTimestamp[%s]: delete snapshot content and remove finalizer from snapshot if needed", utils.SnapshotKey(snapshot)) err = ctrl.checkandRemoveSnapshotFinalizersAndCheckandDeleteContent(snapshot, content, deleteContent) if err != nil { return err } - klog.V(5).Infof("processIfDeletionTimestampSet[%s]: check if we should remove finalizer on snapshot source and remove it if we can", utils.SnapshotKey(snapshot)) + klog.V(5).Infof("processSnapshotWithDeletionTimestamp[%s]: check if we should remove finalizer on snapshot source and remove it if we can", utils.SnapshotKey(snapshot)) // Check if we should remove finalizer on PVC and remove it if we can. errFinalizer := ctrl.checkandRemovePVCFinalizer(snapshot) @@ -231,7 +234,7 @@ func (ctrl *csiSnapshotCommonController) processIfDeletionTimestampSet(snapshot return nil } -// checkandRemoveSnapshotFinalizersAndCheckandDeleteContent deletes the content and removes snapshot finalizers if needed +// checkandRemoveSnapshotFinalizersAndCheckandDeleteContent deletes the content and removes snapshot finalizers (VolumeSnapshotAsSourceFinalizer and VolumeSnapshotBoundFinalizer) if needed func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndCheckandDeleteContent(snapshot *crdv1.VolumeSnapshot, content *crdv1.VolumeSnapshotContent, deleteContent bool) error { klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot)) @@ -278,6 +281,12 @@ func (ctrl *csiSnapshotCommonController) checkandAddSnapshotFinalizers(snapshot addSourceFinalizer := false addBoundFinalizer := false + // NOTE: Source finalizer will be added to snapshot if + // DeletionTimeStamp is nil and it is not set yet. + // This is because the logic to check whether a PVC is being + // created from the snapshot is expensive so we only go thru + // it when we need to remove this finalizer and make sure + // it is removed when it is not needed any more. if utils.NeedToAddSnapshotAsSourceFinalizer(snapshot) { addSourceFinalizer = true }