resource/aws_emr_cluster: Add scale_down_behavior #3063

csghuser · 2018-01-19T10:23:01Z

This provides the ability to configure the scale down behavior of a cluster. From the docs:

ScaleDownBehavior
The way that individual Amazon EC2 instances terminate when an automatic scale-in activity occurs or an instance group is resized. TERMINATE_AT_INSTANCE_HOUR indicates that Amazon EMR terminates nodes at the instance-hour boundary, regardless of when the request to terminate the instance was submitted. This option is only available with Amazon EMR 5.1.0 and later and is the default for clusters created using that version. TERMINATE_AT_TASK_COMPLETION indicates that Amazon EMR blacklists and drains tasks from nodes before terminating the Amazon EC2 instances, regardless of the instance-hour boundary. With either behavior, Amazon EMR removes the least active nodes first and blocks instance termination if it could lead to HDFS corruption. TERMINATE_AT_TASK_COMPLETION available only in Amazon EMR version 4.1.0 and later, and is the default for versions of Amazon EMR earlier than 5.1.0.

Type: String

Valid Values: TERMINATE_AT_INSTANCE_HOUR | TERMINATE_AT_TASK_COMPLETION

Required: No

Source: https://docs.aws.amazon.com/emr/latest/APIReference/API_JobFlowDetail.html

I've added a test which attempts to set the value to TERMINATE_AT_INSTANCE_HOUR for a cluster version of 5.5.0. This is the default for this version, but I didn't want to interfere with the tests as it may prevent the cluster from shutting down should any tasks be running.

I've not yet been able to run this against AWS to test.

csghuser · 2018-01-23T10:10:28Z

I've been able to test this. The tests are failing however. If scale_down_behavior is not set (as it is optional), after cluster creation, when reading the state, it wants to set this value back to an empty string.

 scale_down_behavior: "TERMINATE_AT_TASK_COMPLETION" => "" (forces new resource)

So whilst we are not setting this, it gets defined at cluster creation by AWS. I'm just looking through the code to see how this can be avoided.

csghuser · 2018-01-23T12:47:36Z

I've added some logic to check whether or not the user has defined this new variable. If not then we rely on the default for the EMR version. This seems like a better approach, as opposed to hard coding the following logic into the code:

if release_label >=  5.1.0 
  default = TERMINATE_AT_INSTANCE_HOUR
else if (release_label >= 4.1.0 AND release_label < 5.1.0)
  default = TERMINATE_AT_TASK_COMPLETION
end

As its possible this may change at some point and a new default would be introduced with a later version.

bflad

Hi @csghuser thanks for contributing this! Check out my comments below and let me know if you have any questions or don't have time to finish this up. 😄

We should also add a new acceptance test to cover this new attribute. At least something like on the _basic test:

resource.TestCheckResourceAttrSet("aws_emr_cluster.tf-test-cluster", "scale_down_behavior")

And more preferably a new test that actually tries setting it and confirming its correct.

bflad · 2018-02-15T21:50:13Z

aws/resource_aws_emr_cluster.go

@@ -475,6 +485,11 @@ func resourceAwsEMRClusterRead(d *schema.ResourceData, meta interface{}) error {
 	}

 	d.Set("name", cluster.Name)
+
+	if d.Get("scale_down_behavior").(string) != "" {


You can remove this check by setting Computed: true on the attribute 😄

bflad · 2018-02-15T21:53:18Z

aws/resource_aws_emr_cluster.go

@@ -225,6 +225,11 @@ func resourceAwsEMRCluster() *schema.Resource {
 				ForceNew: true,
 				Required: true,
 			},
+			"scale_down_behavior": {


Can you add a validation function for this please? e.g.
ValidateFunc: validation.StringInSlice([]string{emr.ScaleDownBehaviorTerminateAtInstanceHour, emr.ScaleDownBehaviorTerminateAtTaskCompletion}, false)

csghuser · 2018-02-16T11:28:00Z

@bflad Many thanks for your pointers, very much appreciated. I've addressed each one and reran the basic test, which seems to pass fine. It also passes if no scale_down_behavior config option is set now that the "computed" attribute is set, as you suggested 😃

Would it be possible to check if everything looks ok?

bflad

Two little things then we can get this in, thanks so much!

bflad · 2018-02-16T17:14:55Z

aws/resource_aws_emr_cluster.go

@@ -486,6 +502,7 @@ func resourceAwsEMRClusterRead(d *schema.ResourceData, meta interface{}) error {
 	}

 	d.Set("name", cluster.Name)
+


We do still want the d.Set("scale_down_behavior", cluster.ScaleDownBehavior) here so Terraform is always updating its state correctly from the API each read as well as on import. We just didn't need the wrapping if d.Get() check that was previously in the PR. Sorry I wasn't more clear about what I meant earlier! 😄

bflad · 2018-02-16T17:17:11Z

aws/resource_aws_emr_cluster_test.go

-				Check:  testAccCheckAWSEmrClusterExists("aws_emr_cluster.tf-test-cluster", &cluster),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSEmrClusterExists("aws_emr_cluster.tf-test-cluster", &cluster),
+					resource.TestCheckResourceAttrSet("aws_emr_cluster.tf-test-cluster", "scale_down_behavior"),


Since we know the value (hardcoded in the config below), lets actually check it. 😄

resource.TestCheckResourceAttr("aws_emr_cluster.tf-test-cluster", "scale_down_behavior", "TERMINATE_AT_TASK_COMPLETION"),

…means the AWS default for the release_label will be used, unless the user wants to override this

…idation function, added test to basic cluster

…ale_down_behavior is set.

csghuser · 2018-02-17T10:05:47Z

@bflad I've made the modifications as requested 😃 I've reran the tests, seems to run ok.

bflad

This looks great! Thanks so much! 🚀

=== RUN   TestAccAWSEMRCluster_security_config
--- PASS: TestAccAWSEMRCluster_security_config (403.06s)
=== RUN   TestAccAWSEMRCluster_terminationProtected
--- PASS: TestAccAWSEMRCluster_terminationProtected (432.92s)
=== RUN   TestAccAWSEMRCluster_basic
--- PASS: TestAccAWSEMRCluster_basic (509.79s)
=== RUN   TestAccAWSEMRCluster_bootstrap_ordering
--- PASS: TestAccAWSEMRCluster_bootstrap_ordering (525.05s)
=== RUN   TestAccAWSEMRCluster_instance_group
--- PASS: TestAccAWSEMRCluster_instance_group (537.89s)
=== RUN   TestAccAWSEMRCluster_custom_ami_id
--- PASS: TestAccAWSEMRCluster_custom_ami_id (579.84s)
=== RUN   TestAccAWSEMRCluster_s3Logging
--- PASS: TestAccAWSEMRCluster_s3Logging (639.13s)
=== RUN   TestAccAWSEMRCluster_visibleToAllUsers
--- PASS: TestAccAWSEMRCluster_visibleToAllUsers (978.19s)
=== RUN   TestAccAWSEMRCluster_root_volume_size
--- PASS: TestAccAWSEMRCluster_root_volume_size (1022.69s)
=== RUN   TestAccAWSEMRCluster_tags
--- PASS: TestAccAWSEMRCluster_tags (1076.23s)

bflad · 2018-02-27T12:53:18Z

This has been released in version 1.10.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

csghuser · 2018-02-27T15:31:11Z

@bflad Thanks! 😄

ghost · 2020-04-07T17:47:24Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

radeksimko changed the title ~~Adding scale_down_behavior~~ resource/aws_emr_cluster: Add scale_down_behavior Jan 19, 2018

radeksimko added enhancement Requests to existing resources that expand the functionality or scope. service/emr Issues and PRs that pertain to the emr service. labels Jan 19, 2018

ghost added the size/XXL Managed by automation to categorize the size of a PR. label Feb 5, 2018

csghuser force-pushed the scale_down_behavior branch from cc4244f to f4435b2 Compare February 5, 2018 10:56

ghost added size/M Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Feb 5, 2018

csghuser force-pushed the scale_down_behavior branch from f4435b2 to d0a8bc9 Compare February 5, 2018 11:09

ghost added size/S Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Feb 5, 2018

bflad reviewed Feb 15, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 15, 2018

csghuser force-pushed the scale_down_behavior branch from d0a8bc9 to 48ac8b3 Compare February 16, 2018 09:51

ghost added the size/S Managed by automation to categorize the size of a PR. label Feb 16, 2018

bflad reviewed Feb 16, 2018

View reviewed changes

csghuser and others added 4 commits February 17, 2018 08:49

Adding scale_down_behavior

a41a681

If user has not defined scale_down_behavior, use current value. This …

f4501a2

…means the AWS default for the release_label will be used, unless the user wants to override this

Added compute to attribute, removed check for empty string, added val…

cac0880

…idation function, added test to basic cluster

Ensure Terraform checks the current state each time. Check correct sc…

dac38a9

…ale_down_behavior is set.

csghuser force-pushed the scale_down_behavior branch from b752894 to dac38a9 Compare February 17, 2018 10:01

ghost added the size/S Managed by automation to categorize the size of a PR. label Feb 17, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 19, 2018

bflad approved these changes Feb 19, 2018

View reviewed changes

bflad added this to the v1.10.0 milestone Feb 19, 2018

bflad merged commit 973c4ad into hashicorp:master Feb 19, 2018

bflad added a commit that referenced this pull request Feb 19, 2018

Update CHANGELOG for #3063

f9b0dfa

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_emr_cluster: Add scale_down_behavior #3063

resource/aws_emr_cluster: Add scale_down_behavior #3063

csghuser commented Jan 19, 2018 •

edited

Loading

csghuser commented Jan 23, 2018

csghuser commented Jan 23, 2018

bflad left a comment

bflad Feb 15, 2018

bflad Feb 15, 2018

csghuser commented Feb 16, 2018

bflad left a comment

bflad Feb 16, 2018

bflad Feb 16, 2018

csghuser commented Feb 17, 2018

bflad left a comment

bflad commented Feb 27, 2018

csghuser commented Feb 27, 2018

ghost commented Apr 7, 2020

		@@ -486,6 +502,7 @@ func resourceAwsEMRClusterRead(d *schema.ResourceData, meta interface{}) error {
		}

		d.Set("name", cluster.Name)

resource/aws_emr_cluster: Add scale_down_behavior #3063

resource/aws_emr_cluster: Add scale_down_behavior #3063

Conversation

csghuser commented Jan 19, 2018 • edited Loading

csghuser commented Jan 23, 2018

csghuser commented Jan 23, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Feb 15, 2018

Choose a reason for hiding this comment

bflad Feb 15, 2018

Choose a reason for hiding this comment

csghuser commented Feb 16, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Feb 16, 2018

Choose a reason for hiding this comment

bflad Feb 16, 2018

Choose a reason for hiding this comment

csghuser commented Feb 17, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Feb 27, 2018

csghuser commented Feb 27, 2018

ghost commented Apr 7, 2020

csghuser commented Jan 19, 2018 •

edited

Loading