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

Handle more resource kind health status #418

Open
undera opened this issue Jul 3, 2023 · 11 comments
Open

Handle more resource kind health status #418

undera opened this issue Jul 3, 2023 · 11 comments
Labels
good first issue Good for newcomers

Comments

@undera
Copy link
Collaborator

undera commented Jul 3, 2023

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

  • ExternalSecret
  • Job
  • CustomResourceDefinition
  • PodDisruptionBudget
  • HorizontalPodAutoscaler
  • Namespace

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

@undera undera added the good first issue Good for newcomers label Jul 3, 2023
@iamvikaskumar
Copy link
Contributor

Hi,
I would be happy to work on this.

@undera
Copy link
Collaborator Author

undera commented Jul 25, 2023

Feel free to bring PR @iamvikaskumar

@iamvikaskumar
Copy link
Contributor

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
Copy link
Contributor

Please let me know if I got it wrong ?

@undera
Copy link
Collaborator Author

undera commented Jul 27, 2023

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

@iamvikaskumar
Copy link
Contributor

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
Copy link
Collaborator Author

undera commented Jul 27, 2023

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
Copy link
Contributor

alessandrodetta commented Mar 1, 2024

@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
Copy link
Collaborator Author

undera commented Mar 7, 2024

@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
Copy link

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

@undera
Copy link
Collaborator Author

undera commented Oct 3, 2024

@visionaryfire I believe so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants