Add request-level maximum scale option #2246
Conversation
| ) | ||
| public class SingularityRequest { | ||
| private final String id; | ||
| private final RequestType requestType; | ||
| private final Optional<List<String>> owners; | ||
| private final Optional<Integer> numRetriesOnFailure; | ||
| private final Optional<Integer> maxScale; |
There was a problem hiding this comment.
I'm not entirely clear on how this new option would interact with autoscale
There was a problem hiding this comment.
That resource uses SingularityValidator's checkScale, so you should the checkBadRequest also to that method so that AutoScale is blocked
(or make a checkBadScaleRequest method for both validator methods to use)
There was a problem hiding this comment.
It looks like checkScale doesn't currently check against the global max scale in MesosConfiguration, so AutoScale isn't blocked by that when scaling. Do we want it be blocked by the new request-level max scale option?
There was a problem hiding this comment.
Good catch — we should check both max scale values. Since Singularity has been missing this, we should verify that no active requests would be caught by the current maxNumInstancesPerRequest value before this is rolled out.
SingularityService/src/main/java/com/hubspot/singularity/data/SingularityValidator.java
Show resolved
Hide resolved
|
I wonder if we should just put the validation of the scale in the constructor for SingularityRequest instead? That way it will be enforced everywhere the object is created, even failing client side when possible to make it even more apparent/faster |
| @@ -230,11 +230,17 @@ public SingularityRequest checkSingularityRequest( | |||
| !request.getInstances().isPresent() || request.getInstances().get() > 0, | |||
| "Instances must be greater than 0" | |||
| ); | |||
|
|
|||
| // check if requested number of instances exceeds max scale | |||
| int maxInstances = request.getMaxScale().isPresent() | |||
There was a problem hiding this comment.
This should be the lower of the two config values (i.e. a user shouldn't be able to exceed our global max scale by specifying a request level max scale)
There was a problem hiding this comment.
Sounds good. To confirm, does this mean global does not automatically override request-level and vice versa, and we instead want to always use the lower of the two when validating scale?
There was a problem hiding this comment.
Yes, we should always use the lowest. Users should be able to further restrict the global option with a request level option, but not the other way around.
SingularityService/src/main/java/com/hubspot/singularity/data/SingularityValidator.java
Outdated
Show resolved
Hide resolved
SingularityService/src/main/java/com/hubspot/singularity/data/SingularityValidator.java
Outdated
Show resolved
Hide resolved
|
🚢 |
|
🚢 |
This adds a maximum scale option at the request level for users to specify the maximum number of instances allowed for a particular request. Previously, there only existed a global limit in
MesosConfigurationto prevent overscales that applied to all requests.https://git.hubteam.com/HubSpot/PaaS-Run/issues/1546