AWS: Scaling Policies / CloudWatch Metric Alarms

[xaptronic]

Hello, This commit adds Autoscaling Scaling Policies and CloudWatch metric alarms as per this guide: http://docs.aws.amazon.com/AutoScaling/latest/DeveloperGuide/policy_creating.html, see the section titled "Scaling with Metrics Using the AWS CLI". In particular I wanted these features in terraform in order to add metric based scaling to my infrastructure, however, I may continue to work on this area and add other features of autoscaling and cloudwatch.

I've added a few elementary TestAcc* cases.

I'm a bit new to this way of working so if you need any other information or if I'm missing some major pieces, by all means, let me know. I am looking for some initial feedback and then will begin to fill out the documentation pieces.

Thanks!

[pmoust]

@xaptronic thanks for this! Trying your work out atm.

[xaptronic]

Great, looking forward to your feedback.

[CpuID]

Nice!

[nicgrayson]

@xaptronic did the commits from my PR on your fork get updated here?

[nicgrayson]

@xaptronic it looks like you'll need to merge master into this branch. I've kept a branch up to date on the Banno org if you want a reference.

[xaptronic]

@nicgrayson Branches are all up to date and your changes are merged in.

[xaptronic]

@catsby I added a section to the documentation page that shows how to use alarm_actions with Scaling Policies. It functions the same way that you can add instances to load balancers. Let me know if there needs to be more explanatory text.

[radeksimko]

Just a few questions/suggestions regarding the DSL:

1. aws_autoscaling_scaling_policy => can't that be aws_autoscaling_policy?
2. policy_arn => arn? or better, why not use ARN as ID instead of name as ID? You'll likely need to reference that more often than name I reckon? (alarm_actions = ["${aws_autoscaling_scaling_policy.bat.policy_arn}"])
3. policy_name => name?
4. cooldown => cooldown_in_seconds?

[jedi4ever]

can't wait to have this merged 🍨

[catsby]

Thanks for adding error checking in 3780afa, much appreciated.
Unfortunately I think we'll see a lot of those errors, because we're still not converting the AlarmActions ([]*string), into the []string that alarm_actions is expecting. We'll still be getting this error:

[WARN] Error setting OK Actions: ok_actions.0: '' expected type 'string', got unconvertible type '*string'

You can address this by converting them:

var actionList []string
for _,action := range a.AlarmActions{
  actionList = append(actionList, *action)
} 
if err := d.Set("alarm_actions", actionList); err !=nil {
  // log
}

That should do the trick

[xaptronic]

@catsby Thanks for the example, I was a bit stumped on this. This is my first foray into writing Go :). I'll update my code and update the PR.

[xaptronic]

Any other comments on suggestions from @radeksimko? I based the naming on documentation from AWS, but these suggestions seem like they would make naming shorter and more clear.

[radeksimko]

@xaptronic Based on recent discussion in here: #1974 (comment) I may take back second part of my 2nd point (why not use ARN as ID instead of name as ID), but otherwise I believe the other parameters should be renamed unless someone can convince me it's a bad idea? 😃

[xaptronic]

@radeksimko I am going to make these changes tomorrow morning unless I hear any reason not to. I like the shorter variations.

@catsby I added a helper function to do the []*string to []string conversion. I ran plan/apply with TF_LOG and do not see that warning statement any longer.

[xaptronic]

@radeksimko I've updated the DSL to reflect your comments about shorter naming. I chose, however, not to rename cooldown to cooldown_in_seconds. Amazons API parameter is called cooldown and I feel there is something dirty about putting the unit into the parameter name. None of the other parameters for this have information about what unit the accept. I would prefer to leave the information about seconds to documentation. Putting _in_seconds reminds me of the old example of making a CSS class called .red - Let me know if you have a counter to this logic.

[radeksimko]

I chose, however, not to rename cooldown to cooldown_in_seconds

That's fair enough. I just like to be explicit sometimes as it saves people time since they don't have to lookup if number of minutes or seconds or even hours is expected as input. That said, I'd love to see this in the AWS API/SDK in the first place.

The motivation is mostly coming from The Clean Code, TL;DR version of relevant chapter here: http://blog.goyello.com/2013/05/17/express-names-in-code-bad-vs-clean/

Nonetheless the consistency with existing DSL is equally important (thanks for pointing it out 👍 ), so I'm totally fine with keeping it.

I can give this a more thorough review this weekend and eventually merge, unless @catsby will be quicker and/or wants to merge this sooner.

---

Looking at your 39 commits in total, it would be nice if you could squash these into 1 or 2 (functionality + docs). I believe that any reviewer will benefit from cleaner log, but if you will struggle doing that, we can deal with it. 😺

I usually use combination of git commit --amend + git push feature-branch --force and/or git rebase HEAD~x -i when changing PRs.

[xaptronic]

Great, sounds good. I'll take a shot at squashing.

[xaptronic]

My attempt at git rebase seems to have failed. Will the fact that I have done various upstream merges make this more complicated?

[radeksimko]

@xaptronic No worries then! Just keep it as it is.
We can take care of it then. 😉

Still better to have something that works than a nice git history. 😃

[radeksimko]

Sorry, I didn't have time to review this during the weekend, I was busy with a few other of my own PRs here. I'm leaving it for someone else to review. Although I'd really like to have scaling policies + CloudWatch metrics in Terraform. 😉

[xaptronic]

All in good time :)

[radeksimko]

Closing in favour of #2201

[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.