Skip to content

disk: don't get VGS to check for an unsupported parameter #1386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

disk: don't get VGS to check for an unsupported parameter #1386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 4 additions & 49 deletions pkg/disk/group_volume_snapshot_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package disk

import (
"context"
"fmt"
"slices"
"strings"
Expand All @@ -10,48 +9,16 @@ import (
"github.com/aliyun/alibaba-cloud-sdk-go/services/ecs"
"github.com/container-storage-interface/spec/lib/go/csi"
"github.com/kubernetes-sigs/alibaba-cloud-csi-driver/pkg/common"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/timestamppb"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/klog/v2"
)

func getVolumeGroupSnapshotConfig(req *csi.CreateVolumeGroupSnapshotRequest) (*createGroupSnapshotParams, error) {
var ecsParams createGroupSnapshotParams
if req.Parameters != nil {
err := parseGroupSnapshotParameters(req.Parameters, &ecsParams)
if err != nil {
klog.Errorf("CreateSnapshot:: Snapshot name[%s], parse config failed: %v", req.Name, err)
return nil, err
}
}

vsName := req.Parameters[common.VolumeGroupSnapshotNameKey]
vsNameSpace := req.Parameters[common.VolumeGroupSnapshotNamespaceKey]
// volumesnapshot not in parameters, just return
if vsName == "" || vsNameSpace == "" {
return &ecsParams, nil
}

volumeGroupSnapshot, err := GlobalConfigVar.SnapClient.GroupsnapshotV1alpha1().VolumeGroupSnapshots(vsNameSpace).Get(context.Background(), vsName, metav1.GetOptions{})
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to get VolumeGroupSnapshot: %s/%s: %v", vsNameSpace, vsName, err)
}
err = parseGroupSnapshotAnnotations(volumeGroupSnapshot.Annotations, &ecsParams)
if err != nil {
klog.Errorf("CreateVolumeGroupSnapshot:: Snapshot name[%s], parse annotation failed: %v", req.Name, err)
return nil, err
}
return &ecsParams, nil
}

func parseGroupSnapshotParameters(params map[string]string, ecsParams *createGroupSnapshotParams) (err error) {
func parseGroupSnapshotParameters(params map[string]string) (*createGroupSnapshotParams, error) {
ecsParams := &createGroupSnapshotParams{}
tags := make(map[string]string)
for k, v := range params {
switch k {
case RETENTIONDAYS:
return fmt.Errorf("groupSnapshot do not support retentionDays: %w", err)
return nil, fmt.Errorf("groupSnapshot do not support retentionDays")
case SNAPSHOTRESOURCEGROUPID:
ecsParams.ResourceGroupID = v
case common.VolumeGroupSnapshotNameKey:
Expand Down Expand Up @@ -81,19 +48,7 @@ func parseGroupSnapshotParameters(params map[string]string, ecsParams *createGro
})
}
}
return nil
}

// if volumesnapshot have Annotations, use it first.
// storage.alibabacloud.com/snapshot-ttl
func parseGroupSnapshotAnnotations(anno map[string]string, ecsParams *createGroupSnapshotParams) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BackupCenter may rely on this annotation. As each backup has different expected TTL. It's not considerable to create a new VGSclass everytime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do support setting snapshot-ttl in the future, we should pass that in from external-snapshotter. Get VGS in CSI is not correct. User may have deleted the VGS when we get it. User may even have created another VGS with the same name and we may get wrong info.

For the TTL case. Can we just implement a controller to delete the VS/VGS from k8s, instead of rely on ECS to delete the underlying snapshot?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backup case, VGS/VS will be deleted after it's done and its templated has uploaded to OSS. And to ensure the date security, they will all be set to Retain. And backupcenter controller do not and should not rely on the version of CSI. It's means the similar logic of VS cannot be removed or else TTL will not work for backup.

snapshotTTL := anno[common.SnapshotTTLKey]

if snapshotTTL != "" {
return fmt.Errorf("groupSnapshot do not support retentionDays")
}

return nil
return ecsParams, nil
}

func formatGroupSnapshot(groupSnapshot *ecs.SnapshotGroup) (*csi.VolumeGroupSnapshot, error) {
Expand Down
19 changes: 1 addition & 18 deletions pkg/disk/group_volume_snapshot_utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,6 @@ import (
"google.golang.org/protobuf/types/known/timestamppb"
)

func Test_parseGroupSnapshotAnnotations(t *testing.T) {
annotations := make(map[string]string)
ecsParams := &createGroupSnapshotParams{}

// Test case 1: snapshotTTL is empty
annotations[common.SnapshotTTLKey] = ""
err := parseGroupSnapshotAnnotations(annotations, ecsParams)
assert.Nil(t, err)

// Test case 2: snapshotTTL is not empty
annotations[common.SnapshotTTLKey] = "10"
err = parseGroupSnapshotAnnotations(annotations, ecsParams)
assert.Error(t, err)
}

func Test_formatGroupSnapshot(t *testing.T) {
// Create a mock groupSnapshot object
mockGroupSnapshot := &ecs.SnapshotGroup{
Expand Down Expand Up @@ -455,9 +440,7 @@ func Test_parseGroupSnapshotParameters(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ecsParams := &createGroupSnapshotParams{}

err := parseGroupSnapshotParameters(tt.params, ecsParams)
ecsParams, err := parseGroupSnapshotParameters(tt.params)

if tt.wantError {
assert.Error(t, err)
Expand Down
2 changes: 1 addition & 1 deletion pkg/disk/groupcontrollerserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func (cs *groupControllerServer) CreateVolumeGroupSnapshot(ctx context.Context,
// todo: Do not check source disks here. If need, use `checkSourceVolumes`

// init createSnapshotGroupRequest and parameters
params, err := getVolumeGroupSnapshotConfig(req)
params, err := parseGroupSnapshotParameters(req.Parameters)
if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "get volumeGroupSnapshot %s config failed: %v", req.GetName(), err)
}
Expand Down