[TRTLLMINF-40][fix] Introduce a SLURM dispatcher pod "finalizer"#16446
[TRTLLMINF-40][fix] Introduce a SLURM dispatcher pod "finalizer"#16446dpitman-nvda wants to merge 7 commits into
Conversation
…er pod When the K8s dispatcher pod dies mid-run (kubelet eviction, container termination, agent offline), the inner SLURM retry classified the wrapped "marked offline" as a transient infra failure and started another attempt -- which then executes on the same dead pod and fails immediately, spamming AgentOfflineException and burning the retry budget for nothing. Detect a dispatcher-pod death from the flattened cause chain (robust to the cleanup AbortException that wraps it) and fail closed instead of retrying in place. This is the first half of dispatcher-pod-death handling; off-pod reconciliation of the orphaned SLURM job / Jenkins node follows. Signed-off-by: Derek Pitman <dpitman@nvidia.com>
…d death When a SLURM dispatcher pod dies mid-run, its in-pod cleanup can no longer reach the login node, so the SLURM job and any Jenkins agent node leak (every "Clean Up Slurm Resource" retry fails with AgentOfflineException against the dead pod, right up to End of Pipeline). Add a build-scoped registry (keyed by stageName) of live SLURM resources: the pod wrapper records the dispatcher pod spec, the stage body records the SLURM job / Jenkins node identity, and the normal cleanup path deregisters once the resources are actually torn down. Only serializable primitives are stored (the SlurmCluster is rebuilt from clusterName) so pipeline persistence is unaffected. On a dispatcher-pod death (runnerStarted, isDispatcherPodFailure) the pod wrapper -- now back on the parent context -- launches a fresh cleanup pod and runs the normal scancel/workspace-clean/destroyNode off-pod. A post-build sweep backstops anything the in-catch finalize missed or failed on. Best-effort throughout: a finalizer failure is logged and never masks the stage failure. Together with the fail-closed change, this stops the leak the retry-on-a-dead- pod behavior left behind. Runtime paths (fresh-pod launch inside the catch / post-build finally, @field registry) need CI validation. Signed-off-by: Derek Pitman <dpitman@nvidia.com>
|
/bot run |
📝 WalkthroughWalkthroughThe Jenkins pipeline adds a concurrent SLURM resource registry, off-pod cleanup after dispatcher pod failures, failure-aware retry handling, dispatcher integration across x86 and SBSA paths, and a post-build orphan sweep. ChangesSLURM reconciliation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DispatcherPod
participant JenkinsPipeline
participant SLURMRegistry
participant CleanupPod
DispatcherPod->>JenkinsPipeline: fail during SLURM execution
JenkinsPipeline->>SLURMRegistry: retrieve registered stage resources
JenkinsPipeline->>CleanupPod: run SLURM or node cleanup
CleanupPod->>SLURMRegistry: deregister cleaned resources
JenkinsPipeline->>JenkinsPipeline: sweep remaining entries in Test finally
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jenkins/L0_Test.groovy`:
- Around line 4614-4622: Pass the in-scope attemptPodSpec as the pod-spec
override when calling finalizeOrphanedSlurmResource from the dispatcher-pod
failure branch in runLLMTestlistOnSlurm. Keep the existing failure handling
unchanged while ensuring finalizeSlurmResourceEntry can launch cleanup for later
SLURM attempts after prior cleanup removed the registry entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0eafc435-524c-45ea-9505-5dc98ee693ad
📒 Files selected for processing (1)
jenkins/L0_Test.groovy
|
PR_Github #59513 [ run ] triggered by Bot. Commit: |
|
PR_Github #59513 [ run ] completed with state
|
… wipe
Two fixes to the off-pod reconciliation registry:
- The Jenkins script sandbox forbids `new java.util.concurrent.ConcurrentHashMap`
("Scripts not permitted to use ..."), which failed the pipeline at load. Use
plain map literals: pipeline Groovy runs single-threaded under CPS (parallel
branches interleave at step boundaries, never execute Groovy concurrently) and
each stage writes its own key, so there is no concurrent map corruption to
guard against.
- deregisterSlurmResource removed the whole entry, but podSpec is registered
only once (before the inner SLURM retry loop). After the first attempt's
cleanup deregistered, a later attempt's dispatcher-pod death had no podSpec to
launch a cleanup pod with, and the post-build sweep could not recover it
either. Deregister now clears only the per-attempt job/node identity and keeps
the stage-level podSpec, so both the in-catch finalize and the sweep can always
reconcile. An entry left with only a podSpec is inert (finalize and the sweep
skip entries with no job/node).
Signed-off-by: Derek Pitman <dpitman@nvidia.com>
|
/bot run --disable-fail-fast |
|
PR_Github #59517 [ run ] triggered by Bot. Commit: |
|
PR_Github #59517 [ run ] completed with state
|
|
/bot run |
|
PR_Github #59720 [ run ] triggered by Bot. Commit: |
|
PR_Github #59720 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #59741 [ run ] triggered by Bot. Commit: |
|
PR_Github #59741 [ run ] completed with state
|
|
/bot run |
|
PR_Github #59773 [ run ] triggered by Bot. Commit: |
|
PR_Github #59773 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60007 [ run ] triggered by Bot. Commit: |
|
PR_Github #60007 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60026 [ run ] triggered by Bot. Commit: |
…pod-death-finalizer Signed-off-by: Derek Pitman <dpitman@nvidia.com> # Conflicts: # jenkins/L0_Test.groovy
…ndentation - runLLMTestlistOnSlurm dispatcher-pod-failure branch now passes the in-scope attemptPodSpec to finalizeOrphanedSlurmResource, so the off-pod finalizer never depends on the registry entry's podSpec surviving a prior attempt's deregister (CodeRabbit). - Re-indent the Test-stage body wrapped by the post-build sweep try/finally (chzblych). Signed-off-by: Derek Pitman <dpitman@nvidia.com>
|
/bot run |
|
PR_Github #60766 [ run ] triggered by Bot. Commit: |
|
PR_Github #60766 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60989 [ run ] triggered by Bot. Commit: |
|
PR_Github #60989 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61048 [ run ] triggered by Bot. Commit: |
|
PR_Github #61048 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61083 [ run ] triggered by Bot. Commit: |
|
PR_Github #61083 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61107 [ run ] triggered by Bot. Commit: |
|
PR_Github #61107 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61299 [ run ] triggered by Bot. Commit: |
|
PR_Github #61299 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61322 [ run ] triggered by Bot. Commit: |
|
PR_Github #61322 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61395 [ run ] triggered by Bot. Commit: |
|
PR_Github #61395 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61416 [ run ] triggered by Bot. Commit: |
|
PR_Github #61416 [ run ] completed with state
|
4 participants
Summary by CodeRabbit
Description
If a k8s pod managing a SLURM job dies, the existing retry logic will attempt to reuse that pod and will inevitably fail, which wastes the retry logic. What's more, the cleanup logic also relies on this dead pod, meaning we leak resources to boot.
This PR introduces SLURM management state outside of the code that runs inside the dispatcher pod. The SLURM information (cluster, node, Job ID, submission method) is registered once the pod knows about it. If the pod completes successfully, the resource is deregistered.
At the end of the run, the finalizer routine checks if any SLURM resources are still registered and cleans them up.
NB: we do not attempt a rerun or reattachment of the stage. That is coming in a follow-up.
Test Coverage
N/A, this is a CI change
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.