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

Do not sort instance types #142

Merged
merged 5 commits into from
Aug 3, 2023
Merged

Conversation

xeivieni
Copy link
Contributor

@xeivieni xeivieni commented Mar 14, 2023

what

Remove sorting on instance type list in the node group definition

why

Because the order of the list is used to define priorities on the type of instance to use.

references

Managed node groups use the order of instance types passed in the API to determine which instance type to use first when fulfilling On-Demand capacity. For example, you might specify three instance types in the following order: c5.large, c4.large, and c3.large. When your On-Demand Instances are launched, the managed node group fulfills On-Demand capacity by starting with c5.large, then c4.large, and then c3.large

@xeivieni xeivieni requested review from a team as code owners March 14, 2023 16:10
@xeivieni xeivieni requested review from Gowiem and korenyoni March 14, 2023 16:10
@xeivieni
Copy link
Contributor Author

@Gowiem @korenyoni can I have a review on this please ? 🙏

@Gowiem
Copy link
Member

Gowiem commented Apr 13, 2023

/test all

@Gowiem
Copy link
Member

Gowiem commented Apr 13, 2023

This looks good to me, but since @Nuru is the defacto lead on this module, I'm going to pull him in. Particularly because he is the one who approved #54 where this was originally introduced.

@Nuru any strong opinions here before we move it forward?

@Gowiem Gowiem added the patch A minor, backward compatible change label Apr 13, 2023
@Gowiem Gowiem requested a review from Nuru April 13, 2023 17:12
@Nuru
Copy link
Contributor

Nuru commented May 15, 2023

@xeivieni Thank you for this PR.

The random_pet's keepers should match the behavior of the Terraform provider. When inputs change that cause Terraform to want to replace the node group (rather than update it in place), a new random_pet should be generated, and when the node group can be updated in place, random_pet should not change. Since this module was first written, some settings that used to force new node groups have been enhanced so they can now be updated in-place, so please re-check:

  • Does changing the order of the on-demand instance types cause a new node group to be created?
  • Does changing the set of on-demand instance types cause a new node group to be created?

The keepers should be set to mimic those answers. My guess is that the current

instance_types = join(",", sort(local.ng.instance_types))

is probably not the best choice.

@Nuru Nuru added no-release Do not create a new release (wait for additional code changes) and removed patch A minor, backward compatible change labels Aug 3, 2023
@Nuru Nuru merged commit 856b3ba into cloudposse:main Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-release Do not create a new release (wait for additional code changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants