From 5cf92fc01a08fff0e9dbd62fc551bbbbda10d41f Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Thu, 27 May 2021 11:53:50 -0700 Subject: [PATCH] Return VolumeSnapshotContent from various functions instead of nil Signed-off-by: Grant Griffiths --- pkg/common-controller/snapshot_controller.go | 49 +++++++------ pkg/sidecar-controller/snapshot_controller.go | 69 ++++++++++--------- 2 files changed, 62 insertions(+), 56 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 506c748f..ef3862d9 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -166,7 +166,10 @@ 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) { - return ctrl.setAnnVolumeSnapshotBeingDeleted(content) + // Do not need to use the returned content here, as syncContent will get + // the correct version from the cache next time. It is also not used after this. + _, err = ctrl.setAnnVolumeSnapshotBeingDeleted(content) + return err } return nil @@ -311,10 +314,12 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec // a delete operation whenever the content has deletion timestamp set. if content != nil { klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: Set VolumeSnapshotBeingDeleted annotation on the content [%s]", utils.SnapshotKey(snapshot), content.Name) - if err := ctrl.setAnnVolumeSnapshotBeingDeleted(content); err != nil { + updatedContent, err := ctrl.setAnnVolumeSnapshotBeingDeleted(content) + if err != nil { klog.V(4).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: failed to set VolumeSnapshotBeingDeleted annotation on the content [%s]", utils.SnapshotKey(snapshot), content.Name) return err } + content = updatedContent } // VolumeSnapshot should be deleted. Check and remove finalizers @@ -811,12 +816,12 @@ func (ctrl *csiSnapshotCommonController) addContentFinalizer(content *crdv1.Volu contentClone := content.DeepCopy() contentClone.ObjectMeta.Finalizers = append(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer) - _, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) + newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) if err != nil { return newControllerUpdateError(content.Name, err.Error()) } - _, err = ctrl.storeContentUpdate(contentClone) + _, err = ctrl.storeContentUpdate(newContent) if err != nil { klog.Errorf("failed to update content store %v", err) } @@ -991,13 +996,13 @@ func (ctrl *csiSnapshotCommonController) checkandBindSnapshotContent(snapshot *c newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) if err != nil { klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", contentClone.Name, err) - return nil, err + return content, err } _, err = ctrl.storeContentUpdate(newContent) if err != nil { klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status: cannot update internal cache %v", newContent.Name, err) - return nil, err + return newContent, err } return newContent, nil } @@ -1303,13 +1308,13 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1 // Find default snapshot class if available list, err := ctrl.classLister.List(labels.Everything()) if err != nil { - return nil, nil, err + return nil, snapshot, err } pvDriver, err := ctrl.pvDriverFromSnapshot(snapshot) if err != nil { klog.Errorf("failed to get pv csi driver from snapshot %s/%s: %q", snapshot.Namespace, snapshot.Name, err) - return nil, nil, err + return nil, snapshot, err } defaultClasses := []*crdv1.VolumeSnapshotClass{} @@ -1320,11 +1325,11 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1 } } if len(defaultClasses) == 0 { - return nil, nil, fmt.Errorf("cannot find default snapshot class") + return nil, snapshot, fmt.Errorf("cannot find default snapshot class") } if len(defaultClasses) > 1 { klog.V(4).Infof("get DefaultClass %d defaults found", len(defaultClasses)) - return nil, nil, fmt.Errorf("%d default snapshot classes were found", len(defaultClasses)) + return nil, snapshot, fmt.Errorf("%d default snapshot classes were found", len(defaultClasses)) } klog.V(5).Infof("setDefaultSnapshotClass [%s]: default VolumeSnapshotClassName [%s]", snapshot.Name, defaultClasses[0].Name) snapshotClone := snapshot.DeepCopy() @@ -1394,17 +1399,17 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo if addBoundFinalizer { snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) } - _, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{}) + newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{}) if err != nil { return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) } - _, err = ctrl.storeSnapshotUpdate(snapshotClone) + _, err = ctrl.storeSnapshotUpdate(newSnapshot) if err != nil { klog.Errorf("failed to update snapshot store %v", err) } - klog.V(5).Infof("Added protection finalizer to volume snapshot %s", utils.SnapshotKey(snapshot)) + klog.V(5).Infof("Added protection finalizer to volume snapshot %s", utils.SnapshotKey(newSnapshot)) return nil } @@ -1438,12 +1443,12 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1 if removeBoundFinalizer { snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) } - _, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{}) + newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{}) if err != nil { return newControllerUpdateError(snapshot.Name, err.Error()) } - _, err = ctrl.storeSnapshotUpdate(snapshotClone) + _, err = ctrl.storeSnapshotUpdate(newSnapshot) if err != nil { klog.Errorf("failed to update snapshot store %v", err) } @@ -1477,9 +1482,9 @@ func (ctrl *csiSnapshotCommonController) getSnapshotFromStore(snapshotName strin return snapshot, nil } -func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(content *crdv1.VolumeSnapshotContent) error { +func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) { if content == nil { - return nil + return content, nil } // Set AnnVolumeSnapshotBeingDeleted if it is not set yet if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted) { @@ -1488,7 +1493,7 @@ func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(conten updateContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), content, metav1.UpdateOptions{}) if err != nil { - return newControllerUpdateError(content.Name, err.Error()) + return content, newControllerUpdateError(content.Name, err.Error()) } // update content if update is successful content = updateContent @@ -1496,11 +1501,11 @@ 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 err + return content, err } klog.V(5).Infof("setAnnVolumeSnapshotBeingDeleted: volume snapshot content %+v", content) } - return nil + return content, nil } // checkAndSetInvalidContentLabel adds a label to unlabeled invalid content objects and removes the label from valid ones. @@ -1531,7 +1536,7 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidContentLabel(content return content, newControllerUpdateError(content.Name, err.Error()) } - _, err = ctrl.storeContentUpdate(contentClone) + _, err = ctrl.storeContentUpdate(updatedContent) if err != nil { klog.Errorf("failed to update content store %v", err) } @@ -1573,7 +1578,7 @@ func (ctrl *csiSnapshotCommonController) checkAndSetInvalidSnapshotLabel(snapsho return snapshot, newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) } - _, err = ctrl.storeSnapshotUpdate(snapshotClone) + _, err = ctrl.storeSnapshotUpdate(updatedSnapshot) if err != nil { klog.Errorf("failed to update snapshot store %v", err) } diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 0e2a98f0..ec0c6deb 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -79,9 +79,11 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps // already true. We don't want to keep calling CreateSnapshot // or ListSnapshots CSI methods over and over again for // performance reasons. + var err error 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 - return ctrl.removeAnnVolumeSnapshotBeingCreated(content) + _, err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) + return err } return ctrl.checkandUpdateContentStatus(content) } @@ -101,7 +103,7 @@ func (ctrl *csiSnapshotSideCarController) createSnapshot(content *crdv1.VolumeSn klog.V(5).Infof("createSnapshot for content [%s]: started", content.Name) contentObj, err := ctrl.createSnapshotWrapper(content) if err != nil { - ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot: %v", err)) + ctrl.updateContentErrorStatusWithEvent(contentObj, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot: %v", err)) klog.Errorf("createSnapshot for content [%s]: error occurred in createSnapshotWrapper: %v", content.Name, err) return err } @@ -118,7 +120,7 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatus(content *c klog.V(5).Infof("checkandUpdateContentStatus[%s] started", content.Name) contentObj, err := ctrl.checkandUpdateContentStatusOperation(content) if err != nil { - ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotContentCheckandUpdateFailed", fmt.Sprintf("Failed to check and update snapshot content: %v", err)) + ctrl.updateContentErrorStatusWithEvent(contentObj, v1.EventTypeWarning, "SnapshotContentCheckandUpdateFailed", fmt.Sprintf("Failed to check and update snapshot content: %v", err)) klog.Errorf("checkandUpdateContentStatus [%s]: error occurred %v", content.Name, err) return err } @@ -221,27 +223,27 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c class, err := ctrl.getSnapshotClass(*content.Spec.VolumeSnapshotClassName) if err != nil { klog.Errorf("Failed to get snapshot class %s for snapshot content %s: %v", *content.Spec.VolumeSnapshotClassName, content.Name, err) - return nil, fmt.Errorf("failed to get snapshot class %s for snapshot content %s: %v", *content.Spec.VolumeSnapshotClassName, content.Name, err) + return content, fmt.Errorf("failed to get snapshot class %s for snapshot content %s: %v", *content.Spec.VolumeSnapshotClassName, content.Name, err) } snapshotterListSecretRef, err := utils.GetSecretReference(utils.SnapshotterListSecretParams, class.Parameters, content.GetObjectMeta().GetName(), nil) if err != nil { klog.Errorf("Failed to get secret reference for snapshot content %s: %v", content.Name, err) - return nil, fmt.Errorf("failed to get secret reference for snapshot content %s: %v", content.Name, err) + return content, fmt.Errorf("failed to get secret reference for snapshot content %s: %v", content.Name, err) } snapshotterListCredentials, err = utils.GetCredentials(ctrl.client, snapshotterListSecretRef) if err != nil { // Continue with deletion, as the secret may have already been deleted. klog.Errorf("Failed to get credentials for snapshot content %s: %v", content.Name, err) - return nil, fmt.Errorf("failed to get credentials for snapshot content %s: %v", content.Name, err) + return content, fmt.Errorf("failed to get credentials for snapshot content %s: %v", content.Name, err) } } readyToUse, creationTime, size, err = ctrl.handler.GetSnapshotStatus(content, snapshotterListCredentials) if err != nil { klog.Errorf("checkandUpdateContentStatusOperation: failed to call get snapshot status to check whether snapshot is ready to use %q", err) - return nil, err + return content, err } driverName = content.Spec.Driver snapshotID = *content.Spec.Source.SnapshotHandle @@ -254,7 +256,7 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c updatedContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size) if err != nil { - return nil, err + return content, err } return updatedContent, nil } @@ -268,7 +270,7 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V class, snapshotterCredentials, err := ctrl.getCSISnapshotInput(content) if err != nil { - return nil, fmt.Errorf("failed to get input parameters to create snapshot for content %s: %q", content.Name, err) + return content, fmt.Errorf("failed to get input parameters to create snapshot for content %s: %q", content.Name, err) } // NOTE(xyang): handle create timeout @@ -278,14 +280,14 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V // success or permanent failure. If the request times out, annotation will // remain on the content to avoid potential leaking of a snapshot resource on // the storage system. - err = ctrl.setAnnVolumeSnapshotBeingCreated(content) + content, err = ctrl.setAnnVolumeSnapshotBeingCreated(content) if err != nil { - return nil, fmt.Errorf("failed to add VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err) + return content, fmt.Errorf("failed to add VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err) } parameters, err := utils.RemovePrefixedParameters(class.Parameters) if err != nil { - return nil, fmt.Errorf("failed to remove CSI Parameters of prefixed keys: %v", err) + return content, fmt.Errorf("failed to remove CSI Parameters of prefixed keys: %v", err) } if ctrl.extraCreateMetadata { parameters[utils.PrefixedVolumeSnapshotNameKey] = content.Spec.VolumeSnapshotRef.Name @@ -300,12 +302,13 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V // storage system has responded with an error klog.Infof("createSnapshotWrapper: CreateSnapshot for content %s returned error: %v", content.Name, err) if isCSIFinalError(err) { - if removeAnnotationErr := ctrl.removeAnnVolumeSnapshotBeingCreated(content); removeAnnotationErr != nil { - return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %s", content.Name, removeAnnotationErr) + var removeAnnotationErr error + if content, removeAnnotationErr = ctrl.removeAnnVolumeSnapshotBeingCreated(content); removeAnnotationErr != nil { + return content, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %s", content.Name, removeAnnotationErr) } } - return nil, fmt.Errorf("failed to take snapshot of the volume %s: %q", *content.Spec.Source.VolumeHandle, err) + return content, fmt.Errorf("failed to take snapshot of the volume %s: %q", *content.Spec.Source.VolumeHandle, err) } klog.V(5).Infof("Created snapshot: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse) @@ -317,16 +320,16 @@ 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) + return content, 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 // cut the snapshot - err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) + content, err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) if err != nil { - return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err) + return content, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err) } return content, nil @@ -378,7 +381,7 @@ func (ctrl *csiSnapshotSideCarController) clearVolumeContentStatus( } newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().UpdateStatus(context.TODO(), content, metav1.UpdateOptions{}) if err != nil { - return nil, newControllerUpdateError(contentName, err.Error()) + return content, newControllerUpdateError(contentName, err.Error()) } return newContent, nil } @@ -434,7 +437,7 @@ func (ctrl *csiSnapshotSideCarController) updateSnapshotContentStatus( contentClone.Status = newStatus newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().UpdateStatus(context.TODO(), contentClone, metav1.UpdateOptions{}) if err != nil { - return nil, newControllerUpdateError(content.Name, err.Error()) + return contentObj, newControllerUpdateError(content.Name, err.Error()) } return newContent, nil } @@ -520,13 +523,13 @@ func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.V contentClone := content.DeepCopy() contentClone.ObjectMeta.Finalizers = utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer) - _, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) + updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) if err != nil { return newControllerUpdateError(content.Name, err.Error()) } - klog.V(5).Infof("Removed protection finalizer from volume snapshot content %s", content.Name) - _, err = ctrl.storeContentUpdate(contentClone) + klog.V(5).Infof("Removed protection finalizer from volume snapshot content %s", updatedContent.Name) + _, err = ctrl.storeContentUpdate(updatedContent) if err != nil { klog.Errorf("failed to update content store %v", err) } @@ -566,10 +569,10 @@ func (ctrl *csiSnapshotSideCarController) shouldDelete(content *crdv1.VolumeSnap // setAnnVolumeSnapshotBeingCreated sets VolumeSnapshotBeingCreated annotation // on VolumeSnapshotContent // If set, it indicates snapshot is being created -func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent) error { +func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) { if metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) { // the annotation already exists, return directly - return nil + return content, nil } // Set AnnVolumeSnapshotBeingCreated @@ -579,7 +582,7 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) if err != nil { - return newControllerUpdateError(content.Name, err.Error()) + return content, newControllerUpdateError(content.Name, err.Error()) } // update content if update is successful content = updatedContent @@ -590,32 +593,30 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte } klog.V(5).Infof("setAnnVolumeSnapshotBeingCreated: volume snapshot content %+v", content) - return nil + return content, nil } // removeAnnVolumeSnapshotBeingCreated removes the VolumeSnapshotBeingCreated // annotation from a content if there exists one. -func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent) error { +func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) { if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) { // the annotation does not exist, return directly - return nil + return content, nil } contentClone := content.DeepCopy() delete(contentClone.ObjectMeta.Annotations, utils.AnnVolumeSnapshotBeingCreated) updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) if err != nil { - return newControllerUpdateError(content.Name, err.Error()) + return content, newControllerUpdateError(content.Name, err.Error()) } - // update content if update is successful - content = updatedContent klog.V(5).Infof("Removed VolumeSnapshotBeingCreated annotation from volume snapshot content %s", content.Name) - _, err = ctrl.storeContentUpdate(content) + _, err = ctrl.storeContentUpdate(updatedContent) if err != nil { klog.Errorf("failed to update content store %v", err) } - return nil + return updatedContent, nil } // This function checks if the error is final