-
Notifications
You must be signed in to change notification settings - Fork 251
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.
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
Check hippo health #594
Check hippo health #594
Conversation
72f2e19
to
d4a31a0
Compare
src/commands/deploy.rs
Outdated
.await? | ||
.text() | ||
.await?; | ||
if result == "Unhealthy" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should perhaps be:
if result == "Unhealthy" { | |
if result != "Healthy" { |
Unless I'm forgetting how reqwest
works, I think this would return Ok
on a - for example - 502 Bad Gateway
, as long as the body wasn't Unhealthy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @lann and would also recommend using error_for_status, e.g.:
let result = reqwest::get(hippo_healthz_url.to_string())
.await?
.error_for_status()?
.text()
.await?;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dicej and @lann, thanks for the suggestion. If we use error_for_status
, it looks like we don't need to check result
text.
> curl -k http://localhost:5309/healthz
Unhealthy%
> ~/Project/spin/target/debug/spin deploy -f spin.toml --deploy-existing-bindle
Error: HTTP status server error (503 Service Unavailable) for url (http://localhost:5309/healthz)
# start nomad
> ~/Project/spin/target/debug/spin deploy -f spin.toml --deploy-existing-bindle
Deployed spin-hello-world version 1.0.0+qb632d35
Available Routes:
hello: http://spin-deploy.spin-hello-world.hippofactory.io/hello
A simple component that returns hello.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Hippo still returns 200 with an Unhealthy
status: deislabs/hippo#889
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing it out. I add the check for result != "Healthy"
back.
955d4a9
to
d7d3978
Compare
Signed-off-by: Frank Yang <yangpoan@gmail.com>
d7d3978
to
b998c2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'd like @michelleN to sanity-check the behavior.
FYI in the latest version of hippo, /healthz will now return a 503 if something is unhealthy. |
Going to merge and then we can update. |
resolves #562