Skip to content

feat(engine/slinky): report why useGpuCliqueLabel found no matching nodes#379

Merged
giuliocalzo merged 2 commits into
NVIDIA:mainfrom
giuliocalzo:feat/slinky-clique-diagnostics
Jul 6, 2026
Merged

feat(engine/slinky): report why useGpuCliqueLabel found no matching nodes#379
giuliocalzo merged 2 commits into
NVIDIA:mainfrom
giuliocalzo:feat/slinky-clique-diagnostics

Conversation

@giuliocalzo

Copy link
Copy Markdown
Contributor

Description

When the Slinky engine runs with useGpuCliqueLabel=true and cannot build any block domains, it previously returned a generic error:

useGpuCliqueLabel=true but no matching nodes found; check label "nvidia.com/gpu.clique" and annotation "topograph.nvidia.com/instance"

This gave operators no way to tell which nodes were examined or why each was skipped. In particular, the topograph.nvidia.com/instance annotation is written per-node by the node-data-broker DaemonSet, so a node with the clique label but no annotation points at a broker that hasn't annotated that specific node yet — but the old message hid this.

This PR makes the failure actionable:

  • The 502 error now reports how many nodes were scanned and a breakdown of why each was skipped: no Slurm node mapping (no Ready slurmd pod), missing nvidia.com/gpu.clique label, or missing the topograph.nvidia.com/instance annotation.
  • It lists the node names that carry the clique label but are missing the broker-written annotation (capped to keep the message bounded on large clusters).
  • The per-node warning now includes the node name, its Slurm name, and the clique value.

Example new message:

useGpuCliqueLabel=true but no matching nodes found; check label "nvidia.com/gpu.clique" and annotation "topograph.nvidia.com/instance". Scanned 8 node(s): 2 without a SLURM node mapping (no Ready slurmd pod), 3 missing the "nvidia.com/gpu.clique" label, 3 with the label but missing the "topograph.nvidia.com/instance" annotation (nodes missing annotation: gpu-node-1, gpu-node-4, gpu-node-7)

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • All commits are signed off per DCO (git commit -s).

@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR improves the diagnostic quality of the useGpuCliqueLabel failure path in the Slinky engine, replacing a single generic error with a breakdown of exactly why each scanned node was skipped and a bounded list of node names missing the broker-written annotation.

  • withGPUCliqueDomains: adds an early guard for an empty node list (distinct from "nodes exist but none matched"), then accumulates per-category counters (noSlurmName, noCliqueLabel, missingAnnotation) that are folded into the 502 error message alongside the total scan count.
  • formatMissingAnnotationNodes: new helper that renders the offending node names and caps the list at 10 entries to keep the message bounded on large clusters.
  • Tests: three new test cases cover the empty-node-list path, the missing-annotation path, and extend the existing no-matching-nodes test to assert the new counter fragments in the error string.

Confidence Score: 5/5

Safe to merge — the change is additive, touching only the error-reporting path that is already a failure terminal; no happy-path logic is altered.

All three new code paths are covered by tests and the existing test suite is extended. The diagnostic counters are mutually exclusive and correctly account for every node in the scan.

No files require special attention; all changes are confined to the error-reporting branch of withGPUCliqueDomains in pkg/engines/slinky/engine.go.

Important Files Changed

