komodorio / helm-dashboard

The missing UI for Helm - visualize your releases
Apache License 2.0
4.94k stars 300 forks source link

Handle more resource kind health status #418

Open undera opened 1 year ago

undera commented 1 year ago

In backend call like /api/helm/releases/:ns/:name/resources?health=true, we need to support more resource kinds:

And generally we need to handle unknown object kinds to at least show "Exists" status.

iamvikaskumar commented 1 year ago

Hi, I would be happy to work on this.

undera commented 1 year ago

Feel free to bring PR @iamvikaskumar

iamvikaskumar commented 1 year ago

Thanks @undera . Could you please provide some more details about the issue. My understanding is as below, please let me know if I got it rite : The issue is, that for the above mentioned resource kinds, we are not calculating hdHealth ? Below is an example of a CRD where status is unknown.

image
iamvikaskumar commented 1 year ago

Please let me know if I got it wrong ?

undera commented 1 year ago

Yes, your understanding is correct. We want to have more meaningful hdHealth for each resource, at least checking that it exists.

iamvikaskumar commented 1 year ago

Thanks for your reply @undera . I have one more doubt before I raise the PR... when a resource have more than one condition e.g. in case of CRD (refer the image below), do we mark hdHealth as healthy only if all the conditions has status as "True" ?

image
undera commented 1 year ago

There is no strict rule to set hdHealth based on conditions. Conditions in k8s are too broad in their meaning. So to calculate our hdHealth helper, we have roughly following rules:

  1. If resource does not exist in cluster - it's definitely "NotFound" and unhealthy.
  2. If we have a dedicated custom code for that resource kind - we try to calculate "progressing" yellow status, or more meaningful "Unhealthy/Healthy" status.
  3. Worst case, we fall back to the "Exists/NotFound" logic.

So for your example of CRD above, the custom logic may be to have both conditions True. But that is only for CustomResourceDefinition, and not general rule.

alessandrodetta commented 7 months ago

@undera I tried deleting a resource with kubectl delete namespace|deployment <name>, to see what kind of status could I get for it but I didn't expect to get the unknown "ErrorGettingStatus" status, but unhealthy "NotFound" as you mentioned in your last answer.

image

Should we change the code to broadly follow something like the idea presented below? Or there is already some lines of code that actually checks for "NotFound", which I missed?

...
// custom logic to provide most meaningful status for the resource
    if err != nil {
        if errors.IsNotFound(err) {
            c.Status = Unhealthy
            c.Reason = "NotFound"
            c.Message = err.Error()
        } else {
            c.Status = Unknown //Or Unhealthy anyway? 
            c.Reason = "ErrorGettingStatus"
            c.Message = err.Error()
        }
        ...

After the change, this will be the dashboard output

image

undera commented 7 months ago

@alessandrodetta Your proposition is correct. If we know the resource does not exist - we should set status to that, it's better than generic failure

visionaryfire commented 1 week ago

@undera Is this issue still valid one for the contribution?

undera commented 1 week ago

@visionaryfire I believe so