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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[serve] Add health checking for http proxy actors #34944

Merged
merged 13 commits into from
May 8, 2023
Merged

[serve] Add health checking for http proxy actors #34944

merged 13 commits into from
May 8, 2023

Conversation

zcin
Copy link
Contributor

@zcin zcin commented May 2, 2023 •
edited
Loading

Why are these changes needed?

Add health checking to HTTP Proxy

  • Add no-op check_health() method to proxy actor
  • In controller, periodically call check_health() method
    • Before ready() returns, status is STARTING
    • If an error occurs with check_health(), set UNHEALTHY
    • If request to check_health() times out, set UNHEALTHY

Related issue number

Closes #19151

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

zcin added 5 commits May 1, 2023 23:29
@zcin zcin marked this pull request as ready for review May 5, 2023 04:26
@zcin
api annotation
9801a3e 
Signed-off-by: Cindy Zhang <[email protected]>
zcin added 2 commits May 4, 2023 21:40
@zcin
fix
7f3e0e6 
Signed-off-by: Cindy Zhang <[email protected]>
@zcin
improve
c3809a8 
Signed-off-by: Cindy Zhang <[email protected]>
sihanwang41
sihanwang41 reviewed May 5, 2023
View reviewed changes
Copy link
Contributor

@sihanwang41 sihanwang41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! leave some nits and please make sure unit tests cover all the statues :)

python/ray/serve/_private/http_state.py Outdated
Should only be called after confirming the object ref is ready.
Resets _health_check_obj_ref to None at the end.
"""
assert len(ray.wait([self._health_check_obj_ref], timeout=0)[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need to have this assert?

python/ray/serve/_private/http_state.py Outdated Show resolved Hide resolved
python/ray/serve/_private/http_state.py Outdated Show resolved Hide resolved
python/ray/serve/_private/http_proxy.py Show resolved Hide resolved
@zcin
address comments
ba485f2 
Signed-off-by: Cindy Zhang <[email protected]>
@edoakes
Copy link
Contributor

edoakes commented May 5, 2023

If request times out, set HEALTHY <- did you mean UNHEALTHY ?

@zcin
Copy link
Contributor Author

zcin commented May 5, 2023

If request times out, set HEALTHY <- did you mean UNHEALTHY ?

Oops yes, updated

edoakes
edoakes reviewed May 5, 2023
View reviewed changes
python/ray/serve/schema.py Outdated
Comment on lines 763 to 764
class HTTPProxyDetails(BaseModel):
status: HTTPProxyStatus
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add node_id, ip_address here?

@alanwguo anything else that there would be useful to show in the UI?

oh... maybe logs path?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any metrics related to HTTP Proxy today? I don't think so.

If we did, it would be useful to have some sort of identifier for the HTTP Proxy so i could link or filter the metrics down to the metrics related to a particular HTTP proxy.

For HTTPProxyStatus, do we have any sort of string that can provide more details about errors or anything?

Creation date could be useful I guess also

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we do have some metrics. The unique identifier would be the node_id, though that doesn't handle restarts if they were to happen. So maybe we should add actor_id here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added metadata for http proxy here: https://github.com/ray-project/ray/pull/34944/files#diff-e454e14e8c39af64fcec6997326bd32012fc91afe5d09d6ac1486aed6199afd1R764-R776

zcin added 4 commits May 6, 2023 12:47
@zcin
Merge branch 'master' into http-proxy-status
47894d0 
Signed-off-by: Cindy Zhang <[email protected]>
@zcin
add tests + more info to http proxy details
3443add 
Signed-off-by: Cindy Zhang <[email protected]>
@zcin
fix
74cd0c2 
Signed-off-by: Cindy Zhang <[email protected]>
@zcin
fix
d463fd8 
Signed-off-by: Cindy Zhang <[email protected]>
edoakes
edoakes approved these changes May 8, 2023
View reviewed changes
Copy link
Contributor

@edoakes edoakes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, thanks for picking it up!

@edoakes edoakes merged commit b7eeda3 into ray-project:master May 8, 2023
@edoakes edoakes mentioned this pull request May 16, 2023
Closed
architkulkarni pushed a commit to architkulkarni/ray that referenced this pull request May 16, 2023
@zcin @architkulkarni
[serve] Add health checking for http proxy actors (ray-project#34944)
0bf9335 
Add health checking to HTTP Proxy

Add no-op check_health() method to proxy actor
- In controller, periodically call check_health() method
- Before the first health check returns, status is STARTING
- If an error occurs, set UNHEALTHY
- If request times out, set UNHEALTHY
@edoakes edoakes mentioned this pull request May 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sihanwang41 sihanwang41 sihanwang41 left review comments

@alanwguo alanwguo alanwguo left review comments

@edoakes edoakes edoakes approved these changes

@shrekris-anyscale shrekris-anyscale Awaiting requested review from shrekris-anyscale

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Properly health check HTTP proxy & report its status
4 participants
@zcin @edoakes @alanwguo @sihanwang41