add external scaler enabled flag #57727
Conversation
Signed-off-by: harshit <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an external_scaler_enabled flag to control whether external scalers can modify the number of replicas for an application. The changes correctly plumb this new flag through the system, from the user-facing APIs down to the controller logic, and add validation to prevent conflicts with Serve's built-in autoscaling. The implementation is solid, but I've identified a couple of areas for improvement in the error handling of the new scaling API endpoint. Specifically, one issue involves returning a full traceback in an API error response, and another is incomplete handling of 'not found' errors for deployments, which could lead to unhelpful 503 server errors. I've provided suggestions to address these points for a more robust and user-friendly API.
Signed-off-by: harshit <[email protected]>
abrarsheikh
left a comment
abrarsheikh
left a comment
There was a problem hiding this comment.
add a black box test that using serve config, then introduce enable_external_scaler switchback. First enable then disable then enable. Ensure that works. Make sure to not restart reset between switchback.
Signed-off-by: harshit <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: harshit <[email protected]>
Signed-off-by: harshit <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: harshit <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: harshit <[email protected]>
not sure if you saw this ^. Additionally it would be good to add a test for using external autoscaler in both imperative and declarative case. |
Signed-off-by: harshit <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: harshit <[email protected]>
Signed-off-by: harshit <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: harshit <[email protected]>
| # From customer's viewpoint, the deployment is deleted instead of being deleted | ||
| # as they must have already executed the delete command | ||
| {"error": "Deployment is deleted"}, | ||
| {"error": str(e)}, |
Signed-off-by: harshit <[email protected]>
Signed-off-by: harshit <[email protected]>
Signed-off-by: harshit <[email protected]>
Signed-off-by: harshit <[email protected]>
Signed-off-by: harshit <[email protected]>
abrarsheikh
left a comment
abrarsheikh
left a comment
There was a problem hiding this comment.
looks good. @zcin mind taking a look as well.
|
@harshit-anyscale please resolve comments that have been addressed, makes it easier to review future revisions. |
done, resolved them. |
Signed-off-by: harshit <[email protected]>
| deployment_args_list.append(deployment_args_proto.SerializeToString()) | ||
|
|
||
| application_args_proto = ApplicationArgs() | ||
| application_args_proto.external_scaler_enabled = app.external_scaler_enabled |
There was a problem hiding this comment.
@abrarsheikh fyi we should probably also move route_prefix here, route_prefix being deployment level is legacy
Signed-off-by: harshit <[email protected]>
Signed-off-by: harshit <[email protected]>
Signed-off-by: harshit <[email protected]>
Signed-off-by: harshit <[email protected]>
Signed-off-by: harshit <[email protected]>
adding external scaler enabled flag in the application config, which will dictate, whether to allow the external scalers to update the num replicas for an application or not. this is being done as part of the [custom autoscaling story](https://docs.google.com/document/d/1KtMUDz1O3koihG6eh-QcUqudZjNAX3NsqqOMYh3BoWA/edit?tab=t.0#heading=h.2vf4s2d7ca46) --------- Signed-off-by: harshit <[email protected]> Signed-off-by: YK <[email protected]>
adding external scaler enabled flag in the application config, which will dictate, whether to allow the external scalers to update the num replicas for an application or not. this is being done as part of the [custom autoscaling story](https://docs.google.com/document/d/1KtMUDz1O3koihG6eh-QcUqudZjNAX3NsqqOMYh3BoWA/edit?tab=t.0#heading=h.2vf4s2d7ca46) --------- Signed-off-by: harshit <[email protected]>
adding external scaler enabled flag in the application config, which will dictate, whether to allow the external scalers to update the num replicas for an application or not.
this is being done as part of the custom autoscaling story