-
Notifications
You must be signed in to change notification settings - Fork 218
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
Add health and availableNodes in the OpenSearchCluster status #655
Add health and availableNodes in the OpenSearchCluster status #655
Conversation
Signed-off-by: saketmht <[email protected]>
func (r *ClusterReconciler) UpdateClusterHealth() error { | ||
health := util.GetClusterHealth(r.ctx, r.Client, r.instance) | ||
|
||
err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
if err := r.Get(r.ctx, client.ObjectKeyFromObject(r.instance), r.instance); err != nil { | ||
return err | ||
} | ||
r.instance.Status.Health = health | ||
return r.Status().Update(r.ctx, r.instance) | ||
}) | ||
|
||
return err | ||
} | ||
|
||
func (r *ClusterReconciler) UpdateAvailableNodes() error { | ||
availableNodes := util.GetAvailableOpenSearchNodes(r.ctx, r.Client, r.instance) | ||
|
||
err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
if err := r.Get(r.ctx, client.ObjectKeyFromObject(r.instance), r.instance); err != nil { | ||
return err | ||
} | ||
r.instance.Status.AvailableNodes = availableNodes | ||
return r.Status().Update(r.ctx, r.instance) | ||
}) | ||
|
||
return err | ||
} |
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.
I'd prefer to have both status fields updated in one go, to reduce the number of API interactions.
osClient, err := CreateClientForCluster(ctx, k8sClient, cluster, nil) | ||
|
||
if err != nil { | ||
return opsterv1.OpenSearchUnknownHealth |
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.
The error should be logged at least, for debugging purposes. For the other places also.
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.
I didn't want to log it because when the cluster is first coming up, it will always fail to create the client and the logs are filled with error messages. Same at other places as well, when the sts is just being created, it returns a few not found error
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.
Maybe log it on debug level, then it should normally not appear as the default log level is info. And in case someone has a problem we can ask for debug logs.
// Update the cluster health and available nodes in the status | ||
result.CombineErr(r.UpdateClusterHealth()) | ||
result.CombineErr(r.UpdateAvailableNodes()) |
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.
I think this should happen as the last point in the reconcile function, after everything else
Signed-off-by: saketmht <[email protected]>
@swoehrl-mw Updated based on comments |
Feat #553
Changes -
Health
andNODES
field in OpensearchCluster Status to indicate cluster health and available nodes respectively.Example Output