GIDs are not allocated correctly above certain volume threshold

[RomanBednar]

Is this a bug fix or adding new feature?

Bug fix.

What is this PR about? / Why do we need it?

There are two smaller issues addressed in this PR:

• EFS client call DescribeAccessPointsRequest is limited to 100 results by default, so when more than 100 access points are created for a filesystem we don't get a full listing. Because of this allocator will assign GIDs that are already used which is incorrect.

• When GID range exceeds the current access point limit, allocator picks the upper range, but it makes more sense to use lower GIDs first starting from gidRangeStart/gidMin.

Reproducer:

1. Create StorageClass with GID range 10000-90000:

apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: mysc
provisioner: efs.csi.aws.com
reclaimPolicy: Delete
volumeBindingMode: WaitForFirstConsumer
parameters:
  basePath: "/basePath"
  directoryPerms: "777"
  fileSystemId: fs-068b18a45f696ada6
  gidRangeStart: "10000"
  gidRangeEnd: "90000"
  provisioningMode: efs-ap
  subPathPattern: /subPath/${.PVC.name}/foo
allowVolumeExpansion: false

2. Create more than 100 volumes/access points, here we can observe that above 100 volumes the same GID was is getting reused. Also it started from 89000, but starting from 10000 makes more sense.

fsap-0357469ca94e8555c  /basePath/pvc-5ad04682-04e4-4946-8deb-e655b8afb104      89100 : 89100   89100 : 89100 (777)      Available
fsap-0cc2d107780c7b345  /basePath/pvc-b15acd33-9ffe-4d56-9dac-766ca2f3c894      89100 : 89100   89100 : 89100 (777)      Available
fsap-0b4590b20c00b0bcb  /basePath/pvc-da0aa6b8-ec6b-4822-a78f-63e3b0fb7cf6      89100 : 89100   89100 : 89100 (777)      Available

What testing is done?

Unit tests + manual verification.

[RomanBednar]

/test pull-aws-efs-csi-driver-external-test-eks

[RomanBednar]

/assign @mskanth972 @Ashley-wenyizha
for approval

[mskanth972]

Hi @RomanBednar can you squash the commits?

[RomanBednar]

@mskanth972 Done.

[mskanth972]

/lgtm

[mskanth972]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mskanth972, RomanBednar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [mskanth972]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[joaocc]

Hi. Since we are effectively limiting the max value for gid, should the documentation also be updated to reflect that (instead of continuing to refer the 7000000 upper limit)? Or is there any catch and I got this wrong? Thx

[RomanBednar]

@joaocc There is no new limit being introduced here, this was just a minor bug fix in GID allocations.

However there's a limit for number of access points for EFS itself, recently increased from 120 to 1000, documented here: https://docs.aws.amazon.com/efs/latest/ug/limits.html#limits-efs-resources-per-account-per-region

So this patch just makes sure we discover all possible GIDs allocated/used (max 1000) when calling AWS API to see what GIDs are currently used and new ones allocated correctly - this is because the call returns 100 records only by default even if more are present and would confuse GID allocator. Example: now if you set gidRangeStart to 1000 you should be able to allocate access points getting GIDs within 1000-1999 range.

[joaocc]

Thanks for the clarification.
However, and please excuse me if I got this wrong, but it seems the documentation in the documentation (gidRangeStart=50000 and gidRangeEnd=7000000) are not taking this into account.

[RomanBednar]

I believe you are referring to this documentation - yes it's confusing since the internal limit is not obvious from driver doc. Maybe there's some use case where it makes sense to set higher range, not sure. Feel free to create an issue if you think this should be corrected so one of the maintainers can weight in on this.