Skip to content

docs: Fix description of how autoscaling min and max are set in services #137

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

docs: Fix description of how autoscaling min and max are set in services #137

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55c7147
Update docs to accurately reflect how the autoscaling definition is set
new-guy Nov 16, 2023
c57534d
More accurate stuff
new-guy Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion modules/service/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
Configuration in this directory creates an Amazon ECS Service and associated resources.

Some notable configurations to be aware of when using this module:
1. `desired_count`/`scale` is always ignored; the module is designed to utilize autoscaling by default (though it can be disabled)
1. `desired_count`/`scale` are not the intended way of setting the number of tasks to run. Instead, use `autoscaling_min_capacity`/`autoscaling_max_capacity` to set the number of tasks to run.
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct - what issue are you running into?

Copy link
Author

Choose a reason for hiding this comment

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

It could be that I'm doing something wrong with the way I'm configuring things, but it is unfortunately not true that desired_count is always ignored. See here: https://github.com/terraform-aws-modules/terraform-aws-ecs/blob/master/modules/service/main.tf#L1199-L1200

The min and max autoscaling targets are set to the min and max of their respective variables vs desired_count. If you set the autoscaling_min_capacity variable to 2, and desired_count remains at 1, then the value of autoscaling_min_capacity in the aws_appautoscaling_target resource will be set to 1.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe instead the autoscaling_min_capacity attribute in aws_appautoscaling_target should be the max of desired_count and var.autoscaling_min_capacity?

Copy link
Member

Choose a reason for hiding this comment

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

If you set the autoscaling_min_capacity variable to 2, and desired_count remains at 1, then the value of autoscaling_min_capacity in the aws_appautoscaling_target resource will be set to 1.

Conversely, if you set this on create, it will fail. While I agree its not a perfect scenario, there are always rough edges when two points of modification are interacting - the intent is still the same. The desired value must stay within the bounds of min and mix such that min >= desired >= max

Copy link
Author

@new-guy new-guy Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Conversely, if you set this on create, it will fail.

I am not sure I understand specifically what you mean by this. If you set what on create, what will fail?

Explaining the requirement that desired is between min and max was the objective of the documentation changes I made, though my changes could be worded better. I wasn't able to find this requirement in the existing documentation and had to go into the source code to discover this requirement. The docs currently state that the desired value is always ignored, which is misleading. Do you have ideas on how the docs should instead be modified to make this requirement more clear? Would love for others to benefit from my suffering, haha

Copy link
Author

Choose a reason for hiding this comment

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

Maybe I could update an example to include the min, max, and desired? And update the docs to say what you said about desired count being bound by min and max rather than saying desired count is ignored?

Copy link
Author

Choose a reason for hiding this comment

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

Any thoughts on this? It seems like it should be a relatively easy bit of detail to add

Copy link
Member

Choose a reason for hiding this comment

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

It's not clear what problem we are trying to solve, if there is even a problem. It seems like we're just trying to get a commit added, but I could be wrong

- **Note:** The autoscaling definition will prefer `desired_count` over either of those two autoscaling capacity values. If `desired_count` is less than `autoscaling_min_capacity`, it will use the `desired_count` value as the minimum autoscaling target, and same with the `autoscaling_max_capacity` variable.
2. The default configuration is intended for `FARGATE` use

For more details see the [design doc](https://github.com/terraform-aws-modules/terraform-aws-ecs/blob/master/docs/README.md)
Expand Down