-
Notifications
You must be signed in to change notification settings - Fork 467
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
Let the users decide whether adding a random suffix in cluster and pool's name or not. #496
Conversation
Potential Breaking Changes in d236676: |
I believe you meant to tag @the-technat ☝️ |
Potential Breaking Changes in 93a56a7: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a prone to errors doing it that way, but that should certainly give the user the flexibility to do it the way he wants.
Thanks @lonegunmanb for coding this!
Potential Breaking Changes in b0d89e5: |
Potential Breaking Changes in 7eecde5: |
Potential Breaking Changes in 7b9e248: |
Potential Breaking Changes in a90d786: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works. I tested like this:
locals {
node_pools = {
user = {
name = "user"
vm_size = var.agents_size
enable_auto_scaling = true
node_count = null
min_count = 1
max_count = 3
vnet_subnet_id = lookup(module.network.vnet_subnets_name_id, "aks")
},
ingress = {
name = "ingress"
vm_size = var.agents_size
enable_auto_scaling = false
node_count = 2
vnet_subnet_id = lookup(module.network.vnet_subnets_name_id, "aks")
node_taints = ["node-role.kubernetes.io/ingress=true:NoSchedule"]
},
spot = {
name = "spot"
vm_size = var.agents_size
enable_auto_scaling = true
node_count = null
min_count = 1
max_count = 10
vnet_subnet_id = lookup(module.network.vnet_subnets_name_id, "aks")
eviction_policy = "Delete"
spot_max_price = 0.1
priority = "Spot"
node_taints = ["kubernetes.azure.com/scalesetpriority=spot:NoSchedule"]
node_labels = { "kubernetes.azure.com/scalesetpriority" = "spot" }
zones = ["1", "2", "3"]
create_before_destroy = false
},
}
}
@@ -96,7 +96,7 @@ resource "kubernetes_ingress_v1" "ing" { | |||
} | |||
|
|||
resource "time_sleep" "wait_for_ingress" { | |||
create_duration = "15m" | |||
create_duration = "5m" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is OK but it is unrelated to the goal of this PR.
@@ -34,14 +39,13 @@ resource "azurerm_kubernetes_cluster_node_pool" "node_pool" { | |||
snapshot_id = each.value.snapshot_id | |||
spot_max_price = each.value.spot_max_price | |||
tags = merge(each.value.tags, (/*<box>*/ (var.tracing_tags_enabled ? { for k, v in /*</box>*/ { | |||
avm_yor_name = "node_pool" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change ? It is just changing the order in the tag list, correct ?
variable "cluster_name_random_suffix" { | ||
type = bool | ||
default = false | ||
description = "Whether to add a random suffix on Aks cluster's name or not. `azurerm_kubernetes_cluster` resource defined in this module is `create_before_destroy = true` implicity now(described [here](https://github.com/Azure/terraform-azurerm-aks/issues/389)), without this random suffix we'll not be able to recreate this cluster directly due to the naming conflict." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat: Aks should be all capital. AKS. Add a space in now(described
. Should be now (described
.
test/upgrade/upgrade_test.go
Outdated
MaxRetries: 20, | ||
TimeBetweenRetries: time.Minute, | ||
RetryableTerraformErrors: map[string]string{ | ||
"data.kubernetes_ingress_v1.ing.status[0].load_balancer[0].ingress": "the ingress hasn't been created, need more time", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need this change in this PR and not in a different PR ?
test/e2e/terraform_aks_test.go
Outdated
@@ -157,6 +158,11 @@ func TestExamples_applicationGatewayIngress(t *testing.T) { | |||
"use_brown_field_application_gateway": u.useBrownFieldAppGw, | |||
"create_role_assignments_for_application_gateway": u.createRoleBindingForAppGw, | |||
}, | |||
MaxRetries: 20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need this change in this PR and not in a different PR ?
@@ -1026,6 +1034,7 @@ variable "node_pools" { | |||
})) | |||
workload_runtime = (Optional) Used to specify the workload runtime. Allowed values are `OCIContainer` and `WasmWasi`. WebAssembly System Interface node pools are in Public Preview - more information and details on how to opt into the preview can be found in [this article](https://docs.microsoft.com/azure/aks/use-wasi-node-pools) | |||
zones = (Optional) Specifies a list of Availability Zones in which this Kubernetes Cluster Node Pool should be located. Changing this forces a new Kubernetes Cluster Node Pool to be created. | |||
create_before_destroy = (Optional) Create a new node pool before destroy the old one when Terraform must update an argument that cannot be updated in-place. Set this argument to `true` will add add a random suffix to pool's name to avoid conflict. Default to `true`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now why @the-technat commented that this is error prone.
The create_before_destroy
value is defined per node pool inside the map. Then we cannot enforce this is a boolean value.
We also need to test the mixed scenario where some nodepools have create_before_destroy = true
and others have create_before_destroy = false
.
@lonegunmanb please have a look at the minor comments I left on the PR. It is good to merge |
63a173f
to
5ff67ea
Compare
… whether they'd like to add a random suffix a cluster's name. add create_before_destroy to var.nodes so the users could decide whether they'd like to have a random suffix in node pools' names.
5ff67ea
to
984612f
Compare
As #389 described, now
azurerm_kubernetes_cluster.main
has been set tocreate_before_destroy = true
implicity because anazurerm_kubernetes_cluster_node_pool
withcreate_before_destroy = true
referred it, this meta argument config has been propagated to the aks resource.We also have #491 complaining that they don't want a random suffix in node pool's name. We added
create_before_destroy = true
to the node pool resource in #357 since we'd like to create a new replacement pool before we destroy the old one so all running pods on the old pool could be evicted to the new one before the worker node have been destroyed, so the cluster's operator could avoid downtime when they have to recreate the node pool.We respect that the users should have the right to decide the style they'd adapted already, so this pr replicated a new
azurerm_kubernetes_cluster_node_pool
, and addedcreate_before_destroy
invar.node_pools
object definition.Setting this
create_before_destroy
totrue
, nothing would happen, your node pool would still be created and managed in the old way, with a random name suffix.Setting this
create_before_destroy
tofalse
, the node pool in brown field would be destroyed and recreated in new address, without random name suffix, and when Terraform have to recreate this node pool, you must delete the old pool and create the new replacement pool with a different name. To avoid downtime we encourage the operators to create the replacement pool first, then destroy the old one.We also added a new variable
cluster_name_random_suffix
, setting this variable totrue
would add a random suffix to cluster's name when you're creating an Aks instance in greenfield, but will do nothing to the instances in brownfield. With this random suffix now Terraform could recreate the Aks instance directly without any naming conflict.@zioproto @TechieNat @tdevopsottawa WDYT?
Describe your changes
Issue number
#000
Checklist before requesting a review
CHANGELOG.md
fileThanks for your cooperation!