chore: set tasks fewer than spot_from to fargate cp when min and spot_from equal#4187
chore: set tasks fewer than spot_from to fargate cp when min and spot_from equal#4187mergify[bot] merged 5 commits intoaws:mainlinefrom
spot_from to fargate cp when min and spot_from equal#4187Conversation
|
🍕 Here are the new binary sizes!
|
Codecov Report
@@ Coverage Diff @@
## mainline #4187 +/- ##
============================================
- Coverage 69.30% 69.29% -0.02%
============================================
Files 250 250
Lines 35919 35946 +27
Branches 264 264
============================================
+ Hits 24893 24907 +14
- Misses 9834 9847 +13
Partials 1192 1192
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| // to one less than spotFrom | ||
| if spotFrom > min { | ||
| if spotFrom >= min { | ||
| base := spotFrom - 1 |
There was a problem hiding this comment.
huh so you can you can have spotFrom and min be 0?
and it doesn't error if the base ends up being -1?
According to the commit message, we get:
min: 0 and spot_from: 0 --> 0 Fargate, 1 Spot
I think this is pretty strange, I would have expected 0 Fargate and 0 Spot here.
edit: I take this back 1 fargate spot makes sense if your cpu_percentage is like 70% because when you have 0 tasks the avg cpu is at 0%, so ECS has to scale up resulting in 1 spot task.
There was a problem hiding this comment.
Side-note: should we make sure that min, max and spot_from is >= 0 here:
copilot-cli/internal/pkg/manifest/validate.go
Line 1077 in 5781c53
There was a problem hiding this comment.
this has got to be a bug lol 😂 like what could -1 base mean conceptually
iamhopaul123
added
the
do-not-merge
efekarakus
removed
the
do-not-merge
…`spot_from` equal (aws#4187) Addresses: https://gitter.im/aws/copilot-cli?at=637244e418f21c023bb057cc Previously, the logic to scale into Fargate Spot only applied if the `spot_from` value was greater than the `min` value. So in edge case of users having equal values greater than 1 for those fields, as in the case linked to above, all of the tasks would have Fargate Spot as their capacity provider, rather than the `spot_from` value determining the threshold for Fargate vs Fargate Spot. This kind of config is somewhat atypical, as the `min` value isn't guaranteed if that number task has Fargate Spot as a capacity provider, but if users do set up their scaling this way, at least we are honoring the `spot_from` value. With this change: `min: 3` and `spot_from: 3` --> 2 Fargate, 1 Spot `min: 2` and `spot_from: 2` --> 1 Fargate, 1 Spot `min: 1` and `spot_from: 1` --> 0 Fargate, 1 Spot `min: 0` and `spot_from: 0` --> 0 Fargate, 1 Spot By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
5 participants
Addresses: https://gitter.im/aws/copilot-cli?at=637244e418f21c023bb057cc
Previously, the logic to scale into Fargate Spot only applied if the
spot_fromvalue was greater than theminvalue. So in edge case of users having equal values greater than 1 for those fields, as in the case linked to above, all of the tasks would have Fargate Spot as their capacity provider, rather than thespot_fromvalue determining the threshold for Fargate vs Fargate Spot. This kind of config is somewhat atypical, as theminvalue isn't guaranteed if that number task has Fargate Spot as a capacity provider, but if users do set up their scaling this way, at least we are honoring thespot_fromvalue.With this change:
min: 3andspot_from: 3--> 2 Fargate, 1 Spotmin: 2andspot_from: 2--> 1 Fargate, 1 Spotmin: 1andspot_from: 1--> 0 Fargate, 1 Spotmin: 0andspot_from: 0--> 0 Fargate, 1 SpotBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.