add user node pool & availability zones #9

nellyk · 2024-02-12T12:30:21Z

closes #8
closes #5

zioproto · 2024-02-20T20:53:57Z

Can you please run ./avm pre-commit :) thanks

main.tf

lonegunmanb

Hi @nellyk , some review comments.

main.tf

variables.tf

examples/with_region_having_availability_zone/main.tf

examples/with_region_having_no_availability_zone/main.tf

README.md

examples/with_region_having_no_availability_zone/_header.md

examples/with_region_having_no_availability_zone/main.tf

zioproto · 2024-02-27T08:54:39Z

variables.tf

+    min_count           = number
+    node_count          = number
+    os_sku              = string
+    mode                = optional(string, "User")


Why we have the mode ? Are the possible values just User and System ? Can the customer really create a System nodepool defining it in this variable ?

zioproto · 2024-02-27T08:56:07Z

variables.tf

+      enable_auto_scaling = true
+      max_count           = 110
+      min_count           = 2
+      node_count          = 2


Why you set node_count if enable_auto_scaling is true ?

If the autoscaler changed the number of nodes you risk that a Terraform run will revert the current number of node to node_count. I believe the correct value for node_count is null when the autoscaler is active.

Thanks Saverio, i'll update this

zioproto · 2024-02-27T09:00:43Z

examples/with_region_having_no_availability_zone/README.md

+  features {}
+}
+
+


i'll check if this a problem upstream

examples/with_region_having_no_availability_zone/main.tf

zioproto · 2024-02-27T09:09:20Z

variables.tf

+    mode                = optional(string, "User")
+    os_disk_size_gb     = optional(number, null)
+    tags                = optional(map(string), {})
+    zones               = optional(string)


Should this be a set of string like here ?
https://github.com/Azure/terraform-azurerm-aks/blob/4f7efad06d3471f22e960ad7874b2888e3754f26/variables.tf#L972

examples/with_region_having_availability_zone/main.tf

zioproto · 2024-02-27T09:28:25Z

variables.tf

