Repository navigation
[serve] Surface terminal deployment actor failures outside UPDATING - #66910
jeffreywang88 merged 6 commits into
Conversation
Signed-off-by: GNITOAHC <chaotingchen10@gmail.com>
Signed-off-by: GNITOAHC <chaotingchen10@gmail.com>
![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 ensures that deployment-scoped actor terminal failures are properly surfaced and transition the deployment status to unhealthy, even when there are running replicas or when the failure occurs outside of an update. This is achieved by reordering status checks in check_curr_status and handling the DEPLOYMENT_ACTOR_FAILED trigger across various deployment statuses. Additionally, unused helper methods in the client were cleaned up, and comprehensive unit tests were added to verify these transitions and failure reporting. There are no review comments to address.
Signed-off-by: GNITOAHC <chaotingchen10@gmail.com>
ray-gardener
Bot
added
serve
jeffreywang88
left a comment
jeffreywang88
left a comment
There was a problem hiding this comment.
Repro'd the issue locally by starting a healthy deployment, killing the deployment actor, and forcing every recreation constructor to fail. Prior to this PR, the failure reason is HEALTH_CHECK_FAILED and is now DEPLOYMENT_ACTOR_FAILED. The bug is essentially the stale reason and misleading retry message.
| ) | ||
| self._shutdown = True | ||
|
|
||
| def _wait_for_deployment_healthy(self, name: str, timeout_s: int = -1): |
There was a problem hiding this comment.
Thanks for removing this! Confirmed that these helpers have no callers.
| Previously the DEPLOYMENT_ACTOR_FAILED trigger was dropped outside of | ||
| UPDATING (0 replicas) or never raised (replicas running), so the status | ||
| stayed UNHEALTHY/HEALTH_CHECK_FAILED with a stale "Retrying" message | ||
| even though the controller had stopped recreating the actor. |
There was a problem hiding this comment.
Let's not mention what the previous behavior is. Readers can't understand by reading through the code post PR merge.
| ) | ||
| assert "ctor boom" in ds.curr_status_info.message | ||
| assert ds._replicas.count() == num_replicas | ||
|
|
There was a problem hiding this comment.
Drive the state machine dsm.update until it reaches a healthy state.
…cstring Signed-off-by: GNITOAHC <chaotingchen10@gmail.com>
Signed-off-by: GNITOAHC <chaotingchen10@gmail.com>
jeffreywang88
left a comment
jeffreywang88
left a comment
There was a problem hiding this comment.
🚢 thanks @GNITOAHC
2 participants
Description
When a deployment actor fails its health check and every recreation attempt fails, the controller stops retrying. The deployment status still showed
UNHEALTHY/HEALTH_CHECK_FAILEDwith "Retrying" indefinitely, so there was no signal that a redeploy is needed. There are two causes:DeploymentStatusInfo.handle_transition(common.py) only handledDEPLOYMENT_ACTOR_FAILEDinUPDATINGandDEPLOY_FAILED. InHEALTHY,UPSCALING,DOWNSCALINGandUNHEALTHY, the trigger was silently dropped.DeploymentState.check_curr_status(deployment_state.py) only checked for terminal actor failure when no replica was running at the target version. With any replica running, the trigger was never raised.Related issues
No existing issue.
Additional information
Also includes a small cleanup commit, removing
ServeControllerClient._wait_for_deployment_healthyand_wait_for_deployment_deleted(_private/client.py). Neither has a caller and both are broken.