Skip to content
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

AWS: only cache instance requirements when needed #3

Closed
wants to merge 1 commit into from

Conversation

rrangith
Copy link
Owner

@rrangith rrangith commented Oct 11, 2024

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

This PR reduces the number of unneeded DescribeLaunchTemplateVersions API calls. Previously there was a PR that only looked for instance requirements when needed kubernetes#5550. However this PR kubernetes#6245 introduced a regression where now it looks for instance requirements all the time.

However in the original fix PR it was stated:

But retrieving InstanceRequirements should only be useful when the LT overrides don't already specify an InstanceType, as both are mutually exclusive. The api reference doc states:

If you specify InstanceRequirements, you can't specify InstanceType.

That mutual exclusion is already leveraged by getInstanceTypesForAsgs: we don't look for InstanceRequirements when we have a mixed instance policy specifying instance types overrides.

This PR makes it so we again only look for instance requirements if instance type overrides are not used.

This screenshot shows the huge increase in DescribeLaunchTemplateVersions API calls when we upgraded our cluster autoscaler to include the bug PR at 16:00 kubernetes#6245
Screenshot 2024-10-11 at 10 34 24 AM

Special notes for your reviewer:

Does this PR introduce a user-facing change?

AWS: Only cache instance requirements when needed

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@rrangith rrangith closed this Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant