Skip to content
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.

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

allow updating dataproc cluster autoscaling_policy #3976

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
27 changes: 19 additions & 8 deletions third_party/terraform/resources/resource_dataproc_cluster.go.erb
Original file line number Diff line number Diff line change
Expand Up @@ -530,17 +530,19 @@ by Dataproc`,
},
},
"autoscaling_config": {
Type: schema.TypeList,
Optional: true,
AtLeastOneOf: clusterConfigKeys,
MaxItems: 1,
Description: `The autoscaling policy config associated with the cluster.`,
Type: schema.TypeList,
Optional: true,
AtLeastOneOf: clusterConfigKeys,
MaxItems: 1,
Description: `The autoscaling policy config associated with the cluster.`,
DiffSuppressFunc: emptyOrUnsetBlockDiffSuppress,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"policy_uri": {
Type: schema.TypeString,
Required: true,
Description: `The autoscaling policy used by the cluster.`,
Type: schema.TypeString,
Required: true,
Description: `The autoscaling policy used by the cluster.`,
DiffSuppressFunc: locationDiffSuppress,
},
},
},
Expand Down Expand Up @@ -1206,6 +1208,15 @@ func resourceDataprocClusterUpdate(d *schema.ResourceData, meta interface{}) err
updMask = append(updMask, "config.secondary_worker_config.num_instances")
}

if d.HasChange("cluster_config.0.autoscaling_config") {
desiredPolicy := d.Get("cluster_config.0.autoscaling_config.0.policy_uri").(string)
cluster.Config.AutoscalingConfig = &dataproc.AutoscalingConfig{
PolicyUri: desiredPolicy,
}

updMask = append(updMask, "config.autoscaling_config.policy_uri")
}

<% unless version == 'ga' -%>
if d.HasChange("cluster_config.0.lifecycle_config.0.idle_delete_ttl") {
idleDeleteTtl := d.Get("cluster_config.0.lifecycle_config.0.idle_delete_ttl").(string)
Expand Down
84 changes: 84 additions & 0 deletions third_party/terraform/tests/resource_dataproc_cluster_test.go.erb
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,26 @@ func TestAccDataprocCluster_withKerberos(t *testing.T) {
})
}

func TestAccDataprocCluster_withAutoscalingPolicy(t *testing.T) {
t.Parallel()

rnd := randString(t, 10)

vcrTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckDataprocClusterDestroy(t),
Steps: []resource.TestStep{
{
Config: testAccDataprocCluster_withAutoscalingPolicy(rnd),
},
{
Config: testAccDataprocCluster_removeAutoscalingPolicy(rnd),
},
Copy link
Member

Choose a reason for hiding this comment

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

I suppose we don't need a check here because we are just making sure a diff doesn't exist after applying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we would have an import step, but we don't actually support import yet for dataproc clusters. I suppose it probably makes sense to check the object though to make sure removing the autoscaling policy actually removes it (vs just removing it from state). Will do shortly.

},
})
}

func testAccCheckDataprocClusterDestroy(t *testing.T) resource.TestCheckFunc {
return func(s *terraform.State) error {
config := googleProviderConfig(t)
Expand Down Expand Up @@ -1540,3 +1560,67 @@ resource "google_dataproc_cluster" "kerb" {
}
`, rnd, rnd, rnd, kmsKey)
}

func testAccDataprocCluster_withAutoscalingPolicy(rnd string) string {
return fmt.Sprintf(`
resource "google_dataproc_cluster" "basic" {
name = "tf-test-dataproc-policy-%s"
region = "us-central1"

cluster_config {
autoscaling_config {
policy_uri = google_dataproc_autoscaling_policy.asp.id
}
}
}

resource "google_dataproc_autoscaling_policy" "asp" {
policy_id = "tf-test-dataproc-policy-%s"
location = "us-central1"

worker_config {
max_instances = 3
}

basic_algorithm {
yarn_config {
graceful_decommission_timeout = "30s"
scale_up_factor = 0.5
scale_down_factor = 0.5
}
}
}
`, rnd, rnd)
}

func testAccDataprocCluster_removeAutoscalingPolicy(rnd string) string {
return fmt.Sprintf(`
resource "google_dataproc_cluster" "basic" {
name = "tf-test-dataproc-policy-%s"
region = "us-central1"

cluster_config {
autoscaling_config {
policy_uri = ""
}
}
}

resource "google_dataproc_autoscaling_policy" "asp" {
policy_id = "tf-test-dataproc-policy-%s"
location = "us-central1"

worker_config {
max_instances = 3
}

basic_algorithm {
yarn_config {
graceful_decommission_timeout = "30s"
scale_up_factor = 0.5
scale_down_factor = 0.5
}
}
}
`, rnd, rnd)
}
36 changes: 36 additions & 0 deletions third_party/terraform/utils/common_diff_suppress.go.erb
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,42 @@ func rfc3339TimeDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
return false
}

// Suppress diffs for blocks where one version is completely unset and the other is set
// to an empty block. This might occur in situations where removing a block completely
// is impossible (if it's computed or part of an AtLeastOneOf), so instead the user sets
// its values to empty.
func emptyOrUnsetBlockDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
o, n := d.GetChange(strings.TrimSuffix(k, ".#"))
var l []interface{}
if old == "0" && new == "1" {
l = n.([]interface{})
} else if new == "0" && old == "1" {
l = o.([]interface{})
} else {
// we don't have one set and one unset, so don't suppress the diff
return false
}

contents := l[0].(map[string]interface{})
for _, v := range contents {
if !isEmptyValue(reflect.ValueOf(v)) {
return false
}
}
return true
}

// Suppress diffs for values that are equivalent except for their use of the words "location"
// compared to "region" or "zone"
func locationDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
return locationDiffSuppressHelper(old, new) || locationDiffSuppressHelper(new, old)
}

func locationDiffSuppressHelper(a, b string) bool {
return strings.Replace(a, "/locations/", "/regions/", 1) == b ||
strings.Replace(a, "/locations/", "/zones/", 1) == b
}

<% unless version == 'ga' -%>
// For managed SSL certs, if new is an absolute FQDN (trailing '.') but old isn't, treat them as equals.
func absoluteDomainSuppress(k, old, new string, _ *schema.ResourceData) bool {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@ The `cluster_config` block supports:
* `security_config` (Optional) Security related configuration. Structure defined below.

* `autoscaling_config` (Optional) The autoscaling policy config associated with the cluster.
Note that once set, if `autoscaling_config` is the only field set in `cluster_config`, it can
only be removed by setting `policy_uri = ""`, rather than removing the whole block.
Structure defined below.

* `initialization_action` (Optional) Commands to execute on each node after config is completed.
Expand Down