From 060ee23d6d807d8389d564bbd49ddfbb8ec24d5f Mon Sep 17 00:00:00 2001 From: Raunak Pradip Shah Date: Tue, 7 Nov 2023 11:26:37 +0530 Subject: [PATCH] Delete individual volume snapshots as part of the group snapshot delete API and prevent users from deleting individual volume snapshots if it is part of an existing group snapshot Also revert commit bb29899ca37369fe7c3bc3c861d3186b50e2fc65. --- client/hack/update-crd.sh | 2 +- cmd/csi-snapshotter/main.go | 18 +++----- cmd/snapshot-controller/main.go | 6 +-- .../groupsnapshot_controller_helper.go | 41 ++++++++++-------- pkg/common-controller/snapshot_controller.go | 43 +++++++++++++++++++ pkg/sidecar-controller/content_create_test.go | 6 +-- pkg/sidecar-controller/csi_handler.go | 12 +++--- pkg/sidecar-controller/framework_test.go | 17 ++++---- .../groupsnapshot_helper.go | 1 - pkg/sidecar-controller/snapshot_controller.go | 27 ++++++++---- .../snapshot_delete_test.go | 20 ++++----- pkg/snapshotter/snapshotter.go | 14 +++--- pkg/snapshotter/snapshotter_test.go | 2 +- 13 files changed, 129 insertions(+), 80 deletions(-) diff --git a/client/hack/update-crd.sh b/client/hack/update-crd.sh index 2012837d1..1ad39e98f 100755 --- a/client/hack/update-crd.sh +++ b/client/hack/update-crd.sh @@ -28,7 +28,7 @@ then TMP_DIR=$(mktemp -d); cd $TMP_DIR; go mod init tmp; - go get sigs.k8s.io/controller-tools/cmd/controller-gen@v0.11.3; + go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.12.0; rm -rf $TMP_DIR; CONTROLLER_GEN=$(which controller-gen) fi diff --git a/cmd/csi-snapshotter/main.go b/cmd/csi-snapshotter/main.go index 7fc975b41..a53a0378e 100644 --- a/cmd/csi-snapshotter/main.go +++ b/cmd/csi-snapshotter/main.go @@ -79,14 +79,13 @@ var ( kubeAPIQPS = flag.Float64("kube-api-qps", 5, "QPS to use while communicating with the kubernetes apiserver. Defaults to 5.0.") kubeAPIBurst = flag.Int("kube-api-burst", 10, "Burst to use while communicating with the kubernetes apiserver. Defaults to 10.") - metricsAddress = flag.String("metrics-address", "", "(deprecated) The TCP network address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means metrics endpoint is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.") - httpEndpoint = flag.String("http-endpoint", "", "The TCP network address where the HTTP server for diagnostics, including metrics and leader election health check, will listen (example: `:8080`). The default is empty string, which means the server is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.") - metricsPath = flag.String("metrics-path", "/metrics", "The HTTP path where prometheus metrics will be exposed. Default is `/metrics`.") - retryIntervalStart = flag.Duration("retry-interval-start", time.Second, "Initial retry interval of failed volume snapshot creation or deletion. It doubles with each failure, up to retry-interval-max. Default is 1 second.") - retryIntervalMax = flag.Duration("retry-interval-max", 5*time.Minute, "Maximum retry interval of failed volume snapshot creation or deletion. Default is 5 minutes.") - enableNodeDeployment = flag.Bool("node-deployment", false, "Enables deploying the sidecar controller together with a CSI driver on nodes to manage snapshots for node-local volumes.") - // TODO(xing-yang): Enable enableVolumeGroupSnapshots when the feature is fully implemented - // enableVolumeGroupSnapshots = flag.Bool("enable-volume-group-snapshots", false, "Enables the volume group snapshot feature, allowing the user to create snapshots of groups of volumes.") + metricsAddress = flag.String("metrics-address", "", "(deprecated) The TCP network address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means metrics endpoint is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.") + httpEndpoint = flag.String("http-endpoint", "", "The TCP network address where the HTTP server for diagnostics, including metrics and leader election health check, will listen (example: `:8080`). The default is empty string, which means the server is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.") + metricsPath = flag.String("metrics-path", "/metrics", "The HTTP path where prometheus metrics will be exposed. Default is `/metrics`.") + retryIntervalStart = flag.Duration("retry-interval-start", time.Second, "Initial retry interval of failed volume snapshot creation or deletion. It doubles with each failure, up to retry-interval-max. Default is 1 second.") + retryIntervalMax = flag.Duration("retry-interval-max", 5*time.Minute, "Maximum retry interval of failed volume snapshot creation or deletion. Default is 5 minutes.") + enableNodeDeployment = flag.Bool("node-deployment", false, "Enables deploying the sidecar controller together with a CSI driver on nodes to manage snapshots for node-local volumes.") + enableVolumeGroupSnapshots = flag.Bool("enable-volume-group-snapshots", false, "Enables the volume group snapshot feature, allowing the user to create a snapshot of a group of volumes.") groupSnapshotNamePrefix = flag.String("groupsnapshot-name-prefix", "groupsnapshot", "Prefix to apply to the name of a created group snapshot") groupSnapshotNameUUIDLength = flag.Int("groupsnapshot-name-uuid-length", -1, "Length in characters for the generated uuid of a created group snapshot. Defaults behavior is to NOT truncate.") @@ -228,9 +227,6 @@ func main() { snapShotter := snapshotter.NewSnapshotter(csiConn) var groupSnapshotter group_snapshotter.GroupSnapshotter - // TODO(xing-yang): Remove the following line when the enableVolumeGroupSnapshots feature is enabled - bEnable := false - enableVolumeGroupSnapshots := &bEnable if *enableVolumeGroupSnapshots { supportsCreateVolumeGroupSnapshot, err := supportsGroupControllerCreateVolumeGroupSnapshot(ctx, csiConn) if err != nil { diff --git a/cmd/snapshot-controller/main.go b/cmd/snapshot-controller/main.go index f62fab7cc..e31b19b87 100644 --- a/cmd/snapshot-controller/main.go +++ b/cmd/snapshot-controller/main.go @@ -72,8 +72,7 @@ var ( retryIntervalMax = flag.Duration("retry-interval-max", 5*time.Minute, "Maximum retry interval of failed volume snapshot creation or deletion. Default is 5 minutes.") enableDistributedSnapshotting = flag.Bool("enable-distributed-snapshotting", false, "Enables each node to handle snapshotting for the local volumes created on that node") preventVolumeModeConversion = flag.Bool("prevent-volume-mode-conversion", false, "Prevents an unauthorised user from modifying the volume mode when creating a PVC from an existing VolumeSnapshot.") - // TODO(xing-yang): Enable enableVolumeGroupSnapshots when the feature is fully implemented - // enableVolumeGroupSnapshots = flag.Bool("enable-volume-group-snapshots", false, "Enables the volume group snapshot feature, allowing the user to create snapshots of groups of volumes.") + enableVolumeGroupSnapshots = flag.Bool("enable-volume-group-snapshots", false, "Enables the volume group snapshot feature, allowing the user to create a snapshot of a group of volumes.") retryCRDIntervalMax = flag.Duration("retry-crd-interval-max", 5*time.Second, "Maximum retry interval to wait for CRDs to appear. The default is 5 seconds.") ) @@ -208,9 +207,6 @@ func main() { klog.V(2).Infof("Start NewCSISnapshotController with kubeconfig [%s] resyncPeriod [%+v]", *kubeconfig, *resyncPeriod) - // TODO(xing-yang): Remove the following lines when the enableVolumeGroupSnapshots feature is enabled - bEnable := false - enableVolumeGroupSnapshots := &bEnable ctrl := controller.NewCSISnapshotCommonController( snapClient, kubeClient, diff --git a/pkg/common-controller/groupsnapshot_controller_helper.go b/pkg/common-controller/groupsnapshot_controller_helper.go index bc0b21c3c..543638eb3 100644 --- a/pkg/common-controller/groupsnapshot_controller_helper.go +++ b/pkg/common-controller/groupsnapshot_controller_helper.go @@ -1103,16 +1103,7 @@ func (ctrl *csiSnapshotCommonController) processGroupSnapshotWithDeletionTimesta groupSnapshotContent = nil } - klog.V(5).Infof("processGroupSnapshotWithDeletionTimestamp[%s]: delete group snapshot content and remove finalizer from group snapshot if needed", utils.GroupSnapshotKey(groupSnapshot)) - - return ctrl.checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent(groupSnapshot, groupSnapshotContent, deleteGroupSnapshotContent) -} - -// checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent deletes -// the group snapshot content and removes group snapshot finalizers if needed -func (ctrl *csiSnapshotCommonController) checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent(groupSnapshot *crdv1alpha1.VolumeGroupSnapshot, groupSnapshotContent *crdv1alpha1.VolumeGroupSnapshotContent, deleteGroupSnapshotContent bool) error { - klog.V(5).Infof("checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent VolumeGroupSnapshot[%s]: %s", utils.GroupSnapshotKey(groupSnapshot), utils.GetGroupSnapshotStatusForLogging(groupSnapshot)) - + klog.V(5).Infof("processGroupSnapshotWithDeletionTimestamp[%s]: check if group snapshot is a candidate for deletion", utils.GroupSnapshotKey(groupSnapshot)) if !utils.IsGroupSnapshotDeletionCandidate(groupSnapshot) { return nil } @@ -1135,14 +1126,15 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveGroupSnapshotFinalizersAn } - // regardless of the deletion policy, set the VolumeSnapshotBeingDeleted on - // content object, this is to allow snapshotter sidecar controller to conduct - // a delete operation whenever the content has deletion timestamp set. + // regardless of the deletion policy, set VolumeGroupSnapshotBeingDeleted on + // group snapshot content object, this is to allow snapshotter sidecar controller + // to conduct a delete operation whenever the group snapshot content has deletion + // timestamp set. if groupSnapshotContent != nil { - klog.V(5).Infof("checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent[%s]: Set VolumeGroupSnapshotBeingDeleted annotation on the group snapshot content [%s]", utils.GroupSnapshotKey(groupSnapshot), groupSnapshotContent.Name) + klog.V(5).Infof("processGroupSnapshotWithDeletionTimestamp[%s]: Set VolumeGroupSnapshotBeingDeleted annotation on the group snapshot content [%s]", utils.GroupSnapshotKey(groupSnapshot), groupSnapshotContent.Name) updatedGroupSnapshotContent, err := ctrl.setAnnVolumeGroupSnapshotBeingDeleted(groupSnapshotContent) if err != nil { - klog.V(4).Infof("checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent[%s]: failed to set VolumeGroupSnapshotBeingDeleted annotation on the group snapshot content [%s]", utils.GroupSnapshotKey(groupSnapshot), groupSnapshotContent.Name) + klog.V(4).Infof("processGroupSnapshotWithDeletionTimestamp[%s]: failed to set VolumeGroupSnapshotBeingDeleted annotation on the group snapshot content [%s]: %v", utils.GroupSnapshotKey(groupSnapshot), groupSnapshotContent.Name, err) return err } groupSnapshotContent = updatedGroupSnapshotContent @@ -1150,10 +1142,10 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveGroupSnapshotFinalizersAn // VolumeGroupSnapshot should be deleted. Check and remove finalizers // If group snapshot content exists and has a deletion policy of Delete, set - // DeletionTimeStamp on the content; + // DeletionTimeStamp on the group snapshot content; // VolumeGroupSnapshotContent won't be deleted immediately due to the VolumeGroupSnapshotContentFinalizer if groupSnapshotContent != nil && deleteGroupSnapshotContent { - klog.V(5).Infof("checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent: set DeletionTimeStamp on group snapshot content [%s].", groupSnapshotContent.Name) + klog.V(5).Infof("processGroupSnapshotWithDeletionTimestamp[%s]: set DeletionTimeStamp on group snapshot content [%s].", utils.GroupSnapshotKey(groupSnapshot), groupSnapshotContent.Name) err := ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshotContents().Delete(context.TODO(), groupSnapshotContent.Name, metav1.DeleteOptions{}) if err != nil { ctrl.eventRecorder.Event(groupSnapshot, v1.EventTypeWarning, "GroupSnapshotContentObjectDeleteError", "Failed to delete group snapshot content API object") @@ -1161,7 +1153,20 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveGroupSnapshotFinalizersAn } } - klog.V(5).Infof("checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent: Remove Finalizer for VolumeGroupSnapshot[%s]", utils.GroupSnapshotKey(groupSnapshot)) + klog.V(5).Infof("processGroupSnapshotWithDeletionTimestamp[%s]: Delete individual snapshots that are part of the group snapshot", utils.GroupSnapshotKey(groupSnapshot)) + + // Delete the individual snapshots part of the group snapshot + for _, snapshot := range groupSnapshot.Status.VolumeSnapshotRefList { + err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshot.Namespace).Delete(context.TODO(), snapshot.Name, metav1.DeleteOptions{}) + if err != nil { + msg := fmt.Sprintf("failed to delete snapshot API object %s/%s part of group snapshot %s: %v", snapshot.Namespace, snapshot.Name, utils.GroupSnapshotKey(groupSnapshot), err) + klog.Error(msg) + ctrl.eventRecorder.Event(groupSnapshot, v1.EventTypeWarning, "SnapshotDeleteError", msg) + return fmt.Errorf(msg) + } + } + + klog.V(5).Infof("processGroupSnapshotWithDeletionTimestamp[%s] : Remove Finalizer for VolumeGroupSnapshot", utils.GroupSnapshotKey(groupSnapshot)) // remove VolumeSnapshotBoundFinalizer on the VolumeGroupSnapshot object: // a. If there is no group snapshot content found, remove the finalizer. // b. If the group snapshot content is being deleted, i.e., with deleteGroupSnapshotContent == true, diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 2b1d051e9..d509fb296 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -286,6 +286,21 @@ func (ctrl *csiSnapshotCommonController) processSnapshotWithDeletionTimestamp(sn content = nil } + // Block deletion if this snapshot belongs to a group snapshot. + if snapshot.Status != nil && snapshot.Status.VolumeGroupSnapshotName != nil { + groupSnapshot, err := ctrl.groupSnapshotLister.VolumeGroupSnapshots(snapshot.Namespace).Get(*snapshot.Status.VolumeGroupSnapshotName) + if err == nil { + msg := fmt.Sprintf("deletion of the individual volume snapshot %s is not allowed as it belongs to group snapshot %s. Deleting the group snapshot will trigger the deletion of all the individual volume snapshots that are part of the group.", utils.SnapshotKey(snapshot), utils.GroupSnapshotKey(groupSnapshot)) + klog.Error(msg) + ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "SnapshotDeleteError", msg) + return fmt.Errorf(msg) + } + if !apierrs.IsNotFound(err) { + klog.Errorf("failed to delete snapshot %s: %v", utils.SnapshotKey(snapshot), err) + return err + } + } + klog.V(5).Infof("processSnapshotWithDeletionTimestamp[%s]: delete snapshot content and remove finalizer from snapshot if needed", utils.SnapshotKey(snapshot)) return ctrl.checkandRemoveSnapshotFinalizersAndCheckandDeleteContent(snapshot, content, deleteContent) @@ -1139,6 +1154,27 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.Vo volumeSnapshotErr = content.Status.Error.DeepCopy() } + var groupSnapshotName string + if content.Status != nil && content.Status.VolumeGroupSnapshotHandle != nil { + // If this snapshot belongs to a group snapshot, find the group snapshot + // name from the group snapshot content + groupSnapshotContentList, err := ctrl.groupSnapshotContentLister.List(labels.Everything()) + if err != nil { + return nil, err + } + found := false + for _, groupSnapshotContent := range groupSnapshotContentList { + if groupSnapshotContent.Status != nil && groupSnapshotContent.Status.VolumeGroupSnapshotHandle != nil && *groupSnapshotContent.Status.VolumeGroupSnapshotHandle == *content.Status.VolumeGroupSnapshotHandle { + groupSnapshotName = groupSnapshotContent.Spec.VolumeGroupSnapshotRef.Name + found = true + break + } + } + if !found { + return nil, fmt.Errorf("updateSnapshotStatus: cannot find the group snapshot for VolumeSnapshot [%s], will not update snapshot status", utils.SnapshotKey(snapshot)) + } + } + klog.V(5).Infof("updateSnapshotStatus: updating VolumeSnapshot [%+v] based on VolumeSnapshotContentStatus [%+v]", snapshot, content.Status) snapshotObj, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshot.Namespace).Get(context.TODO(), snapshot.Name, metav1.GetOptions{}) @@ -1162,6 +1198,9 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.Vo if volumeSnapshotErr != nil { newStatus.Error = volumeSnapshotErr } + if groupSnapshotName != "" { + newStatus.VolumeGroupSnapshotName = &groupSnapshotName + } updated = true } else { newStatus = snapshotObj.Status.DeepCopy() @@ -1188,6 +1227,10 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotStatus(snapshot *crdv1.Vo newStatus.Error = volumeSnapshotErr updated = true } + if newStatus.VolumeGroupSnapshotName == nil && groupSnapshotName != "" { + newStatus.VolumeGroupSnapshotName = &groupSnapshotName + updated = true + } } if updated { diff --git a/pkg/sidecar-controller/content_create_test.go b/pkg/sidecar-controller/content_create_test.go index a58ac80df..c96c5df83 100644 --- a/pkg/sidecar-controller/content_create_test.go +++ b/pkg/sidecar-controller/content_create_test.go @@ -49,7 +49,7 @@ func TestSyncContent(t *testing.T) { readyToUse: true, }, }, - expectedListCalls: []listCall{{"sid1-1", map[string]string{}, true, time.Now(), 1, nil}}, + expectedListCalls: []listCall{{"sid1-1", map[string]string{}, true, time.Now(), 1, nil, ""}}, expectSuccess: true, errors: noerrors, test: testSyncContent, @@ -78,7 +78,7 @@ func TestSyncContent(t *testing.T) { size: defaultSize, }, }, - expectedListCalls: []listCall{{"sid1-2", map[string]string{}, true, time.Now(), 1, nil}}, + expectedListCalls: []listCall{{"sid1-2", map[string]string{}, true, time.Now(), 1, nil, ""}}, expectSuccess: true, errors: noerrors, test: testSyncContent, @@ -194,7 +194,7 @@ func TestSyncContent(t *testing.T) { readyToUse: true, }, }, - expectedListCalls: []listCall{{"sid1-6", map[string]string{}, true, time.Now(), 1, nil}}, + expectedListCalls: []listCall{{"sid1-6", map[string]string{}, true, time.Now(), 1, nil, ""}}, errors: noerrors, test: testSyncContent, }, diff --git a/pkg/sidecar-controller/csi_handler.go b/pkg/sidecar-controller/csi_handler.go index 711e7bac3..c0f89a771 100644 --- a/pkg/sidecar-controller/csi_handler.go +++ b/pkg/sidecar-controller/csi_handler.go @@ -34,7 +34,7 @@ import ( type Handler interface { CreateSnapshot(content *crdv1.VolumeSnapshotContent, parameters map[string]string, snapshotterCredentials map[string]string) (string, string, time.Time, int64, bool, error) DeleteSnapshot(content *crdv1.VolumeSnapshotContent, snapshotterCredentials map[string]string) error - GetSnapshotStatus(content *crdv1.VolumeSnapshotContent, snapshotterListCredentials map[string]string) (bool, time.Time, int64, error) + GetSnapshotStatus(content *crdv1.VolumeSnapshotContent, snapshotterListCredentials map[string]string) (bool, time.Time, int64, string, error) CreateGroupSnapshot(content *crdv1alpha1.VolumeGroupSnapshotContent, volumeIDs []string, parameters map[string]string, snapshotterCredentials map[string]string) (string, string, []*csi.Snapshot, time.Time, bool, error) GetGroupSnapshotStatus(content *crdv1alpha1.VolumeGroupSnapshotContent, snapshotterListCredentials map[string]string) (bool, time.Time, error) DeleteGroupSnapshot(content *crdv1alpha1.VolumeGroupSnapshotContent, SnapshotID []string, snapshotterCredentials map[string]string) error @@ -113,7 +113,7 @@ func (handler *csiHandler) DeleteSnapshot(content *crdv1.VolumeSnapshotContent, return nil } -func (handler *csiHandler) GetSnapshotStatus(content *crdv1.VolumeSnapshotContent, snapshotterListCredentials map[string]string) (bool, time.Time, int64, error) { +func (handler *csiHandler) GetSnapshotStatus(content *crdv1.VolumeSnapshotContent, snapshotterListCredentials map[string]string) (bool, time.Time, int64, string, error) { ctx, cancel := context.WithTimeout(context.Background(), handler.timeout) defer cancel() @@ -124,15 +124,15 @@ func (handler *csiHandler) GetSnapshotStatus(content *crdv1.VolumeSnapshotConten } else if content.Spec.Source.SnapshotHandle != nil { snapshotHandle = *content.Spec.Source.SnapshotHandle } else { - return false, time.Time{}, 0, fmt.Errorf("failed to list snapshot for content %s: snapshotHandle is missing", content.Name) + return false, time.Time{}, 0, "", fmt.Errorf("failed to list snapshot for content %s: snapshotHandle is missing", content.Name) } - csiSnapshotStatus, timestamp, size, err := handler.snapshotter.GetSnapshotStatus(ctx, snapshotHandle, snapshotterListCredentials) + csiSnapshotStatus, timestamp, size, groupSnapshotID, err := handler.snapshotter.GetSnapshotStatus(ctx, snapshotHandle, snapshotterListCredentials) if err != nil { - return false, time.Time{}, 0, fmt.Errorf("failed to list snapshot for content %s: %q", content.Name, err) + return false, time.Time{}, 0, "", fmt.Errorf("failed to list snapshot for content %s: %q", content.Name, err) } - return csiSnapshotStatus, timestamp, size, nil + return csiSnapshotStatus, timestamp, size, groupSnapshotID, nil } func makeSnapshotName(prefix, snapshotUID string, snapshotNameUUIDLength int) (string, error) { diff --git a/pkg/sidecar-controller/framework_test.go b/pkg/sidecar-controller/framework_test.go index 849a524bf..e69e38628 100644 --- a/pkg/sidecar-controller/framework_test.go +++ b/pkg/sidecar-controller/framework_test.go @@ -882,10 +882,11 @@ type listCall struct { snapshotID string secrets map[string]string // information to return - readyToUse bool - createTime time.Time - size int64 - err error + readyToUse bool + createTime time.Time + size int64 + err error + groupSnapshotID string } type deleteCall struct { @@ -982,10 +983,10 @@ func (f *fakeSnapshotter) DeleteSnapshot(ctx context.Context, snapshotID string, return call.err } -func (f *fakeSnapshotter) GetSnapshotStatus(ctx context.Context, snapshotID string, snapshotterListCredentials map[string]string) (bool, time.Time, int64, error) { +func (f *fakeSnapshotter) GetSnapshotStatus(ctx context.Context, snapshotID string, snapshotterListCredentials map[string]string) (bool, time.Time, int64, string, error) { if f.listCallCounter >= len(f.listCalls) { f.t.Errorf("Unexpected CSI list Snapshot call: snapshotID=%s, index: %d, calls: %+v", snapshotID, f.createCallCounter, f.createCalls) - return false, time.Time{}, 0, fmt.Errorf("unexpected call") + return false, time.Time{}, 0, "", fmt.Errorf("unexpected call") } call := f.listCalls[f.listCallCounter] f.listCallCounter++ @@ -1002,10 +1003,10 @@ func (f *fakeSnapshotter) GetSnapshotStatus(ctx context.Context, snapshotID stri } if err != nil { - return false, time.Time{}, 0, fmt.Errorf("unexpected call") + return false, time.Time{}, 0, "", fmt.Errorf("unexpected call") } - return call.readyToUse, call.createTime, call.size, call.err + return call.readyToUse, call.createTime, call.size, call.groupSnapshotID, call.err } func newSnapshotError(message string) *crdv1.VolumeSnapshotError { diff --git a/pkg/sidecar-controller/groupsnapshot_helper.go b/pkg/sidecar-controller/groupsnapshot_helper.go index 0be2880ee..f4a0cc4e8 100644 --- a/pkg/sidecar-controller/groupsnapshot_helper.go +++ b/pkg/sidecar-controller/groupsnapshot_helper.go @@ -471,7 +471,6 @@ func (ctrl *csiSnapshotSideCarController) createGroupSnapshotWrapper(groupSnapsh }, }, } - vsc, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Create(context.TODO(), volumeSnapshotContent, metav1.CreateOptions{}) if err != nil { return groupSnapshotContent, err diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 02a93da05..799ff678c 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -22,13 +22,14 @@ import ( "strings" "time" - crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1" - "github.com/kubernetes-csi/external-snapshotter/v6/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" klog "k8s.io/klog/v2" + + crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1" + "github.com/kubernetes-csi/external-snapshotter/v6/pkg/utils" ) // Design: @@ -251,7 +252,7 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c var size int64 readyToUse := false var driverName string - var snapshotID string + var snapshotID, groupSnapshotID string var snapshotterListCredentials map[string]string if content.Spec.Source.SnapshotHandle != nil { @@ -278,7 +279,7 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c } } - readyToUse, creationTime, size, err = ctrl.handler.GetSnapshotStatus(content, snapshotterListCredentials) + readyToUse, creationTime, size, groupSnapshotID, 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 content, err @@ -286,13 +287,13 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c driverName = content.Spec.Driver snapshotID = *content.Spec.Source.SnapshotHandle - klog.V(5).Infof("checkandUpdateContentStatusOperation: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t", driverName, snapshotID, creationTime, size, readyToUse) + klog.V(5).Infof("checkandUpdateContentStatusOperation: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t, groupSnapshotID %s", driverName, snapshotID, creationTime, size, readyToUse, groupSnapshotID) if creationTime.IsZero() { creationTime = time.Now() } - updatedContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size) + updatedContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size, groupSnapshotID) if err != nil { return content, err } @@ -354,7 +355,7 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V creationTime = time.Now() } - newContent, err := ctrl.updateSnapshotContentStatus(content, snapshotID, readyToUse, creationTime.UnixNano(), size) + 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 content, fmt.Errorf("error updating status for volume snapshot content %s: %v", content.Name, err) @@ -429,8 +430,9 @@ func (ctrl *csiSnapshotSideCarController) updateSnapshotContentStatus( snapshotHandle string, readyToUse bool, createdAt int64, - size int64) (*crdv1.VolumeSnapshotContent, error) { - klog.V(5).Infof("updateSnapshotContentStatus: updating VolumeSnapshotContent [%s], snapshotHandle %s, readyToUse %v, createdAt %v, size %d", content.Name, snapshotHandle, readyToUse, createdAt, size) + size int64, + groupSnapshotID string) (*crdv1.VolumeSnapshotContent, error) { + klog.V(5).Infof("updateSnapshotContentStatus: updating VolumeSnapshotContent [%s], snapshotHandle %s, readyToUse %v, createdAt %v, size %d, groupSnapshotID %s", content.Name, snapshotHandle, readyToUse, createdAt, size, groupSnapshotID) contentObj, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Get(context.TODO(), content.Name, metav1.GetOptions{}) if err != nil { @@ -446,6 +448,9 @@ func (ctrl *csiSnapshotSideCarController) updateSnapshotContentStatus( CreationTime: &createdAt, RestoreSize: &size, } + if groupSnapshotID != "" { + newStatus.VolumeGroupSnapshotHandle = &groupSnapshotID + } updated = true } else { newStatus = contentObj.Status.DeepCopy() @@ -468,6 +473,10 @@ func (ctrl *csiSnapshotSideCarController) updateSnapshotContentStatus( newStatus.RestoreSize = &size updated = true } + if newStatus.VolumeGroupSnapshotHandle == nil && groupSnapshotID != "" { + newStatus.VolumeGroupSnapshotHandle = &groupSnapshotID + updated = true + } } if updated { diff --git a/pkg/sidecar-controller/snapshot_delete_test.go b/pkg/sidecar-controller/snapshot_delete_test.go index ecdf6af15..f8c0a69cf 100644 --- a/pkg/sidecar-controller/snapshot_delete_test.go +++ b/pkg/sidecar-controller/snapshot_delete_test.go @@ -171,7 +171,7 @@ func TestDeleteSync(t *testing.T) { readyToUse: true, }, }, - expectedListCalls: []listCall{{"sid1-1", map[string]string{}, true, time.Now(), 1, nil}}, + expectedListCalls: []listCall{{"sid1-1", map[string]string{}, true, time.Now(), 1, nil, ""}}, expectedDeleteCalls: []deleteCall{{"sid1-1", nil, nil}}, expectSuccess: true, test: testSyncContent, @@ -194,7 +194,7 @@ func TestDeleteSync(t *testing.T) { readyToUse: true, }, }, - expectedListCalls: []listCall{{"sid1-2", map[string]string{}, true, time.Now(), 1, nil}}, + expectedListCalls: []listCall{{"sid1-2", map[string]string{}, true, time.Now(), 1, nil, ""}}, expectedDeleteCalls: []deleteCall{{"sid1-2", nil, nil}}, expectSuccess: true, test: testSyncContent, @@ -218,7 +218,7 @@ func TestDeleteSync(t *testing.T) { }, expectedDeleteCalls: []deleteCall{{"sid1-3", nil, fmt.Errorf("mock csi driver delete error")}}, expectedEvents: []string{"Warning SnapshotDeleteError"}, - expectedListCalls: []listCall{{"sid1-3", map[string]string{}, true, time.Now(), 1, nil}}, + expectedListCalls: []listCall{{"sid1-3", map[string]string{}, true, time.Now(), 1, nil, ""}}, test: testSyncContent, }, { @@ -238,7 +238,7 @@ func TestDeleteSync(t *testing.T) { name: "1-5 - csi driver delete snapshot returns error, bound finalizer should remain", initialContents: newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "snap1-5-volumehandle", deletionPolicy, nil, &defaultSize, true, &timeNowMetav1), expectedContents: newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "snap1-5-volumehandle", deletionPolicy, nil, &defaultSize, true, &timeNowMetav1), - expectedListCalls: []listCall{{"sid1-5", map[string]string{}, true, time.Now(), 1000, nil}}, + expectedListCalls: []listCall{{"sid1-5", map[string]string{}, true, time.Now(), 1000, nil, ""}}, expectedDeleteCalls: []deleteCall{{"sid1-5", nil, errors.New("mock csi driver delete error")}}, expectedEvents: []string{"Warning SnapshotDeleteError"}, errors: noerrors, @@ -249,7 +249,7 @@ func TestDeleteSync(t *testing.T) { name: "1-6 - content is deleted before deleting", initialContents: newContentArray("content1-6", "sid1-6", "snap1-6", "sid1-6", classGold, "sid1-6", "", deletionPolicy, nil, nil, true), expectedContents: nocontents, - expectedListCalls: []listCall{{"sid1-6", nil, false, time.Now(), 0, nil}}, + expectedListCalls: []listCall{{"sid1-6", nil, false, time.Now(), 0, nil, ""}}, expectedDeleteCalls: []deleteCall{{"sid1-6", map[string]string{"foo": "bar"}, nil}}, expectedEvents: noevents, errors: noerrors, @@ -265,7 +265,7 @@ func TestDeleteSync(t *testing.T) { initialContents: newContentArrayWithReadyToUse("content1-7", "", "snap1-7", "sid1-7", validSecretClass, "sid1-7", "", deletePolicy, nil, &defaultSize, &True, true), expectedContents: newContentArrayWithReadyToUse("content1-7", "", "snap1-7", "sid1-7", validSecretClass, "sid1-7", "", deletePolicy, nil, &defaultSize, &True, true), expectedEvents: noevents, - expectedListCalls: []listCall{{"sid1-7", map[string]string{}, true, time.Now(), 1000, nil}}, + expectedListCalls: []listCall{{"sid1-7", map[string]string{}, true, time.Now(), 1000, nil, ""}}, expectSuccess: true, initialSecrets: []*v1.Secret{secret()}, errors: noerrors, @@ -276,7 +276,7 @@ func TestDeleteSync(t *testing.T) { initialContents: newContentArrayWithReadyToUse("content1-8", "sid1-8", "none-existed-snapshot", "sid1-8", validSecretClass, "sid1-8", "", retainPolicy, nil, &defaultSize, &True, true), expectedContents: newContentArrayWithReadyToUse("content1-8", "sid1-8", "none-existed-snapshot", "sid1-8", validSecretClass, "sid1-8", "", retainPolicy, nil, &defaultSize, &True, true), expectedEvents: noevents, - expectedListCalls: []listCall{{"sid1-8", map[string]string{}, true, time.Now(), 0, nil}}, + expectedListCalls: []listCall{{"sid1-8", map[string]string{}, true, time.Now(), 0, nil, ""}}, expectSuccess: true, errors: noerrors, test: testSyncContent, @@ -286,7 +286,7 @@ func TestDeleteSync(t *testing.T) { initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime), expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime), expectedEvents: noevents, - expectedListCalls: []listCall{{"sid1-9", map[string]string{}, true, time.Now(), 0, nil}}, + expectedListCalls: []listCall{{"sid1-9", map[string]string{}, true, time.Now(), 0, nil, ""}}, expectSuccess: true, errors: noerrors, initialSecrets: []*v1.Secret{}, // secret does not exist @@ -298,7 +298,7 @@ func TestDeleteSync(t *testing.T) { initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &nonFractionalTime), expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &nonFractionalTime), expectedEvents: noevents, - expectedListCalls: []listCall{{"sid1-10", map[string]string{}, true, time.Now(), 0, nil}}, + expectedListCalls: []listCall{{"sid1-10", map[string]string{}, true, time.Now(), 0, nil, ""}}, expectSuccess: true, errors: noerrors, initialSecrets: []*v1.Secret{}, @@ -319,7 +319,7 @@ func TestDeleteSync(t *testing.T) { initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &nonFractionalTime), expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &nonFractionalTime), expectedEvents: noevents, - expectedListCalls: []listCall{{"sid1-12", map[string]string{}, true, time.Now(), 0, nil}}, + expectedListCalls: []listCall{{"sid1-12", map[string]string{}, true, time.Now(), 0, nil, ""}}, expectSuccess: true, errors: noerrors, initialSecrets: []*v1.Secret{}, diff --git a/pkg/snapshotter/snapshotter.go b/pkg/snapshotter/snapshotter.go index 2a85b35a6..05f6fa660 100644 --- a/pkg/snapshotter/snapshotter.go +++ b/pkg/snapshotter/snapshotter.go @@ -38,7 +38,7 @@ type Snapshotter interface { DeleteSnapshot(ctx context.Context, snapshotID string, snapshotterCredentials map[string]string) (err error) // GetSnapshotStatus returns if a snapshot is ready to use, creation time, and restore size. - GetSnapshotStatus(ctx context.Context, snapshotID string, snapshotterListCredentials map[string]string) (bool, time.Time, int64, error) + GetSnapshotStatus(ctx context.Context, snapshotID string, snapshotterListCredentials map[string]string) (bool, time.Time, int64, string, error) } type snapshot struct { @@ -109,7 +109,7 @@ func (s *snapshot) isListSnapshotsSupported(ctx context.Context) (bool, error) { return false, nil } -func (s *snapshot) GetSnapshotStatus(ctx context.Context, snapshotID string, snapshotterListCredentials map[string]string) (bool, time.Time, int64, error) { +func (s *snapshot) GetSnapshotStatus(ctx context.Context, snapshotID string, snapshotterListCredentials map[string]string) (bool, time.Time, int64, string, error) { klog.V(5).Infof("GetSnapshotStatus: %s", snapshotID) client := csi.NewControllerClient(s.conn) @@ -117,10 +117,10 @@ func (s *snapshot) GetSnapshotStatus(ctx context.Context, snapshotID string, sna // If the driver does not support ListSnapshots, assume the snapshot ID is valid. listSnapshotsSupported, err := s.isListSnapshotsSupported(ctx) if err != nil { - return false, time.Time{}, 0, fmt.Errorf("failed to check if ListSnapshots is supported: %s", err.Error()) + return false, time.Time{}, 0, "", fmt.Errorf("failed to check if ListSnapshots is supported: %s", err.Error()) } if !listSnapshotsSupported { - return true, time.Time{}, 0, nil + return true, time.Time{}, 0, "", nil } req := csi.ListSnapshotsRequest{ SnapshotId: snapshotID, @@ -128,13 +128,13 @@ func (s *snapshot) GetSnapshotStatus(ctx context.Context, snapshotID string, sna } rsp, err := client.ListSnapshots(ctx, &req) if err != nil { - return false, time.Time{}, 0, err + return false, time.Time{}, 0, "", err } if rsp.Entries == nil || len(rsp.Entries) == 0 { - return false, time.Time{}, 0, fmt.Errorf("can not find snapshot for snapshotID %s", snapshotID) + return false, time.Time{}, 0, "", fmt.Errorf("can not find snapshot for snapshotID %s", snapshotID) } creationTime := rsp.Entries[0].Snapshot.CreationTime.AsTime() - return rsp.Entries[0].Snapshot.ReadyToUse, creationTime, rsp.Entries[0].Snapshot.SizeBytes, nil + return rsp.Entries[0].Snapshot.ReadyToUse, creationTime, rsp.Entries[0].Snapshot.SizeBytes, rsp.Entries[0].Snapshot.GroupSnapshotId, nil } diff --git a/pkg/snapshotter/snapshotter_test.go b/pkg/snapshotter/snapshotter_test.go index 54a7f97b6..6f225a287 100644 --- a/pkg/snapshotter/snapshotter_test.go +++ b/pkg/snapshotter/snapshotter_test.go @@ -468,7 +468,7 @@ func TestGetSnapshotStatus(t *testing.T) { } s := NewSnapshotter(csiConn) - ready, createTime, size, err := s.GetSnapshotStatus(context.Background(), test.snapshotID, test.snapshotterListCredentials) + ready, createTime, size, _, err := s.GetSnapshotStatus(context.Background(), test.snapshotID, test.snapshotterListCredentials) if test.expectError && err == nil { t.Errorf("test %q: Expected error, got none", test.name) }