Skip to content

[lifecycle]: skip initialization timeout for nodes running workload pods - #52

Open
lukeraphael wants to merge 1 commit into
mainfrom
luke/init-timeout-workload-guard
Open

[lifecycle]: skip initialization timeout for nodes running workload pods#52
lukeraphael wants to merge 1 commit into
mainfrom
luke/init-timeout-workload-guard

Conversation

@lukeraphael

@lukeraphael lukeraphael commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • NODECLAIM_INITIALIZATION_TIMEOUT deletes a registered NodeClaim whose node never becomes Initialized.
  • A node that is running non-DaemonSet, non-terminal, non-terminating pods is now exempt: the timeout is skipped and re-checked every 5 minutes, so the deletion only fires once the node is genuinely idle.
  • DaemonSet pods do not count as workload.

Test plan

  • go test ./pkg/controllers/nodeclaim/lifecycle/ (full suite)
  • New spec: workload pod blocks the timeout, DaemonSet pod alone does not

Open in Devin Review

…d pods

The initialization timeout deletes a registered NodeClaim whose node never
becomes Initialized. A node that is serving non-DaemonSet pods is not stuck
in that sense, so deleting it would evict running workloads. Such nodes are
left alone and re-checked every 5 minutes in case their pods drain away.

Assisted-by: Claude Code:claude-marshmallow-eap

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@exa-heron-staging exa-heron-staging Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heron review

global-review-orchestrator-guardian

Verdict: ❌ Reject · 1 finding · head 7863c7fafa · openai/gpt-5.6-luna · 3m 5s · session

Severity Rule Location Finding
🛑 blocking high-confidence-regression pkg/controllers/nodeclaim/lifecycle/liveness.go:178 Static mirror pods are incorrectly treated as workload

Last updated 2026-08-24 03:30 UTC · reply @heron dismiss <reason> to dismiss on its comment thread

if err != nil {
return false, err
}
return lo.ContainsBy(pods, func(pod *corev1.Pod) bool {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static mirror pods are incorrectly treated as workload

🛑 blocking · rule high-confidence-regression · confidence 0.88

The new workload predicate excludes only DaemonSet pods, terminal pods, and terminating pods. It does not exclude node-owned static/mirror pods, even though this repository's scheduling contract explicitly treats those as non-reschedulable (pkg/utils/pod/scheduling.go:38-51) and defines IsOwnedByNode as the static-pod check (pkg/utils/pod/scheduling.go:174-176). A node can therefore have only a kubelet-managed mirror pod while still remaining uninitialized (for example, its bootstrap taint or an extended-resource registration is stuck); this predicate returns true, causing the timeout path at lines 151-154 to requeue forever instead of deleting the stranded NodeClaim/instance. Add !podutils.IsOwnedByNode(pod) (or use the repository's corresponding active/reschedulable classification) and add a mirror-pod regression test. This was validated by tracing the changed predicate to deleteNodeClaimForTimeout and comparing it with the existing static-pod exclusion used by the node lifecycle scheduling/disruption paths.

Suggested fix:

Exclude node-owned static/mirror pods from the predicate (for example, add && !podutils.IsOwnedByNode(pod)) and cover a node with only a mirror pod in the initialization-timeout test.

Heron review global-review-orchestrator-guardian · fingerprint fde4ce520718 · reply @heron dismiss <reason> to dismiss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant