Skip to content

fix: add missing ForceNew node_config fields to update-variant keepers #2579

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.

Sign up for GitHub

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

Open
voron wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: add missing ForceNew node_config fields to update-variant keepers #2579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions autogen/main/cluster.tf.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,11 @@ locals {
"flex_start",
"local_ssd_ephemeral_storage_count",
"ephemeral_storage_local_ssd_data_cache_count",
"local_nvme_ssd_count",
"threads_per_core",
"enable_nested_virtualization",
"performance_monitoring_unit",
"node_affinity",
Comment on lines +933 to +937

@gemini-code-assist gemini-code-assist Bot Apr 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The addition of these five fields is correct as they are indeed ForceNew attributes. However, several other critical ForceNew fields that the module accepts in the node_pools map are also missing from this list. Missing these fields will cause the update-variant modules to fail with a 409 conflict when they are changed, as the node pool name won't be rotated, preventing create_before_destroy from working correctly.

Consider adding the following missing fields as well:

  • machine_type
  • disk_size_gb
  • disk_type
  • image_type
  • node_metadata
  • enable_gcfs
  • enable_gvnic
  • enable_fast_socket
  • provisioned_iops
  • provisioned_throughput
  • enable_confidential_nodes

This will make the zero-downtime update logic much more robust for common node pool configuration changes.

    "local_nvme_ssd_count",
    "threads_per_core",
    "enable_nested_virtualization",
    "performance_monitoring_unit",
    "node_affinity",
    "machine_type",
    "disk_size_gb",
    "disk_type",
    "image_type",
    "node_metadata",
    "enable_gcfs",
    "enable_gvnic",
    "enable_fast_socket",
    "provisioned_iops",
    "provisioned_throughput",
    "enable_confidential_nodes",

@voron voron Apr 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks, but I don't think these belong in the keepers list — most of them aren't ForceNew in the provider:

  • machine_type, disk_size_gb, disk_type, image_type — updateable in place
  • enable_gcfs, enable_gvnic, enable_fast_socket — updateable
  • provisioned_iops, provisioned_throughput — updateable (handled as part of disk update)
  • node_metadata — not a current node_config field (legacy alias of workload_metadata_config.mode, which is updateable)
  • enable_confidential_nodes — the ForceNew bits of confidential_nodes are already covered via enable_confidential_storage; the surface this module exposes doesn't add a new ForceNew gap here

Adding any of these would force full pool recreation on routine in-place changes, which is the opposite of what this list is for.

The five fields in this PR were picked by cross-checking node_config in the provider source for ForceNew: true against what the module actually surfaces from node_pools. The reviewers' confidence list in the original write-up enumerated the rest already-covered.

]
}

Expand Down
5 changes: 5 additions & 0 deletions modules/beta-private-cluster-update-variant/cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,11 @@ locals {
"flex_start",
"local_ssd_ephemeral_storage_count",
"ephemeral_storage_local_ssd_data_cache_count",
"local_nvme_ssd_count",
"threads_per_core",
"enable_nested_virtualization",
"performance_monitoring_unit",
"node_affinity",
]
}

Expand Down
5 changes: 5 additions & 0 deletions modules/beta-public-cluster-update-variant/cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,6 +784,11 @@ locals {
"flex_start",
"local_ssd_ephemeral_storage_count",
"ephemeral_storage_local_ssd_data_cache_count",
"local_nvme_ssd_count",
"threads_per_core",
"enable_nested_virtualization",
"performance_monitoring_unit",
"node_affinity",
]
}

Expand Down
5 changes: 5 additions & 0 deletions modules/private-cluster-update-variant/cluster.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,11 @@ locals {
"flex_start",
"local_ssd_ephemeral_storage_count",
"ephemeral_storage_local_ssd_data_cache_count",
"local_nvme_ssd_count",
"threads_per_core",
"enable_nested_virtualization",
"performance_monitoring_unit",
"node_affinity",
]
}

Expand Down