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

Extra eks node group #85

Merged
merged 3 commits into from
Aug 3, 2022
Merged

Extra eks node group #85

merged 3 commits into from
Aug 3, 2022

Conversation

gengmao
Copy link
Contributor

@gengmao gengmao commented Aug 3, 2022

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #84

Motivation

This is a small change to allow adding an extra node pool with a different instance type. Therefore we can use a blue/green strategy to switch instances from the default node pool to a extra node pool via taint/drain. This keeps backward compatibility - the node group names are not changed, it can be used on previously created cluster/node groups.

Modifications

Add a extra_node_pool_instance_types variable

Verifying this change

  • Validated in test environment

Documentation

Check the box below.

Need to update docs?

  • doc

    (If this PR contains doc changes)

@gengmao gengmao requested a review from a team as a code owner August 3, 2022 04:48
@github-actions github-actions bot added the doc This pr contains a document label Aug 3, 2022
@gengmao gengmao requested a review from jrsdav August 3, 2022 04:48
@jrsdav
Copy link
Contributor

jrsdav commented Aug 3, 2022

I wonder if we should be a bit more explicit in the naming of our node groups.

This might be a future optimization, but it might be useful to interpolate the instance class ID -- or just have it perform a lookup in a map. Something like this:

locals {
  instance_size = {
    c6i.large = "tiny"
    c6i.xlarge = "small"
    c6i.2xlarge = "medium"
    ... 
    ...
   }
   
   snc_extra_node_config = (length(var.extra_node_pool_instance_types) == 0 ? {} : { for i, v in var.private_subnet_ids : "snc-${lookup(local.instance_size, var.extra_node_pool_instance_types[i])}-node-pool${i}" => merge(local.node_pool_defaults, { subnets = [var.private_subnet_ids[i]], name = "snc-extra-node-pool${i}", instance_types = var.extra_node_pool_instance_types }) })

I haven't tested this at all to validate, but the idea here is given the following list of var.extra_node_pool_instance_types of ["c6i.large", "c6i.xlarge", "c6i.2xlarge"] (and three items in var.private_subnet_ids, for example) we would end up with node groups named as follows:

snc-tiny-node-pool{0..2}
snc-small-node-pool{0..2}
snc-medium-node-pool{0..3}

Let me know if this makes sense or not or if you think it would be worthwhile.

@gengmao
Copy link
Contributor Author

gengmao commented Aug 3, 2022

Let me know if this makes sense or not or if you think it would be worthwhile.

@jrsdav what you described makes sense, just I couldn't figure out how to write nested "for" loops to get a cartesian product of a list of instance types and a list of subnets. So the extra_node_pool_instance_types is treated as same as node_pool_instance_types, not expanded.
Can we merge this for updating the instance type of existing clusters in short term, then make a breaking change to provision node groups for new clusters in a more desired way?

@jrsdav
Copy link
Contributor

jrsdav commented Aug 3, 2022

Yes, perfectly reasonable. We'll continue to refine node groups in future releases.

@gengmao gengmao merged commit aa2024a into master Aug 3, 2022
@delete-merged-branch delete-merged-branch bot deleted the extra_eks_node_group branch August 3, 2022 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc This pr contains a document
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants