Update Application Auto Scaling to support scaling an Amazon EC2 Spot fleet.

[niclic]

This was released as part of AWS SDK for Go v1.4.6.

I added tests to create aws_appautoscaling_policy and aws_appautoscaling_target resources using a Spot Fleet Request and they pass, without updating the aws-sdk-go vendor dependency. Presumably there are no rules enforced to check the ScalableDimension or ServiceNamespace values.

QUESTION: Should the aws-sdk-go vendor dependency be updated to v1.4.6?

TODO:

• Update documentation
• Clean up redundancies in aws_appautoscaling_target resouce and docs. There are several copy and paste errors in this resource that should be cleaned up.

ALL TestAccAWSAppautoScaling tests currently pass in my test account.

[niclic]

I think this is OK for review.

Some notes:

• There is an alarms attribute on the aws_appautoscaling_policy resource but it is not accounted for in getAwsAppautoscalingPutScalingPolicyInput or the docs for this resource. A separate PR should address this.
• It might be useful to add some validation to ensure correlation between scalable_dimension and service_namespace values. This could be added to getAwsAppautoscalingPutScalingPolicyInput (unless there is a way to reference other attributes in a SchemaValidateFunc). I think this is useful but not essential.
• There is a lot of duplication across test files for related resources, especially configuration. Not sure if there is a built in way of reducing this.

[stack72]

If we are changing these from Optional to required, then we may need to verify that the default value from before ecs:service:DesiredCount is actually correct - otherwise we are breaking backwards compatibility

[niclic]

ecs:service:DesiredCount is still a valid value.

[stack72]

Hi @niclic

thanks for the PR here - there are some alarming changes for backward compatibility purposes - we may need to test this with 0.7.3 and then apply the changes on top and see if the tests still pass

With regards to the SDK update, I will open a PR that will update that - watch out for that tomorrow

Paul

[niclic]

Hi Paul,

RE: changing scalable_dimension and service_namespace from Optional to Required on aws_appautoscaling_policy and aws_appautoscaling_target resources.

ecs:service:DesiredCount is still a valid value for scalable_dimension and ecs is still a valid value for service_namespace, if that's what you are asking in your line comments above.

If this change is too much, the original default values could be restored, although I think this is less than ideal. This only makes sense to maintain some sort of back compatibility; obviously these are not sensible defaults if you intend to scale a spot fleet request. These are service-specific defaults. That was my concern.

With the current service-specific defaults, additional validation would be needed to to ensure that the choice of values for scalable_dimension and service_namespace make sense for spot fleet requests (as well as future services). For example, a default value of ecs:service:DesiredCount for scalable_dimension is incompatible with an explicit value of ec2 for service_namespace. However, with no default values, no additional validation logic would be required.

Ray

[niclic]

I ran a series of test using the test files included below.

I first created the aws_appautoscaling_policy and aws_appautoscaling_target resources using v0.7.3.

I then ran make dev and applied updates to the resources using v0.7.4-dev

When no values for scalable_dimension and service_namespace are supplied, the defaults in v0.7.3 are applied.

When switching to v0.7.4-dev the following expected ERRORS are generated:

vagrant@terraform:/opt/gopath/src/github.com/hashicorp/terraform/niclic$ ../bin/terraform plan
There are warnings and/or errors related to your configuration. Please
fix these before continuing.

Errors:

  * aws_appautoscaling_target.foo: "scalable_dimension": required field is not set
  * aws_appautoscaling_target.foo: "service_namespace": required field is not set
  * aws_appautoscaling_policy.foo: "service_namespace": required field is not set
  * aws_appautoscaling_policy.foo: "scalable_dimension": required field is not set

To proceed, I have to provide explicit values for the now required attributes.

When explicit values for scalable_dimension and service_namespace are supplied, there are no issues modifying resources across versions. The tests associated with these resources supply explicit values, so they pass without any issues.

[niclic]

Test file for above tests. Explicit values are commented out.

resource "aws_ecs_cluster" "foo" {
  name = "${var.name}-ecs-cluster"
}

resource "aws_ecs_task_definition" "foo" {
  family = "${var.name}-ecs-task-definition"
  container_definitions = <<EOF
[
  {
    "name": "simple-app",
    "image": "amazon/amazon-ecs-sample",
    "cpu": 10,
    "memory": 500,
    "essential": true
  }
]
EOF
}

resource "aws_ecs_service" "foo" {
  cluster = "${aws_ecs_cluster.foo.id}"
  desired_count = 1
  name = "${var.name}-ecs-service"
  task_definition = "${aws_ecs_task_definition.foo.arn}"
}

resource "aws_iam_role" "foo" {
  assume_role_policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "Service": "application-autoscaling.amazonaws.com"
      },
      "Action": "sts:AssumeRole"
    }
  ]
}
EOF
}

resource "aws_iam_role_policy_attachment" "foo" {
  policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonEC2ContainerServiceAutoscaleRole"
  role = "${aws_iam_role.foo.name}"
}

resource "aws_appautoscaling_target" "foo" {
  max_capacity = 4
  min_capacity = 1
  resource_id = "service/${aws_ecs_cluster.foo.name}/${aws_ecs_service.foo.name}"
  role_arn = "${aws_iam_role.foo.arn}"
  # scalable_dimension = "ecs:service:DesiredCount"
  # service_namespace = "ecs"
}

resource "aws_appautoscaling_policy" "foo" {
  adjustment_type = "ChangeInCapacity"
  cooldown = 60
  metric_aggregation_type = "Average"
  name = "${var.name}-app-autoscaling-policy"
  resource_id = "service/${aws_ecs_cluster.foo.name}/${aws_ecs_service.foo.name}"
  # scalable_dimension = "ecs:service:DesiredCount"
  # service_namespace = "ecs"

  step_adjustment {
    metric_interval_lower_bound = 0
    scaling_adjustment = 1
  }

  depends_on = ["aws_appautoscaling_target.foo"]
}

[niclic]

@stack72

To reiterate, I recommend removing the current default values, because they no longer make sense for the newly added Amazon EC2 Spot fleet requests service (or any other services that may be added in the future), even though this breaks compatibility with the previous release of these resources.

If the default values stay, then they only make sense for scaling ECS services. Some sort of validation or substitution code will have to be added and maintained to aws_appautoscaling_target and aws_appautoscaling_policy resources.

For example, some validation code like this:

func getAwsAppautoscalingPutScalingPolicyInput(d *schema.ResourceData) (applicationautoscaling.PutScalingPolicyInput, error) {
  // existing code
  var params = applicationautoscaling.PutScalingPolicyInput{
    PolicyName: aws.String(d.Get("name").(string)),
    ResourceId: aws.String(d.Get("resource_id").(string)),
  }

  if v, ok := d.GetOk("service_namespace"); ok {
    params.ServiceNamespace = aws.String(v.(string))
  }

  if v, ok := d.GetOk("scalable_dimension"); ok {
    params.ScalableDimension = aws.String(v.(string))
  }

  // new code
  sp, err := getExpectedScalingParameters(params.ResourceId)
  if err != nil {
    return fmt.Errorf("Error creating AppAutoscaling input", err)
  }

  if params.ScalableDimension != sp.ScalableDimension {
    return fmt.Errorf("scalable_dimension is not valid for this resource type %s", params.ResourceId)
  }

  if params.ServiceNamespace != sp.ServiceNamespace {
    return fmt.Errorf("service_namespace is not valid for this resource type %s", params.ResourceId)
  }

  // ...
}

// new
type ScalingParameters struct {
  ScalableDimension, ServiceNamespace string
}

// new
func getExpectedScalingParameters(resourceId string) (ScalingParameters, error) {
  params := ScalingParameters{}

  i := strings.Index(resourceId, "/")
  if i == -1 {
    return params, fmt.Errorf("Invalid resourceId: %s", resourceId)
  }

  switch service := resourceId[:i]; service {
  case "service":
    params.ScalableDimension = "ecs:service:DesiredCount"
    params.ServiceNamespace = "ecs"
  case "spot-fleet-request":
    params.ScalableDimension = "ec2:spot-fleet-request:TargetCapacity"
    params.ServiceNamespace = "ec2"
  default:
    return params, fmt.Errorf("Unsupported AWS Service: %s", service)
  }

  return params, nil
}

This sort of code can become outdated with each service update. Having said that, I'm sure it's possible to tighten this up (I don't work with the go language).

I still think the best approach is to require the user to enter the appropriate values for scalable_dimension and service_namespace, according to the documentation.

Thoughts?

[niclic]

@stack72

I'd like to resolve the conflicts with this branch and hopefully get it merged in soon. Do you have a preferred approach to doing that (merge commit vs rebase and force push)?

I have made a lot of comments above, but here is the TLDR:

• I suggest removing default values for scalable_dimension and service_namespace from both the aws_appautoscaling_target and aws_appautoscaling_policy resources. These values no longer make sense with the support for new services and the sdk does not provide any default values.
• Existing default values are still valid, but since users may have to update their configuration to provide explicit values, a note should be added to the changelog stating this requirement.

[SmoshySmosh]

+1 Any status on this? I'm waiting on this feature.

[niclic]

To reiterate my last comment, I think this is good to go.

However, since the last update, Application Auto Scaling now supports scaling EMR clusters. This means a new scalable dimension and service namespace combination can be added. This should be a very quick and simple change.

Should this feature be added to this PR? It might be worth adding it here since a rebase is already needed to resolve conflicts in the shared validators files, which would need updating again to support EMR. It would be a shame to wait so long again for this capability to be added.

I await confirmation from @stack72 and anybody else interested in this feature.

I really hope that this can get resolved and merged soon.

Other gaps to be aware of:

• There is no Alarms attribute on aws_appautoscaling_policy, so it is not possible to associate CloudWatch alarms with the scaling policy. I mentioned this in my original PR comments, but didn't feel this should be fixed as part of this PR.

• min_adjustment_magnitude is missing from the aws_appautoscaling_policy resource. This may be a new attribute. Note that there is no separate resource for the StepScalingPolicyConfiguration type. Instead, the associated attributes are part of the aws_appautoscaling_policy resource. This is not necessarily something that has to change (certainly not as part of this PR).

[stack72]

Hi @niclic

Sorry for not getting back to you - I agree, the EMR side of that is outside of the scope of this PR. If possible, please can you rebase this 1 last time and I will give this a final review and get it merged today

Thanks

Paul

[niclic]

@stack72

I have rebased the commits in this PR to resolve the conflicts in validators.go. All checks pass so that should be good.

Can you review and merge soon?

I notice that work has already started on adding EMR support to Application Auto Scaling so this PR should definitely get merged soon to avoid any problems (cf. #11126).

[stack72]

On it :)

[niclic]

Great!

One last point, it is probably a good idea to include a note in the CHANGELOG or Release Notes to indicate that there are no longer any defaults for scalable_dimension and service_namespace values?

[stack72]

Hi @niclic

Thanks for all the work here - the changes now LGTM and the tests are green. You are correct about adding a NOTE to the changelog on this - I will take care of this - thanks for being so patient :)

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAppautoScaling'                                                         ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/02/02 09:29:18 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAppautoScaling -timeout 120m
=== RUN   TestAccAWSAppautoScalingPolicy_basic
--- PASS: TestAccAWSAppautoScalingPolicy_basic (132.74s)
=== RUN   TestAccAWSAppautoScalingPolicy_spotFleetRequest
--- PASS: TestAccAWSAppautoScalingPolicy_spotFleetRequest (107.75s)
=== RUN   TestAccAWSAppautoScalingTarget_basic
--- PASS: TestAccAWSAppautoScalingTarget_basic (118.35s)
=== RUN   TestAccAWSAppautoScalingTarget_spotFleetRequest
--- PASS: TestAccAWSAppautoScalingTarget_spotFleetRequest (74.97s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	433.821s

[tysonmote]

Just for Google-bait in case other people are having the same problem I did with the v0.8.6 patch level release: 0b9fc76 is a backwards-incompatible change that breaks all existing aws_appautoscaling_target resource definitions with a "name" attribute with the following error message:

Errors:

  * aws_appautoscaling_target.main: : invalid or unknown key: name

Just remove the name attribute from your aws_appautoscaling_target resource definition and you'll be back in business.

[niclic]

Thanks for the note @tysonmote

The backwards incompatibilities note in the CHANGELOG should have included the name attribute (and probably the arn attribute as well) for the aws_appautoscaling_target resource. My bad for not being explicit, since it was called out in the PR.

@stack72
Can/should the CHANGELOG for v0.8.6 be updated to include a reference to the aws_appautoscaling_target backwards incompatibilities? In addition to no longer having default values for scalable_dimension and service_namespace, it should mention the name and arn attributes as well.

[niclic]

Just found the Deprecated attribute on the Schema type. Obviously a better approach would have been to initially flag this attribute before removing it in a later release. Lesson learned.

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.