Batch EC2 DescribeInstances calls #1947
Conversation
k8s-ci-robot
added
size/L
Code Coverage Diff
|
| c := newEC2Cloud(region, awsSdkDebugLog, userAgentExtra) | ||
|
|
||
| if batching { | ||
| klog.V(4).InfoS("NewCloud: batching enabled") | ||
| cloudInstance, ok := c.(*cloud) | ||
| if !ok { | ||
| return nil, fmt.Errorf("expected *cloud type but got %T", c) | ||
| } | ||
| cloudInstance.bm = newBatcherManager(cloudInstance.ec2) | ||
| } | ||
|
|
||
| c := newEC2Cloud(region, awsSdkDebugLog, userAgentExtra, batching) |
There was a problem hiding this comment.
Nice! passing the batching status down the helper is much cleaner 👍
pkg/cloud/cloud.go
Outdated
| return nil, r.Err | ||
| } | ||
| if r.Result == nil { | ||
| return nil, fmt.Errorf("batchDescribeInstances: no instance found %s", task) // TODO Q: Should this be Cloud's ErrNotFound instead? |
There was a problem hiding this comment.
TODO Q: Should this be Cloud's ErrNotFound instead?
Yes +1 to this, that error type is more appropriate
There was a problem hiding this comment.
I will change batchDescribeVolumes as well then to be consistent.
AndrewSirenko
force-pushed
the
batching
branch
from
3e14c19 to
ee19d85
Compare
|
/retest |
k8s-ci-robot
added
the
lgtm
|
/hold Waiting on scalability tests. |
k8s-ci-robot
added
the
do-not-merge/hold
AndrewSirenko
force-pushed
the
batching
branch
from
ee19d85 to
f320c1a
Compare
k8s-ci-robot
removed
the
lgtm
|
Settled on 300ms instanceIDBatcher maxDelay by running scalability tests. We found that this value balanced The table below shows total amount DescribeInstances API Calls made when attaching and detaching x volumes in a scalability test.
|
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/retest |
k8s-ci-robot
added
the
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 |
k8s-ci-robot
added
the
approved
|
/retest |
Is this a bug fix or adding new feature?
Feature
What is this PR about? / Why do we need it?
Coalesces EC2 DescribeInstances calls across ControllerPublishVolume/ControllerUnpublishVolume RPCs.
This decreases the likelihood of being exceeding one's Non-mutating API request token limit (ie being throttled for making too many
Describe*calls).What testing is done?
Manual testing on statefulset to check that DescribeInstances calls were batched across RPCs
CI