resource/aws_emr_cluster: Add scale_down_behavior

[csghuser]

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]

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]

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]

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

[bflad]

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

[csghuser]

@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]

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]

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"),

[csghuser]

@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]

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]

@bflad Thanks! 😄

[ghost]

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!