nodehealthcheck_ongoing_remediation metric stuck at 1 after remediation completes due to mismatched label sets

[NyxTrail]

Hi,
Looks like when a node is being remediated, the nodehealthcheck_ongoing_remediation metric uses a label set with name=< node-name >:
https://github.com/medik8s/node-healthcheck-operator/blob/main/controllers/nodehealthcheck_controller.go#L618

And once the remediation is completed, the metric uses a label set with name=< remediation-cr-name >:
https://github.com/medik8s/node-healthcheck-operator/blob/main/controllers/resources/status.go#L66

This results in the metrics appearing as shown below:

In the image, the yellow line (value=0) has name set to remdiation CR name and the blue line (value=1, stuck) has name set to node name.

That is, the metric for the node name is permanently stuck at 1, incorrectly indicating an ongoing remediation.

Steps to reproduce

1. Induce a node to be unhealthy (I usually stop kubelet)
2. Wait for the node to be remediated
3. Observe the metrics.

Sample raw metrics:

      {
        "metric": {
          "__name__": "nodehealthcheck_ongoing_remediation",
          "container": "kube-rbac-proxy",
          "endpoint": "https",
          "exported_instance": "x.x.x.x:yy",
          "exported_job": "node-healthcheck-controller-manager-metrics-service",
          "exported_namespace": "openshift-workload-availability",
          "instance": "my-redacted-instance:my-port",
          "job": "prometheus-federate-job",
          "name": "<node-name>",    // This is the node name
          "namespace": "openshift-workload-availability",
          "pod": "node-healthcheck-controller-manager-7f6475d7d4-zvrxw",
          "prometheus": "openshift-monitoring/k8s",
          "prometheus_replica": "prometheus-k8s-0",
          "remediation": "SelfNodeRemediation",
          "service": "node-healthcheck-controller-manager-metrics-service"
        },
        "value": [
          1771598863.253,
          "1"   // Value is "1" for Node name
        ]
      },
      {
        "metric": {
          "__name__": "nodehealthcheck_ongoing_remediation",
          "container": "kube-rbac-proxy",
          "endpoint": "https",
          "exported_instance": "x.x.x.x:yy",
          "exported_job": "node-healthcheck-controller-manager-metrics-service",
          "exported_namespace": "openshift-workload-availability",
          "instance": "my-redacted-instance:my-port",
          "job": "prometheus-federate",
          "name": "<node-name>-mh7vw",   // This is the remediation CR
          "namespace": "openshift-workload-availability",
          "pod": "node-healthcheck-controller-manager-7f6475d7d4-zvrxw",
          "prometheus": "openshift-monitoring/k8s",
          "prometheus_replica": "prometheus-k8s-0",
          "remediation": "SelfNodeRemediation",
          "service": "node-healthcheck-controller-manager-metrics-service"
        },
        "value": [
          1771598863.253,
          "0"  // Value is "0" for remediation CR
        ]
      }

Looks like the issue was introduced in this PR: #231 , when

metrics.ObserveNodeHealthCheckRemediationDeleted(node.GetName(), remediationCR.GetNamespace(), remediationCR.GetKind())

was moved to deleteRemediationCR method:

metrics.ObserveNodeHealthCheckRemediationDeleted(remediationCR.GetName(), remediationCR.GetNamespace(), remediationCR.GetKind())

[mshitrit]

Hi @NyxTrail , thanks for reaching out and for the analysis !
Looks like a nice catch.

I'll tag our QE team to reproduce and create a Jira for that.
Otherwise feel free to create a PR fix if you like.

[mshitrit]

Jira created at: RHWA-751

[NyxTrail]

Hi @mshitrit ,
Thank you for the acknowledgement and creating the Jira. I am not very fluent in golang, that's why I didn't create a PR.
I shall do so, if time provides. But please don't wait on me.

[slintes]

while there is a mismatch between node name / CR name obviously, we need to carefully check which one we want to track actually...

[clobrano]

Most of the times we use CR's name

// CR Name
./controllers/resources/status.go:66:               metrics.ObserveNodeHealthCheckRemediationDeleted(remediationResource.Name, remediationResource.Namespace, remediationResource.Kind)
// CR Name
./controllers/resources/status.go:67:               metrics.ObserveNodeHealthCheckUnhealthyNodeDuration(remediationResource.Name, remediationResource.Namespace, remediationResource.Kind, duration)
// CR Name
./controllers/machinehealthcheck_controller.go:157: metrics.ObserveMachineHealthCheckShortCircuitDisabled(req.NamespacedName.Name, req.NamespacedName.Namespace)
// CR Name
./controllers/machinehealthcheck_controller.go:207: metrics.ObserveMachineHealthCheckNodesCovered(mhc.Name, mhc.Namespace, totalTargets)
// CR Name
./controllers/machinehealthcheck_controller.go:244: metrics.ObserveMachineHealthCheckShortCircuitEnabled(mhc.Name, mhc.Namespace)
// CR Name
./controllers/machinehealthcheck_controller.go:266: metrics.ObserveMachineHealthCheckShortCircuitDisabled(mhc.Name, mhc.Namespace)
// Node name
./controllers/nodehealthcheck_controller.go:393:    metrics.ObserveNodeHealthCheckOldRemediationCR(node.Name, node.Namespace)
// Node name
./controllers/nodehealthcheck_controller.go:618:    metrics.ObserveNodeHealthCheckRemediationCreated(node.GetName(), remediationCR.GetNamespace(), remediationCR.GetKind())

[NyxTrail]

imo, the CR names are too granular and it is difficult to perform promQL queries with any kind of useful aggregate/range functions against these metrics.

[NyxTrail]

I have been looking into the nodehealthcheck_unhealthy_node_duration_seconds histogram metric, trying to create grafana dashboards for average remediation duration, remediation duration distribution graph over a 24h period etc.

I would love to see the change in nodehealthcheck_unhealthy_node_duration_seconds_sum, nodehealthcheck_unhealthy_node_duration_seconds_count metrics to do these calculations. Currently these metrics produce a constant value once the remediation has been completed, and the next update to the metric goes to a different CR name.

Am I thinking about this right?

[NyxTrail]

For now, I am going ahead with creating generic panels in our dashboard for NHC by omitting the CR name in calculations. Still, it would be good to be able to track remediations for a node across multiple cycles of remediation.

[NyxTrail]

Hi @mshitrit , @slintes , @clobrano

Will you be looking into the unhealthy_node_duration_seconds metrics as well, as part of this issue? Since it is using CR name as a label, we cannot use this metric as a proper histogram.

Let me know if I should open a separate issue for this topic.

[clobrano]

Hi @NyxTrail,
we can take care of unhealthy_node_duration_seconds in the same fix. Thank you for reporting this as well