Skip to content

Commit 0189c77

Browse files
xing-yangxing-yang
committed
Check final error
1 parent 6ca7507 commit 0189c77

File tree

3 files changed

+58
-39
lines changed

3 files changed

+58
-39
lines changed

‎pkg/sidecar-controller/content_create_test.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,11 @@ func TestSyncContent(t *testing.T) {
3030

3131
tests := []controllerTest{
3232
{
33-
name: "1-1: Basic content update ready to use",
34-
initialContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true),
35-
expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true),
36-
expectedEvents: noevents,
33+
name: "1-1: Basic content update ready to use",
34+
initialContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true),
35+
expectedContents: withContentAnnotations(newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true),
36+
map[string]string{}),
37+
expectedEvents: noevents,
3738
expectedCreateCalls: []createCall{
3839
{
3940
volumeHandle: "volume-handle-1-1",
@@ -162,7 +163,7 @@ func TestSyncContent(t *testing.T) {
162163
SnapshotHandle: toStringPointer("sid1-6"),
163164
RestoreSize: &defaultSize,
164165
ReadyToUse: &False,
165-
Error: newSnapshotError("Failed to check and update snapshot content: failed to get input parameters to create snapshot content1-6: \"failed to retrieve snapshot class bad-class from the informer: \\\"volumesnapshotclass.snapshot.storage.k8s.io \\\\\\\"bad-class\\\\\\\" not found\\\"\""),
166+
Error: newSnapshotError("Failed to check and update snapshot content: failed to get input parameters to create snapshot for content content1-6: \"failed to retrieve snapshot class bad-class from the informer: \\\"volumesnapshotclass.snapshot.storage.k8s.io \\\\\\\"bad-class\\\\\\\" not found\\\"\""),
166167
}),
167168
expectedEvents: []string{"Warning SnapshotContentCheckandUpdateFailed"},
168169
expectedCreateCalls: []createCall{

‎pkg/sidecar-controller/snapshot_controller.go‎

Lines changed: 48 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -278,29 +278,21 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c
278278
}
279279
driverName = content.Spec.Driver
280280
snapshotID = *content.Spec.Source.SnapshotHandle
281-
} else {
282-
class, snapshotterCredentials, err := ctrl.getCSISnapshotInput(content)
283-
if err != nil {
284-
return nil, fmt.Errorf("failed to get input parameters to create snapshot %s: %q", content.Name, err)
281+
282+
klog.V(5).Infof("checkandUpdateContentStatusOperation: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse)
283+
284+
if creationTime.IsZero() {
285+
creationTime = time.Now()
285286
}
286287

287-
driverName, snapshotID, creationTime, size, readyToUse, err = ctrl.handler.CreateSnapshot(content, class.Parameters, snapshotterCredentials)
288+
updatedContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size)
288289
if err != nil {
289-
klog.Errorf("checkandUpdateContentStatusOperation: failed to call create snapshot to check whether the snapshot is ready to use %q", err)
290290
return nil, err
291291
}
292+
return updatedContent, nil
293+
} else {
294+
return ctrl.createSnapshotOperation(content)
292295
}
293-
klog.V(5).Infof("checkandUpdateContentStatusOperation: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse)
294-
295-
if creationTime.IsZero() {
296-
creationTime = time.Now()
297-
}
298-
299-
updateContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size)
300-
if err != nil {
301-
return nil, err
302-
}
303-
return updateContent, nil
304296
}
305297

