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

Cleaning up and deduplicating launch template related code #370

Merged
merged 5 commits into from
May 7, 2019
Merged

Cleaning up and deduplicating launch template related code #370

merged 5 commits into from
May 7, 2019

Conversation

max-rocket-internet
Copy link
Contributor

@max-rocket-internet max-rocket-internet commented May 3, 2019

In this PR I'm having a large cleanup of all code related to Launch Templates (LTs)! This is part of an effort to make everything related to LTs first class. Apart from simply being the direction AWS is going, LTs have some other distinct advantages:

  • Allows the use of overrides in the ASG for multiple instance types (e.g. c4 and m4 instances mixed)
  • Allows the use of mixed_instances_policy ASG for multiple instance lifecycle options (e.g. spot and on-demand instances mixed)
  • More network configuration options
  • More placement options
  • More options for everything

Reducing duplication

I am removing local.workers_group_launch_template_defaults_defaults as it's 96% the same as local.workers_group_defaults_defaults. The reason for the initial separation was that when this was merged, LTs were new to both AWS and this module. So we made all LT related stuff separate. Now I think we merge them to reduce duplication. This also means removing variable worker_group_launch_template_tags and local.workers_group_launch_template_defaults.

I have commented in local.tf to show the few options that apply only to LT based workers.

Removing instance overrides in ASG

Currently we have in aws_autoscaling_group.workers_launch_template 2 instance override and mixed_instances_policy. I am removing these and just using the single instance type from the LT. The reason is that if you are wanting to have LT based worker groups, then having 2 instance types is just confusing. e.g. here. And if you want to run a mixed worker group, then 2 instance types is not nearly enough.

I think after this is merged I will create a new PR for a worker group ASG that has 4-6 instance overrides and a mixed_instances_policy to match.

Misc

  • Add service_linked_role_arn option to aws_autoscaling_group.workers

Checklist

  • terraform fmt and terraform validate both work from the root and examples/eks_test_fixture directories (look in CI for an example)
  • Tests for the changes have been added and passing (for bug fixes/features)
  • Test results are pasted in this PR (in lieu of CI)
  • I've added my change to CHANGELOG.md
  • Any breaking changes are highlighted above

@max-rocket-internet max-rocket-internet requested a review from a team May 3, 2019 09:36
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@max-rocket-internet max-rocket-internet merged commit 2439c25 into terraform-aws-modules:master May 7, 2019
@max-rocket-internet max-rocket-internet deleted the lt_fixes2 branch May 7, 2019 13:41
@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 19, 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.

2 participants