Skip to content

Heavy pass can delete foreign node taints (atomic list restamped from a stale snapshot) #447

Description

@ayuskauskas

What

SaveNodesAndSkyhook patches each changed node with a two-way strategic merge diff against the pass's own snapshot:

patch := client.StrategicMergeFrom(clusterState.tracker.GetOriginal(node.GetNode()))
err := r.Patch(ctx, node.GetNode(), patch)

corev1.NodeSpec.Taints is +listType=atomic, so a strategic merge replaces the whole list rather than merging by key. Whenever the pass changes any single taint, the patch carries the full list as the pass saw it at snapshot time — and any taint added by another writer since that snapshot is silently deleted.

Why it matters

Node taints are a shared surface. Writers that are routinely present alongside the operator:

  • cluster-autoscaler (ToBeDeletedByClusterAutoscaler, DeletionCandidateOfClusterAutoscaler)
  • Karpenter (karpenter.sh/disruption)
  • GPU Operator / device plugins
  • node-problem-detector
  • a human with kubectl taint

Deleting ToBeDeletedByClusterAutoscaler mid-scale-down, or a GPU Operator taint mid-driver-install, un-gates scheduling onto a node that is deliberately gated.

Reproduction (by construction)

  1. Node has taint T (added by the autoscaler) — not present in the pass's snapshot.
  2. The pass adds or removes the runtime-required taint on that node.
  3. The pass patches; spec.taints is replaced with the snapshot's list.
  4. T is gone.

The window is the whole pass — cluster-state build through SaveNodesAndSkyhook — not just the write.

Scope

Pre-existing on main. The same code is reached on feature/package-as-jobs.

A merge was drafted for this in #413 (applyTaintChanges, replaying only the taints the pass added/removed onto a fresh read) but was deliberately dropped from that PR to keep it to the one bug it was fixing. That function is a reasonable starting point if picked up — with the caveat that its first loop restamped the pass's value over a concurrently-edited one, so a fix needs to compare by Value in both directions, and preserve TimeAdded on taints it did not change.

Related: spec.unschedulable is not affected — it is a scalar, so an untouched cordon never enters the diff.

Metadata

Metadata

Assignees

No one assigned

    Labels

    component/operatorSkyhook operator (controller-manager)

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions