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.

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

[nexus] Add /v1/ping endpoint #3925

Merged
merged 2 commits into from
Oct 9, 2023
Merged

[nexus] Add /v1/ping endpoint #3925

merged 2 commits into from
Oct 9, 2023

Conversation

david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Aug 21, 2023

Closes #3923

Adds /v1/ping that always returns { "status": "ok" } if it returns anything at all. I went with ping over the initial /v1/system/health because the latter is vague about its meaning, whereas everyone know ping means a trivial request and response. I also thought it was weird to put an endpoint with no auth check under /v1/system, where ~all the other endpoints require fleet-level perms.

This doesn't add too much over hitting an existing endpoint, but I think it's worth it because

  • It doesn't hit the DB
  • It has no auth check
  • It gives a very simple answer to "what endpoint should I use to ping the API?" (a question we have gotten at least once)
  • It's easy (I already did it)

Questions that occurred to me while working through this:

  • Should we actually attempt to do something in the handler that would tell us, e.g., whether the DB is up?
    • No, that would be more than a ping
    • Raises DoS questions if not auth gated
    • Could add a db status endpoint or or you could use any endpoint that returns data
  • What tag should this be under?
    • Initially added a system tag because a) this doesn't fit under existing system/blah tags and b) it really does feel miscellaneous
    • Changed to system/status, with the idea that if we add other kinds of checks, they would be new endpoints under this tag.

@david-crespo david-crespo requested a review from ahl August 21, 2023 18:45
@jmpesp
Copy link
Contributor

jmpesp commented Aug 22, 2023

Should we actually attempt to do something in the handler that would tell us, e.g., whether the DB is up?

Maybe SELECT 1?

@david-crespo
Copy link
Contributor Author

I think I should change the status key to something more specific (like api) so we can return status info on various things, like DB, existing VMs, provisioning new VMs. We may not yet have operational definitions of "up" for all of those things, so we can add them as we figure that out.

@david-crespo david-crespo marked this pull request as draft September 14, 2023 17:56
@david-crespo david-crespo force-pushed the health-check-endpoint branch from 5915d70 to a5e5cbe Compare October 4, 2023 01:59
@david-crespo david-crespo changed the title [nexus] Add /v1/system/health endpoint [nexus] Add /v1/ping endpoint Oct 4, 2023
@david-crespo david-crespo marked this pull request as ready for review October 4, 2023 02:26
@ahl
Copy link
Contributor

ahl commented Oct 4, 2023

The code looks good, but I'd suggest asking @davepacheco to evaluate the concept.

@davepacheco
Copy link
Collaborator

Code looks fine and I appreciate that it's transparent about what it is. This doesn't really solve most of the problems I expect a customer to have in this area but I could see it being useful as a low bar for availability.

@david-crespo
Copy link
Contributor Author

Yeah, I figure it's a trivially easy nice-to-have. Pretty low value, but the value is still higher than the cost.

@david-crespo david-crespo merged commit 47a6b42 into main Oct 9, 2023
22 checks passed
@david-crespo david-crespo deleted the health-check-endpoint branch October 9, 2023 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An Oxide health check endpoint for customer monitoring system to use
4 participants