From 2c42912eaacd3e1c2bc19e4a73bdb7dab9b134a4 Mon Sep 17 00:00:00 2001 From: phuhung273 Date: Fri, 7 Nov 2025 20:06:50 +0700 Subject: [PATCH] Add Volume deletion validation Signed-off-by: phuhung273 --- pkg/cloud/cloud.go | 39 +++++++++++- pkg/cloud/cloud_test.go | 112 +++++++++++++++++++++++++++++++-- pkg/cloud/fakes.go | 23 ++++++- pkg/driver/controller.go | 19 +++++- pkg/driver/controller_test.go | 67 +++++++++++++++++++- pkg/driver/mocks/mock_cloud.go | 28 +++++++++ 6 files changed, 275 insertions(+), 13 deletions(-) diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index bfe53f7..b603b93 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -19,6 +19,10 @@ import ( "context" "errors" "fmt" + "os" + "strings" + "time" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/aws-sdk-go-v2/service/fsx" @@ -26,9 +30,6 @@ import ( "github.com/kubernetes-sigs/aws-fsx-openzfs-csi-driver/pkg/util" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" - "os" - "strings" - "time" ) // Polling @@ -102,11 +103,13 @@ type Cloud interface { DeleteFileSystem(ctx context.Context, parameters map[string]string) error DescribeFileSystem(ctx context.Context, fileSystemId string) (*FileSystem, error) WaitForFileSystemAvailable(ctx context.Context, fileSystemId string) error + WaitForFileSystemDeletion(ctx context.Context, fileSystemId string) error WaitForFileSystemResize(ctx context.Context, fileSystemId string, resizeGiB int32) error CreateVolume(ctx context.Context, parameters map[string]string) (*Volume, error) DeleteVolume(ctx context.Context, parameters map[string]string) error DescribeVolume(ctx context.Context, volumeId string) (*Volume, error) WaitForVolumeAvailable(ctx context.Context, volumeId string) error + WaitForVolumeDeletion(ctx context.Context, volumeId string) error WaitForVolumeResize(ctx context.Context, volumeId string, resizeGiB int32) error CreateSnapshot(ctx context.Context, options map[string]string) (*Snapshot, error) DeleteSnapshot(ctx context.Context, parameters map[string]string) error @@ -242,6 +245,21 @@ func (c *cloud) WaitForFileSystemAvailable(ctx context.Context, fileSystemId str return err } +func (c *cloud) WaitForFileSystemDeletion(ctx context.Context, fileSystemId string) error { + err := wait.PollUntilContextTimeout(ctx, PollCheckInterval, PollCheckTimeout, true, func(ctx context.Context) (done bool, err error) { + _, err = c.getFileSystem(ctx, fileSystemId) + if err == ErrNotFound { + return true, nil + } else if err != nil { + return true, err + } + klog.V(2).InfoS("WaitForFileSystemDeletion", "filesystem", fileSystemId) + return false, nil + }) + + return err +} + func (c *cloud) WaitForFileSystemResize(ctx context.Context, fileSystemId string, resizeGiB int32) error { err := wait.Poll(PollCheckInterval, PollCheckTimeout, func() (done bool, err error) { updateAction, err := c.getUpdateResizeFilesystemAdministrativeAction(ctx, fileSystemId, resizeGiB) @@ -340,6 +358,21 @@ func (c *cloud) WaitForVolumeAvailable(ctx context.Context, volumeId string) err return err } +func (c *cloud) WaitForVolumeDeletion(ctx context.Context, volumeId string) error { + err := wait.PollUntilContextTimeout(ctx, PollCheckInterval, PollCheckTimeout, true, func(ctx context.Context) (done bool, err error) { + _, err = c.getVolume(ctx, volumeId) + if err == ErrNotFound { + return true, nil + } else if err != nil { + return true, err + } + klog.V(2).InfoS("WaitForVolumeDeletion", "volume", volumeId) + return false, nil + }) + + return err +} + // WaitForVolumeResize TODO: Remove this function and its associated tests. func (c *cloud) WaitForVolumeResize(ctx context.Context, volumeId string, resizeGiB int32) error { err := wait.Poll(PollCheckInterval, PollCheckTimeout, func() (done bool, err error) { diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go index e65fb4c..a77787d 100644 --- a/pkg/cloud/cloud_test.go +++ b/pkg/cloud/cloud_test.go @@ -18,16 +18,17 @@ package cloud import ( "context" "errors" + "reflect" + "strconv" + "testing" + "time" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/fsx" "github.com/aws/aws-sdk-go-v2/service/fsx/types" "github.com/golang/mock/gomock" "github.com/kubernetes-sigs/aws-fsx-openzfs-csi-driver/pkg/cloud/mocks" "github.com/kubernetes-sigs/aws-fsx-openzfs-csi-driver/pkg/util" - "reflect" - "strconv" - "testing" - "time" ) func TestCreateFileSystem(t *testing.T) { @@ -594,6 +595,57 @@ func TestWaitForFileSystemAvailable(t *testing.T) { } } +func TestWaitForFileSystemDeletion(t *testing.T) { + fileSystemId := "fs-123456789abcdefgh" + testCases := []struct { + name string + testFunc func(t *testing.T) + }{ + { + name: "success: DescribeFileSystems return not found", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockFSx := mocks.NewMockFSx(mockCtl) + c := &cloud{ + fsx: mockFSx, + } + + ctx := context.Background() + mockFSx.EXPECT().DescribeFileSystems(gomock.Any(), gomock.Any()).Return(nil, ErrNotFound) + err := c.WaitForFileSystemDeletion(ctx, fileSystemId) + if err != nil { + t.Fatalf("WaitForFileSystemDeletion is failed: %v", err) + } + + mockCtl.Finish() + }, + }, + { + name: "fail: DescribeFileSystems return other error", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockFSx := mocks.NewMockFSx(mockCtl) + c := &cloud{ + fsx: mockFSx, + } + + ctx := context.Background() + mockFSx.EXPECT().DescribeFileSystems(gomock.Any(), gomock.Any()).Return(nil, errors.New("")) + err := c.WaitForFileSystemDeletion(ctx, fileSystemId) + if err == nil { + t.Fatalf("WaitForFileSystemDeletion is not failed: %v", err) + } + + mockCtl.Finish() + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, tc.testFunc) + } +} + func TestWaitForFileSystemResize(t *testing.T) { var ( filesystemId = "fs-1234" @@ -1149,6 +1201,58 @@ func TestWaitForVolumeAvailable(t *testing.T) { } } +func TestWaitForVolumeDeletion(t *testing.T) { + volumeId := "fsvol-0987654321abcdefg" + + testCases := []struct { + name string + testFunc func(t *testing.T) + }{ + { + name: "success: DescribeVolumes return empty", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockFSx := mocks.NewMockFSx(mockCtl) + c := &cloud{ + fsx: mockFSx, + } + + ctx := context.Background() + mockFSx.EXPECT().DescribeVolumes(gomock.Any(), gomock.Any()).Return(nil, ErrNotFound) + err := c.WaitForVolumeDeletion(ctx, volumeId) + if err != nil { + t.Fatalf("WaitForVolumeDeletion is failed: %v", err) + } + + mockCtl.Finish() + }, + }, + { + name: "fail: DescribeVolumes return other error", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockFSx := mocks.NewMockFSx(mockCtl) + c := &cloud{ + fsx: mockFSx, + } + + ctx := context.Background() + mockFSx.EXPECT().DescribeVolumes(gomock.Any(), gomock.Any()).Return(nil, errors.New("")) + err := c.WaitForVolumeDeletion(ctx, volumeId) + if err == nil { + t.Fatalf("WaitForVolumeDeletion is not failed: %v", err) + } + + mockCtl.Finish() + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, tc.testFunc) + } +} + func TestWaitForVolumeResize(t *testing.T) { var ( volumeId = "fsvol-1234" diff --git a/pkg/cloud/fakes.go b/pkg/cloud/fakes.go index 3a7da35..08db983 100644 --- a/pkg/cloud/fakes.go +++ b/pkg/cloud/fakes.go @@ -18,12 +18,13 @@ package cloud import ( "context" "fmt" - "github.com/aws/aws-sdk-go-v2/service/fsx/types" "math/rand" "reflect" "strconv" "strings" "time" + + "github.com/aws/aws-sdk-go-v2/service/fsx/types" ) var random *rand.Rand @@ -117,8 +118,14 @@ func (c *FakeCloudProvider) ResizeFileSystem(ctx context.Context, fileSystemId s } func (c *FakeCloudProvider) DeleteFileSystem(ctx context.Context, parameters map[string]string) error { + // parameters["FileSystemId"] in Sanity test is JSON string, eg: "\"fs-1234\"" + // While actual id is "fs-1234" + // For backward compatibility, we can remove both the JSON string and the unquote string + unquoteId := strings.Trim(parameters["FileSystemId"], "\"") delete(c.fileSystems, parameters["FileSystemId"]) + delete(c.fileSystems, unquoteId) delete(c.fileSystemsParameters, parameters["FileSystemId"]) + delete(c.fileSystemsParameters, unquoteId) return nil } @@ -135,6 +142,10 @@ func (c *FakeCloudProvider) WaitForFileSystemAvailable(ctx context.Context, file return nil } +func (c *FakeCloudProvider) WaitForFileSystemDeletion(ctx context.Context, fileSystemId string) error { + return nil +} + func (c *FakeCloudProvider) WaitForFileSystemResize(ctx context.Context, fileSystemId string, newSizeGiB int32) error { return nil } @@ -171,8 +182,14 @@ func (c *FakeCloudProvider) CreateVolume(ctx context.Context, parameters map[str } func (c *FakeCloudProvider) DeleteVolume(ctx context.Context, parameters map[string]string) (err error) { + // parameters["VolumeId"] in Sanity test is JSON string, eg: "\"fsvol-1234\"" + // While actual id is "fsvol-1234" + // For backward compatibility, we can remove both the JSON string and the unquote string + unquoteId := strings.Trim(parameters["VolumeId"], "\"") delete(c.volumes, parameters["VolumeId"]) + delete(c.volumes, unquoteId) delete(c.volumesParameters, parameters["VolumeId"]) + delete(c.volumesParameters, unquoteId) return nil } @@ -189,6 +206,10 @@ func (c *FakeCloudProvider) WaitForVolumeAvailable(ctx context.Context, volumeId return nil } +func (c *FakeCloudProvider) WaitForVolumeDeletion(ctx context.Context, volumeId string) error { + return nil +} + func (c *FakeCloudProvider) WaitForVolumeResize(ctx context.Context, volumeId string, newSizeGiB int32) error { return nil } diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 0fa835d..e1a0117 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -19,6 +19,10 @@ import ( "context" "errors" "fmt" + "os" + "strconv" + "strings" + "github.com/aws/aws-sdk-go-v2/service/fsx/types" "github.com/container-storage-interface/spec/lib/go/csi" "github.com/kubernetes-sigs/aws-fsx-openzfs-csi-driver/pkg/cloud" @@ -28,9 +32,6 @@ import ( "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/timestamppb" "k8s.io/klog/v2" - "os" - "strconv" - "strings" ) var ( @@ -369,6 +370,18 @@ func (d *controllerService) DeleteVolume(ctx context.Context, req *csi.DeleteVol return nil, status.Errorf(codes.Internal, "Could not delete volume ID %q: %v", volumeID, err) } + switch splitVolumeId[0] { + case cloud.FilesystemPrefix: + err = d.cloud.WaitForFileSystemDeletion(ctx, volumeID) + case cloud.VolumePrefix: + err = d.cloud.WaitForVolumeDeletion(ctx, volumeID) + } + + if err != nil { + return nil, status.Errorf(codes.Internal, "Could not delete volume ID %q: %v", volumeID, err) + } + + klog.V(4).InfoS("DeleteVolume: volume not found, returning with success", "volumeId", volumeID) return &csi.DeleteVolumeResponse{}, nil } diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index c2f90ef..31d80c8 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -18,6 +18,9 @@ package driver import ( "context" "errors" + "testing" + "time" + "github.com/aws/aws-sdk-go-v2/service/fsx/types" "github.com/container-storage-interface/spec/lib/go/csi" "github.com/golang/mock/gomock" @@ -26,8 +29,6 @@ import ( "github.com/kubernetes-sigs/aws-fsx-openzfs-csi-driver/pkg/driver/mocks" "github.com/kubernetes-sigs/aws-fsx-openzfs-csi-driver/pkg/util" "google.golang.org/protobuf/types/known/timestamppb" - "testing" - "time" ) func TestCreateVolume(t *testing.T) { @@ -1028,6 +1029,7 @@ func TestDeleteVolume(t *testing.T) { ctx := context.Background() mockCloud.EXPECT().GetDeleteParameters(gomock.Eq(ctx), gomock.Any()).Return(filesystemParameters, nil) mockCloud.EXPECT().DeleteFileSystem(gomock.Eq(ctx), gomock.Any()).Return(nil) + mockCloud.EXPECT().WaitForFileSystemDeletion(gomock.Eq(ctx), gomock.Any()).Return(nil) _, err := driver.DeleteVolume(ctx, req) if err != nil { @@ -1056,6 +1058,7 @@ func TestDeleteVolume(t *testing.T) { ctx := context.Background() mockCloud.EXPECT().GetDeleteParameters(gomock.Eq(ctx), gomock.Any()).Return(map[string]string{}, nil) mockCloud.EXPECT().DeleteFileSystem(gomock.Eq(ctx), gomock.Any()).Return(nil) + mockCloud.EXPECT().WaitForFileSystemDeletion(gomock.Eq(ctx), gomock.Any()).Return(nil) _, err := driver.DeleteVolume(ctx, req) if err != nil { @@ -1084,6 +1087,7 @@ func TestDeleteVolume(t *testing.T) { ctx := context.Background() mockCloud.EXPECT().GetDeleteParameters(gomock.Eq(ctx), gomock.Any()).Return(volumeParameters, nil) mockCloud.EXPECT().DeleteVolume(gomock.Eq(ctx), gomock.Any()).Return(nil) + mockCloud.EXPECT().WaitForVolumeDeletion(gomock.Eq(ctx), gomock.Any()).Return(nil) _, err := driver.DeleteVolume(ctx, req) if err != nil { @@ -1112,6 +1116,7 @@ func TestDeleteVolume(t *testing.T) { ctx := context.Background() mockCloud.EXPECT().GetDeleteParameters(gomock.Eq(ctx), gomock.Any()).Return(map[string]string{}, nil) mockCloud.EXPECT().DeleteVolume(gomock.Eq(ctx), gomock.Any()).Return(nil) + mockCloud.EXPECT().WaitForVolumeDeletion(gomock.Eq(ctx), gomock.Any()).Return(nil) _, err := driver.DeleteVolume(ctx, req) if err != nil { @@ -1204,6 +1209,35 @@ func TestDeleteVolume(t *testing.T) { mockCtl.Finish() }, }, + { + name: "fail: WaitForFileSystemDeletion returns error", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := controllerService{ + cloud: mockCloud, + inFlight: internal.NewInFlight(), + driverOptions: &DriverOptions{}, + } + + req := &csi.DeleteVolumeRequest{ + VolumeId: fileSystemId, + } + + ctx := context.Background() + mockCloud.EXPECT().GetDeleteParameters(gomock.Eq(ctx), gomock.Any()).Return(volumeParameters, nil) + mockCloud.EXPECT().DeleteFileSystem(gomock.Eq(ctx), gomock.Any()).Return(nil) + mockCloud.EXPECT().WaitForFileSystemDeletion(gomock.Eq(ctx), gomock.Any()).Return(errors.New("")) + + _, err := driver.DeleteVolume(ctx, req) + if err == nil { + t.Fatal("DeleteVolume is not failed") + } + + mockCtl.Finish() + }, + }, { name: "success: DeleteVolume returns ErrNotFound", testFunc: func(t *testing.T) { @@ -1257,6 +1291,35 @@ func TestDeleteVolume(t *testing.T) { t.Fatal("DeleteVolume is not failed") } + mockCtl.Finish() + }, + }, + { + name: "fail: WaitForVolumeDeletion returns error", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := controllerService{ + cloud: mockCloud, + inFlight: internal.NewInFlight(), + driverOptions: &DriverOptions{}, + } + + req := &csi.DeleteVolumeRequest{ + VolumeId: volumeId, + } + + ctx := context.Background() + mockCloud.EXPECT().GetDeleteParameters(gomock.Eq(ctx), gomock.Any()).Return(volumeParameters, nil) + mockCloud.EXPECT().DeleteVolume(gomock.Eq(ctx), gomock.Any()).Return(nil) + mockCloud.EXPECT().WaitForVolumeDeletion(gomock.Eq(ctx), gomock.Any()).Return(errors.New("")) + + _, err := driver.DeleteVolume(ctx, req) + if err == nil { + t.Fatal("DeleteVolume is not failed") + } + mockCtl.Finish() }, }, diff --git a/pkg/driver/mocks/mock_cloud.go b/pkg/driver/mocks/mock_cloud.go index 3a5b6c8..8c1bf16 100644 --- a/pkg/driver/mocks/mock_cloud.go +++ b/pkg/driver/mocks/mock_cloud.go @@ -226,6 +226,20 @@ func (mr *MockCloudMockRecorder) WaitForFileSystemAvailable(arg0, arg1 interface return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WaitForFileSystemAvailable", reflect.TypeOf((*MockCloud)(nil).WaitForFileSystemAvailable), arg0, arg1) } +// WaitForFileSystemDeletion mocks base method. +func (m *MockCloud) WaitForFileSystemDeletion(arg0 context.Context, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WaitForFileSystemDeletion", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// WaitForFileSystemDeletion indicates an expected call of WaitForFileSystemDeletion. +func (mr *MockCloudMockRecorder) WaitForFileSystemDeletion(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WaitForFileSystemDeletion", reflect.TypeOf((*MockCloud)(nil).WaitForFileSystemDeletion), arg0, arg1) +} + // WaitForFileSystemResize mocks base method. func (m *MockCloud) WaitForFileSystemResize(arg0 context.Context, arg1 string, arg2 int32) error { m.ctrl.T.Helper() @@ -268,6 +282,20 @@ func (mr *MockCloudMockRecorder) WaitForVolumeAvailable(arg0, arg1 interface{}) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WaitForVolumeAvailable", reflect.TypeOf((*MockCloud)(nil).WaitForVolumeAvailable), arg0, arg1) } +// WaitForVolumeDeletion mocks base method. +func (m *MockCloud) WaitForVolumeDeletion(arg0 context.Context, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WaitForVolumeDeletion", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// WaitForVolumeDeletion indicates an expected call of WaitForVolumeDeletion. +func (mr *MockCloudMockRecorder) WaitForVolumeDeletion(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WaitForVolumeDeletion", reflect.TypeOf((*MockCloud)(nil).WaitForVolumeDeletion), arg0, arg1) +} + // WaitForVolumeResize mocks base method. func (m *MockCloud) WaitForVolumeResize(arg0 context.Context, arg1 string, arg2 int32) error { m.ctrl.T.Helper()