feat: add "health" command #299
Conversation
This commit adds a new "health" command to pebble. The health command
queries the health of configured pebble checks. It outputs "healthy" and
returns an exit code of 0 if all the specified checks are healthy.
Otherwise, it outputs "unhealthy" and returns with an exit code of 1.
The "health" command accepts two options with arguments:
- The --level option takes an argument, either "alive" or "ready". It
queries the health of only those checks with the same level.
Note that, ready implies alive. Thus, if the ready checks are
healthy but one of the alive checks are not, the overall output will
be "unhealthy".
- The --format option takes an argument, either "text" or "json". It
controls how the output will look like. The default option is
"text".
Usage:
pebble health [health-OPTIONS] [<check>...]
This command does not add any new logic to pebble, as it relies entirely
on the already existing implementation of the pebble health API.
This commit includes a few tests too for the relevant changes.
|
Hi @benhoyt, @cjdcordeiro. Will you please take a look at this PR when you are free? Cheers. |
| // Health fetches healthy status of specified checks. | ||
| func (client *Client) Health(opts *HealthOptions) (*HealthInfo, error) { | ||
| query := make(url.Values) | ||
| if opts.Level != UnsetLevel { |
There was a problem hiding this comment.
We have a pre-existing bug here: UnsetLevel of what? Being a constant specific to checks, this needs to be named UnsetCheckLevel or similar. This is a client, so unfortunately we cannot make changes trivially without risking breaking other people's code. We might risk this based on how recent the feature is and hope that people are not using Pebble's API directly yet, but even that needs coordination, at least with Ben. Can you please check his opinion on this?
There was a problem hiding this comment.
To be clear: this PR shoud not hold on this decision. It should be fixed independently, and fixing the one instance above can be done together with the PR that addresses the issue.
There was a problem hiding this comment.
Yep, I'm on board with changing that (but agreed -- in a separate PR). I think it's highly unlikely any external users are using this constant (Juju certainly isn't). And we would only have to change UnsetLevel -- AliveLevel and ReadyLevel we could consider changing too, but I think they're okay as they include the word "alive" and "ready", which gives them context.
There was a problem hiding this comment.
I agree, we should change it to be more specific. I will add a separate PR after this gets merged.
| cs.rsp = `{ | ||
| "type": "sync", | ||
| "status-code": 502, | ||
| "status": "Bad Gateway", |
There was a problem hiding this comment.
The /v1/health endpoint returns a "Bad Gateway" status with 502 code when unhealthy.
{"type":"sync","status-code":502,"status":"Bad Gateway","result":{"healthy":false}}There was a problem hiding this comment.
Yeah, we (I can't remember who exactly) debated which HTTP status code this should return at the time, and ended up with 502. It's nice a perfect fit, but we wanted it to be a 5xx code, and Pebble is kind of acting as a proxy/gateway here, so it seemed like a reasonable one.
There was a problem hiding this comment.
This seems a bit awkward. Generally those errors mean "I can't handle your request", because of reason 5XX. The request here is "How's your health?", so in theory the response should be "Yep, the check you're asking for is not ready/alive.".
This seems analogous to asking to pull a file and getting a 404 to say the file doesn't exist. That's a bad practice as we're mixing the success of the request with the result of the operation requested. We would get a similar 404 if we were talking to a different server altogether, for example.
Maybe this was done to satisfy Kubernetes, when poking at health URLs using its internal probing?
Includes: - Drop --format option to health command - Drop client.HealthInfo type - client.Health function signature changes - Changes in comments
This PR adds a new "health" command to Pebble. The health command queries the health of configured pebble checks. It outputs "healthy" and returns an exit code of 0 if all the specified checks are healthy. Otherwise, it outputs "unhealthy" and returns with an exit code of 1.
The "health" command accepts
twoone option with arguments:Usage:
This PR includes a few tests too for the relevant changes.
This command does not add any new logic to Pebble, as it relies entirely on the already existing implementation of the Pebble health API. Nonetheless, the hereby proposed feature (and corresponding spec) identifies certain edge cases where the resulting health outcome isn’t the most intuitive or predictable. Examples:
pebble health --level=readycan be healthy even ifpebble health --level=alivewill be healthy even if ready checks are downMajor changes from the initial commit(s)
checksandhealthappear in a new category inpebble help.--formatoption fromhealthcommand due to recent feedbacks.