fix: record conflict failures and fix missing status patch metric by rawadhossain · Pull Request #320 · kubernetes-sigs/node-readiness-controller

rawadhossain · 2026-07-22T19:23:00Z

Description

This change improves failure observability by recording conflict-specific failure reasons in node_readiness_failures_total, fixing a missing failure metric in the node reconciler, and adding logs where API conflicts can actually occur.

Previously, conflict exhausted taint operations were recorded as generic taint errors, so it wasn't possible to distinguish them from other patch errors. The rule status patch error path in the node reconciler also logged failures without recording a failure metric.

To fix this:

add conflict-specific failure reasons:
- AddTaintConflictExhausted
- RemoveTaintConflictExhausted
- StatusPatchConflictExhausted
record status patch failures in the node reconciler
add structured V(1) conflict logs with rule and operation at addTaintBySpec and removeTaint
update monitoring.md with the new reason label values

Related to Issue #182

Type of Change

/kind feature

Checklist

make test passes
make lint passes

kubernetes-prow · 2026-07-22T19:23:03Z

@rawadhossain: The label(s) kind/improvement cannot be applied, because the repository doesn't have them.

Details

In response to this:

Description

This change improves failure observability by recording conflict-specific failure reasons in node_readiness_failures_total and fixing a missing failure metric in the node reconciler.

Previously, conflict exhausted taint operations were recorded as generic taint errors, so it wasn't possible to distinguish them from other patch errors. The rule status patch error path in the node reconciler also logged failures without recording a failure metric.

To fix this:

add conflict-specific failure reasons:

AddTaintConflictExhausted

RemoveTaintConflictExhausted

StatusPatchConflictExhausted

record status patch failures in the node reconciler

add a structured operation field to evaluateRuleForNode error logs

update monitoring.md with the new reason label values

Related to Issue #182

Type of Change

/kind improvement

Checklist

make test passes

make lint passes

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.

kubernetes-prow · 2026-07-22T19:23:07Z

[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

netlify · 2026-07-22T19:23:07Z

✅ Deploy Preview for node-readiness-controller ready!

Name	Link
🔨 Latest commit	`f04128c`
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a89f347acb792000898f316
😎 Deploy Preview	https://deploy-preview-320--node-readiness-controller.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

kubernetes-prow · 2026-07-22T19:23:10Z

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

left my initial thoughts here!

AvineshTripathi · 2026-07-23T18:27:38Z

 			"ruleResourceVersion", rule.ResourceVersion)

 		if err := r.evaluateRuleForNode(ctx, rule, node); err != nil {
+			operation := "unknown"


can we make a util function here?

AvineshTripathi · 2026-07-23T18:29:53Z

+			case errors.Is(err, errRemoveTaintFailed):
+				operation = "remove_taint"
+			}
 			log.Error(err, "Failed to evaluate rule for node",


I don't think adding operations would add value. We already are logging the error. If the idea here is to standardise error messaging, then that is a whole different discussion.

AvineshTripathi · 2026-07-23T18:32:42Z

+				case errors.Is(err, errRemoveTaintFailed):
+					operation = "remove_taint"
+				}
+				log.Error(err, "Failed to evaluate node for rule", "rule", rule.Name, "node", node.Name, "operation", operation)


same as above. we already log errors.

AvineshTripathi · 2026-07-23T18:35:27Z

 	log := ctrl.LoggerFrom(ctx)

-	var errors []string
+	var cleanupErrors []string


any particular reason to change this?

AvineshTripathi · 2026-07-23T18:38:19Z

cc @Priyankasaggu11929 I remember you had some thoughts on this

Signed-off-by: Rawad Hossain <rawad.hossain00@gmail.com>

rawadhossain · 2026-08-22T19:30:17Z

Hi @AvineshTripathi
gave this a fresh look. Actually built api_conflicts_total locally and live tested it to see if it was worth keeping. Most of what it would show either duplicates the ConflictExhausted reasons already in this PR, or just stay at zero where conflicts don't happen.
So I reworked this PR to go with the logs approach instead, which also addresses your earlier comments. PTAL.

ajaysundark · 2026-08-22T19:58:01Z

The largest source of api conflicts (409s) don't rise up because they are handled as merge patch. We should check this after #343 is merged, only after when status conflicts will 'surface'

Deferred to kubernetes-sigs#320/kubernetes-sigs#288 per review feedback so conflict metrics get handled holistically instead of piecemeal here.

kubernetes-prow Bot requested review from dchen1107 and mrunalp July 22, 2026 19:23

kubernetes-prow Bot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 22, 2026

kubernetes-prow Bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 22, 2026

rawadhossain mentioned this pull request Jul 22, 2026

feat: Add reconciliation retry observability metrics #288

Draft

2 tasks

AvineshTripathi reviewed Jul 23, 2026

View reviewed changes

kubernetes-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2026

AvineshTripathi mentioned this pull request Aug 6, 2026

docs: add observability design #344

Open

2 tasks

kubernetes-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 22, 2026

rawadhossain added 2 commits August 23, 2026 00:50

Improve failure metrics

f1857cc

Signed-off-by: Rawad Hossain <rawad.hossain00@gmail.com>

record errors and logs

f04128c

rawadhossain force-pushed the fix/failures-metric-conflict-exhaustion branch from 9d27a99 to f04128c Compare August 22, 2026 19:06

kubernetes-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2026

ajaysundark mentioned this pull request Aug 22, 2026

fix: use optimistic locking on rule status patches #343

Open

2 tasks

rawadhossain mentioned this pull request Aug 23, 2026

REQUEST: New membership for rawadhossain kubernetes/org#6576

Closed

11 tasks

Uh oh!

Conversation

rawadhossain commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related to Issue #182

Type of Change

Checklist

Uh oh!

kubernetes-prow Bot commented Jul 22, 2026

Description

Related to Issue #182

Type of Change

Checklist

Uh oh!

kubernetes-prow Bot commented Jul 22, 2026

Uh oh!

netlify Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for node-readiness-controller ready!

Uh oh!

kubernetes-prow Bot commented Jul 22, 2026

Uh oh!

AvineshTripathi left a comment

Choose a reason for hiding this comment

Uh oh!

AvineshTripathi Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

AvineshTripathi Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

AvineshTripathi Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

AvineshTripathi Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

AvineshTripathi commented Jul 23, 2026

Uh oh!

rawadhossain commented Aug 22, 2026

Uh oh!

ajaysundark commented Aug 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rawadhossain commented Jul 22, 2026 •

edited

Loading

netlify Bot commented Jul 22, 2026 •

edited

Loading