[BUG]: in-progress short-circuit in GetNodesForNextBatch starves already-selected nodes, so post-interrupt never runs

[ayuskauskas]

Skyhook Version

Operator v0.15.0 (Helm chart skyhook-operator-v0.15.0)

Installation method

Helm

Kubernetes Version

v1.34.10

Component

Operator (controller-manager)

Describe the bug

GetNodesForNextBatch short-circuits on "a batch is in progress" and returns only nodes whose status is InProgress. Any node that has already been selected into the batch, still has work remaining, but is not currently InProgress gets dropped from the returned set — so the reconciler never runs its next stage.

In practice this starves the post-interrupt stage. A node that finishes its interrupt sits at stage=interrupt, state=complete with node status waiting. It needs one more pod (post-interrupt) to become complete. But as long as any other node in the compartment is InProgress, that node is never selected and post-interrupt never runs.

operator/internal/wrapper/compartment.go:109-127:

func (c *Compartment) GetNodesForNextBatch() []SkyhookNode {
	if c.Strategy != nil && c.BatchState.ShouldStop {
		return nil
	}

	// If there's a batch in progress (nodes are InProgress), don't start a new one
	if c.getInProgressCount() > 0 {
		return c.getInProgressNodes()      // <-- only InProgress nodes
	}

	// Sticky batch: nodes in NodePriority that aren't Complete yet should
	// continue processing before we pick new nodes. This handles the case where
	// IntrospectNode transitions nodes from InProgress -> Waiting between packages.
	if stickyNodes := c.getStickyBatchNodes(); len(stickyNodes) > 0 {
		return stickyNodes
	}

	// No batch in progress, create a new one
	return c.createNewBatch()
}

getInProgressNodes (compartment.go:152) filters strictly on node.Status() == v1alpha1.StatusInProgress.

The sticky-batch branch exists precisely for this case — its own comment says it handles nodes transitioning InProgress -> Waiting between packages. But it sits below the in-progress short-circuit, so it is unreachable whenever even one node in the compartment is InProgress.

The result returned here is what the reconciler iterates (operator/internal/controller/skyhook_controller.go:603), so a node excluded from it never reaches RunNext() / ProcessInterrupt / ApplyPackage, and no pod is created for it.

It is self-sustaining. The stranded node cannot become InProgress on its own — only the reconciler sets that, when it runs a package on the node, which requires the node to have been selected. With a large interruption budget and a long-running stage, the compartment may never reach zero InProgress, so the backlog only grows.

Expected: a node that is already in the batch (present in Status.NodePriority) and still has work should continue to be returned regardless of whether other nodes are currently InProgress.

Actual: it is dropped until the entire compartment momentarily has zero InProgress nodes.

Observed symptom

On an affected cluster with a package that has interrupt: {type: reboot} and interruptionBudget: {percent: 100}:

node package state         node status    count
--------------------       -----------    -----
config/in_progress         in_progress       35
interrupt/complete         waiting           12   <-- stranded
config/erroring            erroring           1
config/complete            waiting            1

pods: 35 config, 0 post-interrupt

All 12 stranded nodes were present in status.nodePriority, so getStickyBatchNodes() would have returned them — the in-progress short-circuit fired first every time.

Verified the rest of the path is fine for those nodes, i.e. they would proceed correctly if selected:

• IsNodeReadyForSkyhook passes — all predecessor Skyhooks are state=complete on those nodes.
• NextStage returns StagePostInterrupt (stage=interrupt, state=complete, hasInterrupt=true).
• ProcessInterrupt walks to its final return true, nil; the status.Stage == StageInterrupt && status.State != StateComplete guard is false because the state is complete.

Steps to reproduce

# 1. A Skyhook whose package has an interrupt, over a node set large enough
#    that stages overlap, with a budget big enough to keep several nodes busy.
apiVersion: skyhook.nvidia.com/v1alpha1
kind: Skyhook
metadata:
  name: repro
spec:
  interruptionBudget:
    percent: 100
  packages:
    demo:
      version: 1.0.0
      image: <package image with a long-running config step>
      interrupt:
        type: reboot

# 2. Let some nodes finish their interrupt while others are still in an
#    earlier stage. The finished ones land at stage=interrupt/state=complete
#    with node status `waiting`.
#
# 3. Observe: no post-interrupt pod is created for them for as long as any
#    other node in the compartment remains `in_progress`. They are in
#    status.nodePriority the whole time.

Additional context

Suggested fix directions:

• Union the two sets rather than returning early — return getInProgressNodes() plus getStickyBatchNodes(), so already-selected nodes with remaining work keep progressing alongside the in-flight ones. Batch sizing is already bounded by NodePriority membership, so this does not widen the blast radius.
• Or move the sticky check above the in-progress short-circuit, since sticky membership is the stricter condition (it requires the node to already be in NodePriority).

Workaround: lower spec.interruptionBudget enough that batches actually drain to zero InProgress, which makes the sticky branch reachable again. That is a mitigation, not a fix — it only reduces the probability of the starvation window staying open.

Related: #585 (also in GetNodesForNextBatch node-selection behavior).

Code of Conduct

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