feat: Add reconciliation retry observability metrics

[rawadhossain]

Description

This PR adds two new metrics

What each metric does

node_readiness_api_conflicts_total

Tracks how many API conflicts occur during controller operations. Previously, only conflicts that exhausted all retries were visible through node_readiness_failures_total. If a retry succeeded, there was no signal that a conflict had happened. This metric makes those retries visible and helps identify API server contention earlier.

node_readiness_reconcile_requeue_total

Tracks why a rule reconciliation is requeued. Right now, we know a rule is being requeued, but we don't know which stage caused it. This metric records the reason for the requeue, making it easier to identify where reconciliation is failing.

The metric is labeled by reason, currently covering:

• status_update_error
• cleanup_error
• taint_cleanup_error

Related to Issue #182

Type of Change

/kind feature

Testing

• Added tests coverages.

Checklist

• make test passes
• make lint passes

[netlify]

✅ Deploy Preview for node-readiness-controller canceled.

Name	Link
🔨 Latest commit	326cfa5
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a47d485a60e0400086287b5

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rawadhossain
Once this PR has been reviewed and has the lgtm label, please assign mrunalp for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kubernetes-prow]

Hi @rawadhossain. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[AvineshTripathi]

/assign

[AvineshTripathi]

I did some initial review and I had some questions:

• Are we planning to visualize these metrics? If so, then how?
• What type of answer will an operator get from these? I feel these are metrics for the controller itself, which will tell how controller is behaving in concurrent env

[AvineshTripathi]

there are other places too in Reconcile where we requeue for e.g. https://github.com/kubernetes-sigs/node-readiness-controller/blob/main/internal/controller/nodereadinessrule_controller.go#L151

maybe a defer function would make more sense here. Thoughts?

[rawadhossain]

I looked into using a defer, but I don't think it simplifies things much. We'd still need to set the reason at each requeue site so the deferred function knows which label to increment. It would also require named returns, handling early exits before rule is available, and a separate defer in reconcileDelete() for taint_cleanup_error, so I'm not sure if a defer would help much here.

For the line you linked (L151), we actually left that one out on purpose, processAllNodesForRule() always returns nil, so that error path is never reached. But updateRuleStatus(), can actually fail and return a real error, so I felt that was worth instrumenting.

[AvineshTripathi]

I agree but in past we have add a discussion to allow processAllNodesForRule to return error. Also there are other places too like https://github.com/kubernetes-sigs/node-readiness-controller/blob/main/internal/controller/nodereadinessrule_controller.go#L122

[rawadhossain]

I did some initial review and I had some questions:

• Are we planning to visualize these metrics? If so, then how?
• What type of answer will an operator get from these? I feel these are metrics for the controller itself, which will tell how controller is behaving in concurrent env

That's a fair point. Both metrics are meant to land in Grafana as alert companions along with the existing SLOs.

It may feel a bit controller centric, but here's how I was looking at it.

api_conflicts_total, gives the operator an early signal that the controller is starting to experience API server write contention. Right now we only know about it once retries are exhausted and an operation fails. This metric makes those retries visible earlier. To me, that felt useful as the operator gets a chance to notice the issue before it starts affecting the controller.

For reconcile_requeue_total, I think you're right. It mainly tells us which stage inside the reconciler is causing the requeue, which is probably more useful for controller than operator. I'm okay dropping this metric from the PR and revisit later if we feel there's any need.

[AvineshTripathi]

I dont mind having these metrics but not sure if they should go on any dashboards. wdyt @ajaysundark

[rawadhossain]

Hey @AvineshTripathi @ajaysundark, just to close the loop on this
api_conflicts_total does have a dashboard panel (API Conflict Rate by Operation) in the Rule Bottleneck Ranking section. The idea is to make API conflicts visible before they become a problem.

I'm okay to drop reconcile_requeue_total from this PR though and revisit it later if there's a stronger operator use case.

[AvineshTripathi]

From the last meeting, did we see the possibility of api_error metrics going as a log and not metrics? Intuitively I too feel being a log makes more sense

[rawadhossain]

Hi @AvineshTripathi, you're right and I dug into this properly after the meeting discussion.

I also looked into moving it fully to logs, but the project has no log aggregation setup.
So went with a different approach, I updated the existing node_readiness_failures_total metric with conflict specific reason labels. While doing this, I also found and fixed a missing failure metric in the status patch path.

I've raised PR #320 with this approach, so I'll close this PR.

[ajaysundark]

Controller-runtime already exposes requeue metrics: https://github.com/kubernetes-sigs/controller-runtime/blob/6ec95076a056c7a36c0d8acc8316da796426df7e/pkg/internal/controller/metrics/metrics.go#L39. We dont need to duplicate them.