306298
// The function goes through the snapshot creation process.
@@ -335,16 +327,13 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1
335327
driverName, snapshotID, creationTime, size, readyToUse, err := ctrl.handler.CreateSnapshot(content, class.Parameters, snapshotterCredentials)
336328
if err != nil {
337329
// NOTE(xyang): handle create timeout
338-
// If it is not a timeout error, remove annotation to indicate
330+
// If it is a final error, remove annotation to indicate
339331
// storage system has responded with an error
340332
klog.Infof("createSnapshotOperation: CreateSnapshot for content %s returned error: %v", content.Name, err)
341-
if e, ok := status.FromError(err); ok {
342-
klog.Infof("createSnapshotOperation: CreateSnapshot for content %s error status: %+v", content.Name, e)
343-
if e.Code() != codes.DeadlineExceeded {
344-
err = ctrl.removeAnnVolumeSnapshotBeingCreated(content)
345-
if err != nil {
346-
return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %q", content.Name, err)
347-
}
333+
if isFinalError(err) {
334+
err = ctrl.removeAnnVolumeSnapshotBeingCreated(content)
335+
if err != nil {
336+
return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %q", content.Name, err)
348337
}
349338
}
350339

@@ -353,8 +342,11 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1
353342

354343
klog.V(5).Infof("Created snapshot: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse)
355344

356-
timestamp := creationTime.UnixNano()
357-
newContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, timestamp, size)
345+
if creationTime.IsZero() {
346+
creationTime = time.Now()
347+
}
348+
349+
newContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size)
358350
if err != nil {
359351
strerr := fmt.Sprintf("error updating volume snapshot content status for snapshot %s: %v.", content.Name, err)
360352
klog.Error(strerr)
@@ -614,12 +606,12 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte
614606
contentClone := content.DeepCopy()
615607
metav1.SetMetaDataAnnotation(&contentClone.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated, "yes")
616608

617-
updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone)
609+
updatedContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone)
618610
if err != nil {
619611
return newControllerUpdateError(content.Name, err.Error())
620612
}
621613
// update content if update is successful
622-
content = updateContent
614+
content = updatedContent
623615

624616
_, err = ctrl.storeContentUpdate(content)
625617
if err != nil {
@@ -641,12 +633,12 @@ func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(con
641633
contentClone := content.DeepCopy()
642634
delete(contentClone.ObjectMeta.Annotations, utils.AnnVolumeSnapshotBeingCreated)
643635

644-
updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone)
636+
updatedContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone)
645637
if err != nil {
646638
return newControllerUpdateError(content.Name, err.Error())
647639
}
648640
// update content if update is successful
649-
content = updateContent
641+
content = updatedContent
650642

651643
klog.V(5).Infof("Removed VolumeSnapshotBeingCreated annotation from volume snapshot content %s", content.Name)
652644
_, err = ctrl.storeContentUpdate(content)
@@ -655,3 +647,28 @@ func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(con
655647
}
656648
return nil
657649
}
650+
651+
// This function checks if the error is final
652+
func isFinalError(err error) bool {
653+
// Sources:
654+
// https://github.com/grpc/grpc/blob/master/doc/statuscodes.md
655+
// https://github.com/container-storage-interface/spec/blob/master/spec.md
656+
st, ok := status.FromError(err)
657+
if !ok {
658+
// This is not gRPC error. The operation must have failed before gRPC
659+
// method was called, otherwise we would get gRPC error.
660+
// We don't know if any previous CreateSnapshot is in progress, be on the safe side.
661+
return false
662+
}
663+
switch st.Code() {
664+
case codes.Canceled, // gRPC: Client Application cancelled the request
665+
codes.DeadlineExceeded, // gRPC: Timeout
666+
codes.Unavailable, // gRPC: Server shutting down, TCP connection broken - previous CreateSnapshot() may be still in progress.
667+
codes.ResourceExhausted, // gRPC: Server temporarily out of resources - previous CreateSnapshot() may be still in progress.
668+
codes.Aborted: // CSI: Operation pending for Snapshot
669+
return false
670+
}
671+
// All other errors mean that creating snapshot either did not
672+
// even start or failed. It is for sure not in progress.
673+
return true
674+
}

‎pkg/utils/util.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,10 @@ const (
8282
// If it is set, it indicates that the csi-snapshotter
8383
// sidecar has sent the create snapshot request to the storage system and
8484
// is waiting for a response of success or failure.
85-
// This annotation will be removed if the driver's CreateSnapshot
86-
// CSI function returns success or failure. If the create snapshot
87-
// request times out, retry will happen and the annotation will remain.
85+
// This annotation will be removed once the driver's CreateSnapshot
86+
// CSI function returns success or a final error (determined by isFinalError()).
87+
// If the create snapshot request fails with a non-final error such as timeout,
88+
// retry will happen and the annotation will remain.
8889
// This only applies to dynamic provisioning of snapshots because
8990
// the create snapshot CSI method will not be called for pre-provisioned
9091
// snapshots.

0 commit comments

Comments
 (0)