Allow setting temporary max_instances overrides #4078
Conversation
Just as setting temporary min_instances overrides is useful, so would being able to do the same for max_instances.
nemacysts
requested review from
EvanKrall,
jfongatyelp,
cuza,
bobokun,
EmanElsaban and
KaspariK
| @@ -156,19 +156,24 @@ def set_autoscaling_override(request): | |||
|
|
|||
| json_body = request.swagger_data.get("json_body", {}) | |||
| min_instances_override = json_body.get("min_instances") | |||
| max_instances_override = json_body.get("max_instances") | |||
There was a problem hiding this comment.
Nit, but should we add max_instances to the docstring?
| if not expire_after: | ||
| raise ApiFailure("expire_after is required", 400) | ||
|
|
||
| max_instances = instance_config.get_max_instances() | ||
| min_instances = min_instances_override or instance_config.get_min_instances() | ||
| max_instances = max_instances_override or instance_config.get_max_instances() | ||
| if max_instances is None: |
There was a problem hiding this comment.
Is this check dead given the previous validation?
There was a problem hiding this comment.
This may or may not be based on my misunderstanding of how this falls back...
There was a problem hiding this comment.
yes and no - max_instances can still be None here: but I think we need to move this around a bit so that we can't use this command to start autoscaling something that's not autoscaled (since most of the time we don't actually want cpu autoscaling)
...and i also just realized that maybe I should add an cleanup cronjob first or something - if you set a min_instances override N days in the past and then go to set a max_instances override later one, the min_instances override will kick in as well.
| @@ -75,6 +75,14 @@ def add_subparser(subparsers): | |||
| else autoscale_parser.error("Minimum instances must be >= 1"), | |||
| default=None, | |||
| ) | |||
| override_group.add_argument( | |||
There was a problem hiding this comment.
Do we want a max max to avoid some edge case where a typo + some bad behaviour overloads a cluster? Or is that too paranoid?
There was a problem hiding this comment.
we talked a little bit in our 1:1 - a max max is probably fine to obviate, but we might want some sort of similar concept for min (probably just ask for confirmation rather than hard-block things)
| if int(x) >= 1 | ||
| else autoscale_parser.error("Maximum instances must be >= 1"), | ||
| default=None, | ||
| ) | ||
| override_group.add_argument( |
There was a problem hiding this comment.
Super nit, but --for applies to both set-min and set-max now, right?
Just as setting temporary
min_instancesoverrides is useful, so would being able to do the same formax_instances.