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: Get on_demand_allocation_strategy from local.workers_group_defaults when deciding to use mixed_instances_policy #908

Conversation

trnubo
Copy link
Contributor

@trnubo trnubo commented Jun 5, 2020

Get on_demand_allocation_strategy from local.workers_group_defaults when deciding to use mixed_instances_policy

PR o'clock

Description

This fix uses the default from local.workers_group_defaults instead of null (the workers_group_defaults_defaults is null anyway) when the dynamic blocks mixed_instances_policy and launch_template are being decided on.

We use many similar worker groups (worker group per AZ, on-demand and spot groups) via worker_groups_launch_template and in an attempt to reduce duplicated config between worker groups we use workers_group_defaults. However setting on_demand_allocation_strategy and override_instance_types in the defaults doesn't have any effect when on_demand_allocation_strategy isn't set in each worker group.

This fix allows on_demand_allocation_strategy to be set in workers_group_defaults which will enable the mixed_instances_policy block and use the default override_instance_types even when neither are defined in the worker group. I don't see the default on_demand_allocation_strategy used anywhere else.

Checklist

@dpiddockcmp
Copy link
Contributor

Should the on_demand_allocation_strategy block around line 100 do a lookup for local.workers_group_defaults["on_demand_allocation_strategy"]? Although at the moment the only valid value is prioritized

@barryib barryib changed the title fix: Get on_demand_allocation_strategy from local.workers_group_defaults fix: Get on_demand_allocation_strategy from local.workers_group_defaults Jun 24, 2020
Copy link
Member

@barryib barryib left a comment

Choose a reason for hiding this comment

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

Nice catch. Thanks @trnubo

@barryib barryib changed the title fix: Get on_demand_allocation_strategy from local.workers_group_defaults fix: Get on_demand_allocation_strategy from local.workers_group_defaults when deciding to use mixed_instances_policy Jun 24, 2020
@barryib barryib merged commit c4edc6f into terraform-aws-modules:master Jun 24, 2020
barryib pushed a commit to Polyconseil/terraform-aws-eks that referenced this pull request Oct 25, 2020
@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 17, 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.

None yet

3 participants