-
Notifications
You must be signed in to change notification settings - Fork 808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Generic Batching Framework + Batch DescribeVolume API requests #1819
Conversation
Kustomize CI job needs to be updated. Driver fails to enter a running state because |
24a8e7b
to
bddbe0b
Compare
Re Kustomize CI job needs to be updated -- we decided to run An additional advantage in doing the above is that we implicitly validate image promotion for new releases moving forward since we pull from the official public ECR gallery. |
/hold will retest once changes to test-infra have been merged. |
/retest |
/close |
@ConnorJC3: Closed this PR. In response to this:
Instructions 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/test-infra repository. |
/reopen |
@ConnorJC3: Reopened this PR. In response to this:
Instructions 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/test-infra repository. |
/unhold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs rebase, one comment, otherwise lgtm
Signed-off-by: Eddie Torres <torredil@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
(Minus the cloud_test merge conflict and 1 question)
Signed-off-by: Eddie Torres <torredil@amazon.com>
/lgtm /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ConnorJC3 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 |
/retest |
For peace of mind, investigated the resizing failure in
In conclusion, the error observed above is unrelated to this PR and occurs when a volume takes longer than 1 minute to transition from an Based on my observation, it is fairly common to see a volume remain in
Given that we recently introduced more tests that exercise this code path in single-az, we are more likely to observe the error moving forward if that timeout value is not relaxed. /retest |
@torredil: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions 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/test-infra repository. I understand the commands that are listed here. |
According to cloud, a modification is considered done if the volume is an aws-ebs-csi-driver/pkg/cloud/cloud.go Lines 1320 to 1323 in d071db8
In the case where aws-ebs-csi-driver/pkg/cloud/cloud.go Lines 1298 to 1300 in d071db8
|
/retest |
/unhold |
Is this a bug fix or adding new feature?
New feature.
What is this PR about? / Why do we need it?
This PR implements a generic batching framework, designed to facilitate the aggregation and execution of tasks -- most notably batched API requests for
DescribeVolume
-- used by the driver to poll for volume state and determine attachment status.Today, the driver makes one DV API call per volume, however, DV can return up to 500 results. These changes allow for utilizing the API's capacity to handle up to 500 volumes at once, which significantly helps mitigate rate limit issues with DV and enhance performance when using the driver under heavy load scenarios as a result.
There are two parts to this PR, separated by commits:
Generic Batching Framework.
Batch DescribeVolume API Requests.
What testing is done?
make test