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

Changing instance_types in MNGs causes destroy/recreate #1415

Closed
cabrinha opened this issue May 28, 2021 · 5 comments
Closed

Changing instance_types in MNGs causes destroy/recreate #1415

cabrinha opened this issue May 28, 2021 · 5 comments

Comments

@cabrinha
Copy link
Contributor

Description

Seems like if I deploy a MNG with a list of instance types, then remove or add an instance type, I get the error:

ResourceInUseException: NodeGroup already exists with name <MNG name> and cluster name <cluster name>

Versions

  • Terraform: v0.13.7

  • Provider(s):

  • provider registry.terraform.io/hashicorp/aws v3.42.0
  • provider registry.terraform.io/hashicorp/cloudinit v2.2.0
  • provider registry.terraform.io/hashicorp/kubernetes v1.13.3
  • provider registry.terraform.io/hashicorp/local v2.1.0
  • provider registry.terraform.io/hashicorp/null v3.1.0
  • provider registry.terraform.io/hashicorp/random v3.1.0
  • provider registry.terraform.io/hashicorp/template v2.2.0
  • provider registry.terraform.io/terraform-aws-modules/http v2.4.1
  • Module: v17.0.2

Reproduction

Steps to reproduce the behavior:
Create a MNG with instance_types of any value.

Edit the instance_types list by adding or removing any instance types.

Code Snippet to Reproduce

Expected behavior

I expect the launch template to be updated and don't expect the MNG to need to be recreated entirely.

Actual behavior

ResourceInUseException says the MNG already exists with the same name.

Additional context

I wonder if create_before_destroy should be implemented here if the MNGs really do need to be recreated due to instance types changing.

@barryib
Copy link
Member

barryib commented May 28, 2021

Is this still happen in v17.x.x ? We dropped random pets. Can you please try to upgrade your module ?

Please see https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/docs/upgrades.md and changelog for more info.

@cabrinha
Copy link
Contributor Author

Is this still happen in v17.x.x ? We dropped random pets. Can you please try to upgrade your module ?

Please see https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/docs/upgrades.md and changelog for more info.

I missed the (by removing the name argument) part. Seems to be working well now, I'll do a little more testing and then close the issue.

@cabrinha
Copy link
Contributor Author

Working well now! Closing.

@jaimehrubiks
Copy link
Contributor

jaimehrubiks commented Jun 9, 2021

@cabrinha @barryib
Is it even possible to change the instance_types of managed node groups in this module? My terraform will destroy/recreate the whole node group when attempt so.

Thing is, I don't even see that option in the AWS console. When I click on modify Node Group, I can only change number of instances, labels, taints and tags. I also see that the instance type is specified only at the node group level (which is also reflected in the default instance list of the underlying autoscaling group), but not on the launch template itself.

According to aws/containers-roadmap#746 it is not possible.

But also, it says that this is solved by the introduction of launch templates:
aws/containers-roadmap#585 (comment)

So this means that it order to benefit from this feature we should be passing the instance type to the launch template.

This is related to #1386

It turns out that this module is specifying the instance_type on the node group creation API call, and leaving it empty on the launch template (It is clear that you cannot do it in both places). I would expect that if the "create_launch_template" variable is set to true (or a new variable not to cause breaking changes), the instance_type should go there, in the template, so we can benefit from rolling updates.

Edit: Didn't know that the variable "set_instance_types_on_lt" existed. So it is fine, we have both options :) I guess that for regular instances it is better to set it to true and use LT for the instance so you benefit from rolling updates. For spot I think it is better false because you can have more instance types in the node groups than you can in the LT

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants