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

AWS Application AutoScaling #7663

Closed
wants to merge 5 commits into from
Closed

Conversation

andskli
Copy link
Contributor

@andskli andskli commented Jul 15, 2016

Initial work on two new resource types in order to support AWS Application AutoScaling used by for example EC2 Container Service:

  • aws_appautoscaling_target
  • aws_appautoscaling_policy

In a sense, a lot of the code are similar or even the same as for the aws_autoscaling_* resources.

Example:

resource "aws_appautoscaling_target" "tgt" {
  service_namespace = "ecs"
  resource_id = "service/testcluster/testECSservice"
  scalable_dimension = "ecs:service:DesiredCount"
  role_arn = "arn:aws:iam::AWSAccountID:role/ecsAutoscaleRole"
  min_capacity = 1
  max_capacity = 4
}

resource "aws_appautoscaling_policy" "up" {
  name = "scale-up"
  service_namespace = "ecs"
  resource_id = "service/testcluster/testECSservice"
  scalable_dimension = "ecs:service:DesiredCount"
  adjustment_type = "ChangeInCapacity"
  cooldown = 60
  metric_aggregation_type = "Maximum"

  step_adjustment {
    metric_interval_lower_bound = 0
    scaling_adjustment = 1
  }

  depends_on = ["aws_appautoscaling_target.tgt"]
}

resource "aws_appautoscaling_policy" "down" {
  name = "scale-down"
  service_namespace = "ecs"
  resource_id = "service/testcluster/testECSservice"
  scalable_dimension = "ecs:service:DesiredCount"

  adjustment_type = "ChangeInCapacity"
  cooldown = 60
  metric_aggregation_type = "Maximum"

  step_adjustment {
    metric_interval_lower_bound = 0
    scaling_adjustment = -1
  }

  depends_on = ["aws_appautoscaling_target.tgt"]
}

resource "aws_cloudwatch_metric_alarm" "service_cpu_high" {
  alarm_name = "testhest-cpuutilization-high"
  comparison_operator = "GreaterThanOrEqualToThreshold"
  evaluation_periods = "2"
  metric_name = "CPUUtilization"
  namespace = "AWS/ECS"
  period = "60"
  statistic = "Maximum"
  threshold = "85"

  dimensions {
    ClusterName = "testcluster"
    ServiceName = "testECSservice"
  }

  alarm_actions = ["${aws_appautoscaling_policy.up.arn}"]
}

I've also included the applicationautoscaling part of the AWS SDK for Go using govendor as described in the contribution docs.

This is my first resource implementation for Terraform and I'm a novice in Go, so all feedback is welcome.

@andskli
Copy link
Contributor Author

andskli commented Jul 15, 2016

Just added a commit that bumps the aws-sdk-go version to v1.2.5.. I'm not sure how that's supposed to be handled, but as I state in the commit message, a version upgrade is necessary to support Application AutoScaling.

I upgraded all of the services and subsets of aws-sdk-go since a change in regards to where the V4 signer resides have been introduced, which many (if not all) service modules seems to depend on.

Maybe the aws-sdk-go update should be a separate PR?

@stack72
Copy link
Contributor

stack72 commented Jul 15, 2016

Hi @andskli

I actually merged a PR that bumps the aws-sdk-go to 1.2.5 today :)

P.

@andskli
Copy link
Contributor Author

andskli commented Jul 16, 2016

Great! I've removed my commit doing that exact same thing and added a commit that adds just the applicationautoscaling part of the aws-sdk-go.
Acceptance tests works at my end (requires $AWS_ACCOUNT_ID to be set though).

The docs should be updated as well, anything else that I can or should do? :)

@stack72
Copy link
Contributor

stack72 commented Jul 21, 2016

Hi @andskli

Please can you have a look at rebasing this? Then we can start reviewing it

Paul

@stack72 stack72 added the waiting-response An issue/pull request is waiting for a response from the community label Jul 21, 2016
@stack72 stack72 self-assigned this Jul 21, 2016
Initial work on two new resource types:
* `aws_appautoscaling_target`
* `aws_appautoscaling_policy`
using: `govendor add
github.com/aws/aws-sdk-go/service/[email protected]`
@andskli
Copy link
Contributor Author

andskli commented Jul 22, 2016

@stack72 done!

@stack72
Copy link
Contributor

stack72 commented Jul 22, 2016

Hi @andskli

Thanks so much for the work here. The code looks good but the tests don't pass I'm afraid

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAppautoscalingPolicy_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAppautoscalingPolicy_ -timeout 120m
=== RUN   TestAccAWSAppautoscalingPolicy_basic
--- FAIL: TestAccAWSAppautoscalingPolicy_basic (112.53s)
    testing.go:264: Step 0 error: Error applying: 1 error(s) occurred:

        * aws_appautoscaling_target.tgt: Error creating application autoscaling target: ValidationException: Unable to assume IAM role: arn:aws:iam::xxxxxxx:role/ecsAutoscaleRole
            status code: 400, request id: 0c7dfe7b-4ffe-11e6-b1dc-798d7fb58f3e
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    112.550s
make: *** [testacc] Error 1

Can we make the ecsAutoscaleRole part of the test? Once we get this taken care of, this is ready for merge

Thanks

Paul

@stack72
Copy link
Contributor

stack72 commented Jul 25, 2016

Ping @andskli :)

Due to possible inconsistencies in IAM, let's retry creation of the scalable target before we fail.
@andskli
Copy link
Contributor Author

andskli commented Jul 25, 2016

@stack72 sure, that makes sense. I think the fixes I just pushed takes care of what you're looking for.

I totally missed that the ecsAutoscaleRole is created only after fiddling around with AppAutoscaling + ECS in the GUI.

This also introduces a retry behaviour to creation of the scalable target, to avoid inconsistencies in IAM.

@stack72
Copy link
Contributor

stack72 commented Jul 26, 2016

Hi @andskli

Just a FYI, I made a few tiny changes to the tests (76aea01) as i was getting a panic.

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAppautoScaling'                             ✹
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAppautoScaling -timeout 120m
=== RUN   TestAccAWSAppautoScalingPolicy_basic
--- PASS: TestAccAWSAppautoScalingPolicy_basic (124.41s)
=== RUN   TestAccAWSAppautoScalingTarget_basic
--- PASS: TestAccAWSAppautoScalingTarget_basic (112.84s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    237.270s

I just rebased and merged this :) Thanks for all the work here!

Paul

@stack72 stack72 closed this Jul 26, 2016
@andskli
Copy link
Contributor Author

andskli commented Jul 26, 2016

@stack72 thx! Happy to contribute.

@ghost
Copy link

ghost commented Apr 24, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement provider/aws waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants