[BUG] bootstrap-only: grandfathering marks bootstrap completed concurrently with taint addition, permanently orphaning the taint

[patrickshan]

What happened?

Rolling out the controller (v0.4.1) plus a condition-reporting DaemonSet
(node-problem-detector with custom conditions) simultaneously to a cluster of
long-lived, healthy nodes left one node with a permanently orphaned
readiness.k8s.io/newlyjoinednode:NoSchedule taint.

Two controllers raced on the same node within the same second, on the controller's
first-ever reconcile sweep:

1. The rule controller evaluated the node before the reporter DaemonSet had
   stamped any of the rule's conditions, so every condition evaluated as
   observed: "Unknown" vs required: "False" → allConditionsSatisfied: false
   → it added the taint.
2. The node controller concurrently ran the grandfathering path for the
   pre-existing node and marked bootstrap completed
   (readiness.k8s.io/bootstrap-completed-<rule-uid> annotation written).
3. Every subsequent reconcile of that node now logs
   Skipping bootstrap-only rule - already completed, which short-circuits the
   entire rule — including taint removal.

End state (still true hours later): node is healthy, all rule conditions read
False (satisfied), the bootstrap-completed annotation is present, and the
rule's NoSchedule taint is still on the node. Nothing will ever remove it; it
had to be removed manually.

Other nodes tainted in the same initial sweep were cleaned up correctly — they
won the race (taint removed before the completion mark landed). Only the
interleaving order differed.

Steps to Reproduce

1. Cluster with existing Ready worker nodes that do not yet carry the
   rule's condition types (reporter not yet deployed).

2. Deploy, at effectively the same time, a bootstrap-only NodeReadinessRule
   and the DaemonSet that reports its conditions:

   apiVersion: readiness.node.x-k8s.io/v1alpha1
   kind: NodeReadinessRule
   metadata:
     name: newly-joined-node-rule
   spec:
     enforcementMode: bootstrap-only
     conditions:
     - type: CustomCheckA
       requiredStatus: "False"
     - type: CustomCheckB
       requiredStatus: "False"
     nodeSelector:
       matchExpressions:
       - key: node-role.kubernetes.io/control-plane
         operator: DoesNotExist
     taint:
       key: readiness.k8s.io/newlyjoinednode
       value: "true"
       effect: NoSchedule
   

3. On the controller's first sweep, nodes evaluate Unknown on all conditions
   and get tainted, while the node controller concurrently grandfathers them as
   bootstrap-completed. Depending on interleaving, some nodes end up completed
   with the taint still present, and it is never removed.

The race is timing-dependent; with ~20 matching nodes, one node hit it.

Log evidence (sanitized hostnames, otherwise verbatim)

07:43:10Z DEBUG Condition evaluation ... "node": "node-01", "conditionType": "CustomCheckA", "observed": "Unknown", "effective": "Unknown", "required": "False", "satisfied": false, "reconcileID": "4593ceed-..."
07:43:10Z INFO Evaluation result ... "node": "node-01", "allConditionsSatisfied": false, "hasTaint": false, "reconcileID": "4593ceed-..."
07:43:10Z INFO Adding taint ... "node": "node-01", "taint": "readiness.k8s.icileID": "4593ceed-..." <- rule controller
07:43:10Z INFO Marked bootstrap completed ... "controllerKind": "Node", "node": "node-01", "reconcileID": "bd456526-..." <- node controller, same second
07:43:10Z INFO Skipping bootstrap-only rule - already completed ... "node": "node-01"-..." <- all later reconciles

Node state hours later: conditions all False (healthy, heartbeats current),
readiness.k8s.io/bootstrap-completed- annotation present, taint
readiness.k8s.io/newlyjoinednode:NoSchedule still present.

Expected Behavior

Either of these invariants would prevent the orphaned taint:

• markBootstrapCompleted removes (or verifies absence of) any taint owned by
  the rule before/atomically with writing the completion annotation, or
• the skip-if-completed branch first checks whether the rule's taint is still
  present on the node and lifts it before skipping.

Related: #207 notes that completion-marking is intended to happen during taint
removal. This report is the mirror image — the grandfathering path marks
completion via the node controller while the rule controller's taint from a
concurrent reconcile is still in flight, so the "completed" state and the taint
coexist.

Controller Version / Image Tag

v0.4.1

Kubernetes Version

v1.34.x

Controller Logs

No response

Additional Environment Details

No response

[DsThakurRawat]

Reproduced the absorbing half of this on main at d74dabd: once a node carries both the rule's taint and the bootstrap-completed-<uid> annotation, no code path ever removes the taint. I tested this against a bootstrap-only rule where the node has the taint, the completion annotation, and all conditions satisfied; running processNodeAgainstAllRules leaves the taint intact. The node path short-circuits at the completion check in processNodeAgainstAllRules (node_controller.go:137) before any taint logic runs, so your "still true hours later" observation is structural, not a delay.

