-
Notifications
You must be signed in to change notification settings - Fork 357
Set MaxResults if it is not set #274
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
Set MaxResults if it is not set #274
Conversation
|
kubernetes/kubernetes#102927 this updated the cache to have a ttl of just 10 minutes. Seems like a very bad idea if we are trying to optimize our calls. |
* EC2 pagination doesn't work if MaxResults is not set. * Only set if InstanceIds isn't set.
9c63fa9 to
431d0fb
Compare
|
/unhold |
|
do you mind double checking that describeAllInstancesUncached
|
|
describeAllInstancesUncached calls c.cloud.describeInstances (c is for cache)
cloud-provider-aws/pkg/providers/v1/aws.go Line 5002 in 58bc609
c.cloud.describeInstances calls c.ec2.DescribeInstances (now c is for cloud) cloud-provider-aws/pkg/providers/v1/aws.go Line 5007 in 58bc609
cloud.ec2 is a v1.EC2 interface cloud-provider-aws/pkg/providers/v1/aws.go Line 334 in 58bc609
and the "real"/non-mock implementation of it by awsSdkEC2 is cloud-provider-aws/pkg/providers/v1/aws.go Line 964 in 58bc609
which calls s.ec2.DescribeInstances (now ec2 is an aws-sdk-go ec2.EC2 not a v1.EC2/awsSdkEC2) cloud-provider-aws/pkg/providers/v1/aws.go Line 731 in 58bc609
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nckturner, wongma7 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 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The describeInstances API doesn't paginate unless maxResults is set.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
/hold
Does this PR introduce a user-facing change?: