Add opportunistic_maintenance_strategy to GKE node_config host_maintenance_policy - #18492
Add opportunistic_maintenance_strategy to GKE node_config host_maintenance_policy#18492DrFaust92 wants to merge 3 commits into
Conversation
Adds the beta-only opportunistic_maintenance_strategy block under node_config.host_maintenance_policy for google_container_cluster and google_container_node_pool, exposing the GKE opportunistic maintenance feature (gcloud `--opportunistic-maintenance`). Fields: node_idle_time_window, maintenance_availability_window, min_nodes_per_pool.
|
Googlers: For automatic test runs see go/terraform-auto-test-runs. @c2thorn, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
c2thorn
left a comment
There was a problem hiding this comment.
I'd rather beta-guard the field, similar to ephemeral_storage_config, although I acknowledge the other field not doing so.
No need to make a larger breaking change, let's just not introduce this specific field to GA.
Keep opportunistic_maintenance_strategy out of the GA provider (schema + cluster/node-pool meta.yaml), matching the ephemeral_storage_config pattern. Expand/flatten were already inside the beta-only host_maintenance_policy helpers. Verified via mmv1 generation: GA renders 0 occurrences, beta unchanged.
|
Done — beta-guarded Verified locally via mmv1 generation: GA renders 0 |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit 8066006: Diff reportYour PR generated the following diffs in downstream repositories:
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_container_cluster" "primary" {
node_config {
host_maintenance_policy {
opportunistic_maintenance_strategy {
maintenance_availability_window = # value needed
min_nodes_per_pool = # value needed
node_idle_time_window = # value needed
}
}
}
node_pool {
node_config {
host_maintenance_policy {
opportunistic_maintenance_strategy {
maintenance_availability_window = # value needed
min_nodes_per_pool = # value needed
node_idle_time_window = # value needed
}
}
}
}
}
Missing doc report (experimental)The following resources have fields missing in documents.
Test reportImportant Manual Verification Required VCR cannot automatically execute the following tests added in this PR. Please verify them manually: 🔴 TestAccContainerNodePool_withOpportunisticMaintenance Analytics
Affected Service Packages
Step 1: Replaying Mode 🟢 All tests passed in Replaying mode! No Recording was needed. Caution Issues requiring attention before PR completion 🔴 Manual Verification Required: New tests were added that are skipped in presubmit tests. See the "Manual Verification Required" section at the top for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the replaying VCR build log @DrFaust92, @c2thorn VCR tests complete for 8066006! |
|
@c2thorn This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
Adds the beta-only
opportunistic_maintenance_strategyblock undernode_config.host_maintenance_policy, shared bygoogle_container_clusterandgoogle_container_node_pool. This exposes GKE's opportunistic maintenance feature — the Terraform equivalent of the gcloudcontainer node-pools create --opportunistic-maintenanceflag.Fields (all beta, ForceNew):
node_idle_time_window— idle time before maintenance is triggeredmaintenance_availability_window— window in which opportunistic maintenance may runmin_nodes_per_pool— minimum available nodes; blocks maintenance below thisMaps to the API's
HostMaintenancePolicy.opportunisticMaintenanceStrategy(container/v1beta1), which is absent from GA — so it sits under the existing beta guard onhost_maintenance_policy.min_nodes_per_poolusesForceSendFieldsso an explicit0is sent rather than dropped byomitempty.Also documents
host_maintenance_policyin the cluster docs (it was previously undocumented).Testing
Verified end-to-end against a real GKE project:
400: opportunistic maintenance strategy is supported with TPU or GPU node pools only, confirming the field is parsed and validated by the API.Because opportunistic maintenance requires a GPU/TPU node pool and a host-maintenance allowlist,
TestAccContainerNodePool_withOpportunisticMaintenanceist.Skip'd by default (consistent with the existingTestAccContainerNodePool_withHostMaintenancePolicy) and intended to be run manually in an allowlisted project with GPU capacity.Note for maintainers
host_maintenance_policyis beta-only (absent from the GAcontainer/v1API and Go client), but its schema block is currently not beta-guarded — only itsexpand/flattenwiring is. So the field (and nowopportunistic_maintenance_strategy) appears in the GA provider schema while being inert there. This is pre-existing behavior —maintenance_intervalalready does this — and I followed the existing pattern rather than change it in this PR. Happy to additionally wrap the wholehost_maintenance_policyschema block in the beta guard if you'd prefer to fix the leak (note that would remove the field from the GA schema, a minor breaking change for any GA config that references it).