From f2814e5a18f801c7bbd801866d954750794750aa Mon Sep 17 00:00:00 2001 From: xing-yang Date: Tue, 17 Dec 2019 02:29:13 +0000 Subject: [PATCH] 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 }