From 356f448d45d6fdd43590a120784a7e96bb888ceb Mon Sep 17 00:00:00 2001 From: Modular Magician Date: Thu, 15 Apr 2021 22:11:04 +0000 Subject: [PATCH] Make GCE Instance node affinities updatable (#4402) Co-authored-by: upodroid Co-authored-by: Cameron Thornton Signed-off-by: Modular Magician --- .changelog/4402.txt | 3 + google/compute_instance_helpers.go | 48 +++++++-- google/resource_compute_instance.go | 31 +++++- google/resource_compute_instance_test.go | 126 ++++++++++++++++++++++- 4 files changed, 193 insertions(+), 15 deletions(-) create mode 100644 .changelog/4402.txt diff --git a/.changelog/4402.txt b/.changelog/4402.txt new file mode 100644 index 00000000000..9159c11535b --- /dev/null +++ b/.changelog/4402.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +compute: marked scheduling.0.node_affinities as updatable in `google_compute_instance` +``` diff --git a/google/compute_instance_helpers.go b/google/compute_instance_helpers.go index bdc1b46b947..2c9588ea91b 100644 --- a/google/compute_instance_helpers.go +++ b/google/compute_instance_helpers.go @@ -17,18 +17,15 @@ func instanceSchedulingNodeAffinitiesElemSchema() *schema.Resource { "key": { Type: schema.TypeString, Required: true, - ForceNew: true, }, "operator": { Type: schema.TypeString, Required: true, - ForceNew: true, ValidateFunc: validation.StringInSlice([]string{"IN", "NOT_IN"}, false), }, "values": { Type: schema.TypeSet, Required: true, - ForceNew: true, Elem: &schema.Schema{Type: schema.TypeString}, Set: schema.HashString, }, @@ -378,9 +375,18 @@ func flattenEnableDisplay(displayDevice *computeBeta.DisplayDevice) interface{} return displayDevice.EnableDisplay } +// Node affinity updates require a reboot +func schedulingHasChangeRequiringReboot(d *schema.ResourceData) bool { + o, n := d.GetChange("scheduling") + oScheduling := o.([]interface{})[0].(map[string]interface{}) + newScheduling := n.([]interface{})[0].(map[string]interface{}) + + return hasNodeAffinitiesChanged(oScheduling, newScheduling) +} + // Terraform doesn't correctly calculate changes on schema.Set, so we do it manually // https://github.com/hashicorp/terraform-plugin-sdk/issues/98 -func schedulingHasChange(d *schema.ResourceData) bool { +func schedulingHasChangeWithoutReboot(d *schema.ResourceData) bool { if !d.HasChange("scheduling") { // This doesn't work correctly, which is why this method exists // But it is here for posterity @@ -389,8 +395,11 @@ func schedulingHasChange(d *schema.ResourceData) bool { o, n := d.GetChange("scheduling") oScheduling := o.([]interface{})[0].(map[string]interface{}) newScheduling := n.([]interface{})[0].(map[string]interface{}) - originalNa := oScheduling["node_affinities"].(*schema.Set) - newNa := newScheduling["node_affinities"].(*schema.Set) + + if schedulingHasChangeRequiringReboot(d) { + return false + } + if oScheduling["automatic_restart"] != newScheduling["automatic_restart"] { return true } @@ -407,5 +416,30 @@ func schedulingHasChange(d *schema.ResourceData) bool { return true } - return reflect.DeepEqual(newNa, originalNa) + return false +} + +func hasNodeAffinitiesChanged(oScheduling, newScheduling map[string]interface{}) bool { + oldNAs := oScheduling["node_affinities"].(*schema.Set).List() + newNAs := newScheduling["node_affinities"].(*schema.Set).List() + if len(oldNAs) != len(newNAs) { + return true + } + for i := range oldNAs { + oldNodeAffinity := oldNAs[i].(map[string]interface{}) + newNodeAffinity := newNAs[i].(map[string]interface{}) + if oldNodeAffinity["key"] != newNodeAffinity["key"] { + return true + } + if oldNodeAffinity["operator"] != newNodeAffinity["operator"] { + return true + } + + // convertStringSet will sort the set into a slice, allowing DeepEqual + if !reflect.DeepEqual(convertStringSet(oldNodeAffinity["values"].(*schema.Set)), convertStringSet(newNodeAffinity["values"].(*schema.Set))) { + return true + } + } + + return false } diff --git a/google/resource_compute_instance.go b/google/resource_compute_instance.go index 9c434924106..6e2f8be28fd 100644 --- a/google/resource_compute_instance.go +++ b/google/resource_compute_instance.go @@ -512,7 +512,7 @@ func resourceComputeInstance() *schema.Resource { // !!! IMPORTANT !!! // We have a custom diff function for the scheduling block due to issues with Terraform's // diff on schema.Set. If changes are made to this block, they must be reflected in that - // method. See schedulingHasChange in compute_instance_helpers.go + // method. See schedulingHasChangeWithoutReboot in compute_instance_helpers.go Schema: map[string]*schema.Schema{ "on_host_maintenance": { Type: schema.TypeString, @@ -543,7 +543,6 @@ func resourceComputeInstance() *schema.Resource { Type: schema.TypeSet, Optional: true, AtLeastOneOf: schedulingKeys, - ForceNew: true, Elem: instanceSchedulingNodeAffinitiesElemSchema(), DiffSuppressFunc: emptyOrDefaultStringSuppress(""), Description: `Specifies node affinities or anti-affinities to determine which sole-tenant nodes your instances and managed instance groups will use as host systems.`, @@ -1344,7 +1343,9 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } - if schedulingHasChange(d) { + bootRequiredSchedulingChange := schedulingHasChangeRequiringReboot(d) + bootNotRequiredSchedulingChange := schedulingHasChangeWithoutReboot(d) + if bootNotRequiredSchedulingChange { scheduling, err := expandScheduling(d.Get("scheduling")) if err != nil { return fmt.Errorf("Error creating request data to update scheduling: %s", err) @@ -1629,7 +1630,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } - needToStopInstanceBeforeUpdating := scopesChange || d.HasChange("service_account.0.email") || d.HasChange("machine_type") || d.HasChange("min_cpu_platform") || d.HasChange("enable_display") || d.HasChange("shielded_instance_config") || len(updatesToNIWhileStopped) > 0 + needToStopInstanceBeforeUpdating := scopesChange || d.HasChange("service_account.0.email") || d.HasChange("machine_type") || d.HasChange("min_cpu_platform") || d.HasChange("enable_display") || d.HasChange("shielded_instance_config") || len(updatesToNIWhileStopped) > 0 || bootRequiredSchedulingChange if d.HasChange("desired_status") && !needToStopInstanceBeforeUpdating { desiredStatus := d.Get("desired_status").(string) @@ -1663,7 +1664,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err desiredStatus := d.Get("desired_status").(string) if statusBeforeUpdate == "RUNNING" && desiredStatus != "TERMINATED" && !d.Get("allow_stopping_for_update").(bool) { - return fmt.Errorf("Changing the machine_type, min_cpu_platform, service_account, enable_display, shielded_instance_config, " + + return fmt.Errorf("Changing the machine_type, min_cpu_platform, service_account, enable_display, shielded_instance_config, scheduling.node_affinities " + "or network_interface.[#d].(network/subnetwork/subnetwork_project) on a started instance requires stopping it. " + "To acknowledge this, please set allow_stopping_for_update = true in your config. " + "You can also stop it by setting desired_status = \"TERMINATED\", but the instance will not be restarted after the update.") @@ -1768,6 +1769,26 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } + if bootRequiredSchedulingChange { + scheduling, err := expandScheduling(d.Get("scheduling")) + if err != nil { + return fmt.Errorf("Error creating request data to update scheduling: %s", err) + } + + op, err := config.NewComputeBetaClient(userAgent).Instances.SetScheduling( + project, zone, instance.Name, scheduling).Do() + if err != nil { + return fmt.Errorf("Error updating scheduling policy: %s", err) + } + + opErr := computeOperationWaitTime( + config, op, project, "scheduling policy update", userAgent, + d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + } + // If the instance stops it can invalidate the fingerprint for network interface. // refresh the instance to get a new fingerprint if len(updatesToNIWhileStopped) > 0 { diff --git a/google/resource_compute_instance_test.go b/google/resource_compute_instance_test.go index eabd28347cc..c93532831c9 100644 --- a/google/resource_compute_instance_test.go +++ b/google/resource_compute_instance_test.go @@ -843,14 +843,22 @@ func TestAccComputeInstance_soleTenantNodeAffinities(t *testing.T) { Providers: testAccProviders, CheckDestroy: testAccCheckComputeInstanceDestroyProducer(t), Steps: []resource.TestStep{ + { + Config: testAccComputeInstance_withoutNodeAffinities(instanceName, templateName, groupName), + }, + computeInstanceImportStep("us-central1-a", instanceName, []string{"allow_stopping_for_update"}), { Config: testAccComputeInstance_soleTenantNodeAffinities(instanceName, templateName, groupName), }, - computeInstanceImportStep("us-central1-a", instanceName, []string{}), + computeInstanceImportStep("us-central1-a", instanceName, []string{"allow_stopping_for_update"}), { Config: testAccComputeInstance_soleTenantNodeAffinitiesUpdated(instanceName, templateName, groupName), }, - computeInstanceImportStep("us-central1-a", instanceName, []string{}), + computeInstanceImportStep("us-central1-a", instanceName, []string{"allow_stopping_for_update"}), + { + Config: testAccComputeInstance_soleTenantNodeAffinitiesReduced(instanceName, templateName, groupName), + }, + computeInstanceImportStep("us-central1-a", instanceName, []string{"allow_stopping_for_update"}), }, }) } @@ -1515,7 +1523,7 @@ func TestAccComputeInstance_updateRunning_desiredStatusRunning_allowStoppingForU }) } -const errorAllowStoppingMsg = "Changing the machine_type, min_cpu_platform, service_account, enable_display, shielded_instance_config, or network_interface.\\[#d\\].\\(network/subnetwork/subnetwork_project\\) on a started instance requires stopping it. To acknowledge this, please set allow_stopping_for_update = true in your config. You can also stop it by setting desired_status = \"TERMINATED\", but the instance will not be restarted after the update." +const errorAllowStoppingMsg = "Changing the machine_type, min_cpu_platform, service_account, enable_display, shielded_instance_config, scheduling.node_affinities or network_interface.\\[#d\\].\\(network/subnetwork/subnetwork_project\\) on a started instance requires stopping it. To acknowledge this, please set allow_stopping_for_update = true in your config. You can also stop it by setting desired_status = \"TERMINATED\", but the instance will not be restarted after the update." func TestAccComputeInstance_updateRunning_desiredStatusNotSet_notAllowStoppingForUpdate(t *testing.T) { t.Parallel() @@ -4544,6 +4552,53 @@ resource "google_compute_instance" "foobar" { `, instance) } +func testAccComputeInstance_withoutNodeAffinities(instance, nodeTemplate, nodeGroup string) string { + return fmt.Sprintf(` +data "google_compute_image" "my_image" { + family = "debian-9" + project = "debian-cloud" +} + +resource "google_compute_instance" "foobar" { + name = "%s" + machine_type = "n1-standard-8" // can't be e2 because of sole tenancy + zone = "us-central1-a" + allow_stopping_for_update = true + + boot_disk { + initialize_params { + image = data.google_compute_image.my_image.self_link + } + } + + network_interface { + network = "default" + } +} + +resource "google_compute_node_template" "nodetmpl" { + name = "%s" + region = "us-central1" + + node_affinity_labels = { + tfacc = "test" + } + + node_type = "n1-node-96-624" + + cpu_overcommit_type = "ENABLED" +} + +resource "google_compute_node_group" "nodes" { + name = "%s" + zone = "us-central1-a" + + size = 1 + node_template = google_compute_node_template.nodetmpl.self_link +} +`, instance, nodeTemplate, nodeGroup) +} + func testAccComputeInstance_soleTenantNodeAffinities(instance, nodeTemplate, nodeGroup string) string { return fmt.Sprintf(` data "google_compute_image" "my_image" { @@ -4555,6 +4610,7 @@ resource "google_compute_instance" "foobar" { name = "%s" machine_type = "n1-standard-8" // can't be e2 because of sole tenancy zone = "us-central1-a" + allow_stopping_for_update = true boot_disk { initialize_params { @@ -4623,6 +4679,7 @@ resource "google_compute_instance" "foobar" { name = "%s" machine_type = "n1-standard-8" // can't be e2 because of sole tenancy zone = "us-central1-a" + allow_stopping_for_update = true boot_disk { initialize_params { @@ -4680,6 +4737,69 @@ resource "google_compute_node_group" "nodes" { `, instance, nodeTemplate, nodeGroup) } +func testAccComputeInstance_soleTenantNodeAffinitiesReduced(instance, nodeTemplate, nodeGroup string) string { + return fmt.Sprintf(` +data "google_compute_image" "my_image" { + family = "debian-9" + project = "debian-cloud" +} + +resource "google_compute_instance" "foobar" { + name = "%s" + machine_type = "n1-standard-8" // can't be e2 because of sole tenancy + zone = "us-central1-a" + allow_stopping_for_update = true + + boot_disk { + initialize_params { + image = data.google_compute_image.my_image.self_link + } + } + + network_interface { + network = "default" + } + + scheduling { + node_affinities { + key = "tfacc" + operator = "IN" + values = ["test", "updatedlabel"] + } + + node_affinities { + key = "compute.googleapis.com/node-group-name" + operator = "IN" + values = [google_compute_node_group.nodes.name] + } + + min_node_cpus = 6 + } +} + +resource "google_compute_node_template" "nodetmpl" { + name = "%s" + region = "us-central1" + + node_affinity_labels = { + tfacc = "test" + } + + node_type = "n1-node-96-624" + + cpu_overcommit_type = "ENABLED" +} + +resource "google_compute_node_group" "nodes" { + name = "%s" + zone = "us-central1-a" + + size = 1 + node_template = google_compute_node_template.nodetmpl.self_link +} +`, instance, nodeTemplate, nodeGroup) +} + func testAccComputeInstance_shieldedVmConfig(instance string, enableSecureBoot bool, enableVtpm bool, enableIntegrityMonitoring bool) string { return fmt.Sprintf(` data "google_compute_image" "my_image" {