On how the state arises: the taint switch in evaluateRuleForNode is shared by both reconcilers, and both can reach markBootstrapCompleted (nodereadinessrule_controller.go:415 after a taint removal, :458 on the no-action path). The completion annotation is written with a conflict-retrying patch, but nothing ties it to the taint state that reconcile assumed. So one reconcile can conclude "satisfied, no taint, nothing to do" and mark completion, while the other, working from a read taken moments earlier with conditions still Unknown, adds the taint. Both writes succeed. That matches your two reconcileIDs landing in the same second, and it also matches only some nodes hitting it: whichever ordering the API server happens to serialize decides the outcome per node.

Either invariant you proposed would close it. The narrower fix is to make markBootstrapCompleted refuse (or defer) when the rule's taint is currently on the node, since bootstrap being "complete" while the bootstrap taint is still present is exactly the contradiction; the node path would then clean up on its next reconcile. That's one conditional in the existing retry loop rather than a new coordination mechanism. Can send a PR for whichever direction maintainers prefer, or leave it with this analysis if someone else is picking it up.

[ajaysundark]

Thanks for reporting this issue. I see a possible reason from code as both the controllers are trying to act on a same node, but acted from different node states.

(1) rule-reconciliation started processing the taint addition as the condition was not originally satisfied
(2) node-reconciliation (triggered by the required-condition update that landed after the node is queued up from 1) sets the annotation
(3) 2 does not check resource version (not locked), so both the updates go through.

Currently only taint updates are locked optimistically for concurrency control. we had a discussion in #343 as annotation being atomic doesnt need it.

Looking at
https://github.com/kubernetes-sigs/node-readiness-controller/blob/main/internal/controller/nodereadinessrule_controller.go#L401-L431

would remove-taint and annotation as a single 'transaction' patch handle this case?

I think in retrospect, a single writer would have been better for the node updates: node-reconciler only handling the node updates, and rule-reconciliation queueing up the nodes to reconcile instead of processAllNodesForRule in parallel, but may need a larger refactor.

cc @Karthik-K-N

[ajaysundark]

/assign @ajaysundark

I'm looking into the fix.

@vitorfloriano for vis. Tagging this as priority urgent as it is a bug-fix that could go into 0.5.0

[Karthik-K-N]

I think in retrospect, a single writer would have been better for the node updates: node-reconciler only handling the node updates, and rule-reconciliation queueing up the nodes to reconcile instead of processAllNodesForRule in parallel, but may need a larger refactor.

Yes, I can't agree more on this point, This was my point from long time and this is the idea behind me proposing single controller but which got ruled out because of scale concerns, How I envision various controllers are

1. Node controller should take care of taint addition and removal, any change to the Rule we should enqueue request for node-controller
2. Rule controller should only take care of updating its summary/aggrigated status
3. NRE should take care of pernode rule results.

Just updated and rebased old half baked PoC - #389

[ajaysundark]

I dont have the short-term mitigation fix ready for this yet. We are discussing risk of carrying this bug into 0.5.0, or wait the release until we have the patch ready..

cc @vitorfloriano @Karthik-K-N

[DsThakurRawat]

the release-vs-patch question is the awkward part here, so: i have a mitigation working locally if it's any use to you. it's yours to take, ignore, or rewrite, i'm not picking the issue up.

the thing that made it fixable in a small diff is the bit you already called out, that the annotation write isn't resource-version checked. markBootstrapCompleted does re-read the node inside its RetryOnConflict loop, but it patches with a plain client.MergeFrom, so whatever it decided from that read is still applied even if the taint landed in between. the #343 reasoning that an annotation write is atomic is right, it's just that the thing needing to be atomic here is check-then-write, not the write.

three changes, and the third one isn't optional:

• annotation patch switches to MergeFromWithOptimisticLock, same as addTaintBySpec and removeTaintBySpec already use on that object.
• inside the same retry loop, refuse to mark when the freshly read node still carries the rule's taint. refusing is self-healing rather than terminal: the node keeps taint-and-no-annotation, which is exactly the state the node path still acts on, so the next reconcile removes the taint and marks completion then.
• symmetrically, addTaintBySpec refuses to taint a node whose bootstrap this rule already completed. without this one the mark-then-add ordering still gets you an orphan, since processNodeAgainstAllRules short-circuits at the completion check on line 137 before any taint logic runs.

on the cost: putting a lock on the annotation patch does mean it now conflicts against unrelated node writes, and kubelet updates node status constantly, so that patch will retry more often than it used to. retry.DefaultRetry absorbed it in every run i did, but on a large cluster it's a real change in write pressure and you may not want it in a patch release.

for the test, the race is deterministic with an interceptor on Get: hand markBootstrapCompleted a taint-free read, then land the taint before it patches. reverting the three guards and keeping the test, that spec fails with Marked bootstrap completed in the log and both the taint and the annotation on the node, which is the state from your original report. with the guards it passes, and the full suite plus both linters are green on the same tree.

happy to push it as a PR if you want it, or just paste the diff here, or drop it entirely if you'd rather go straight to the single-writer split in #389. it's a stopgap, not an argument against that refactor.