From a80e7b01e2fc8dc3f278b714d6c1ec1545dcfaef Mon Sep 17 00:00:00 2001 From: Oscar Torreno Date: Fri, 29 Dec 2023 22:06:08 +0100 Subject: [PATCH] Reduce calls to EFS API With #850, the new way of allocating GIDs introduced a new call to the ListAccessPoints endpoint of EFS API, that is generating problems on systems where EFS CSI driver is under high load (lots of PVC created within a short time period). In this PR, we are extracting the ListAccessPoints call from gid_allocator, moving it one level up. In case of dynamic provisioining of GIDs we can reuse the ListAccessPoints call to check for the file system existence (thus removing the DescribeFileSystem call in such case). In case of a fixed UID/GID, we continue calling DescribeFileSystem, and no calls to ListAccessPoints. In addition to the change explained above, gidMin and gidMax have been converted to int64. #850 made both uid and gid int64, but gidMin and gidMax were nout touched. Also changing the default value for gidMin, as the value of 50000 was spamming the logs with a message coming from gid_allocator (i.e. range bigger than the max number of access points). Setting the value to 6999000 (the value that gid_allocator was setting by substracting 1000 to gidMax). Removing an unused function and field from gid_allocator too. --- pkg/cloud/cloud.go | 4 +- pkg/driver/controller.go | 27 ++-- pkg/driver/controller_test.go | 225 +++++++++++++++++++--------------- pkg/driver/gid_allocator.go | 38 ++---- 4 files changed, 157 insertions(+), 137 deletions(-) diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index f6051abd8..ad064eb60 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -300,10 +300,10 @@ func (c *cloud) ListAccessPoints(ctx context.Context, fileSystemId string) (acce res, err := c.efs.DescribeAccessPointsWithContext(ctx, describeAPInput) if err != nil { if isAccessDenied(err) { - return + return nil, ErrAccessDenied } if isFileSystemNotFound(err) { - return + return nil, ErrNotFound } err = fmt.Errorf("List Access Points failed: %v", err) return diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 6f701981f..3626a2b27 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -38,8 +38,8 @@ const ( AccessPointMode = "efs-ap" AzName = "az" BasePath = "basePath" - DefaultGidMin = 50000 - DefaultGidMax = 7000000 + DefaultGidMin = int64(6999000) + DefaultGidMax = int64(7000000) DefaultTagKey = "efs.csi.aws.com/cluster" DefaultTagValue = "true" DirectoryPerms = "directoryPerms" @@ -120,8 +120,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) azName string basePath string gid int64 - gidMin int - gidMax int + gidMin int64 + gidMax int64 localCloud cloud.Cloud provisioningMode string roleArn string @@ -189,7 +189,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) } if value, ok := volumeParams[GidMin]; ok { - gidMin, err = strconv.Atoi(value) + gidMin, err = strconv.ParseInt(value, 10, 64) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", GidMin, err) } @@ -203,7 +203,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) if gidMin == 0 { return nil, status.Errorf(codes.InvalidArgument, "Missing %v parameter", GidMin) } - gidMax, err = strconv.Atoi(value) + gidMax, err = strconv.ParseInt(value, 10, 64) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "Failed to parse invalid %v: %v", GidMax, err) } @@ -241,20 +241,27 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) return nil, err } - // Check if file system exists. Describe FS handles appropriate error codes - if _, err = localCloud.DescribeFileSystem(ctx, accessPointsOptions.FileSystemId); err != nil { + // Check if file system exists. Describe FS or List APs handle appropriate error codes + // With dynamic uid/gid provisioning we can save a call to describe FS, as list APs fails if FS ID does not exist + var accessPoints []*cloud.AccessPoint + if uid == -1 || gid == -1 { + accessPoints, err = localCloud.ListAccessPoints(ctx, accessPointsOptions.FileSystemId) + } else { + _, err = localCloud.DescribeFileSystem(ctx, accessPointsOptions.FileSystemId) + } + if err != nil { if err == cloud.ErrAccessDenied { return nil, status.Errorf(codes.Unauthenticated, "Access Denied. Please ensure you have the right AWS permissions: %v", err) } if err == cloud.ErrNotFound { return nil, status.Errorf(codes.InvalidArgument, "File System does not exist: %v", err) } - return nil, status.Errorf(codes.Internal, "Failed to fetch File System info: %v", err) + return nil, status.Errorf(codes.Internal, "Failed to fetch Access Points for File System: %v", err) } var allocatedGid int64 if uid == -1 || gid == -1 { - allocatedGid, err = d.gidAllocator.getNextGid(ctx, localCloud, accessPointsOptions.FileSystemId, gidMin, gidMax) + allocatedGid, err = d.gidAllocator.getNextGid(accessPointsOptions.FileSystemId, accessPoints, gidMin, gidMax) if err != nil { return nil, err } diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index e4755cbab..f2457a8fc 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -203,9 +203,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -230,7 +227,6 @@ func TestCreateVolume(t *testing.T) { } var expectedGid int64 = 1001 //1003 and 1002 are taken, next available is 1001 - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(accessPoint, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -289,9 +285,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } ap1 := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -329,7 +322,6 @@ func TestCreateVolume(t *testing.T) { accessPoints := []*cloud.AccessPoint{ap1, ap2, ap3} var expectedGid int64 = 1004 // 1001-1003 is taken. - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(ap2, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -347,7 +339,6 @@ func TestCreateVolume(t *testing.T) { accessPoints = []*cloud.AccessPoint{} expectedGid = 1001 // 1001 is now free and lowest possible, if no GID return would happen allocator would pick 1005. - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(ap3, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -365,7 +356,6 @@ func TestCreateVolume(t *testing.T) { accessPoints = []*cloud.AccessPoint{ap1, ap4} expectedGid = 1002 // 1001 and 1004 are now taken, lowest available is 1002 - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(ap2, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -424,13 +414,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoints := []*cloud.AccessPoint{} - for i := 0; i < ACCESS_POINT_PER_FS_LIMIT; i++ { - gidMin, err := strconv.Atoi(req.Parameters[GidMin]) + for i := int64(0); i < ACCESS_POINT_PER_FS_LIMIT; i++ { + gidMin, err := strconv.ParseInt(req.Parameters[GidMin], 10, 64) if err != nil { t.Fatalf("Failed to convert GidMax Parameter to int.") } @@ -439,8 +426,8 @@ func TestCreateVolume(t *testing.T) { AccessPointId: apId, FileSystemId: fsId, PosixUser: &cloud.PosixUser{ - Gid: int64(userGid), - Uid: int64(userGid), + Gid: userGid, + Uid: userGid, }, } accessPoints = append(accessPoints, ap) @@ -456,7 +443,6 @@ func TestCreateVolume(t *testing.T) { } expectedGid := 2000 - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), false).Return(lastAccessPoint, nil). Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) { @@ -477,7 +463,6 @@ func TestCreateVolume(t *testing.T) { } accessPoints = append(accessPoints, lastAccessPoint) - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) // All 1000 GIDs are taken now, internal limit should take effect causing CreateVolume to fail. @@ -520,9 +505,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -532,7 +514,6 @@ func TestCreateVolume(t *testing.T) { }, } accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Eq(volumeName), gomock.Any(), gomock.Any()).Return(accessPoint, nil) @@ -581,9 +562,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -592,7 +570,6 @@ func TestCreateVolume(t *testing.T) { }, } accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil) @@ -643,9 +620,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -655,7 +629,6 @@ func TestCreateVolume(t *testing.T) { }, } accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil) @@ -706,9 +679,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -718,7 +688,6 @@ func TestCreateVolume(t *testing.T) { }, } accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil) @@ -773,9 +742,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, @@ -786,7 +752,6 @@ func TestCreateVolume(t *testing.T) { }, } accessPoints := []*cloud.AccessPoint{accessPoint} - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(accessPoints, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Eq(get64LenHash(pvcNameVal)), gomock.Any(), gomock.Any()).Return(accessPoint, nil) @@ -845,14 +810,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -917,14 +878,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -992,14 +949,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1067,14 +1020,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1141,14 +1090,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1210,14 +1155,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1280,14 +1221,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -1352,14 +1289,10 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } accessPoint := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil). @@ -2110,7 +2043,7 @@ func TestCreateVolume(t *testing.T) { }, }, { - name: "Fail: File system does not exist", + name: "Fail: File system does not exist with fixed uid/gid", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) mockCloud := mocks.NewMockCloud(mockCtl) @@ -2135,6 +2068,8 @@ func TestCreateVolume(t *testing.T) { GidMin: "1000", GidMax: "2000", DirectoryPerms: "777", + Uid: "1000", + Gid: "1001", }, } @@ -2148,7 +2083,45 @@ func TestCreateVolume(t *testing.T) { }, }, { - name: "Fail: DescribeFileSystem Access Denied", + name: "Fail: File system does not exist", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + }, + } + + ctx := context.Background() + mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, cloud.ErrNotFound) + _, err := driver.CreateVolume(ctx, req) + if err == nil { + t.Fatal("CreateVolume did not fail") + } + mockCtl.Finish() + }, + }, + { + name: "Fail: DescribeFileSystem Access Denied with fixed uid/gid", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) mockCloud := mocks.NewMockCloud(mockCtl) @@ -2173,6 +2146,8 @@ func TestCreateVolume(t *testing.T) { GidMin: "1000", GidMax: "2000", DirectoryPerms: "777", + Uid: "1000", + Gid: "1001", }, } @@ -2186,7 +2161,45 @@ func TestCreateVolume(t *testing.T) { }, }, { - name: "Fail: Describe File system call fails", + name: "Fail: DescribeFileSystem Access Denied", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + }, + } + + ctx := context.Background() + mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, cloud.ErrAccessDenied) + _, err := driver.CreateVolume(ctx, req) + if err == nil { + t.Fatal("CreateVolume did not fail") + } + mockCtl.Finish() + }, + }, + { + name: "Fail: Describe File system call fails with fixed uid/gid", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) mockCloud := mocks.NewMockCloud(mockCtl) @@ -2211,6 +2224,8 @@ func TestCreateVolume(t *testing.T) { GidMin: "1000", GidMax: "2000", DirectoryPerms: "777", + Uid: "1000", + Gid: "1001", }, } @@ -2224,7 +2239,7 @@ func TestCreateVolume(t *testing.T) { }, }, { - name: "Fail: Create Access Point call fails", + name: "Fail: List access points call fails", testFunc: func(t *testing.T) { mockCtl := gomock.NewController(t) mockCloud := mocks.NewMockCloud(mockCtl) @@ -2253,10 +2268,44 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, + mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, errors.New("ListAccessPoints failed")) + _, err := driver.CreateVolume(ctx, req) + if err == nil { + t.Fatal("CreateVolume did not fail") } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) + mockCtl.Finish() + }, + }, + { + name: "Fail: Create Access Point call fails", + testFunc: func(t *testing.T) { + mockCtl := gomock.NewController(t) + mockCloud := mocks.NewMockCloud(mockCtl) + + driver := &Driver{ + endpoint: endpoint, + cloud: mockCloud, + gidAllocator: NewGidAllocator(), + } + + req := &csi.CreateVolumeRequest{ + Name: volumeName, + VolumeCapabilities: []*csi.VolumeCapability{ + stdVolCap, + }, + CapacityRange: &csi.CapacityRange{ + RequiredBytes: capacityRange, + }, + Parameters: map[string]string{ + ProvisioningMode: "efs-ap", + FsId: fsId, + GidMin: "1000", + GidMax: "2000", + DirectoryPerms: "777", + }, + } + + ctx := context.Background() mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return([]*cloud.AccessPoint{}, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("CreateAccessPoint call failed")) _, err := driver.CreateVolume(ctx, req) @@ -2296,10 +2345,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return([]*cloud.AccessPoint{}, nil) mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, cloud.ErrAccessDenied) _, err := driver.CreateVolume(ctx, req) @@ -2339,9 +2384,6 @@ func TestCreateVolume(t *testing.T) { } ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } ap1 := &cloud.AccessPoint{ AccessPointId: apId, FileSystemId: fsId, @@ -2358,7 +2400,6 @@ func TestCreateVolume(t *testing.T) { Uid: 1001, }, } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil).AnyTimes() mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return([]*cloud.AccessPoint{ap1, ap2}, nil).AnyTimes() mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(ap2, nil).AnyTimes() @@ -2450,11 +2491,7 @@ func TestCreateVolume(t *testing.T) { ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) _, err := driver.CreateVolume(ctx, req) @@ -2499,11 +2536,7 @@ func TestCreateVolume(t *testing.T) { ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) _, err := driver.CreateVolume(ctx, req) @@ -2548,11 +2581,7 @@ func TestCreateVolume(t *testing.T) { ctx := context.Background() - fileSystem := &cloud.FileSystem{ - FileSystemId: fsId, - } - mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil) mockCloud.EXPECT().ListAccessPoints(gomock.Eq(ctx), gomock.Any()).Return(nil, nil) _, err := driver.CreateVolume(ctx, req) diff --git a/pkg/driver/gid_allocator.go b/pkg/driver/gid_allocator.go index d820d0684..3656ec9cb 100644 --- a/pkg/driver/gid_allocator.go +++ b/pkg/driver/gid_allocator.go @@ -1,7 +1,6 @@ package driver import ( - "context" "fmt" "sync" @@ -12,32 +11,29 @@ import ( "k8s.io/klog/v2" ) -var ACCESS_POINT_PER_FS_LIMIT int = 1000 +var ACCESS_POINT_PER_FS_LIMIT int64 = int64(1000) type FilesystemID struct { - gidMin int - gidMax int + gidMin int64 + gidMax int64 } type GidAllocator struct { - fsIdGidMap map[string]*FilesystemID mu sync.Mutex } func NewGidAllocator() GidAllocator { - return GidAllocator{ - fsIdGidMap: make(map[string]*FilesystemID), - } + return GidAllocator{} } // Retrieves the next available GID -func (g *GidAllocator) getNextGid(ctx context.Context, localCloud cloud.Cloud, fsId string, gidMin, gidMax int) (int64, error) { +func (g *GidAllocator) getNextGid(fsId string, accessPoints []*cloud.AccessPoint, gidMin, gidMax int64) (int64, error) { g.mu.Lock() defer g.mu.Unlock() - klog.V(5).Infof("Recieved getNextGid for fsId: %v, min: %v, max: %v", fsId, gidMin, gidMax) + klog.V(5).Infof("Received getNextGid for fsId: %v, min: %v, max: %v", fsId, gidMin, gidMax) - usedGids, err := g.getUsedGids(ctx, localCloud, fsId) + usedGids, err := g.getUsedGids(fsId, accessPoints) if err != nil { return 0, status.Errorf(codes.Internal, "Failed to discover used GIDs for filesystem: %v: %v ", fsId, err) } @@ -49,23 +45,11 @@ func (g *GidAllocator) getNextGid(ctx context.Context, localCloud cloud.Cloud, f "Please create a new storage class with a new file-system", fsId) } - return int64(gid), nil - + return gid, nil } -func (g *GidAllocator) removeFsId(fsId string) { - g.mu.Lock() - defer g.mu.Unlock() - delete(g.fsIdGidMap, fsId) -} - -func (g *GidAllocator) getUsedGids(ctx context.Context, localCloud cloud.Cloud, fsId string) (gids []int64, err error) { +func (g *GidAllocator) getUsedGids(fsId string, accessPoints []*cloud.AccessPoint) (gids []int64, err error) { gids = []int64{} - accessPoints, err := localCloud.ListAccessPoints(ctx, fsId) - if err != nil { - err = fmt.Errorf("failed to list access points: %v", err) - return - } if len(accessPoints) == 0 { return gids, nil } @@ -82,7 +66,7 @@ func (g *GidAllocator) getUsedGids(ctx context.Context, localCloud cloud.Cloud, return } -func getNextUnusedGid(usedGids []int64, gidMin, gidMax int) (nextGid int, err error) { +func getNextUnusedGid(usedGids []int64, gidMin, gidMax int64) (nextGid int64, err error) { requestedRange := gidMax - gidMin if requestedRange > ACCESS_POINT_PER_FS_LIMIT { @@ -93,7 +77,7 @@ func getNextUnusedGid(usedGids []int64, gidMin, gidMax int) (nextGid int, err er var lookup func(usedGids []int64) lookup = func(usedGids []int64) { for gid := gidMin; gid <= gidMax; gid++ { - if !slices.Contains(usedGids, int64(gid)) { + if !slices.Contains(usedGids, gid) { nextGid = gid return }