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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace adjustments with min/max for scale in and out #12

Closed
wants to merge 1 commit into from

Conversation

@lox
Copy link
Contributor

commented Apr 28, 2019

Currently we have scale-in-adjustment and cooldown, but when I went to extend this to a scale-out adjustment I realized that there are two distinct use cases, a minimum bound and a maximum bound for each scaling direction.

For scale-in, you might want to slow down scale-in, for which you'd set a scale-in-max of say -2, which would ensure that the scale-in didn't move by more than -2. Alternately, you might (for some reason) want to scale in always by the maximum, so you could set a scale-in-min of -100. This one is pretty tenuous, but I implemented it to match it's more useful scale-out-max.

For scale-out, you might want to have faster scale-out, so you set a scale-out-min of 20, which means that you always scale by at least 20 instances. If you set scale-out-min, you can control the speed at which scale out runs.

I also implemented a disable switch for scale-out and a cooldown timer if you are so inclined.

@etaoins I'd love your feedback on scale-in-adjustment vs scale-in-min and scale-in-max. Min/max with negative numbers is super confusing but I couldn't figure out how to handle the scale out and keep them consistent 馃

@etaoins
Copy link
Contributor

left a comment

I think it is possible to merge the scale-in and scale-out cases. The trick is that because scale-in params are negative the largest possible scale-in value is actually the minimum value mathematically while the smallest possible is the maximum value. I don't think we'd want to expose this in the name of the parameters so we'd have to flip them in the negative case when we built the ScaleParams, e.g:

MaxAdjustment: *scaleInMinAdjustment,
MinAdjustment: *scaleInMaxAdjustment,

We'd also need to change some of the < 0 to != 0 so it would work for both negative and positive values.

While that would end up being less code I'm leaning towards that being too clever. The current code is pretty explicit and has tailored log messages for each case.

if scaleInMin, err = strconv.ParseInt(v, 10, 64); err != nil {
return "", err
}
if scaleInMax >= 0 {

This comment has been minimized.

Copy link
@etaoins

etaoins Apr 28, 2019

Contributor

Looks like this is a copy-and-paste (should be scaleInMin)

@@ -9,10 +9,19 @@ import (
)

type ScaleInParams struct {

This comment has been minimized.

Copy link
@etaoins

etaoins Apr 28, 2019

Contributor

Could these be merged in to ScaleParams? They seem symmetric with each other.

This comment has been minimized.

Copy link
@lox

lox Apr 28, 2019

Author Contributor

Yeah, probably. I kind of liked having the type safety and in the current world of positive vs negative changes there didn't seem much economy to combining them.

@lox

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

What if rather than min/max or adjustment we went with ScaleOutFactor and ScaleInFactor (or Multiplier), which was a decimal multiplier applied to the DesiredCount change?

That would handle the two main uses cases:

  • Slow down scale in: ScaleInFactor=0.25 this would reduce scale-ins to only 25% of what they would be otherwise. For instance, scale-in of 10 would end up as a scale in of 3.

  • Faster scale out: ScaleOutFactor=2 this would increase scale-outs by 200%, so a scale out of 10 would end up 20.

@etaoins

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

That seems like it would work better with build queues that have wildly varying load (eg a corporate build queue might have 20 jobs during the day and only a couple overnight). Makes sense to making the scaling scalable 馃榾

We鈥檇 have to make sure we wouldn鈥檛 hit an asymptote when scaling down. For example, if we鈥檙e scaling down by 25% with only 3 agents we鈥檇 need to make sure we eventually scale to 2 instead of always rounding to 3.

@lox

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

We鈥檇 have to make sure we wouldn鈥檛 hit an asymptote when scaling down. For example, if we鈥檙e scaling down by 25% with only 3 agents we鈥檇 need to make sure we eventually scale to 2 instead of always rounding to 3.

Rounding down should do it, right?

@etaoins

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

Yup, I think rounding down when scaling down and rounding up when scaling up would handle it.

@lox

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Closing in favour of #13.

@lox lox closed this May 5, 2019

@lox lox deleted the add-scale-out-min-max branch May 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.