Repository navigation
Print StorageCluster CR health table from odf get health - #429
AnikaYadav1 wants to merge 4 commits into
Conversation
|
@parth-gr @subhamkrai @travisn |
| Example: "odf get health", | ||
| PreRun: func(cmd *cobra.Command, args []string) { | ||
| // verify operator pod is running | ||
| if !healthVerbose { |
There was a problem hiding this comment.
Some basic useful info will come from the operator pod, so it seems valid to wait for it even if not verbose
There was a problem hiding this comment.
now it waits in both cases, with and without --verbose.
| - `odf get`: | ||
| - `recovery-profile`: Get the recovery profile value. | ||
| - `health`: Check health of the cluster and common configuration issues. | ||
| - `health`: Print StorageCluster-managed CR status. Use `--verbose` for checking health of the cluster and common configuration issues. |
There was a problem hiding this comment.
I would recommend the odf get health command print as much status as useful, as long as it is quick to gather. There is more useful info by default than only the CR status.
There was a problem hiding this comment.
After discussing with the team and Eran we had decided that odf get health should be a quick look at StorageCluster CR status only. odf get health --verbose includes everything else. The original odf get health output was already a subset of --verbose, so everything that used to be in odf get health is still in odf get health --verbose.
There was a problem hiding this comment.
Let's get a written agreement on the epic, just to confirm everyone is on the same page. On the epic, could you show the following info?
- Example output for the proposed get health output
- Example output for the full verbose get health output
- Point out which verbose status takes longer to detect, and which verbose status is quick to collect. Then we can finalize if any quick info would be helpful in the main output instead.
- Confirm what we will collect with the must-gather. (IIRC we discussed the mg will not gather the verbose info)
There was a problem hiding this comment.
Started a slack thread for this:
https://ibm-systems-storage.slack.com/archives/C0C3WJBCUMP/p1793095017298569
17b67d4 to
954299b
Compare
|
Here are the screenshots testing it on a cluster |
subhamkrai
left a comment
There was a problem hiding this comment.
I'll review in details tomorrow. Also, please share the new output in github comments, and also, update the CI tests.
| var clusterHealth = &cobra.Command{ | ||
| Use: "health", | ||
| Short: "check health of the cluster and common configuration issues", | ||
| Short: "print StorageCluster-managed CR status, or detailed pod checks with --verbose", |
There was a problem hiding this comment.
| Short: "print StorageCluster-managed CR status, or detailed pod checks with --verbose", | |
| Short: "print ODF CR status, or detailed status with --verbose flag", |
Let's add Long as well, if needed, to describe the default command and the --verbose command
| }, | ||
| namespace := "" | ||
| namespaceSet := false | ||
| if f := cmd.Flag("namespace"); f != nil && f.Changed { |
There was a problem hiding this comment.
| if f := cmd.Flag("namespace"); f != nil && f.Changed { | |
| if flagNamespace := cmd.Flag("namespace"); f != nil && f.Changed { |
Please use meaningful variable names
954299b to
e92db75
Compare
| {component: "Consumer", group: "ocs.openshift.io", version: "v1alpha1", kind: "StorageConsumer", namespaced: true, observedPath: []string{"status", "state"}}, | ||
| {component: "SVG", group: "ceph.rook.io", version: "v1", kind: "CephFilesystemSubVolumeGroup", namespaced: true, observedPath: []string{"status", "phase"}}, | ||
| {component: "RNS", group: "ceph.rook.io", version: "v1", kind: "CephBlockPoolRadosNamespace", namespaced: true, observedPath: []string{"status", "phase"}}, |
There was a problem hiding this comment.
| {component: "Consumer", group: "ocs.openshift.io", version: "v1alpha1", kind: "StorageConsumer", namespaced: true, observedPath: []string{"status", "state"}}, | |
| {component: "SVG", group: "ceph.rook.io", version: "v1", kind: "CephFilesystemSubVolumeGroup", namespaced: true, observedPath: []string{"status", "phase"}}, | |
| {component: "RNS", group: "ceph.rook.io", version: "v1", kind: "CephBlockPoolRadosNamespace", namespaced: true, observedPath: []string{"status", "phase"}}, | |
| {component: "StorageConsumer", group: "ocs.openshift.io", version: "v1alpha1", kind: "StorageConsumer", namespaced: true, observedPath: []string{"status", "state"}}, | |
| {component: "SubVolumeGroup", group: "ceph.rook.io", version: "v1", kind: "CephFilesystemSubVolumeGroup", namespaced: true, observedPath: []string{"status", "phase"}}, | |
| {component: "RadosNamespace", group: "ceph.rook.io", version: "v1", kind: "CephBlockPoolRadosNamespace", namespaced: true, observedPath: []string{"status", "phase"}}, |
Those abbreviations are not always known to customers reading this command
|
I would recommend refining the verbose option further. How about this?
This would really provide the most useful approach to showing overall status, and include verbose details when needed. |
Wont we still show the My future plans for this were as a follow-up: |
Yes, I would expect the ceph status to be shown always with
Yes, that's a nice simplification to show only a summary for the healthy resources. |
|
@AnikaYadav1 @parth-gr What about this, Let me know if the above is not clear. |
e92db75 to
8633c86
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AnikaYadav1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
8633c86 to
1a3e2ad
Compare
travisn
left a comment
There was a problem hiding this comment.
Per the discussion today, we will plan on printing all status info with the main odf get health command. If not verbose, we will just hide the details and items sections. After these updates, I'll plan on reviewing in more detail. Thanks!
| shell: bash | ||
| run: | | ||
| ./bin/odf --help | ||
| # CI deploys Rook only. Install a dummy StorageCluster API so |
There was a problem hiding this comment.
How about move this to the github-action-helper.sh? It helps keep this github action simpler to read.
|
Updated output after discussion: odf get health --verbose:
Detailed testing results : https://docs.google.com/document/d/1CIohaA1w7uHO5YxDGqFbISjwH60p_krfYkdteBaR7pw/edit?tab=t.wdon487g3fct |
6e374a6 to
9203eab
Compare
travisn
left a comment
There was a problem hiding this comment.
The new output looks good to me, where the verbose only adds the details and items, thanks!
| Object store ocs-storagecluster-cephobjectstore Ready Object store created successfully | ||
| Consumer internal Ready Last heartbeat from the client was 20s ago | ||
| SubVolumeGroup csi Ready Subvolume group created successfully | ||
| RadosNamespace ocs-storagecluster-cephblockpool-builtin-implicit Ready Rados namespace created successfully |
There was a problem hiding this comment.
Does the tool output have this alignment issue too? It appears we just need one more space for alignment.
There was a problem hiding this comment.
The tool output does not have the alignment issue (as we can see in the output screenshots).
It was misaligned in the docs/get.md file . I have corrected it .
9203eab to
2a4276c
Compare
2a4276c to
949aba1
Compare
Pinned kubectl-rook-ceph in go.mod to b2e68a2 (rook/kubectl-rook-ceph#483). odf get health now includes that change, so Details and Items print only with --verbose. ptal @travisn |
949aba1 to
f1c32ce
Compare
travisn
left a comment
There was a problem hiding this comment.
LGTM, just a few nits to simplify the output.
@subhamkrai PTAL
| - `odf get`: | ||
| - `recovery-profile`: Get the recovery profile value. | ||
| - `health`: Check health of the cluster and common configuration issues. | ||
| - `health`: Print StorageCluster-managed CR status, then the cluster health report. Use `--verbose` to include Details and Items. |
There was a problem hiding this comment.
| - `health`: Print StorageCluster-managed CR status, then the cluster health report. Use `--verbose` to include Details and Items. | |
| - `health`: Print StorageCluster-managed CR status, then the cluster health report. Use `--verbose` for more details. |
| var clusterHealth = &cobra.Command{ | ||
| Use: "health", | ||
| Short: "check health of the cluster and common configuration issues", | ||
| Short: "print ODF CR status and cluster health, or include Details and Items with --verbose", |
There was a problem hiding this comment.
No need tell them the implementation about where the report comes from
| Short: "print ODF CR status and cluster health, or include Details and Items with --verbose", | |
| Short: "print cluster health, and include more details with --verbose", |
|
|
||
| func init() { | ||
| clusterHealth.Flags().BoolVar(&healthVerbose, "verbose", false, "shows detailed check for pods") | ||
| clusterHealth.Flags().BoolVar(&healthVerbose, "verbose", false, "include Details and Items in the cluster health report") |
There was a problem hiding this comment.
| clusterHealth.Flags().BoolVar(&healthVerbose, "verbose", false, "include Details and Items in the cluster health report") | |
| clusterHealth.Flags().BoolVar(&healthVerbose, "verbose", false, "include more details in the cluster health report") |
| ## health | ||
|
|
||
| The health command checks the health of the Ceph cluster and common configuration issues. The health command validates these configurations: | ||
| `odf get health` prints the status of each StorageCluster and the CRs it manages, then the cluster health report. `odf get health --verbose` prints the same sections and includes Details and Items for each check. |
There was a problem hiding this comment.
| `odf get health` prints the status of each StorageCluster and the CRs it manages, then the cluster health report. `odf get health --verbose` prints the same sections and includes Details and Items for each check. | |
| `odf get health` prints the status of each StorageCluster and the CRs it manages, then the cluster health report. `odf get health --verbose` prints more details for each check. |
| Info: Checking if at least one mgr pod is running | ||
| rook-ceph-mgr-a-54f7dbddcd-h7j7m Running openshift-storage ip-10-0-64-239.us-west-1.compute.internal | ||
| ``` | ||
| Both commands print the CR status table first, then the cluster health report. `--verbose` adds Details and Items. |
There was a problem hiding this comment.
No need for this line
| Both commands print the CR status table first, then the cluster health report. `--verbose` adds Details and Items. |
ef388a9 to
a86dcf8
Compare
| namespace := "" | ||
| namespaceSet := false | ||
| if flagNamespace := cmd.Flag("namespace"); flagNamespace != nil && flagNamespace.Changed { | ||
| namespace = flagNamespace.Value.String() | ||
| namespaceSet = true | ||
| } |
There was a problem hiding this comment.
What are we trying to do here?
There was a problem hiding this comment.
-n always has a default (openshift-storage), so the flag value alone cannot tell us whether the user passed it. This block checks Flag.Changed and then uses that to decide how we list StorageClusters.
- namespace starts empty and namespaceSet starts false (user did not pass -n).
- We look up the global --namespace flag.
- If that flag exists and Changed is true, the user typed -n / --namespace. We store the value in namespace and set namespaceSet to true.
- If Changed is false (odf get health with no -n), we skip the block. PrintCRHealth then lists StorageClusters in all namespaces, for example both openshift-storage and openshift-storage-extended.
-If the user passes -n openshift-storage or -n openshift-storage-extended, namespaceSet is true. - PrintCRHealth lists only that namespace, and health.Health() uses it as the cluster namespace for the Ceph report.
can refer to output screenshots when -n flag is used
There was a problem hiding this comment.
If Changed is false (odf get health with no -n), we skip the block. PrintCRHealth then lists StorageClusters in all namespaces.
I was thinking to let's by default print for openshfit-storage namespace and if users passes other namespace or more than one than we can search all.
There was a problem hiding this comment.
Instead of the namespaceSet variable, could you just set namespace="" to indicate that the namespace is not set? Then you wouldn't need to pass around two variables everywhere for the namespace.
Another thought is that everything about the cluster health seems related to a single namespace. So perhaps we need a iterate through each namespace and print health reports separately. Then we just need to pass the specific namespace everywhere since the top-level entry method would decide which namespace is being analyzed.
There was a problem hiding this comment.
Dropped namespaceSet, an empty namespace means -n was not set and StorageClusters are listed in all namespaces.
odf get health now iterates through the namespaces and prints the health report separately. In case we specify a particular namespace ( e.g. odf get health -n openshift-storage) , only that cluster health report is printed .
testing screenshots -
https://docs.google.com/document/d/1CIohaA1w7uHO5YxDGqFbISjwH60p_krfYkdteBaR7pw/edit?tab=t.iadch5j96qay
Pinned kubectl-rook-ceph to the v0.10.0 release as well
@travisn @subhamkrai pal
| } | ||
|
|
||
| // listStorageClusters lists all StorageClusters present on the cluster. | ||
| // If namespaceSet is true, the list is limited to that namespace. |
There was a problem hiding this comment.
What is the default behaviour, are we searching all the namespace to find storageCluster or by default we are looking for openshift-storage namespace only?
There was a problem hiding this comment.
By default we search all namespaces, not only openshift-storage. odf get health with no -n lists every StorageCluster (for example both openshift-storage and openshift-storage-extended). If the user passes -n openshift-storage or -n openshift-storage-extended, we list only that namespace.
updated the comment as well
pls see the output screenshots
| if !spec.namespaced { | ||
| items = matchingStorageClients(items, sc.Name) |
There was a problem hiding this comment.
I'm not sure I understand this. Could you explain the goal here?
There was a problem hiding this comment.
StorageClient is cluster-scoped, unlike the other kinds, so we cannot list it by StorageCluster namespace. listKind returns every StorageClient on the cluster (usually only the internal one anyway) . Through this check we are just ensuring that we are keeping only the local Storage Client (the one whose name matches this StorageCluster)
| // nestedStringOrDash reads a nested string field (for example status.phase). | ||
| // If the field is missing, is not a string, or is empty, it returns "-". | ||
| func nestedStringOrDash(obj map[string]interface{}, fields ...string) string { | ||
| s, found, err := unstructured.NestedString(obj, fields...) |
There was a problem hiding this comment.
| s, found, err := unstructured.NestedString(obj, fields...) | |
| <meaningfull small name>, found, err := unstructured.NestedString(obj, fields...) |
instead of s, can we have a more meaningful var name
There was a problem hiding this comment.
Renamed s to fieldValue.
This helper reads a nested string from the CR at whatever path the caller passes, so it is used for both table columns. OBSERVED calls it with spec.observedPath. STATUS MESSAGE calls it with status.message.
| // "Last heartbeat from the client was 20s ago". | ||
| // Missing, empty, or unparsable timestamps return "-". | ||
| func lastHeartbeatMessage(obj map[string]interface{}, now time.Time) string { | ||
| s, found, err := unstructured.NestedString(obj, "status", "lastHeartbeat") |
There was a problem hiding this comment.
same here
| s, found, err := unstructured.NestedString(obj, "status", "lastHeartbeat") | |
| <meaningful name>, found, err := unstructured.NestedString(obj, "status", "lastHeartbeat") |
There was a problem hiding this comment.
Renamed s to lastHeartbeat. This is status.lastHeartbeat from the StorageConsumer
a86dcf8 to
80f435e
Compare
80f435e to
715344c
Compare
715344c to
11b1122
Compare
travisn
left a comment
There was a problem hiding this comment.
@AnikaYadav1 For final review, could you share the output again? Or is it about the same since posted last week?
| Use: "health", | ||
| Short: "check health of the cluster and common configuration issues", | ||
| Short: "print cluster health, and include more details with --verbose", | ||
| Long: `Print the status of each StorageCluster and the CRs it manages, then the cluster health report (Ceph MON, MGR, and OSD checks, pod status, placement groups, and object storage). --verbose includes Details and Items for each check.`, |
There was a problem hiding this comment.
--verbose is obvious, let's simplify this a bit:
| Long: `Print the status of each StorageCluster and the CRs it manages, then the cluster health report (Ceph MON, MGR, and OSD checks, pod status, placement groups, and object storage). --verbose includes Details and Items for each check.`, | |
| Long: `Print the status of each StorageCluster and the CRs it manages, then the cluster health report (Ceph MON, MGR, and OSD checks, pod status, placement groups, and object storage).`, |
The output screenshots are here |
d014ede to
e685b0a
Compare
Add PrintCRHealth, which reports the status of each StorageCluster and the custom resources it manages. Each StorageCluster gets a header with its basic details, followed by a table showing the state of its managed resources. Signed-off-by: AnikaYadav1 <Anika.Yadav1@ibm.com>
odf get health prints the CR status table, then the cluster health report.odf get health --verbose prints the same output with more details. Passing -n limits the table to that namespace. Without -n the command searches all namespaces. Signed-off-by: AnikaYadav1 <Anika.Yadav1@ibm.com>
The Go Test workflow deploys Rook only, so listing StorageClusters fails without the CRD. Apply a minimal StorageCluster CRD and CR before odf get health. Other smoke tests are unchanged. Signed-off-by: AnikaYadav1 <Anika.Yadav1@ibm.com>
Bump github.com/rook/kubectl-rook-ceph to v0.10.0. odf get health then prints Details and Items only with --verbose. Signed-off-by: AnikaYadav1 <Anika.Yadav1@ibm.com>
e685b0a to
dee9fda
Compare













RHSTOR-9956
RHSTOR-9957
Add CR health table to odf get health
Add PrintCRHealth, which reports the status of each StorageCluster
and the custom resources it manages, and wire it into
odf get health so the table is printed by default.
Each StorageCluster gets a header with its basic details, followed
by a table showing the state of its managed resources.
The header shows:
StorageCluster: name of the StorageCluster
Namespace: namespace it runs in
Phase: current phase from its status
Mode: internal or external
The table has these columns:
COMPONENT: kind of managed resource (for example
CephCluster, Block pool, NooBaa)
NAME: name of the resource
OBSERVED: current state reported by the resource
STATUS MESSAGE: message explaining that state, if any
Fields that are missing or empty are shown as "-".
Changes to odf get health:
odf get health prints the CR health table, then the cluster health
report.
odf get health --verbose prints the same output with more details.
-n limits the table to that namespace.
Without it, all namespaces are searched.