Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@xing-yang
xing-yang committed Apr 2, 2020
1 parent f32313b commit 9f382ac
Showing 1 changed file with 40 additions and 39 deletions.
79 changes: 40 additions & 39 deletions pkg/sidecar-controller/snapshot_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
// 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)
Expand Down Expand Up @@ -128,10 +133,17 @@ func (ctrl *csiSnapshotSideCarController) createSnapshot(content *crdv1.VolumeSn
klog.V(5).Infof("createSnapshot for content [%s]: started", content.Name)
opName := fmt.Sprintf("create-%s", content.Name)
ctrl.scheduleOperation(opName, func() error {
contentObj, err := ctrl.createSnapshotOperation(content)
// content.Status will be created for the first time after a snapshot
// is created by the CSI driver. If content.Status is not nil,
// we should update content status without creating snapshot again.
if content.Status != nil && content.Status.Error != nil && content.Status.Error.Message != nil && !isControllerUpdateFailError(content.Status.Error) {
klog.V(4).Infof("error is already set in snapshot, do not retry to create: %s", *content.Status.Error.Message)
return nil
}
contentObj, err := ctrl.createSnapshotWrapper(content)
if err != nil {
ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot: %v", err))
klog.Errorf("createSnapshot [%s]: error occurred in createSnapshotOperation: %v", opName, err)
klog.Errorf("createSnapshot [%s]: error occurred in createSnapshotWrapper: %v", opName, err)
return err
}

Expand Down Expand Up @@ -295,21 +307,6 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c
}
}

// The function goes through the snapshot creation process.
func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) {
klog.Infof("createSnapshotOperation: Creating snapshot for content %s through the plugin ...", content.Name)

// content.Status will be created for the first time after a snapshot
// is created by the CSI driver. If content.Status is not nil,
// we should update content status without creating snapshot again.
if content.Status != nil && content.Status.Error != nil && content.Status.Error.Message != nil && !isControllerUpdateFailError(content.Status.Error) {
klog.V(4).Infof("error is already set in snapshot, do not retry to create: %s", *content.Status.Error.Message)
return content, nil
}

return ctrl.createSnapshotWrapper(content)
}

// This is a wrapper function for the snapshot creation process.
func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) {
klog.Infof("createSnapshotWrapper: Creating snapshot for content %s through the plugin ...", content.Name)
Expand Down Expand Up @@ -337,7 +334,7 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V
// If it is a final error, remove annotation to indicate
// storage system has responded with an error
klog.Infof("createSnapshotWrapper: CreateSnapshot for content %s returned error: %v", content.Name, err)
if isFinalError(err) {
if isCSIFinalError(err) {
err = ctrl.removeAnnVolumeSnapshotBeingCreated(content)
if err != nil {
return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %q", content.Name, err)
Expand All @@ -355,8 +352,8 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V

newContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size)
if err != nil {
strerr := fmt.Sprintf("error updating volume snapshot content status for snapshot %s: %v.", content.Name, err)
klog.Error(strerr)
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
}
Expand Down Expand Up @@ -607,34 +604,38 @@ func (ctrl *csiSnapshotSideCarController) shouldDelete(content *crdv1.VolumeSnap
// on VolumeSnapshotContent
// If set, it indicates snapshot is being created
func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent) error {
// Set AnnVolumeSnapshotBeingCreated if it is not set yet
if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) {
klog.V(5).Infof("setAnnVolumeSnapshotBeingCreated: set annotation [%s:yes] on content [%s].", utils.AnnVolumeSnapshotBeingCreated, content.Name)
contentClone := content.DeepCopy()
metav1.SetMetaDataAnnotation(&contentClone.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated, "yes")
if metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) {
// the annotation already exists, return directly
return nil
}

updatedContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone)
if err != nil {
return newControllerUpdateError(content.Name, err.Error())
}
// update content if update is successful
content = updatedContent
// Set AnnVolumeSnapshotBeingCreated
klog.V(5).Infof("setAnnVolumeSnapshotBeingCreated: set annotation [%s:yes] on content [%s].", utils.AnnVolumeSnapshotBeingCreated, content.Name)
contentClone := content.DeepCopy()
metav1.SetMetaDataAnnotation(&contentClone.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated, "yes")

_, err = ctrl.storeContentUpdate(content)
if err != nil {
klog.V(4).Infof("setAnnVolumeSnapshotBeingCreated for content [%s]: cannot update internal cache %v", content.Name, err)
return err
}
klog.V(5).Infof("setAnnVolumeSnapshotBeingCreated: volume snapshot content %+v", content)
updatedContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone)
if err != nil {
return newControllerUpdateError(content.Name, err.Error())
}
// update content if update is successful
content = updatedContent

_, err = ctrl.storeContentUpdate(content)
if err != nil {
klog.V(4).Infof("setAnnVolumeSnapshotBeingCreated for content [%s]: cannot update internal cache %v", content.Name, err)
return err
}
klog.V(5).Infof("setAnnVolumeSnapshotBeingCreated: volume snapshot content %+v", content)

return nil
}

// removeAnnVolumeSnapshotBeingCreated removes the VolumeSnapshotBeingCreated
// annotation from a content if there exists one.
func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent) error {
if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) {
// the annotation does not exit, return directly
// the annotation does not exist, return directly
return nil
}
contentClone := content.DeepCopy()
Expand All @@ -656,7 +657,7 @@ func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(con
}

// This function checks if the error is final
func isFinalError(err error) bool {
func isCSIFinalError(err error) bool {
// Sources:
// https://github.com/grpc/grpc/blob/master/doc/statuscodes.md
// https://github.com/container-storage-interface/spec/blob/master/spec.md
Expand Down

0 comments on commit 9f382ac

Please sign in to comment.