Fix GID allocator #850
Merged
Fix GID allocator #850
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
size/L
RomanBednar
force-pushed
the
gid-allocator
branch
from
0d6264d
to
378d22a
Compare
k8s-ci-robot
added
size/XL
RomanBednar
force-pushed
the
gid-allocator
branch
from
378d22a
to
3d65fbd
Compare
k8s-ci-robot
added
size/XXL
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
Here is what I've tested manually to verify the patches: Test GID allocator avoids used GIDS
Test that GID allocator can re-use released GIDs
Test GID Range propagation
|
This was referenced Dec 14, 2022
|
@wongma7 @nckturner what do you think about the change? |
Dockerfile
Outdated
| @@ -12,7 +12,7 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| FROM golang:1.17 as builder | |||
There was a problem hiding this comment.
Could we separate Go upgrade in a separate PR?
There was a problem hiding this comment.
Sure, opened here: #867
|
Hi Roman Thanks for providing this fix. We are currently going through your code changes internally from EFS. One thing we want to call out is, also as you pointed out here #693 (comment) that with the patch, it would check all possible GIDs currently is 120 from given range each time a volume is created and this might not scale well as we have plan to increase this to 1000 early next year. AP increase internally dev is complete but waiting for deployment and official launch. We will need to do a performance test upon the new limit and get back to you. Sorry about the potential delay, but what to let you know that we are looking into this and having internal tracking as well. Will provide more update upon this once we have more performance testing data. Thank you! |
mskanth972
reviewed
Dec 29, 2022
|
/hold until #867 merged |
k8s-ci-robot
added
the
do-not-merge/hold
RomanBednar
force-pushed
the
gid-allocator
branch
from
3d65fbd
to
b1a40b9
Compare
k8s-ci-robot
added
the
needs-rebase
RomanBednar
force-pushed
the
gid-allocator
branch
from
b1a40b9
to
b193158
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Hello @Ashley-wenyizha, just checking in - anything new regarding the performance test? |
k8s-ci-robot
added
the
needs-rebase
RomanBednar
force-pushed
the
gid-allocator
branch
from
b193158
to
4fa0e4e
Compare
|
Hi @RomanBednar Testing looks good, if not hearing back we will fix the merge conflict and merge in early next week. Thanks for your contribution! |
This code is needed to allow listing of access points. This can be used by GID allocator to avoid assigning GIDs that might already be used.
Internal limit of EFS is 120 Access Points per Filesystem ID. There is no reason to check the entire GID range specified by user if we can't allocate those GIDs anyway. Considering the internal limit there is no need to track the GIDs using heap structure and we can always look up the next GID from the full range which won't exceed 120. Because the limit is relatively small this won't impact performance.
RomanBednar
force-pushed
the
gid-allocator
branch
from
bbaab09
to
3e32348
Compare
k8s-ci-robot
removed
the
needs-rebase
|
@Ashley-wenyizha Thank you for the update, I've resolved the conflicts now. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ashley-wenyizha, 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Is the plan to leave I see this comment marked resolved - #850 (comment) I'd definitely like to increase the value in my deployment. Is there still a performance concern listing large numbers of access points? Can we make this value configurable, so that users can make the trade-off themselves? |
|
@johnpmayer, No the comment is just telling that Access points limit has been increased to 1000 from 120 recently. |
|
I see this is being addressed in #1119 |
otorreno
added a commit
to otorreno/aws-efs-csi-driver
that referenced
this pull request
Dec 29, 2023
With kubernetes-sigs#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. kubernetes-sigs#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)
otorreno
added a commit
to otorreno/aws-efs-csi-driver
that referenced
this pull request
Dec 29, 2023
With kubernetes-sigs#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. kubernetes-sigs#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.
otorreno
added a commit
to otorreno/aws-efs-csi-driver
that referenced
this pull request
Dec 29, 2023
With kubernetes-sigs#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. kubernetes-sigs#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.
otorreno
added a commit
to otorreno/aws-efs-csi-driver
that referenced
this pull request
Jan 2, 2024
With kubernetes-sigs#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. kubernetes-sigs#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.
otorreno
added a commit
to otorreno/aws-efs-csi-driver
that referenced
this pull request
Jan 2, 2024
With kubernetes-sigs#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. kubernetes-sigs#850 made both uid and gid int64, but gidMin and gidMax were nout touched. Also changing the default value for gidMax, as the value of 7000000 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 51000 (the value that gid_allocator was setting by adding 1000 to gidMin). Removing an unused function and field from gid_allocator too.
mskanth972
pushed a commit
to mskanth972/aws-efs-csi-driver
that referenced
this pull request
Jan 3, 2024
With kubernetes-sigs#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. kubernetes-sigs#850 made both uid and gid int64, but gidMin and gidMax were nout touched. Also changing the default value for gidMax, as the value of 7000000 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 51000 (the value that gid_allocator was setting by adding 1000 to gidMin). Removing an unused function and field from gid_allocator too. (cherry picked from commit dbbc733)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Is this a bug fix or adding new feature?
Bug fix.
What is this PR about? / Why do we need it?
Dynamically provisioning EFS volume creates an access point (AP) for which it allocates a GID from default range or a range defined in SC parameters.
The GID allocator did not check what GIDs might be already in use. For example let's say we have an EFS with access point for
/mydirpath which is owned by GID1000with permissions of770that grant rwx access to group1000. Then we create another access point with same path or "higher" (can be also/) with the same GID of1000. Now whoever mounts the second access point will have rwx access to/mydirbecause the GID matches. The old code does not prevent this and so volume creation done by the driver might result in creating access point with a GID that is already used, and when it gets mounted to a pod this pod might get access to other pods data.Additionally, when user changed the SC parameters it was not reflected in the driver (controller pod) and GID allocator kept allocating from the range that got in first (for a given FS ID). In order for GID range changes to take effect the pod had to be restarted.
What testing is done?