Batch AWS getInstancesByNodeNames calls with FilterNodeLimit #47516
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/M
|
This is just a simpler alternate way of fixing the filter limit for #45050 in case, we didn't had a chance to get caching fix merge. |
|
|
||
| // Number of node names that can be added to a filter. The AWS limit is 200 | ||
| // but we are using a lower limit on purpose | ||
| FilterNodeLimit = 150 |
There was a problem hiding this comment.
This does not seem to be used externally and thus should be lowercase.
We are going to limit the getInstancesByNodeNames call with a batch size of 150
gnufied
force-pushed
the
fix-filter-limit-aws
branch
from
d1d1798 to
ffa622f
Compare
| @@ -157,6 +157,10 @@ const ( | |||
| createTagInitialDelay = 1 * time.Second | |||
| createTagFactor = 2.0 | |||
| createTagSteps = 9 | |||
|
|
|||
| // Number of node names that can be added to a filter. The AWS limit is 200 | |||
| // but we are using a lower limit on purpose | |||
There was a problem hiding this comment.
Sounds good. If nothing else, it leaves us room for our own additional filters.
| Name: aws.String("private-dns-name"), | ||
| Values: names, | ||
| } | ||
| for i := 0; i < len(names); i += filterNodeLimit { |
There was a problem hiding this comment.
Might be worth splitting this into a partition function, just because it is tricky and it would be great to have a unit test on it. I am surprised there isn't one already, but maybe I'm looking in the wrong place.
| tag.Value = aws.String("") | ||
| tags := []*ec2.Tag{&tag} | ||
| nodeNames := []string{} | ||
| for i := 0; i < 200; i++ { |
There was a problem hiding this comment.
Ah - I suppose this has (some) coverage of the partition logic.
There was a problem hiding this comment.
Yeah "some", :-) . Because we still don't test if partitioning correctly splits the results too. I can extract this into a separate function easily enough. Will open a github issue for myself.
|
So I think this looks good. I actually think the best option here is probably to get the cache logic in for ELBs, and to get this logic in for EBS. For ELBs we really don't need to be doing a lookup by name anyway, now that we have the node object. For EBS the logic is much more complicated, as I'm sure we can all attest to by now. But also getting both this and #47410 into 1.7 means that backporting won't be a nightmare, because hopefully the basic code structure won't then change too much. |
|
/approve |
k8s-github-robot
added
approved
|
I added an AWS prefix to the release note and tagged the markdown block with |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied, justinsb Associated issue: 47271 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/remove-priority P0 |
k8s-ci-robot
added
priority/critical-urgent
|
Automatic merge from submit-queue (batch tested with PRs 47510, 47516, 47482, 47521, 47537) |
Automatic merge from submit-queue Fix AWS instance information fetch for large clusters This is a manually cherry-pick for two PRs - #46463 and #47516 ```release-note AWS cloudprovider plugin: Fix for large clusters (200+ nodes). Also fix bug with volumes not getting detached from a node after reboot. ``` cc @enisoc @justinsb @wongma7
We are going to limit the getInstancesByNodeNames call with a batch
size of 150.
Fixes - #47271