Skip to content

Conversation

@romancin
Copy link

This PR enables Default Compute Class support for Cluster Autoscaler.

@romancin romancin requested review from a team, apeabody and ericyz as code owners September 12, 2025 05:08
@romancin romancin marked this pull request as draft September 12, 2025 05:20
@romancin romancin marked this pull request as ready for review September 12, 2025 05:25
Copy link
Collaborator

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @romancin!

Please resolve the linter error:

╷
│ Error: Unsupported argument
│ 
│   on ../../modules/beta-private-cluster/cluster.tf line 179, in resource "google_container_cluster" "primary":
│  179:     default_compute_class_enabled = lookup(var.cluster_autoscaling, "enable_default_compute_class", false)
│ 
│ An argument named "default_compute_class_enabled" is not expected here.

@romancin
Copy link
Author

Thanks for the contribution @romancin!

Please resolve the linter error:

╷
│ Error: Unsupported argument
│ 
│   on ../../modules/beta-private-cluster/cluster.tf line 179, in resource "google_container_cluster" "primary":
│  179:     default_compute_class_enabled = lookup(var.cluster_autoscaling, "enable_default_compute_class", false)
│ 
│ An argument named "default_compute_class_enabled" is not expected here.

Which google provider version is the linter using? Because this argument is available only since v7.

@apeabody
Copy link
Collaborator

Thanks for the contribution @romancin!
Please resolve the linter error:

╷
│ Error: Unsupported argument
│ 
│   on ../../modules/beta-private-cluster/cluster.tf line 179, in resource "google_container_cluster" "primary":
│  179:     default_compute_class_enabled = lookup(var.cluster_autoscaling, "enable_default_compute_class", false)
│ 
│ An argument named "default_compute_class_enabled" is not expected here.

Which google provider version is the linter using? Because this argument is available only since v7.

Got it - It's not the linter per say, but all the relevant versions constraints in the Terraform code as the highest allowed is selected. In this case it's the most recent v6.x.

I recommend you start by bumping the relevant minimum versions in https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/main/autogen/main/versions.tf.tmpl#L27, as that will more clearly illustrate the dependencies. I know at a minimum we are waiting on #2456 for some examples.

@apeabody
Copy link
Collaborator

Thanks for the contribution @romancin!

The code base is now TPGv7 compatible, and initial support was added in #2434. However this implementation might be preferable?

@stdmje
Copy link

stdmje commented Oct 21, 2025

Any news on this one? 1.33 is the now in the STABLE channel and ComputeClasses are such a nice feature!

@NeckBeardPrince
Copy link

Can we please get an update on this? The last release enabled this feature, but it doesn't work without this PR.

@romancin
Copy link
Author

romancin commented Oct 30, 2025

This one #2434 included the same change. I suppose this one is not needed anymore, right @apeabody ?

@apeabody
Copy link
Collaborator

initial support was added in #2434. However this implementation might be preferable?

Hi @romancin - Initial support was added in #2434. However this more complete implementation might be preferable?

@apeabody
Copy link
Collaborator

This one #2434 included the same change. I suppose this one is not needed anymore, right @apeabody ?

Hi @romancin - Initial support was added in #2434. However this more complete implementation might be preferable?

@NeckBeardPrince
Copy link

Bump

@romancin romancin force-pushed the feat-default-compute-class branch 2 times, most recently from 3a4cf85 to 0fde8c6 Compare November 17, 2025 06:11
@romancin
Copy link
Author

This one #2434 included the same change. I suppose this one is not needed anymore, right @apeabody ?

Hi @romancin - Initial support was added in #2434. However this more complete implementation might be preferable?

@apeabody Done the changes. Can you execute the linter now please? Thanks!

@romancin
Copy link
Author

romancin commented Nov 17, 2025

Ok, I see it is failing because in autopilot mode, no cluster_autoscaling variable is defined... So we want to maintain both cases? Define an independent variable called default_compute_class_enabled for using it only on autopilot clusters, and for standard ones keep that as an attribute of cluster_autoscaling variable?

@apeabody
Copy link
Collaborator

Ok, I see it is failing because in autopilot mode, no cluster_autoscaling variable is defined... So we want to maintain both cases? Define an independent variable called default_compute_class_enabled for using it only on autopilot clusters, and for standard ones keep that as an attribute of cluster_autoscaling variable?

Hi @romancin - I haven't looked through the PR in depth, but wanted to pass along that the templates can have different variables/behavior/etc for standard vs autopilot clusters. For example {% if autopilot_cluster != true %} {% else %} {% endif %}

@apeabody
Copy link
Collaborator

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the configuration for enabling the default compute class in the cluster autoscaler. The default_compute_class_enabled variable is moved into the cluster_autoscaling object, which improves configuration structure. However, this change introduces a critical issue in the autopilot cluster modules (beta-autopilot-private-cluster and beta-autopilot-public-cluster). These modules now reference var.cluster_autoscaling, which is not defined for them, and this will cause Terraform deployments to fail. The template files need to be adjusted to correctly handle the configuration for autopilot clusters.

Remove default_compute_class_enabled variable and update references to use cluster_autoscaling.enable_default_compute_class instead. This change simplifies the configuration by eliminating the need for a separate variable for enabling Spot VMs as the default compute class for Node Auto-Provisioning across multiple modules and documentation.
This change introduces the default_compute_class_enabled variable
to enable default compute class for Node Auto-Provisioning.
This allows users to configure the compute class more flexibly
when setting up their Kubernetes clusters.
@romancin romancin force-pushed the feat-default-compute-class branch from 36e2adb to cf486f5 Compare November 18, 2025 07:16
@romancin
Copy link
Author

Kept the variable for autopilot. This can be reviewed again.

@romancin romancin requested a review from apeabody November 18, 2025 15:44
@apeabody
Copy link
Collaborator

Kept the variable for autopilot. This can be reviewed again.

Hi @romancin - From the linter:

terraform_validate ./test/fixtures/simple_autopilot_private
╷
│ Error: Reference to undeclared input variable
│ 
│   on ../../../modules/beta-autopilot-private-cluster/cluster.tf line 106, in resource "google_container_cluster" "primary":
│  106:     default_compute_class_enabled = var.default_compute_class_enabled
│ 
│ An input variable with the name "default_compute_class_enabled" has not
│ been declared. This variable can be declared with a variable
│ "default_compute_class_enabled" {} block.
╵
terraform_validate ./test/fixtures/simple_autopilot_public
╷
│ Error: Reference to undeclared input variable
│ 
│   on ../../../modules/beta-autopilot-public-cluster/cluster.tf line 106, in resource "google_container_cluster" "primary":
│  106:     default_compute_class_enabled = var.default_compute_class_enabled
│ 
│ An input variable with the name "default_compute_class_enabled" has not
│ been declared. This variable can be declared with a variable
│ "default_compute_class_enabled" {} block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants