cloudposse / terraform-aws-eks-node-group

Terraform module to provision a fully managed AWS EKS Node Group
https://cloudposse.com/accelerate
Apache License 2.0
90 stars 128 forks source link

Do not sort instance types #142

Closed xeivieni closed 1 year ago

xeivieni commented 1 year ago

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 commented 1 year ago

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

Gowiem commented 1 year ago

/test all

Gowiem commented 1 year ago

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?

Nuru commented 1 year ago

@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:

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.