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

fix: ManagedNodeGroups using LaunchTemplate and while specifying Disk Size #1239

Conversation

theothermike
Copy link

@theothermike theothermike commented Feb 12, 2021

If using Managed Node Groups with a Launch Template, the disk size MUST be specified in the Launch Template and not in the Managed Node Group definition otherwise AWS API produces an error that Disk Size is specified

PR o'clock

Description

If the launch_template_id is specified, specify null for disk size in managed node group resource

Checklist

…ST be specified in the Launch Template and node in the Managed Node Group definition
@theothermike theothermike changed the title Fix for ManagedNodeGroups using LaunchTemplate and specifying Disk Size fix: ManagedNodeGroups using LaunchTemplate and while specifying Disk Size Feb 12, 2021
@theothermike
Copy link
Author

I don't understand what the error is on Docs, the failure doesn't seem to indicate a reason

@ArchiFleKs
Copy link
Contributor

This is included here https://github.com/terraform-aws-modules/terraform-aws-eks/pull/1138/files#diff-7a3fc6c7df17fda0c341e61255461bf1f149256a9ddf14d4a18ab6f020d08136R17

@philicious
Copy link
Contributor

@theothermike do you actually face an error here?

I use MNG + LT since #997 and dont have problems with disk_size. in fact, disk_size is not defaulted for MNG, so if you dont specify it on the node_group and only in the LT, there shouldnt be a problem

@theothermike
Copy link
Author

theothermike commented Feb 19, 2021

@theothermike do you actually face an error here?

I use MNG + LT since #997 and dont have problems with disk_size. in fact, disk_size is not defaulted for MNG, so if you dont specify it on the node_group and only in the LT, there shouldnt be a problem

@philicious

Yes, the API throws an error that either you don't have a disk defined in the launch template, or if you do, and don't have this fix, another error saying disk size is defined. but the other fix mentioned above is more comprehensive, so I'd like to use it.. but its been waiting merge for a while. I was about to do another round of testing that uses this code, I will paste the specific errors here, since they're lost HISTORY

Here is the error if i specify disk size in Launch Template, using 14.0.0 of this module

Error: error creating EKS Node Group (ekscluster:nodegroup): InvalidParameterException: Disk size must be specified within the launch template.
{
  RespMetadata: {
    StatusCode: 400,
    RequestID: "f8b20083-250e-4148-b456-2e929008b65c"
  },
  ClusterName: "ekscluster",
  Message_: "Disk size must be specified within the launch template.",
  NodegroupName: "nodegroup"
}

I think I know the use case. I specify the disk_size in the node_group config, and then before passing it on to the EKS module, i create a launch template for each node group to be able to specify different characteristics.. so the launch template is defined with that disk size, but the disk_size key still exists in the node_groups config, and then gets passed on to the managed node group resource without this patch

@theothermike
Copy link
Author

closing in favour of #1138

@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, 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 15, 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

Successfully merging this pull request may close these issues.

3 participants