Merge pull request #1182 from RomanBednar/inc-gid-alloc

allocate GIDs in increasing order
kubernetes-sigs · Nov 16, 2023 · 9ac1d62 · 9ac1d62
2 parents ce3388f + 493cea2
commit 9ac1d62
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 23 deletions.
diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go
@@ -80,12 +80,12 @@ func TestCreateVolume(t *testing.T) {
 				}
 				mockCloud.EXPECT().DescribeFileSystem(gomock.Eq(ctx), gomock.Any()).Return(fileSystem, nil)
 				mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Eq(volumeName), gomock.Any(), gomock.Eq(false)).Return(accessPoint, nil).
-					Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) {
-						if accessPointOpts.Uid != 1000 {
-							t.Fatalf("Uid mimatched. Expected: %v, actual: %v", accessPointOpts.Uid, 1000)
+					Do(func(ctx context.Context, clientToken string, accessPointsOptions *cloud.AccessPointOptions, reuseAccessPointName bool) {
+						if accessPointsOptions.Uid != 1000 {
+							t.Fatalf("Uid mismatched. Expected: %v, actual: %v", 1000, accessPointsOptions.Uid)
 						}
-						if accessPointOpts.Gid != 1001 {
-							t.Fatalf("Gid mimatched. Expected: %v, actual: %v", accessPointOpts.Uid, 1001)
+						if accessPointsOptions.Gid != 1001 {
+							t.Fatalf("Gid mismatched. Expected: %v, actual: %v", 1001, accessPointsOptions.Gid)
 						}
 					})
 
@@ -149,10 +149,10 @@ func TestCreateVolume(t *testing.T) {
 				mockCloud.EXPECT().CreateAccessPoint(gomock.Eq(ctx), gomock.Any(), gomock.Any(), gomock.Any()).Return(accessPoint, nil).
 					Do(func(ctx context.Context, clientToken string, accessPointOpts *cloud.AccessPointOptions, reuseAccessPointName bool) {
 						if accessPointOpts.Uid != 1000 {
-							t.Fatalf("Uid mimatched. Expected: %v, actual: %v", accessPointOpts.Uid, 1000)
+							t.Fatalf("Uid mismatched. Expected: %v, actual: %v", 1000, accessPointOpts.Uid)
 						}
 						if accessPointOpts.Gid != 1001 {
-							t.Fatalf("Gid mimatched. Expected: %v, actual: %v", accessPointOpts.Uid, 1001)
+							t.Fatalf("Gid mismatched. Expected: %v, actual: %v", 1001, accessPointOpts.Uid)
 						}
 					})
 
@@ -197,7 +197,7 @@ func TestCreateVolume(t *testing.T) {
 						FsId:             fsId,
 						DirectoryPerms:   "777",
 						BasePath:         "test",
-						GidMin:           "1000",
+						GidMin:           "1001",
 						GidMax:           "1003",
 					},
 				}
@@ -283,8 +283,8 @@ func TestCreateVolume(t *testing.T) {
 						FsId:             fsId,
 						DirectoryPerms:   "777",
 						BasePath:         "test",
-						GidMin:           "1000",
-						GidMax:           "1004",
+						GidMin:           "1001",
+						GidMax:           "1005",
 					},
 				}
 
@@ -326,8 +326,8 @@ func TestCreateVolume(t *testing.T) {
 				}
 
 				// Let allocator jump over some GIDS.
-				accessPoints := []*cloud.AccessPoint{ap3, ap4}
-				var expectedGid int64 = 1002 // 1003 and 1004 is taken.
+				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)
@@ -345,7 +345,7 @@ func TestCreateVolume(t *testing.T) {
 
 				// 2. Simulate access point removal and verify their GIDs returned to allocator.
 				accessPoints = []*cloud.AccessPoint{}
-				expectedGid = 1004 // 1003 and 1004 are now free, if no GID return would happen allocator would pick 1001.
+				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)
@@ -360,10 +360,11 @@ func TestCreateVolume(t *testing.T) {
 					})
 
 				res, err = driver.CreateVolume(ctx, req)
-				////
+
+				// 3. Simulate access point ID gap and verify their GIDs returned to allocator
 				accessPoints = []*cloud.AccessPoint{ap1, ap4}
+				expectedGid = 1002 // 1001 and 1004 are now taken, lowest available is 1002
 
-				expectedGid = 1003
 				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).
@@ -428,12 +429,12 @@ func TestCreateVolume(t *testing.T) {
 				}
 
 				accessPoints := []*cloud.AccessPoint{}
-				for i := 0; i < ACCESS_POINT_PER_FS_LIMIT-1; i++ {
-					gidMax, err := strconv.Atoi(req.Parameters[GidMax])
+				for i := 0; i < ACCESS_POINT_PER_FS_LIMIT; i++ {
+					gidMin, err := strconv.Atoi(req.Parameters[GidMin])
 					if err != nil {
 						t.Fatalf("Failed to convert GidMax Parameter to int.")
 					}
-					userGid := gidMax - i
+					userGid := gidMin + i
 					ap := &cloud.AccessPoint{
 						AccessPointId: apId,
 						FileSystemId:  fsId,
@@ -449,12 +450,12 @@ func TestCreateVolume(t *testing.T) {
 					AccessPointId: apId,
 					FileSystemId:  fsId,
 					PosixUser: &cloud.PosixUser{
-						Gid: 1001,
-						Uid: 1001,
+						Gid: 2000,
+						Uid: 2000,
 					},
 				}
 
-				expectedGid := 1001
+				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).
@@ -472,7 +473,7 @@ func TestCreateVolume(t *testing.T) {
 				// Allocate last available GID
 				_, err = driver.CreateVolume(ctx, req)
 				if err != nil {
-					t.Fatalf("CreateVolume failed.")
+					t.Fatalf("CreateVolume failed unexpectedly: %v", err)
 				}
 
 				accessPoints = append(accessPoints, lastAccessPoint)

diff --git a/pkg/driver/gid_allocator.go b/pkg/driver/gid_allocator.go
@@ -94,7 +94,7 @@ func getNextUnusedGid(usedGids []int64, gidMin, gidMax int) (nextGid int, err er
 
 	var lookup func(usedGids []int64)
 	lookup = func(usedGids []int64) {
-		for gid := gidMax; gid > gidMin; gid-- {
+		for gid := gidMin; gid <= gidMax; gid++ {
 			if !slices.Contains(usedGids, int64(gid)) {
 				nextGid = gid
 				return