diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 5ca56d7f..46ca0793 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -22,8 +22,6 @@ import ( "strings" "time" - crdv1 "github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1" - "github.com/kubernetes-csi/external-snapshotter/v2/pkg/utils" v1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" @@ -34,6 +32,9 @@ import ( ref "k8s.io/client-go/tools/reference" "k8s.io/klog" "k8s.io/kubernetes/pkg/util/slice" + + crdv1 "github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1" + "github.com/kubernetes-csi/external-snapshotter/v2/pkg/utils" ) // ================================================================== @@ -334,8 +335,8 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec func (ctrl *csiSnapshotCommonController) checkandAddSnapshotFinalizers(snapshot *crdv1.VolumeSnapshot) error { // get the content for this Snapshot var ( - content *crdv1.VolumeSnapshotContent = nil - err error = nil + content *crdv1.VolumeSnapshotContent + err error ) if snapshot.Spec.Source.VolumeSnapshotContentName != nil { content, err = ctrl.getPreprovisionedContentFromStore(snapshot) @@ -374,7 +375,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 fmt.Errorf("snapshot %s is not bound to a content.", utils.SnapshotKey(snapshot)) + return fmt.Errorf("snapshot %s is not bound to a content", utils.SnapshotKey(snapshot)) } content, err := ctrl.getContentFromStore(*snapshot.Status.BoundVolumeSnapshotContentName) if err != nil { @@ -464,30 +465,29 @@ func (ctrl *csiSnapshotCommonController) syncUnreadySnapshot(snapshot *crdv1.Vol if snapshot.Spec.Source.PersistentVolumeClaimName == nil { ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotPVCSourceMissing", fmt.Sprintf("PVC source for snapshot %s is missing", uniqueSnapshotName)) return fmt.Errorf("expected PVC source for snapshot %s but got nil", uniqueSnapshotName) - } else { - var err error - var content *crdv1.VolumeSnapshotContent - if content, err = ctrl.createSnapshotContent(snapshot); err != nil { - ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotContentCreationFailed", fmt.Sprintf("Failed to create snapshot content with error %v", err)) - return err - } + } + var err error + var content *crdv1.VolumeSnapshotContent + if content, err = ctrl.createSnapshotContent(snapshot); err != nil { + ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotContentCreationFailed", fmt.Sprintf("Failed to create snapshot content with error %v", err)) + return err + } - // Update snapshot status with BoundVolumeSnapshotContentName - for i := 0; i < ctrl.createSnapshotContentRetryCount; i++ { - klog.V(5).Infof("syncUnreadySnapshot [%s]: trying to update snapshot status", utils.SnapshotKey(snapshot)) - _, err = ctrl.updateSnapshotStatus(snapshot, content) - if err == nil { - break - } - klog.V(4).Infof("failed to update snapshot %s status: %v", utils.SnapshotKey(snapshot), err) - time.Sleep(ctrl.createSnapshotContentInterval) + // Update snapshot status with BoundVolumeSnapshotContentName + for i := 0; i < ctrl.createSnapshotContentRetryCount; i++ { + klog.V(5).Infof("syncUnreadySnapshot [%s]: trying to update snapshot status", utils.SnapshotKey(snapshot)) + _, err = ctrl.updateSnapshotStatus(snapshot, content) + if err == nil { + break } + klog.V(4).Infof("failed to update snapshot %s status: %v", utils.SnapshotKey(snapshot), err) + time.Sleep(ctrl.createSnapshotContentInterval) + } - if err != nil { - // update snapshot status failed - ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotStatusUpdateFailed", fmt.Sprintf("Snapshot status update failed, %v", err)) - return err - } + if err != nil { + // update snapshot status failed + ctrl.updateSnapshotErrorStatusWithEvent(snapshot, v1.EventTypeWarning, "SnapshotStatusUpdateFailed", fmt.Sprintf("Snapshot status update failed, %v", err)) + return err } } return nil @@ -1356,14 +1356,14 @@ func (ctrl *csiSnapshotCommonController) getSnapshotFromStore(snapshotName strin klog.V(4).Infof("getSnapshotFromStore: 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("getSnapshotFromStore: snapshot %s found", snapshotName) } + 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("getSnapshotFromStore: snapshot %s found", snapshotName) + return snapshot, nil } diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 61816abf..dac6792f 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -67,33 +67,33 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps // 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 { - if content.Spec.Source.VolumeHandle != nil && content.Status == nil { - klog.V(5).Infof("syncContent: Call CreateSnapshot for content %s", content.Name) - ctrl.createSnapshot(content) - } else { - // Skip checkandUpdateContentStatus() if ReadyToUse is - // already true. We don't want to keep calling CreateSnapshot - // or ListSnapshots CSI methods over and over again for - // performance reasons. - if content.Status != nil && content.Status.ReadyToUse != nil && *content.Status.ReadyToUse == true { - // Try to remove AnnVolumeSnapshotBeingCreated if it is not removed yet for some reason - err := ctrl.removeAnnVolumeSnapshotBeingCreated(content) - if err != nil { - return fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %q", content.Name, err) - } - return nil - } - ctrl.checkandUpdateContentStatus(content) } + // 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) + } + if content.Spec.Source.VolumeHandle != nil && content.Status == nil { + klog.V(5).Infof("syncContent: Call CreateSnapshot for content %s", content.Name) + ctrl.createSnapshot(content) + } else { + // Skip checkandUpdateContentStatus() if ReadyToUse is + // already true. We don't want to keep calling CreateSnapshot + // or ListSnapshots CSI methods over and over again for + // performance reasons. + if content.Status != nil && content.Status.ReadyToUse != nil && *content.Status.ReadyToUse == true { + // Try to remove AnnVolumeSnapshotBeingCreated if it is not removed yet for some reason + err := ctrl.removeAnnVolumeSnapshotBeingCreated(content) + if err != nil { + return fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %q", content.Name, err) + } + return nil + } + ctrl.checkandUpdateContentStatus(content) + } + return nil } @@ -296,9 +296,9 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c return nil, err } return updatedContent, nil - } else { - return ctrl.createSnapshotWrapper(content) } + return ctrl.createSnapshotWrapper(content) + } // This is a wrapper function for the snapshot creation process. @@ -347,10 +347,9 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V newContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size) if err != nil { klog.Errorf("error updating status for volume snapshot content %s: %v.", content.Name, err) - return nil, fmt.Errorf("error updating status for volume snapshot content %s: %v.", content.Name, err) - } else { - content = newContent + return nil, fmt.Errorf("error updating status for volume snapshot content %s: %v", content.Name, err) } + content = newContent // NOTE(xyang): handle create timeout // Remove annotation to indicate storage system has successfully diff --git a/pkg/sidecar-controller/snapshot_delete_test.go b/pkg/sidecar-controller/snapshot_delete_test.go index ae25a8bb..d5274d06 100644 --- a/pkg/sidecar-controller/snapshot_delete_test.go +++ b/pkg/sidecar-controller/snapshot_delete_test.go @@ -30,7 +30,7 @@ import ( ) var defaultSize int64 = 1000 -var emptySize int64 = 0 +var emptySize int64 var deletePolicy = crdv1.VolumeSnapshotContentDelete var retainPolicy = crdv1.VolumeSnapshotContentRetain var timeNow = time.Now() diff --git a/pkg/utils/util.go b/pkg/utils/util.go index a8262120..009a1d3b 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -215,10 +215,10 @@ func verifyAndGetSecretNameAndNamespaceTemplate(secret secretParamsMap, snapshot } else if numName == 0 { // No secrets specified return "", "", nil - } else { - // THIS IS NOT A VALID CASE - return "", "", fmt.Errorf("unknown error with getting secret name and namespace templates") } + // THIS IS NOT A VALID CASE + return "", "", fmt.Errorf("unknown error with getting secret name and namespace templates") + } // getSecretReference returns a reference to the secret specified in the given nameTemplate