Skip to content

Commit eab7eda

Browse files
committed
fix the fix
Signed-off-by: Ajay Mishra <ajmishra@nvidia.com>
1 parent 72d8504 commit eab7eda

2 files changed

Lines changed: 22 additions & 15 deletions

File tree

labeler/pkg/labeler/labeler.go

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -659,19 +659,19 @@ func (l *Labeler) nodeRequiresReconciliation(oldObj, newObj any) bool {
659659
}
660660

661661
// resyncNeedsRepair reports whether an informer resync is looking at a node whose
662-
// labels have drifted from what the caches say they should be.
662+
// driver label has drifted from what the pod caches say it should be.
663663
//
664-
// The labeler derives every label it writes from state it watches, but nothing it
665-
// writes is itself a watched input, so a label stamped from a lagging cache is never
666-
// revisited. A driver pod deleted while the startup sweep is running is the case that
667-
// bites: the delete handler correctly drops the label, the sweep re-stamps it from an
668-
// indexer that has not caught up yet, and no later event ever disagrees. The resync
669-
// replay is the only pass left that can repair it.
664+
// reconcileAllNodes reads the pod cache, then gets the node, then writes. A driver pod
665+
// deleted inside that window is lost: the delete handler clears the label, and the
666+
// sweep writes back the answer it formed before the pod went away. Nothing the labeler
667+
// writes is an input it watches, so no event disagrees with the stale value afterwards.
668+
// A replacement pod normally heals it, which leaves the case where none arrives — a
669+
// drained or decommissioned node still claiming a driver it no longer has.
670670
//
671-
// The check runs entirely against informer caches, so a node that is already correct
672-
// costs no API call — which is what makes it affordable against a 30s resync period.
673-
// A node that has drifted is reconciled twice over, once here and once for real, so
674-
// the repair logs its label changes twice; that only happens on the rare repair.
671+
// The check compares one label against the caches without mutating the node, emitting
672+
// metrics, or logging, so a node that is already correct costs nothing. That is what
673+
// makes it affordable against every node on every 30s resync. The repair it triggers
674+
// is an ordinary reconcile, with all the side effects that normally carries.
675675
func (l *Labeler) resyncNeedsRepair(oldObj, newObj any) bool {
676676
oldNode, oldOk := oldObj.(*v1.Node)
677677

@@ -692,13 +692,20 @@ func (l *Labeler) resyncNeedsRepair(oldObj, newObj any) bool {
692692
return false
693693
}
694694

695-
driverLabel, dcgmVersion, err := l.desiredNodeLabels(newNode.Name)
695+
// Opted-out nodes are driven by the managed label, which is a watched input, so
696+
// drift there already raises an event without help from the resync.
697+
optedOut, err := managed.IsNodeOptedOut(l.ctx, l.nodeLister, newNode.Name)
698+
if err != nil || optedOut {
699+
return false
700+
}
701+
702+
driverLabel, err := l.getDriverLabelForNode(newNode.Name, nil)
696703
if err != nil {
697704
slog.Debug("Skipping resync repair check", "node", newNode.Name, "error", err)
698705
return false
699706
}
700707

701-
return l.reconcileNodeLabelsInPlace(newNode.DeepCopy(), driverLabel, dcgmVersion)
708+
return newNode.Labels[DriverInstalledLabel] != driverLabel
702709
}
703710

704711
const gpuPresentLabel = "nvidia.com/gpu.present"

labeler/pkg/labeler/labeler_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2493,7 +2493,7 @@ func interleavingLabeler(t *testing.T, node *corev1.Node,
24932493
}, clientset
24942494
}
24952495

2496-
// TestStartupSweepRacesDriverPodDeletion pins the interleaving behind the flake in
2496+
// TestReconcileAllNodes_DriverPodDeletedDuringSweep_ReStampsStaleLabel pins the interleaving behind the flake in
24972497
// TestLabeler_handlePodEvent/driver_pod_deletion_removes_driver_label.
24982498
//
24992499
// Two code paths read the same pod indexer and disagree about what is on it. The
@@ -2508,7 +2508,7 @@ func interleavingLabeler(t *testing.T, node *corev1.Node,
25082508
// the delete handler clears the label, and the sweep then writes a decision it formed
25092509
// before the pod went away. Pinning the indexer contents, as these cases do, produces
25102510
// the same outcome as the sweep having read before the delete, without the timing.
2511-
func TestStartupSweepRacesDriverPodDeletion(t *testing.T) {
2511+
func TestReconcileAllNodes_DriverPodDeletedDuringSweep_ReStampsStaleLabel(t *testing.T) {
25122512
const nodeName = "test-node"
25132513

25142514
// Mirrors the CI fixture: the node already carries the label the deletion should

0 commit comments

Comments
 (0)