From 55d78bbf2c05427758f695ded0b8b63f3c3bc2d4 Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Wed, 17 Jan 2018 00:54:23 +0000 Subject: [PATCH 1/2] Fix autoscaler - was unnecessarily forcing to a single metric when multiple metrics are supported. --- google/resource_compute_autoscaler.go | 43 +++---- google/resource_compute_autoscaler_test.go | 133 +++++++++++++++------ 2 files changed, 113 insertions(+), 63 deletions(-) diff --git a/google/resource_compute_autoscaler.go b/google/resource_compute_autoscaler.go index f2095f24b12..538e2969eae 100644 --- a/google/resource_compute_autoscaler.go +++ b/google/resource_compute_autoscaler.go @@ -34,6 +34,7 @@ var autoscalingPolicy *schema.Schema = &schema.Schema{ "cpu_utilization": &schema.Schema{ Type: schema.TypeList, Optional: true, + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "target": &schema.Schema{ @@ -69,6 +70,7 @@ var autoscalingPolicy *schema.Schema = &schema.Schema{ "load_balancing_utilization": &schema.Schema{ Type: schema.TypeList, Optional: true, + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "target": &schema.Schema{ @@ -145,6 +147,8 @@ func buildAutoscaler(d *schema.ResourceData) (*compute.Autoscaler, error) { scaler.Description = v.(string) } + // You can only have 0 or 1 autoscaling policy per autoscaler, but HCL can't easily express + // "optional object", so instead we have "list of maximum size 1". prefix := "autoscaling_policy.0." scaler.AutoscalingPolicy = &compute.AutoscalingPolicy{ @@ -153,39 +157,30 @@ func buildAutoscaler(d *schema.ResourceData) (*compute.Autoscaler, error) { CoolDownPeriodSec: int64(d.Get(prefix + "cooldown_period").(int)), } - // Check that only one autoscaling policy is defined - policyCounter := 0 + // This list is MaxItems = 1 as well - you can only have 0 or 1 cpu utilization target per autoscaler. if _, ok := d.GetOk(prefix + "cpu_utilization"); ok { if d.Get(prefix+"cpu_utilization.0.target").(float64) != 0 { - cpuUtilCount := d.Get(prefix + "cpu_utilization.#").(int) - if cpuUtilCount != 1 { - return nil, fmt.Errorf("The autoscaling_policy must have exactly one cpu_utilization, found %d.", cpuUtilCount) - } - policyCounter++ scaler.AutoscalingPolicy.CpuUtilization = &compute.AutoscalingPolicyCpuUtilization{ UtilizationTarget: d.Get(prefix + "cpu_utilization.0.target").(float64), } } } - if _, ok := d.GetOk("autoscaling_policy.0.metric"); ok { - if d.Get(prefix+"metric.0.name") != "" { - policyCounter++ - metricCount := d.Get(prefix + "metric.#").(int) - if metricCount != 1 { - return nil, fmt.Errorf("The autoscaling_policy must have exactly one metric, found %d.", metricCount) - } - scaler.AutoscalingPolicy.CustomMetricUtilizations = []*compute.AutoscalingPolicyCustomMetricUtilization{ - { - Metric: d.Get(prefix + "metric.0.name").(string), - UtilizationTarget: d.Get(prefix + "metric.0.target").(float64), - UtilizationTargetType: d.Get(prefix + "metric.0.type").(string), - }, + var customMetrics []*compute.AutoscalingPolicyCustomMetricUtilization + if metricCount, ok := d.GetOk(prefix + "metric.#"); ok { + for m := 0; m < metricCount.(int); m++ { + if d.Get(fmt.Sprintf("%smetric.%d.name", prefix, m)) != "" { + customMetrics = append(customMetrics, &compute.AutoscalingPolicyCustomMetricUtilization{ + Metric: d.Get(fmt.Sprintf("%smetric.%d.name", prefix, m)).(string), + UtilizationTarget: d.Get(fmt.Sprintf("%smetric.%d.target", prefix, m)).(float64), + UtilizationTargetType: d.Get(fmt.Sprintf("%smetric.%d.type", prefix, m)).(string), + }) } } } + fmt.Printf("customMetrics: %s\n", customMetrics) + scaler.AutoscalingPolicy.CustomMetricUtilizations = customMetrics if _, ok := d.GetOk("autoscaling_policy.0.load_balancing_utilization"); ok { if d.Get(prefix+"load_balancing_utilization.0.target").(float64) != 0 { - policyCounter++ lbuCount := d.Get(prefix + "load_balancing_utilization.#").(int) if lbuCount != 1 { return nil, fmt.Errorf("The autoscaling_policy must have exactly one load_balancing_utilization, found %d.", lbuCount) @@ -196,10 +191,6 @@ func buildAutoscaler(d *schema.ResourceData) (*compute.Autoscaler, error) { } } - if policyCounter != 1 { - return nil, fmt.Errorf("One policy must be defined for an autoscaler.") - } - return scaler, nil } @@ -271,6 +262,8 @@ func flattenAutoscalingPolicy(policy *compute.AutoscalingPolicy) []map[string]in for _, customMetricUtilization := range policy.CustomMetricUtilizations { metricUtil := make(map[string]interface{}) metricUtil["target"] = customMetricUtilization.UtilizationTarget + metricUtil["name"] = customMetricUtilization.Metric + metricUtil["type"] = customMetricUtilization.UtilizationTargetType metricUtils = append(metricUtils, metricUtil) } policyMap["metric"] = metricUtils diff --git a/google/resource_compute_autoscaler_test.go b/google/resource_compute_autoscaler_test.go index 99465f5f939..affda1ec287 100644 --- a/google/resource_compute_autoscaler_test.go +++ b/google/resource_compute_autoscaler_test.go @@ -77,6 +77,32 @@ func TestAccComputeAutoscaler_update(t *testing.T) { }) } +func TestAccComputeAutoscaler_multicondition(t *testing.T) { + t.Parallel() + + var it_name = fmt.Sprintf("autoscaler-test-%s", acctest.RandString(10)) + var tp_name = fmt.Sprintf("autoscaler-test-%s", acctest.RandString(10)) + var igm_name = fmt.Sprintf("autoscaler-test-%s", acctest.RandString(10)) + var autoscaler_name = fmt.Sprintf("autoscaler-test-%s", acctest.RandString(10)) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeAutoscalerDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccComputeAutoscaler_multicondition(it_name, tp_name, igm_name, autoscaler_name), + Check: testAccCheckComputeAutoscalerMultifunction("google_compute_autoscaler.foobar"), + }, + resource.TestStep{ + ResourceName: "google_compute_autoscaler.foobar", + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + func testAccCheckComputeAutoscalerDestroy(s *terraform.State) error { config := testAccProvider.Meta().(*Config) @@ -124,6 +150,39 @@ func testAccCheckComputeAutoscalerExists(n string, ascaler *compute.Autoscaler) } } +func testAccCheckComputeAutoscalerMultifunction(n string) resource.TestCheckFunc { + return func(s *terraform.State) error { + rs, ok := s.RootModule().Resources[n] + if !ok { + return fmt.Errorf("Not found: %s", n) + } + + if rs.Primary.ID == "" { + return fmt.Errorf("No ID is set") + } + + config := testAccProvider.Meta().(*Config) + + found, err := config.clientCompute.Autoscalers.Get( + config.Project, rs.Primary.Attributes["zone"], rs.Primary.ID).Do() + if err != nil { + return err + } + + if found.Name != rs.Primary.ID { + return fmt.Errorf("Autoscaler not found") + } + + if found.AutoscalingPolicy.CpuUtilization.UtilizationTarget == 0.5 && found.AutoscalingPolicy.LoadBalancingUtilization.UtilizationTarget == 0.5 { + return nil + } + return fmt.Errorf("Util target for CPU: %d, for LB: %d - should have been 0.5 for each.", + found.AutoscalingPolicy.CpuUtilization.UtilizationTarget, + found.AutoscalingPolicy.LoadBalancingUtilization.UtilizationTarget) + + } +} + func testAccCheckComputeAutoscalerUpdated(n string, max int64) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -151,7 +210,7 @@ func testAccCheckComputeAutoscalerUpdated(n string, max int64) resource.TestChec } } -func testAccComputeAutoscaler_basic(it_name, tp_name, igm_name, autoscaler_name string) string { +func testAccComputeAutoscaler_scaffolding(it_name, tp_name, igm_name string) string { return fmt.Sprintf(` resource "google_compute_instance_template" "foobar" { name = "%s" @@ -192,7 +251,12 @@ resource "google_compute_instance_group_manager" "foobar" { base_instance_name = "foobar" zone = "us-central1-a" } +`, it_name, tp_name, igm_name) + +} +func testAccComputeAutoscaler_basic(it_name, tp_name, igm_name, autoscaler_name string) string { + return testAccComputeAutoscaler_scaffolding(it_name, tp_name, igm_name) + fmt.Sprintf(` resource "google_compute_autoscaler" "foobar" { description = "Resource created for Terraform acceptance testing" name = "%s" @@ -208,51 +272,31 @@ resource "google_compute_autoscaler" "foobar" { } } -`, it_name, tp_name, igm_name, autoscaler_name) +`, autoscaler_name) } func testAccComputeAutoscaler_update(it_name, tp_name, igm_name, autoscaler_name string) string { - return fmt.Sprintf(` -resource "google_compute_instance_template" "foobar" { + return testAccComputeAutoscaler_scaffolding(it_name, tp_name, igm_name) + fmt.Sprintf(` +resource "google_compute_autoscaler" "foobar" { + description = "Resource created for Terraform acceptance testing" name = "%s" - machine_type = "n1-standard-1" - can_ip_forward = false - tags = ["foo", "bar"] - - disk { - source_image = "debian-cloud/debian-8-jessie-v20160803" - auto_delete = true - boot = true - } - - network_interface { - network = "default" - } - - metadata { - foo = "bar" - } - - service_account { - scopes = ["userinfo-email", "compute-ro", "storage-ro"] + zone = "us-central1-a" + target = "${google_compute_instance_group_manager.foobar.self_link}" + autoscaling_policy = { + max_replicas = 10 + min_replicas = 1 + cooldown_period = 60 + cpu_utilization = { + target = 0.5 + } } -} -resource "google_compute_target_pool" "foobar" { - description = "Resource created for Terraform acceptance testing" - name = "%s" - session_affinity = "CLIENT_IP_PROTO" } - -resource "google_compute_instance_group_manager" "foobar" { - description = "Terraform test instance group manager" - name = "%s" - instance_template = "${google_compute_instance_template.foobar.self_link}" - target_pools = ["${google_compute_target_pool.foobar.self_link}"] - base_instance_name = "foobar" - zone = "us-central1-a" +`, autoscaler_name) } +func testAccComputeAutoscaler_multicondition(it_name, tp_name, igm_name, autoscaler_name string) string { + return testAccComputeAutoscaler_scaffolding(it_name, tp_name, igm_name) + fmt.Sprintf(` resource "google_compute_autoscaler" "foobar" { description = "Resource created for Terraform acceptance testing" name = "%s" @@ -265,8 +309,21 @@ resource "google_compute_autoscaler" "foobar" { cpu_utilization = { target = 0.5 } + load_balancing_utilization = { + target = 0.5 + } + metric { + name = "compute.googleapis.com/instance/network/received_bytes_count" + target = 75 + type = "GAUGE" + } + metric { + name = "compute.googleapis.com/instance/network/sent_bytes_count" + target = 50 + type = "GAUGE" + } } } -`, it_name, tp_name, igm_name, autoscaler_name) +`, autoscaler_name) } From e075348ed66ee3e16a927637f54807427a903bcf Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Wed, 17 Jan 2018 01:02:51 +0000 Subject: [PATCH 2/2] Remove stray debugging comment - sorry about that --- google/resource_compute_autoscaler.go | 1 - 1 file changed, 1 deletion(-) diff --git a/google/resource_compute_autoscaler.go b/google/resource_compute_autoscaler.go index 538e2969eae..7bee4d917ae 100644 --- a/google/resource_compute_autoscaler.go +++ b/google/resource_compute_autoscaler.go @@ -177,7 +177,6 @@ func buildAutoscaler(d *schema.ResourceData) (*compute.Autoscaler, error) { } } } - fmt.Printf("customMetrics: %s\n", customMetrics) scaler.AutoscalingPolicy.CustomMetricUtilizations = customMetrics if _, ok := d.GetOk("autoscaling_policy.0.load_balancing_utilization"); ok { if d.Get(prefix+"load_balancing_utilization.0.target").(float64) != 0 {