+    tags                = optional(map(string), {})
+    zones               = optional(string)
+  }))
+  default = {


This is going to create 3 nodepools also in those regions that don't support AZ, correct ? Is this what we want ?

i've added a condition to only create 1 if the zone isn't available, unfortunately i still have to keep the 3 node pool values since i can't use count on a list to control the generation of user node pool

zioproto · 2024-02-27T09:40:48Z

main.tf

@@ -55,6 +63,21 @@ resource "azurerm_management_lock" "this" {
  scope      = azurerm_kubernetes_cluster.this.id
 }

+resource "azurerm_kubernetes_cluster_node_pool" "this" {
+  # set max nodepools created to 3
+  for_each = var.node_pools


Looping on the node_pools is correct, but the intent is to have multiple node_pools in the map for multiple workloads ( e.g. ingress nodepool and application nodepool).

When we need here is a nested loop, that in Terraform is not straightforward.

We need to loop once o the node_pools map, and each element has to be expanded to 1 node_pool per zone.

I would ask the help of @lonegunmanb to understand if this can be implemented with a local and then we loop on the local value.

i have implemented a local now that provides the zones if they are available creates three node pools with each having an availability zone

zioproto · 2024-02-27T09:45:14Z

variables.tf

+    zones               = optional(string)
+  }))
+  default = {
+    "1" = {


To avoid confusion you could call it "workload1" "workload2" and so on

added this to the when the resource is being created

commit 2d3630b Author: nellyk <[email protected]> Date: Mon Mar 4 19:55:30 2024 +0000 review comments commit 861af28 Author: nellyk <[email protected]> Date: Mon Mar 4 19:45:52 2024 +0000 review comments commit 1dc4d1b Author: nellyk <[email protected]> Date: Mon Mar 4 19:40:56 2024 +0000 review comments commit deeee1c Author: nellyk <[email protected]> Date: Mon Mar 4 11:08:26 2024 +0000 review comments commit ab7c134 Author: nellyk <[email protected]> Date: Wed Feb 21 12:06:49 2024 +0000 fix failing test commit 93d2a9f Author: nellyk <[email protected]> Date: Wed Feb 21 11:48:42 2024 +0000 fix failing test commit 1401bf8 Author: nellyk <[email protected]> Date: Wed Feb 21 10:36:29 2024 +0000 add pr review comments commit 715659a Author: nellyk <[email protected]> Date: Fri Feb 9 22:16:17 2024 +0000 set up aks cluster user node pools and availability zone commit 02a8464 Author: nellyk <[email protected]> Date: Tue Feb 6 19:02:21 2024 +0000 add availability zone setup commit 4dda197 Author: nellyk <[email protected]> Date: Tue Feb 6 18:57:21 2024 +0000 add availability zone setup commit bd20d79 Merge: 680b00c 05cfc97 Author: nellyk <[email protected]> Date: Tue Feb 6 17:16:36 2024 +0000 Merge remote-tracking branch 'origin' into availability-zones commit 680b00c Author: nellyk <[email protected]> Date: Tue Feb 6 17:15:31 2024 +0000 add availability zone setup commit e873a28 Author: nellyk <[email protected]> Date: Tue Feb 6 16:20:04 2024 +0000 add availability zone setup commit 05cfc97 Merge: cfc986e d4255cd Author: Saverio Proto <[email protected]> Date: Tue Feb 6 16:38:38 2024 +0100 Merge pull request Azure#3 from nellyk/main rename to avm-ptn-aks-production commit bf2c904 Author: nellyk <[email protected]> Date: Tue Feb 6 15:33:52 2024 +0000 add availability zone setup commit cfc986e Merge: 7e4b10b af7a2cc Author: Saverio Proto <[email protected]> Date: Tue Feb 6 09:46:25 2024 +0100 Merge pull request #2 from nellyk/main Initial draft with default node pool

zioproto · 2024-03-05T10:50:40Z

main.tf

+resource "azurerm_kubernetes_cluster_node_pool" "this" {
+  # if the region has zone create a node pool per zone
+  # if the region does not have zone create a single node pool with the zone as null
+  # if node pools are not emplty check if the node has a zone if yes then create a node pool per zone otherwise create a single node pool


emplty -> empty

zioproto · 2024-03-11T18:51:39Z

closing in favor of #15

nellyk changed the title ~~Feat/aks cluster~~ add user node pool Feb 12, 2024

nellyk mentioned this pull request Feb 12, 2024

Availability zones #5

Closed

nellyk changed the title ~~add user node pool~~ add user node pool & availability zones Feb 12, 2024

nellyk requested review from zioproto and Jingwei-MS February 12, 2024 12:32

nellyk temporarily deployed to test February 12, 2024 12:32 — with GitHub Actions Inactive

nellyk requested a review from lonegunmanb February 12, 2024 12:33

nellyk temporarily deployed to test February 12, 2024 12:38 — with GitHub Actions Inactive

zioproto requested changes Feb 20, 2024

View reviewed changes

main.tf Outdated Show resolved Hide resolved

lonegunmanb requested changes Feb 21, 2024

View reviewed changes

main.tf Outdated Show resolved Hide resolved

variables.tf Outdated Show resolved Hide resolved

examples/with_region_having_availability_zone/main.tf Outdated Show resolved Hide resolved

examples/with_region_having_no_availability_zone/main.tf Outdated Show resolved Hide resolved

nellyk force-pushed the feat/aks_cluster branch from 29cf373 to 715659a Compare February 21, 2024 10:12

nellyk temporarily deployed to test February 21, 2024 10:37 — with GitHub Actions Inactive

nellyk had a problem deploying to test February 21, 2024 10:37 — with GitHub Actions Failure

nellyk temporarily deployed to test February 21, 2024 10:37 — with GitHub Actions Inactive

nellyk had a problem deploying to test February 21, 2024 11:51 — with GitHub Actions Error

nellyk had a problem deploying to test February 21, 2024 11:51 — with GitHub Actions Failure

nellyk mentioned this pull request Feb 21, 2024

monitoring+networking #10

Closed

1 task

nellyk temporarily deployed to test February 21, 2024 12:08 — with GitHub Actions Inactive

nellyk temporarily deployed to test February 21, 2024 12:20 — with GitHub Actions Inactive

zioproto requested changes Feb 27, 2024

View reviewed changes

zioproto reviewed Feb 27, 2024

View reviewed changes

nellyk force-pushed the feat/aks_cluster branch from 2d3630b to 7493892 Compare March 4, 2024 20:03

nellyk had a problem deploying to test March 4, 2024 20:06 — with GitHub Actions Failure

nellyk temporarily deployed to test March 4, 2024 20:06 — with GitHub Actions Inactive

fix failing builds

d00a628

nellyk had a problem deploying to test March 5, 2024 10:22 — with GitHub Actions Error

nellyk had a problem deploying to test March 5, 2024 10:22 — with GitHub Actions Failure

fix failing builds

e47d118

nellyk had a problem deploying to test March 5, 2024 10:37 — with GitHub Actions Failure

nellyk had a problem deploying to test March 5, 2024 10:46 — with GitHub Actions Failure

zioproto reviewed Mar 5, 2024

View reviewed changes

nellyk added 5 commits March 5, 2024 11:22

fix failing builds

be82331

fix failing builds

daeb296

fix failing builds

c14ddf9

fix failing builds

57430f7

fix failing builds

8b56bf2

nellyk had a problem deploying to test March 5, 2024 12:19 — with GitHub Actions Failure

nellyk temporarily deployed to test March 5, 2024 12:19 — with GitHub Actions Inactive

nellyk had a problem deploying to test March 5, 2024 12:19 — with GitHub Actions Failure

nellyk added 3 commits March 6, 2024 08:35

use flatten list

c9b1b62

fix zone and nodepool generation

87056ed

fix zone and nodepool generation

8872d10

nellyk had a problem deploying to test March 8, 2024 09:44 — with GitHub Actions Failure

nellyk temporarily deployed to test March 8, 2024 09:44 — with GitHub Actions Inactive

nellyk had a problem deploying to test March 8, 2024 09:44 — with GitHub Actions Failure

fix zone and nodepool generation

70c6de3

zioproto mentioned this pull request Mar 11, 2024

add user node pool & availability zones #15

Merged

zioproto closed this Mar 11, 2024

nellyk had a problem deploying to test April 7, 2024 21:22 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add user node pool & availability zones #9

add user node pool & availability zones #9

nellyk commented Feb 12, 2024 •

edited

Loading

zioproto commented Feb 20, 2024

lonegunmanb left a comment

zioproto Feb 27, 2024

zioproto Feb 27, 2024

nellyk Mar 4, 2024

zioproto Feb 27, 2024

nellyk Feb 27, 2024

zioproto Feb 27, 2024

zioproto Feb 27, 2024

nellyk Mar 4, 2024

zioproto Feb 27, 2024 •

edited

Loading

nellyk Mar 4, 2024

zioproto Feb 27, 2024

nellyk Mar 4, 2024

zioproto Mar 5, 2024

zioproto commented Mar 11, 2024

add user node pool & availability zones #9

add user node pool & availability zones #9

Conversation

nellyk commented Feb 12, 2024 • edited Loading

zioproto commented Feb 20, 2024

lonegunmanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zioproto Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zioproto commented Mar 11, 2024

nellyk commented Feb 12, 2024 •

edited

Loading

zioproto Feb 27, 2024 •

edited

Loading