Filename Overview
pkg/engines/slinky/engine.go Adds early empty-node check, diagnostic counters (noSlurmName, noCliqueLabel, missingAnnotation), enriched warning log, and formatMissingAnnotationNodes helper. Logic is correct and counts are mutually exclusive.
pkg/engines/slinky/engine_test.go Adds three new test cases covering: no-nodes-selected, missing-annotation, and extends the existing no-matching-nodes test to check the new diagnostic counters in the error message. Truncation path (>10 nodes) is not covered.
CHANGELOG.md CHANGELOG entry added under the existing Changed section, accurately describing the new diagnostic behaviour.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[withGPUCliqueDomains called] --> B{clusterNodes.nodes empty?}
    B -- yes --> C[502: no selected Kubernetes nodes found]
    B -- no --> D[Init counters: totalNodes, noSlurmName, noCliqueLabel, missingAnnotation]
    D --> E[For each node]
    E --> F{Has SLURM name in nodeMap?}
    F -- no --> G[noSlurmName++ / continue]
    F -- yes --> H{Has non-empty gpu.clique label?}
    H -- no --> I[noCliqueLabel++ / continue]
    H -- yes --> J{Has instance annotation?}
    J -- no --> K[missingAnnotation append / klog.Warning / continue]
    J -- yes --> L[domains.AddHost]
    G & I & K & L --> E
    E -- done --> M{len domains == 0?}
    M -- yes --> N[502: detailed diagnostic with counts and node list]
    M -- no --> O[Return graph with domains]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[withGPUCliqueDomains called] --> B{clusterNodes.nodes empty?}
    B -- yes --> C[502: no selected Kubernetes nodes found]
    B -- no --> D[Init counters: totalNodes, noSlurmName, noCliqueLabel, missingAnnotation]
    D --> E[For each node]
    E --> F{Has SLURM name in nodeMap?}
    F -- no --> G[noSlurmName++ / continue]
    F -- yes --> H{Has non-empty gpu.clique label?}
    H -- no --> I[noCliqueLabel++ / continue]
    H -- yes --> J{Has instance annotation?}
    J -- no --> K[missingAnnotation append / klog.Warning / continue]
    J -- yes --> L[domains.AddHost]
    G & I & K & L --> E
    E -- done --> M{len domains == 0?}
    M -- yes --> N[502: detailed diagnostic with counts and node list]
    M -- no --> O[Return graph with domains]
Loading

Reviews (3): Last reviewed commit: "feat(engine/slinky): distinguish empty n..." | Re-trigger Greptile

…odes

When useGpuCliqueLabel=true produces no block domains, the engine returned a
generic "no matching nodes found" error that gave operators no way to tell
whether the problem was missing slurmd pods, absent GPU clique labels, or
node-data-broker annotations that had not landed yet.

The error and per-node warnings now report how many nodes were scanned and why
each was skipped, and list the nodes that carry the nvidia.com/gpu.clique label
but are missing the node-data-broker-written topograph.nvidia.com/instance
annotation (capped to keep the message bounded on large clusters).

Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
@giuliocalzo
giuliocalzo force-pushed the feat/slinky-clique-diagnostics branch from 0d3fe61 to a7378a9 Compare July 6, 2026 07:44
Comment thread pkg/engines/slinky/engine.go
…match

Address review feedback: when useGpuCliqueLabel selects zero Kubernetes nodes
(e.g. a too-narrow nodeSelector), return a dedicated error pointing at the
engine nodeSelector instead of the generic "no matching nodes found" message,
which only applies when nodes exist but none carry the clique label/annotation.

Signed-off-by: Giulio Calzolari <gcalzolari@nvidia.com>
@giuliocalzo

Copy link
Copy Markdown
Contributor Author

Thanks for the review @dmitsh. Pushed 8468abe addressing your suggestion:

  • withGPUCliqueDomains now short-circuits with a dedicated error — no selected Kubernetes nodes found; check engine nodeSelector — when zero Kubernetes nodes are selected, keeping it distinct from the "nodes exist but none matched the clique label/annotation" diagnostic.
  • Added TestWithGPUCliqueDomainsNoSelectedNodes for the empty-selection path; all TestWithGPUCliqueDomains* tests pass and the CHANGELOG entry is updated.

The branch was also rebased onto latest main earlier to clear the CHANGELOG conflict. Ready for another look when you have a moment.

@giuliocalzo
giuliocalzo requested a review from dmitsh July 6, 2026 08:09
@giuliocalzo
giuliocalzo merged commit 2cd65cf into NVIDIA:main Jul 6, 2026
7 checks passed
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.

2 participants