From 313507f866f6d9d573b6b963108176207d6346b8 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Thu, 27 Feb 2020 21:02:26 +0000 Subject: [PATCH 01/10] Add AnnVolumeSnapshotBeingCreated --- pkg/sidecar-controller/snapshot_controller.go | 72 ++++++++++++++++++- pkg/utils/util.go | 16 +++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 8c2963b68..c5ac96524 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -322,14 +322,45 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 return nil, fmt.Errorf("failed to get input parameters to create snapshot for content %s: %q", content.Name, err) } + // NOTE(xyang): handle create timeout + // Add annotation and set to Yes to indicate create snapshot request is + // sent to the storage system and wait for a response of success or failure. + // Annotation will be set to No only after storage system has responded + // with success or failure. If the request times out, retry will happen + // and annotation will stay as Yes to avoid leaking of snapshot + // resources on the storage system + err = ctrl.setAnnVolumeSnapshotBeingCreated(content, utils.AnnVolumeSnapshotBeingCreated_Yes) + if err != nil { + return nil, fmt.Errorf("failed to set VolumeSnapshotBeingCreated annotation to Yes on the content %s: %q", content.Name, err) + } + driverName, snapshotID, creationTime, size, readyToUse, err := ctrl.handler.CreateSnapshot(content, class.Parameters, snapshotterCredentials) if err != nil { + // NOTE(xyang): handle create timeout + // If it is not a timeout error, set annotation to No to indicate + // storage system has responded with an error + errStr := fmt.Sprintf("%q", err) + if !strings.Contains(errStr, "DeadlineExceeded") { + err = ctrl.setAnnVolumeSnapshotBeingCreated(content, utils.AnnVolumeSnapshotBeingCreated_No) + if err != nil { + return nil, fmt.Errorf("failed to set VolumeSnapshotBeingCreated annotation to No on the content %s: %q", content.Name, err) + } + } + return nil, fmt.Errorf("failed to take snapshot of the volume, %s: %q", *content.Spec.Source.VolumeHandle, err) } if driverName != class.Driver { return nil, fmt.Errorf("failed to take snapshot of the volume, %s: driver name %s returned from the driver is different from driver %s in snapshot class", *content.Spec.Source.VolumeHandle, driverName, class.Driver) } + // NOTE(xyang): handle create timeout + // Set annotation to No to indicate storage system has successfully + // cut the snapshot + err = ctrl.setAnnVolumeSnapshotBeingCreated(content, utils.AnnVolumeSnapshotBeingCreated_No) + if err != nil { + return nil, fmt.Errorf("failed to set VolumeSnapshotBeingCreated annotation to No on the content %s: %q", content.Name, err) + } + klog.V(5).Infof("Created snapshot: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse) timestamp := creationTime.UnixNano() @@ -564,9 +595,48 @@ func (ctrl *csiSnapshotSideCarController) shouldDelete(content *crdv1.VolumeSnap if content.Spec.Source.SnapshotHandle != nil && content.Spec.VolumeSnapshotRef.UID == "" { return true } - // 2) shouldDelete returns true if AnnVolumeSnapshotBeingDeleted annotation is set + + // NOTE(xyang): Handle create snapshot timeout + // 2) shouldDelete returns false if AnnVolumeSnapshotBeingCreated + // annotation is set and its value is Yes. This indicates a + // CreateSnapshot CSI RPC has not responded with success or failure. + // We need to keep waiting for a response from the CSI driver. + if metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) && content.ObjectMeta.Annotations[utils.AnnVolumeSnapshotBeingCreated] == utils.AnnVolumeSnapshotBeingCreated_Yes { + return false + } + + // 3) shouldDelete returns true if AnnVolumeSnapshotBeingDeleted annotation is set if metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted) { return true } return false } + +// setAnnVolumeSnapshotBeingCreated sets VolumeSnapshotBeingCreated annotation +// on VolumeSnapshotContent +// If beingCreated is true, it indicates snapshot is being created +// If beingCreated if false, it indicates CreateSnapshot CSI RPC returns +// success or failure +func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent, annBeingCreated string) error { + // Set AnnVolumeSnapshotBeingCreated if it is not set yet or if it is + // set but has a different value + if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) || content.ObjectMeta.Annotations[utils.AnnVolumeSnapshotBeingCreated] != annBeingCreated { + klog.V(5).Infof("setAnnVolumeSnapshotBeingCreated: set annotation [%s:%s] on content [%s].", utils.AnnVolumeSnapshotBeingCreated, annBeingCreated, content.Name) + metav1.SetMetaDataAnnotation(&content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated, annBeingCreated) + + updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(content) + if err != nil { + return newControllerUpdateError(content.Name, err.Error()) + } + // update content if update is successful + content = updateContent + + _, 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 +} diff --git a/pkg/utils/util.go b/pkg/utils/util.go index 0bd64435b..09f9ee48e 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -78,6 +78,22 @@ const ( // backing the snapshot content. AnnVolumeSnapshotBeingDeleted = "snapshot.storage.kubernetes.io/volumesnapshot-being-deleted" + // AnnVolumeSnapshotBeingCreated annotation applies to VolumeSnapshotContents. + // If it is set and value is "yes", it indicates that the csi-snapshotter + // sidecar has sent the create snapshot request to the storage system and + // is waiting for a response of success or failure. + // This annotation will be set to "no" if the driver's CreateSnapshot + // CSI function returns success or failure. If the create snapshot + // request times out, retry will happen and the annotation value will + // stay as "yes". + // This only applies to dynamic provisioning of snapshots because + // the create snapshot CSI method will not be called for pre-provisioned + // snapshots. + AnnVolumeSnapshotBeingCreated = "snapshot.storage.kubernetes.io/volumesnapshot-being-created" + // VolumeSnapshotBeingCreated annotation values can be "yes" or "no" + AnnVolumeSnapshotBeingCreated_Yes = "yes" + AnnVolumeSnapshotBeingCreated_No = "no" + // Annotation for secret name and namespace will be added to the content // and used at snapshot content deletion time. AnnDeletionSecretRefName = "snapshot.storage.kubernetes.io/deletion-secret-name" From 4c38cc8c6e4228b347d421d4b0ba4b577e9ddaff Mon Sep 17 00:00:00 2001 From: xing-yang Date: Thu, 5 Mar 2020 21:18:37 +0000 Subject: [PATCH 02/10] Remove VolumeSnapshotBeingCreated annotation after driver response --- pkg/sidecar-controller/content_create_test.go | 3 +- pkg/sidecar-controller/snapshot_controller.go | 79 ++++++++++++------- pkg/utils/util.go | 10 +-- 3 files changed, 56 insertions(+), 36 deletions(-) diff --git a/pkg/sidecar-controller/content_create_test.go b/pkg/sidecar-controller/content_create_test.go index 5185b5ed2..475224b2e 100644 --- a/pkg/sidecar-controller/content_create_test.go +++ b/pkg/sidecar-controller/content_create_test.go @@ -52,8 +52,9 @@ func TestSyncContent(t *testing.T) { name: "1-2: Basic sync content create snapshot", initialContents: withContentStatus(newContentArray("content1-2", "snapuid1-2", "snap1-2", "sid1-2", defaultClass, "", "volume-handle-1-2", retainPolicy, nil, &defaultSize, true), nil), - expectedContents: withContentStatus(newContentArray("content1-2", "snapuid1-2", "snap1-2", "sid1-2", defaultClass, "", "volume-handle-1-2", retainPolicy, nil, &defaultSize, true), + expectedContents: withContentAnnotations(withContentStatus(newContentArray("content1-2", "snapuid1-2", "snap1-2", "sid1-2", defaultClass, "", "volume-handle-1-2", retainPolicy, nil, &defaultSize, true), &crdv1.VolumeSnapshotContentStatus{SnapshotHandle: toStringPointer("snapuid1-2"), RestoreSize: &defaultSize, ReadyToUse: &True}), + map[string]string{}), expectedEvents: noevents, expectedCreateCalls: []createCall{ { diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index c5ac96524..66c4ebff0 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -323,13 +323,13 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 } // NOTE(xyang): handle create timeout - // Add annotation and set to Yes to indicate create snapshot request is + // Add annotation to indicate create snapshot request is // sent to the storage system and wait for a response of success or failure. - // Annotation will be set to No only after storage system has responded + // Annotation will be removed only after storage system has responded // with success or failure. If the request times out, retry will happen - // and annotation will stay as Yes to avoid leaking of snapshot + // and annotation will remain to avoid leaking of snapshot // resources on the storage system - err = ctrl.setAnnVolumeSnapshotBeingCreated(content, utils.AnnVolumeSnapshotBeingCreated_Yes) + err = ctrl.setAnnVolumeSnapshotBeingCreated(content) if err != nil { return nil, fmt.Errorf("failed to set VolumeSnapshotBeingCreated annotation to Yes on the content %s: %q", content.Name, err) } @@ -337,11 +337,11 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 driverName, snapshotID, creationTime, size, readyToUse, err := ctrl.handler.CreateSnapshot(content, class.Parameters, snapshotterCredentials) if err != nil { // NOTE(xyang): handle create timeout - // If it is not a timeout error, set annotation to No to indicate + // If it is not a timeout error, remove annotation to indicate // storage system has responded with an error errStr := fmt.Sprintf("%q", err) if !strings.Contains(errStr, "DeadlineExceeded") { - err = ctrl.setAnnVolumeSnapshotBeingCreated(content, utils.AnnVolumeSnapshotBeingCreated_No) + err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) if err != nil { return nil, fmt.Errorf("failed to set VolumeSnapshotBeingCreated annotation to No on the content %s: %q", content.Name, err) } @@ -353,14 +353,6 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 return nil, fmt.Errorf("failed to take snapshot of the volume, %s: driver name %s returned from the driver is different from driver %s in snapshot class", *content.Spec.Source.VolumeHandle, driverName, class.Driver) } - // NOTE(xyang): handle create timeout - // Set annotation to No to indicate storage system has successfully - // cut the snapshot - err = ctrl.setAnnVolumeSnapshotBeingCreated(content, utils.AnnVolumeSnapshotBeingCreated_No) - if err != nil { - return nil, fmt.Errorf("failed to set VolumeSnapshotBeingCreated annotation to No on the content %s: %q", content.Name, err) - } - klog.V(5).Infof("Created snapshot: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse) timestamp := creationTime.UnixNano() @@ -372,6 +364,14 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 content = newContent } + // NOTE(xyang): handle create timeout + // Remove annotation to indicate storage system has successfully + // cut the snapshot + err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) + if err != nil { + return nil, fmt.Errorf("failed to set VolumeSnapshotBeingCreated annotation to No on the content %s: %q", content.Name, err) + } + // Update content in the cache store _, err = ctrl.storeContentUpdate(content) if err != nil { @@ -598,10 +598,10 @@ func (ctrl *csiSnapshotSideCarController) shouldDelete(content *crdv1.VolumeSnap // NOTE(xyang): Handle create snapshot timeout // 2) shouldDelete returns false if AnnVolumeSnapshotBeingCreated - // annotation is set and its value is Yes. This indicates a - // CreateSnapshot CSI RPC has not responded with success or failure. + // annotation is set. This indicates a CreateSnapshot CSI RPC has + // not responded with success or failure. // We need to keep waiting for a response from the CSI driver. - if metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) && content.ObjectMeta.Annotations[utils.AnnVolumeSnapshotBeingCreated] == utils.AnnVolumeSnapshotBeingCreated_Yes { + if metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) { return false } @@ -614,17 +614,15 @@ func (ctrl *csiSnapshotSideCarController) shouldDelete(content *crdv1.VolumeSnap // setAnnVolumeSnapshotBeingCreated sets VolumeSnapshotBeingCreated annotation // on VolumeSnapshotContent -// If beingCreated is true, it indicates snapshot is being created -// If beingCreated if false, it indicates CreateSnapshot CSI RPC returns -// success or failure -func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(content *crdv1.VolumeSnapshotContent, annBeingCreated string) error { - // Set AnnVolumeSnapshotBeingCreated if it is not set yet or if it is - // set but has a different value - if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) || content.ObjectMeta.Annotations[utils.AnnVolumeSnapshotBeingCreated] != annBeingCreated { - klog.V(5).Infof("setAnnVolumeSnapshotBeingCreated: set annotation [%s:%s] on content [%s].", utils.AnnVolumeSnapshotBeingCreated, annBeingCreated, content.Name) - metav1.SetMetaDataAnnotation(&content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated, annBeingCreated) - - updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(content) +// 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") + + updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone) if err != nil { return newControllerUpdateError(content.Name, err.Error()) } @@ -640,3 +638,28 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte } 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 + return nil + } + contentClone := content.DeepCopy() + delete(contentClone.ObjectMeta.Annotations, utils.AnnVolumeSnapshotBeingCreated) + + updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone) + if err != nil { + return newControllerUpdateError(content.Name, err.Error()) + } + // update content if update is successful + content = updateContent + + klog.V(5).Infof("Removed VolumeSnapshotBeingCreated annotation from volume snapshot content %s", content.Name) + _, err = ctrl.storeContentUpdate(content) + if err != nil { + klog.Errorf("failed to update content store %v", err) + } + return nil +} diff --git a/pkg/utils/util.go b/pkg/utils/util.go index 09f9ee48e..b0fc07496 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -79,20 +79,16 @@ const ( AnnVolumeSnapshotBeingDeleted = "snapshot.storage.kubernetes.io/volumesnapshot-being-deleted" // AnnVolumeSnapshotBeingCreated annotation applies to VolumeSnapshotContents. - // If it is set and value is "yes", it indicates that the csi-snapshotter + // If it is set, it indicates that the csi-snapshotter // sidecar has sent the create snapshot request to the storage system and // is waiting for a response of success or failure. - // This annotation will be set to "no" if the driver's CreateSnapshot + // This annotation will be removed if the driver's CreateSnapshot // CSI function returns success or failure. If the create snapshot - // request times out, retry will happen and the annotation value will - // stay as "yes". + // request times out, retry will happen and the annotation will remain. // This only applies to dynamic provisioning of snapshots because // the create snapshot CSI method will not be called for pre-provisioned // snapshots. AnnVolumeSnapshotBeingCreated = "snapshot.storage.kubernetes.io/volumesnapshot-being-created" - // VolumeSnapshotBeingCreated annotation values can be "yes" or "no" - AnnVolumeSnapshotBeingCreated_Yes = "yes" - AnnVolumeSnapshotBeingCreated_No = "no" // Annotation for secret name and namespace will be added to the content // and used at snapshot content deletion time. From aa2bc2fb9c2752ade87a8a078bfa4dac9deb82c5 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Thu, 5 Mar 2020 21:55:57 +0000 Subject: [PATCH 03/10] Remove the unnecessary driver name check --- pkg/sidecar-controller/snapshot_controller.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 66c4ebff0..481c187e3 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -349,9 +349,6 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 return nil, fmt.Errorf("failed to take snapshot of the volume, %s: %q", *content.Spec.Source.VolumeHandle, err) } - if driverName != class.Driver { - return nil, fmt.Errorf("failed to take snapshot of the volume, %s: driver name %s returned from the driver is different from driver %s in snapshot class", *content.Spec.Source.VolumeHandle, driverName, class.Driver) - } klog.V(5).Infof("Created snapshot: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse) From 7930dd261d52f30092baa664d3ab399ef65c1d2a Mon Sep 17 00:00:00 2001 From: xing-yang Date: Fri, 6 Mar 2020 03:59:51 +0000 Subject: [PATCH 04/10] Check error code DeadlineExceeded --- pkg/sidecar-controller/snapshot_controller.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 481c187e3..6bf91cca5 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -23,6 +23,8 @@ import ( crdv1 "github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1" "github.com/kubernetes-csi/external-snapshotter/v2/pkg/utils" + codes "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog" @@ -331,7 +333,7 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 // resources on the storage system err = ctrl.setAnnVolumeSnapshotBeingCreated(content) if err != nil { - return nil, fmt.Errorf("failed to set VolumeSnapshotBeingCreated annotation to Yes on the content %s: %q", content.Name, err) + return nil, fmt.Errorf("failed to add VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err) } driverName, snapshotID, creationTime, size, readyToUse, err := ctrl.handler.CreateSnapshot(content, class.Parameters, snapshotterCredentials) @@ -339,11 +341,12 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 // NOTE(xyang): handle create timeout // If it is not a timeout error, remove annotation to indicate // storage system has responded with an error - errStr := fmt.Sprintf("%q", err) - if !strings.Contains(errStr, "DeadlineExceeded") { - err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) - if err != nil { - return nil, fmt.Errorf("failed to set VolumeSnapshotBeingCreated annotation to No on the content %s: %q", content.Name, err) + if e, ok := status.FromError(err); ok { + if e.Code() == codes.DeadlineExceeded { + err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) + if err != nil { + return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %q", content.Name, err) + } } } @@ -366,7 +369,7 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 // cut the snapshot err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) if err != nil { - return nil, fmt.Errorf("failed to set VolumeSnapshotBeingCreated annotation to No on the content %s: %q", content.Name, err) + return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err) } // Update content in the cache store From 6ca7507bc3c70b75ef0ccf2a4f49c6b343fb2691 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Sun, 8 Mar 2020 01:21:15 +0000 Subject: [PATCH 05/10] Address review comments --- pkg/sidecar-controller/snapshot_controller.go | 28 +++++++------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 6bf91cca5..b519c5658 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -303,11 +303,7 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c return updateContent, nil } -// The function goes through the whole snapshot creation process. -// 1. Trigger the snapshot through csi storage provider. -// 2. Update VolumeSnapshot status with creationtimestamp information -// 3. Create the VolumeSnapshotContent object with the snapshot id information. -// 4. Bind the VolumeSnapshot and VolumeSnapshotContent object +// 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) @@ -325,12 +321,12 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 } // NOTE(xyang): handle create timeout - // Add annotation to indicate create snapshot request is - // sent to the storage system and wait for a response of success or failure. - // Annotation will be removed only after storage system has responded - // with success or failure. If the request times out, retry will happen - // and annotation will remain to avoid leaking of snapshot - // resources on the storage system + // Add an annotation to indicate the snapshot creation request has been + // sent to the storage system and the controller is waiting for a response. + // The annotation will be removed after the storage system has responded with + // 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) if err != nil { return nil, fmt.Errorf("failed to add VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err) @@ -341,8 +337,10 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 // NOTE(xyang): handle create timeout // If it is not a timeout error, remove annotation to indicate // storage system has responded with an error + klog.Infof("createSnapshotOperation: CreateSnapshot for content %s returned error: %v", content.Name, err) if e, ok := status.FromError(err); ok { - if e.Code() == codes.DeadlineExceeded { + klog.Infof("createSnapshotOperation: CreateSnapshot for content %s error status: %+v", content.Name, e) + if e.Code() != codes.DeadlineExceeded { err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) if err != nil { return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %q", content.Name, err) @@ -372,12 +370,6 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err) } - // Update content in the cache store - _, err = ctrl.storeContentUpdate(content) - if err != nil { - klog.Errorf("failed to update content store %v", err) - } - return content, nil } From 0189c77f5aa3e489250e2c8cc6f926d05bfe3f73 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Mon, 16 Mar 2020 01:49:58 +0000 Subject: [PATCH 06/10] Check final error --- pkg/sidecar-controller/content_create_test.go | 11 +-- pkg/sidecar-controller/snapshot_controller.go | 79 +++++++++++-------- pkg/utils/util.go | 7 +- 3 files changed, 58 insertions(+), 39 deletions(-) diff --git a/pkg/sidecar-controller/content_create_test.go b/pkg/sidecar-controller/content_create_test.go index 475224b2e..babc144a8 100644 --- a/pkg/sidecar-controller/content_create_test.go +++ b/pkg/sidecar-controller/content_create_test.go @@ -30,10 +30,11 @@ func TestSyncContent(t *testing.T) { tests := []controllerTest{ { - name: "1-1: Basic content update ready to use", - initialContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true), - expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true), - expectedEvents: noevents, + name: "1-1: Basic content update ready to use", + initialContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true), + expectedContents: withContentAnnotations(newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true), + map[string]string{}), + expectedEvents: noevents, expectedCreateCalls: []createCall{ { volumeHandle: "volume-handle-1-1", @@ -162,7 +163,7 @@ func TestSyncContent(t *testing.T) { SnapshotHandle: toStringPointer("sid1-6"), RestoreSize: &defaultSize, ReadyToUse: &False, - 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\\\"\""), + 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\\\"\""), }), expectedEvents: []string{"Warning SnapshotContentCheckandUpdateFailed"}, expectedCreateCalls: []createCall{ diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index b519c5658..4742dbc1a 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -278,29 +278,21 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c } driverName = content.Spec.Driver snapshotID = *content.Spec.Source.SnapshotHandle - } else { - class, snapshotterCredentials, err := ctrl.getCSISnapshotInput(content) - if err != nil { - return nil, fmt.Errorf("failed to get input parameters to create snapshot %s: %q", content.Name, err) + + klog.V(5).Infof("checkandUpdateContentStatusOperation: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse) + + if creationTime.IsZero() { + creationTime = time.Now() } - driverName, snapshotID, creationTime, size, readyToUse, err = ctrl.handler.CreateSnapshot(content, class.Parameters, snapshotterCredentials) + updatedContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size) if err != nil { - klog.Errorf("checkandUpdateContentStatusOperation: failed to call create snapshot to check whether the snapshot is ready to use %q", err) return nil, err } + return updatedContent, nil + } else { + return ctrl.createSnapshotOperation(content) } - klog.V(5).Infof("checkandUpdateContentStatusOperation: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse) - - if creationTime.IsZero() { - creationTime = time.Now() - } - - updateContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size) - if err != nil { - return nil, err - } - return updateContent, nil } // The function goes through the snapshot creation process. @@ -335,16 +327,13 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 driverName, snapshotID, creationTime, size, readyToUse, err := ctrl.handler.CreateSnapshot(content, class.Parameters, snapshotterCredentials) if err != nil { // NOTE(xyang): handle create timeout - // If it is not a timeout error, remove annotation to indicate + // If it is a final error, remove annotation to indicate // storage system has responded with an error klog.Infof("createSnapshotOperation: CreateSnapshot for content %s returned error: %v", content.Name, err) - if e, ok := status.FromError(err); ok { - klog.Infof("createSnapshotOperation: CreateSnapshot for content %s error status: %+v", content.Name, e) - if e.Code() != codes.DeadlineExceeded { - err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) - if err != nil { - return nil, fmt.Errorf("failed to remove VolumeSnapshotBeingCreated annotation from the content %s: %q", content.Name, err) - } + if isFinalError(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) } } @@ -353,8 +342,11 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 klog.V(5).Infof("Created snapshot: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse) - timestamp := creationTime.UnixNano() - newContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, timestamp, size) + if creationTime.IsZero() { + creationTime = time.Now() + } + + 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) @@ -614,12 +606,12 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte contentClone := content.DeepCopy() metav1.SetMetaDataAnnotation(&contentClone.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated, "yes") - updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone) + updatedContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone) if err != nil { return newControllerUpdateError(content.Name, err.Error()) } // update content if update is successful - content = updateContent + content = updatedContent _, err = ctrl.storeContentUpdate(content) if err != nil { @@ -641,12 +633,12 @@ func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(con contentClone := content.DeepCopy() delete(contentClone.ObjectMeta.Annotations, utils.AnnVolumeSnapshotBeingCreated) - updateContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone) + updatedContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone) if err != nil { return newControllerUpdateError(content.Name, err.Error()) } // update content if update is successful - content = updateContent + content = updatedContent klog.V(5).Infof("Removed VolumeSnapshotBeingCreated annotation from volume snapshot content %s", content.Name) _, err = ctrl.storeContentUpdate(content) @@ -655,3 +647,28 @@ func (ctrl csiSnapshotSideCarController) removeAnnVolumeSnapshotBeingCreated(con } return nil } + +// This function checks if the error is final +func isFinalError(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 + st, ok := status.FromError(err) + if !ok { + // This is not gRPC error. The operation must have failed before gRPC + // method was called, otherwise we would get gRPC error. + // We don't know if any previous CreateSnapshot is in progress, be on the safe side. + return false + } + switch st.Code() { + case codes.Canceled, // gRPC: Client Application cancelled the request + codes.DeadlineExceeded, // gRPC: Timeout + codes.Unavailable, // gRPC: Server shutting down, TCP connection broken - previous CreateSnapshot() may be still in progress. + codes.ResourceExhausted, // gRPC: Server temporarily out of resources - previous CreateSnapshot() may be still in progress. + codes.Aborted: // CSI: Operation pending for Snapshot + return false + } + // All other errors mean that creating snapshot either did not + // even start or failed. It is for sure not in progress. + return true +} diff --git a/pkg/utils/util.go b/pkg/utils/util.go index b0fc07496..99fcdc9aa 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -82,9 +82,10 @@ const ( // If it is set, it indicates that the csi-snapshotter // sidecar has sent the create snapshot request to the storage system and // is waiting for a response of success or failure. - // This annotation will be removed if the driver's CreateSnapshot - // CSI function returns success or failure. If the create snapshot - // request times out, retry will happen and the annotation will remain. + // This annotation will be removed once the driver's CreateSnapshot + // CSI function returns success or a final error (determined by isFinalError()). + // If the create snapshot request fails with a non-final error such as timeout, + // retry will happen and the annotation will remain. // This only applies to dynamic provisioning of snapshots because // the create snapshot CSI method will not be called for pre-provisioned // snapshots. From f32313bdb8bd39bbb08e6626f2f04afca2100115 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Wed, 1 Apr 2020 04:07:45 +0000 Subject: [PATCH 07/10] checkandUpdateContentStatusOperation should not call createSnapshotOperation --- pkg/sidecar-controller/snapshot_controller.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 4742dbc1a..5db2e0bfa 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -291,7 +291,7 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c } return updatedContent, nil } else { - return ctrl.createSnapshotOperation(content) + return ctrl.createSnapshotWrapper(content) } } @@ -307,6 +307,13 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 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) + 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) @@ -329,7 +336,7 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotOperation(content *crdv1 // NOTE(xyang): handle create timeout // If it is a final error, remove annotation to indicate // storage system has responded with an error - klog.Infof("createSnapshotOperation: CreateSnapshot for content %s returned error: %v", content.Name, err) + klog.Infof("createSnapshotWrapper: CreateSnapshot for content %s returned error: %v", content.Name, err) if isFinalError(err) { err = ctrl.removeAnnVolumeSnapshotBeingCreated(content) if err != nil { From 9f382acd4329d8cd2da5ba3a8e0769e4b4948eca Mon Sep 17 00:00:00 2001 From: xing-yang Date: Thu, 2 Apr 2020 03:22:01 +0000 Subject: [PATCH 08/10] Address review comments --- pkg/sidecar-controller/snapshot_controller.go | 79 ++++++++++--------- 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 5db2e0bfa..eedf60854 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -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) @@ -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 } @@ -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) @@ -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) @@ -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 } @@ -607,26 +604,30 @@ 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 } @@ -634,7 +635,7 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte // 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() @@ -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 From 2295e6aea3b25d7421128ab3204113e68ab5e3d1 Mon Sep 17 00:00:00 2001 From: xing-yang Date: Fri, 3 Apr 2020 01:37:54 +0000 Subject: [PATCH 09/10] Add error status check in createSnapshotWrapper --- pkg/sidecar-controller/snapshot_controller.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index eedf60854..04440b73c 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -133,13 +133,6 @@ 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 { - // 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)) @@ -311,6 +304,14 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) { klog.Infof("createSnapshotWrapper: 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 + } + 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) @@ -624,7 +625,6 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte _, 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) From f688d7baef900d55b742f0c3b17811910522013d Mon Sep 17 00:00:00 2001 From: xing-yang Date: Fri, 3 Apr 2020 18:34:57 +0000 Subject: [PATCH 10/10] Remove error status check --- pkg/sidecar-controller/snapshot_controller.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 04440b73c..68e48ac0f 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -304,14 +304,6 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotContent, error) { klog.Infof("createSnapshotWrapper: 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 - } - 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)