Add maxInflightMountCalls and volumeAttachLimit args#1706
Conversation
|
Hi @DavidXU12345. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3eb7f6a to
2f0bbed
Compare
| klog.Fatalln(err) | ||
| } | ||
| drv := driver.NewDriver(*endpoint, etcAmazonEfs, *efsUtilsStaticFilesPath, *tags, *volMetricsOptIn, *volMetricsRefreshPeriod, *volMetricsFsRateLimit, *deleteAccessPointRootDir, *adaptiveRetryMode) | ||
| drv := driver.NewDriver(*endpoint, etcAmazonEfs, *efsUtilsStaticFilesPath, *tags, *volMetricsOptIn, *volMetricsRefreshPeriod, *volMetricsFsRateLimit, *deleteAccessPointRootDir, *adaptiveRetryMode, *maxInflightMountCallsOptIn, *maxInflightMountCalls, *volumeAttachLimitOptIn, *volumeAttachLimit) |
There was a problem hiding this comment.
We should probably add all options into its own type (similar as EBS CSI driver) so that we can pass option around rather than passing each value into NewDriver for example. We can also encapsulate validation logic in option as well (e.g. check if maxInflightMountCallsOptIn is true, maxInflightMountCalls has to be a positive value).
I may consider to do the refactoring in next PR since the size of this PR is already very large.
60029aa to
0fe78c3
Compare
| tags = flag.String("tags", "", "Space separated key:value pairs which will be added as tags for EFS resources. For example, 'environment:prod region:us-east-1'") | ||
| adaptiveRetryMode = flag.Bool("adaptive-retry-mode", true, "Opt out to use standard sdk retry configuration. By default, adaptive retry mode will be used to more heavily client side rate limit EFS API requests.") | ||
| tags = flag.String("tags", "", "Space separated key:value pairs which will be added as tags for EFS resources. For example, 'environment:prod region:us-east-1'") | ||
| maxInflightMountCallsOptIn = flag.Bool("max-inflight-mount-calls-opt-in", false, "Opt in to use max inflight mount calls limit.") |
There was a problem hiding this comment.
Do you think we could simplify this by just creating one flag for each? If no is maxInflightCalls or volumeLimit is provided we just use current behavior (not limited at all)?
There was a problem hiding this comment.
I prefer to keep the OptIn flag primarily for future-proofing. If we later implement default calculations for maxInflightCalls or volumeLimit, users could set OptIn=true without specifying values for parameters like maxInflightMountCalls.
While we could remove the OptIn feature gate later if we decide not to provide default calculations, adding it back would be more disruptive. From a user perspective, without OptIn=true, a configuration with maxInflightMountCalls=10 would be ignored, breaking backward compatibility.
0fe78c3 to
7de4bf0
Compare
7de4bf0 to
943e17b
Compare
|
/lgtm |
|
/ok-to-test |
|
/retest |
c20bee8 to
be64472
Compare
1. maxInflightMountCalls with OptIn arg In order to use maxInflightMountCalls, user has to set maxInflightMountCallsOptIn to true and set a positive value for maxInflightMountCalls. 2. volumeAttachLimit with OptIn arg To use volumeAttachLimit, user has to set volumeAttachLimitOptIn to be true and set volumeAttachLimit to a positive value.
be64472 to
4dfcfa1
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dankova22, DavidXU12345 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Is this a bug fix or adding new feature?
Adding new feature.
What is this PR about? / Why do we need it?
Add maxInflightMountCalls and volumeAttachLimit args. We need them to prevent OOM killed issue for efs-plugin container since it takes 30MiB for mounting operation and 12MiB for each EFS volume due to efs-proxy process.
What testing is done?
maxInflightMountCallsandvolumeAttachLimitbeing set