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

[WORKFLOWS-44] Add Managed Scaling #53

Merged
merged 1 commit into from
Sep 2, 2021
Merged

[WORKFLOWS-44] Add Managed Scaling #53

merged 1 commit into from
Sep 2, 2021

Conversation

tthyer
Copy link
Contributor

@tthyer tthyer commented Sep 2, 2021

Adds a CapacityProvider to implement managed scaling. This allows the cluster to add an instance which will accommodate a new, updated task before spinning down the old one. It takes 5-6 minutes for this turnover to happen and then another ~15 minutes to shut down the second cluster instance. Possibly we could shorten the latter by adjusting TargetCapacity -- but this seems to be working nicely now.

@tthyer tthyer marked this pull request as ready for review September 2, 2021 01:09
@tthyer tthyer requested a review from a team as a code owner September 2, 2021 01:09
@tthyer tthyer requested a review from a team September 2, 2021 01:09
Copy link
Contributor

@BrunoGrandePhD BrunoGrandePhD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an ECS expert, but I don't see anything wrong here. 🚀

Copy link
Contributor

@zaro0508 zaro0508 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good in general. can approve as-is but just had a few questions.

Comment on lines +162 to +164
TerminationPolicies:
- OldestLaunchConfiguration
- OldestInstance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason why not just let AWS manage this with Default setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I want to be explicit about how this is going to behave so that anyone supporting it knows what to expect. It's also a little different from the default behavior. You can look that up.

Comment on lines +84 to +86
MinimumScalingStepSize: 1
MaximumScalingStepSize: 1
TargetCapacity: 90
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs seem to indicate that if status is set to enabled then AWS auto scales for you. I think that means that it will just ignore these scaling params? or am i reading it wrong?..

The ManagedScaling property specifies the settings for the Auto
Scaling group capacity provider.

When managed scaling is enabled, Amazon ECS manages the scale-in
and scale-out actions of the Auto Scaling group. Amazon ECS manages a 
target tracking scaling policy using an Amazon ECS-managed CloudWatch 
metric with the specified targetCapacity value as the target value for the 
metric. For more information, see Using Managed Scaling in the Amazon 
Elastic Container Service Developer Guide.

If managed scaling is disabled, the user must manage the scaling of the 
Auto Scaling group. 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zaro0508 Good question. It's the opposite of how you're reading it; in my experience with clusters there's always some metric or metrics used as the measure so that it knows when to autoscale. Now, TargetCapacity is not a required value so I assume there's some default value they're using, but they don't specify what it is. I went with 90 because the aws-samples code I was looking at used that value; it works well enough so I left it at that.

Look at this set of instructions for creating the capacity provider in the console -- there's more explanation at #9. You can also read the deep dive blog post.

@tthyer tthyer merged commit c1a5ca1 into main Sep 2, 2021
@tthyer tthyer deleted the WORKFLOWS-44 branch September 2, 2021 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants