fix(inference): admit a remediable storage conflict in the topology gate - #8802
Conversation
The two-node topology gate rejected a readiness report that the serving resolver, which runs immediately after it, already accepted. Both consumers now share one predicate, so a node whose only blocking readiness finding has an available storage remediation continues to qualify. A node with any other blocking finding, or with no available remediation, still fails closed. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds shared storage-remediation detection, updates managed-cluster readiness validation and resolver wiring, adds discovery and topology coverage, and documents the qualification exception for remediable Docker storage conflicts. ChangesStorage readiness qualification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedClusterDiscovery
participant ManagedClusterTopology
participant StorageRemediation
ManagedClusterDiscovery->>ManagedClusterTopology: validate node readiness
ManagedClusterTopology->>StorageRemediation: check remediable storage conflict
StorageRemediation-->>ManagedClusterTopology: return qualification decision
ManagedClusterTopology-->>ManagedClusterDiscovery: return ready or no-match
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8802.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: Manual-only E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Security review for the latest PR commit: PASS
NemoClaw-specific review found no sandbox escape, SSRF, credential leak, blueprint tampering, installer trust, or workflow trusted-code-boundary change. LOC increase flag: +163/-32, net +131. Regression tests account for 114 additions, documentation accounts for 6 additions, and production code is +43/-32, net +11. Validation passed: 98 focused inference tests, CLI and plugin type checks, the documentation build with 0 errors and 2 existing warnings, and the complete repository hook set. GitHub reports every PR commit as Verified, and all commits retain DCO sign-offs. The earlier Nemotron advisor failure was review infrastructure. The model emitted text before the required terminology-tool result completed. Its preserved analysis reported all nine security categories as passing and found no correctness issue. A new advisor run is evaluating the refreshed branch. |
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed latest PR commit 6633a83a6; no blocking correctness or security finding.
The extracted predicate admits only the exact remediable-storage shape: incompatible/exit 2, one blocking storage finding with the expected capability reference, storage compatibility absent, remediation present, and no second blocking/fatal finding. Other readiness failures remain fail-closed. No secret, auth, dependency, logging, crypto, permission, or network boundary changes.
No approval because base 9257fe0f6 is behind current main and the current required checks are missing.
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed commit 3142dc4. The shared predicate admits only the exact incompatible/exit-2 storage case with one blocking storage finding, absent storage compatibility, and present remediation; additional blocking or fatal findings remain rejected. I found no blocking diff issue. The CLI check failures came from GitHub checkout certificate verification in the trusted-action checkout, and both advisor checks are also failing, so this head is not green and I am not approving it.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Readiness updateThe branch now includes the current Validation on the refreshed branch:
The prior gateway-shard failures were inherited from the older base and are covered by the current |
Summary
A two-node managed cluster stopped at the node readiness gate on a DGX Spark host that runs Docker with the containerd image store, because the topology gate rejected a readiness report that the serving resolver running immediately after it already accepted. Both consumers now read one predicate, so a node whose only blocking readiness finding is the remediable Docker storage conflict continues to qualify. A node with any other blocking finding, or with no available remediation, still fails closed with the same message.
Related Issue
Fixes #8789
Changes
src/lib/readiness/storage-remediation.tswithhasRemediableStorageConflictandSTORAGE_COMPATIBLE_CAPABILITY, lifted unchanged out ofsrc/lib/inference/serving/resolver.ts. The current requirement is that both managed inference consumers reach the same verdict on the same readiness report. The current consumers areresolver.tsandsrc/lib/inference/serving/managed-cluster-topology.ts. Copying the predicate into the topology gate instead is what allowed the two to disagree, so a second copy does not meet the requirement. The topology and discovery tests below protect the shared contract.resolver.tsand delete its local copy. Resolver behavior is unchanged.managed-cluster-topology.tsvalidateReadinessbefore it returnsreadiness-incompatible, matching the resolver.docs/inference/set-up-vllm-on-two-dgx-sparks.mdxand link the capability definitions to System Readiness.qualifyManagedClusterTopologytests: a qualifying node set whose only blocking readiness finding has an available storage remediation, a node with no available remediation, and a node with a second blocking readiness finding.probeManagedClusterManagedServingCapabilitytests: a detected cluster with an available remediation, and an ordinary automatic no-match without one.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/inference/set-up-vllm-on-two-dgx-sparks.mdxdocuments the bounded storage exception: the storage conflict is the sole blocking finding, no fatal finding remains, remediation is present, and every other required capability is present.npm run docsreports 0 errors and 2 existing warnings.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/inference src/lib/readiness— 104 files, 2036 tests passed.npm run typecheck:cliandnpm run checks:repositoryboth exited 0.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation