-
Notifications
You must be signed in to change notification settings - Fork 573
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
Added rpk cluster health
command
#4295
Conversation
d76444b
to
69588e2
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.
very short review. looks good on a first pass.
I think we can close the other PR with the 5 commits which add the health summary? I'm building on top of this PR for other stuff, too.
auto ec = co_await maybe_refresh_cluster_health( | ||
force_refresh::no, deadline); |
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.
Is the idea here that any node can serve a health report, and that it will be refreshed if it is stale? I wonder if we should always retrieve health reports from the controller?
I see that ec
is used to determine is_healthly
. Presumably it said the cluster isn't healthy if we couldn't refresh?
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.
exactly, i assumed that if we can not refresh cluster is in bad condition.
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.
hmm I'm not very familiar with this code so please tell me if I have some wrong assumptions.
but I think if the cluster was healthy, but we couldn't refresh, then we may have cluster_health_overview
with no nodes down, all nodes have leaders, but is_healthy
is false, which may be confusing.
Maybe we should add a field like obsolete cluster state
to the cluster_health_overview
?
46e725f
to
011fc22
Compare
Introduced `cluster_health_overview` type that represents a simple view of cluster health. The type provides a simple flag indicating if cluster is healthy and if not it provides some additional information that should make it easier to understand what makes the cluster unhealthy. Signed-off-by: Michal Maslanka <michal@vectorized.io>
Implemented logic that goes over all the available node health reports to build the cluster health overview. The `get_cluster_health_overview` method should be a place where more complicated health overview logic may be added in future. Signed-off-by: Michal Maslanka <michal@vectorized.io>
Signed-off-by: Michal Maslanka <michal@vectorized.io>
Added `GET /v1/cluster/health_overview` path to redpanda admin server. Signed-off-by: Michal Maslanka <michal@vectorized.io>
Signed-off-by: Michal Maslanka <michal@vectorized.io>
011fc22
to
c5724bb
Compare
Signed-off-by: Michal Maslanka <michal@vectorized.io>
c5724bb
to
c31eca7
Compare
rpk admin cluster health
commandrpk cluster health
command
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.
cc @dotnwat and @jcsp: Are we planning to have any other command related to health / extend health?
At the moment this reserves rpk cluster health
-- do we have a guess as to how many commands we are going to stuff under rpk cluster
? Currently it's config
, maintenance
, and health
, I'm wondering if we have some list that we plan to add for the next 3 to 6mo. Mostly, I'm wary of rpk cluster
becoming a dumping ground for 30 commands.
From what I can tell, some of what's under rpk redpanda admin
can / should live next to these cluster commands: rpk redpanda admin brokers {list,decommission,recommission}
potentially (since these are commands on the cluster), the soon-to-be-merged rpk redpanda admin partitions list
(and in the future, move). Perhaps even rpk redpanda admin config {log-level,print}
should be deleted (should this be merged with rpk cluster config
?)
If we can get a few thoughts here documented, I think I'd be more comfortable merging this (and more comfortable with the recent maintenance
-- it wasn't that long ago that I was proposing deprecating rpk cluster
entirely).
c31eca7
to
bd6ce9d
Compare
Yes.
I don't think we'll get to 30, but there will probably be a bunch. Globally, rpk is going to have lots of commands between the on-prem tooling, the cloud client stuff, the BYOC client stuff, and the kafka client stuff -- grouping the commands that are about managing clusters within a prefix up-front feels like the right thing to do, and I think we've got to get away from the confusing "redpanda" prefix that to us means "the daemon" but to a user is just the name of our company+product.
"redpanda admin config log-level" is a per-node thing, I would be inclined to put that under a "node" or "daemon" prefix if we can settle on that as a category for commands that operate directly on a single daemon. It's also just a kind of weird command, because we don't provide true configuration for log levels (it's one of the things people have to mess with systemd to set permanently). Once we add real config for log levels, that would be just another property in "cluster config", but I think we'd keep the node-level ephemeral log settings too: it's a useful pattern in support to turn up logging but just briefly.
This is what I think it should look like: Basically this is two namespaces that logically belong to the core team: "cluster" and "node". These are all things that an FMC user wouldn't ever touch, and would naturally break off into a "redpandactl" tool for administrators, if we ever departed from the single CLI binary model. |
* Health overview is based on the information available in health monitor. | ||
* Cluster is considered as healthy when follwing conditions are met: | ||
* | ||
* - all nodes that are are responding | ||
* - all partitions have leaders | ||
* - cluster controller is present (_raft0 leader) | ||
*/ |
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.
Also this comment doesn't mention that cluster is not healthy if we couldn't refresh health information (does that mean that some nodes are not responding?)
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.
This is implicitly covered in all cluster nodes are responding
, I've changed the comment to fix error in the fist item
Added `rpk redpanda cluster health` command allowing users to query for cluster health overview, the command provides two flags `-w --watch` that blocks and prints out all the cluster health changes and `-e --exit-on-healthy` which when passed in with `--watch` exits after cluster becomes healthy. Signed-off-by: Michal Maslanka <michal@vectorized.io>
bd6ce9d
to
0319db5
Compare
cmd := cobra.Command{ | ||
Use: "health", | ||
Short: "Queries cluster for health overview.", | ||
Long: ` |
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.
Extraneous newline to begin this long text -- how about
Long: `Queries health overview.
Health overview is created based on the health reports collected periodically
from all nodes in the cluster. A cluster is considered healthy when the
following conditions are met:
* all cluster nodes are responding
* all partitions have leaders
* the cluster controller is present
`,
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.
lgtm pending a long help text fix; preemptively approving now to remove my approval block.
I like the thoughts above on rpk cluster
vs rpk node
. We can then eventually do away with rpk redpanda
.
Signed-off-by: Noah Watkins <noah@redpanda.com>
Signed-off-by: Noah Watkins <noah@redpanda.com>
I fixed this for Michal. |
/backport v22.1.x |
Failed to run cherry-pick command. see workflow
|
Cover letter
Added an admin API endpoint
GET /v1/cluster/health_overview
returning simple overview of cluster health. The endpoint is intended to be used as a simple indicator of overall cluster health. It providesis_healthy
flag that is a cluster health indicator. Additional fields available in the overview should make debugging potential health issues easierFixes: #4526
Release notes
Features
added GET /v1/cluster/health_overview admin api endpoint