[BUG]: TrackReboots writes every NodeWright's status each reconcile, causing conflict storms that abort Reconcile before node selection

[ayuskauskas]

Skyhook Version

Found on operator v0.15.0; still present on main (f4eb0582) — code paths verified unchanged.

Installation method

Helm

Kubernetes Version

v1.34.10

Component

Operator (controller-manager)

Describe the bug

TrackReboots writes the status of every NodeWright in the cluster state on every reconcile of any NodeWright, using a full optimistic-locked Status().Update(). With more than one NodeWright CR, node churn makes those writes contend with each other, and a conflict aborts the entire Reconcile before any node work happens.

Three pieces combine (line numbers from main, operator/internal/controller/skyhook_controller.go):

1. TrackReboots (:1211) iterates all NodeWrights (:1216) and writes each one whose status changed (:1283):

for _, skyhook := range clusterState.skyhooks {
    ...
    if skyhook.GetSkyhook().Updated {
        updates = true
        err := r.Status().Update(ctx, skyhook.GetSkyhook().NodeWright)
        if err != nil {
            errs = append(errs, fmt.Errorf("error updating nodewright status after reboot [%s]: %w", ...))
        }
    }
}

Updated is set whenever a node is new to NodeBootIds or its boot id changed — so on any cluster with node churn (and especially with interrupt: {type: reboot}), this fires constantly.

2. It runs before node selection (:487, with NewNodePicker at :492):

if yes, result, err := shouldReturn(r.TrackReboots(ctx, clusterState)); yes {
    return result, err
}

nodePicker := NewNodePicker(logger, r.opts.GetRuntimeRequiredTolerations())

3. shouldReturn bails on any error:

func shouldReturn(updates bool, err error) (bool, ctrl.Result, error) {
	if err != nil {
		return true, ctrl.Result{}, err
	}
	if updates {
		return true, ctrl.Result{RequeueAfter: time.Second * 2}, nil
	}
	return false, ctrl.Result{}, nil
}

So a conflict in TrackReboots aborts the reconcile before the node picker, package execution, and status recomputation ever run. It is also self-reinforcing: the more nodes reboot, the more boot ids churn, the more status writes, the more conflicts, the fewer reconciles complete.

Note the second branch too — updates == true returns RequeueAfter: 2s before the picker even with no error, so a steady stream of boot-id changes can starve node selection without a single error being logged.

Observed behavior

On a cluster with 4 NodeWright CRs and a reboot-interrupt rollout in flight, over a 10 minute window:

92 reconciler errors; 90 were this same conflict, e.g.

error updating skyhook status after reboot [skyhook-c]:
Operation cannot be fulfilled on skyhooks.skyhook.nvidia.com "skyhook-c":
the object has been modified; please apply your changes to the latest version and try again

Attributing each failure to (reconciling CR → CR whose status it failed to write):

reconciling=skyhook-a  -> failed writing skyhook-c   31
reconciling=skyhook-b  -> failed writing skyhook-c   26
reconciling=skyhook-c  -> failed writing skyhook-c   21
reconciling=skyhook-d  -> failed writing skyhook-c   12

69 of 90 conflicts came from reconcilers of other NodeWrights writing skyhook-c's status. Only 21 were self-inflicted — i.e. the cross-CR fan-out is the dominant source, not ordinary contention.

During this window the rollout made no progress: nodes that had finished their interrupt accumulated waiting for a post-interrupt that was never scheduled, because the reconcile never reached the picker.

The fix is already written in this file

The same function already patches Nodes rather than updating them, for exactly this reason (:1256-1262):

// Persist the reset before recording the new boot id. We Patch rather than
// Update because a busy node's resourceVersion churns constantly under other
// controllers, and a full Update would lose that optimistic-concurrency race; a
// strategic merge of only our annotation/label changes does not.

That reasoning applies verbatim to the NodeWright status write ~20 lines below it — same function, same reconcile, same churn source. There is a second reference at :1170 citing it as the established tradeoff. It simply was not carried across to the status write.

Suggested fix

Any of these, roughly in order of preference:

• Only write the NodeWright currently being reconciled. The fan-out is what makes this bad; a reconcile of CR A has no reason to persist CR B's status. Other CRs will track their own reboots on their own reconciles.
• Patch instead of Update, consistent with the Node write directly above, or wrap in retry.RetryOnConflict (already used elsewhere in this file at :1524 and :3443).
• Do not abort the whole reconcile on a status-write conflict. A conflict here is expected and self-healing on the next pass; aborting before the picker converts a benign race into a stall.

Steps to reproduce

# 1. Two or more NodeWright CRs in one cluster (they need not overlap in node selection).
# 2. At least one with a package that has an interrupt, so nodes reboot:
apiVersion: nodewright.nvidia.com/v1alpha1
kind: NodeWright
metadata:
  name: reboots
spec:
  interruptionBudget:
    percent: 50
  packages:
    demo:
      version: 1.0.0
      image: <package image>
      interrupt:
        type: reboot

# 3. Let the rollout run so boot ids change on many nodes.
# 4. Watch the operator log:
#      kubectl logs -n <ns> deploy/<operator> | grep 'status after reboot'
#    Expect repeated conflicts, attributed to reconciles of the *other* CRs.
# 5. Node selection stalls for as long as the conflicts continue.

Additional context

Severity: medium, in the reporter's estimation — leaving the Priority field for triage. It does not corrupt state and it self-clears once node churn subsides (it did, on the affected cluster, after roughly an hour). But while it is active the operator performs no node work at all, and the trigger — more than one NodeWright CR plus rebooting nodes — is an ordinary production configuration rather than an edge case. It is also invisible unless someone reads the operator log, since the CR status simply stops advancing.

Single-NodeWright clusters are largely unaffected, which is likely why it has not shown up in testing.

Related: #585, #587, #588 (found in the same investigation).

Code of Conduct

• I agree to follow NodeWright's Code of Conduct
• I have searched the open bugs and have found no duplicates for this bug report