Skip to content

fix(inference): admit a remediable storage conflict in the topology gate - #8802

Merged
prekshivyas merged 8 commits into
mainfrom
fix/managed-cluster-storage-gate
Aug 12, 2026
Merged

fix(inference): admit a remediable storage conflict in the topology gate#8802
prekshivyas merged 8 commits into
mainfrom
fix/managed-cluster-storage-gate

Conversation

@laitingsheng

@laitingsheng laitingsheng commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

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

  • Add src/lib/readiness/storage-remediation.ts with hasRemediableStorageConflict and STORAGE_COMPATIBLE_CAPABILITY, lifted unchanged out of src/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 are resolver.ts and src/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.
  • Import the shared predicate in resolver.ts and delete its local copy. Resolver behavior is unchanged.
  • Consult the predicate in managed-cluster-topology.ts validateReadiness before it returns readiness-incompatible, matching the resolver.
  • Document the admitted case in docs/inference/set-up-vllm-on-two-dgx-sparks.mdx and link the capability definitions to System Readiness.
  • Add three qualifyManagedClusterTopology tests: 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.
  • Add two probeManagedClusterManagedServingCapability tests: a detected cluster with an available remediation, and an ordinary automatic no-match without one.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Nine-category review passed with no findings: fix(inference): admit a remediable storage conflict in the topology gate #8802 (comment)
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: docs-updated
  • Evidence: docs/inference/set-up-vllm-on-two-dgx-sparks.mdx documents 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 docs reports 0 errors and 2 existing warnings.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: npx vitest run --project cli src/lib/inference src/lib/readiness — 104 files, 2036 tests passed. npm run typecheck:cli and npm run checks:repository both exited 0.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • New Features

    • Hosts with remediable Docker storage incompatibilities can qualify for supported two-node profiles when no other blocking issues are present.
    • Automatic discovery remains ready when storage remediation is available.
  • Bug Fixes

    • Hosts with unavailable remediation or additional blocking findings continue to be rejected appropriately.
  • Documentation

    • Added guidance explaining storage readiness handling and linked to the System Readiness definitions.

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>
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a38cef77-93fd-4990-8889-e6b7d7b1b24c

📥 Commits

Reviewing files that changed from the base of the PR and between f92a5bd and c019ae8.

📒 Files selected for processing (1)
  • test/nemoclaw-start-gateway-marker.test.ts

📝 Walkthrough

Walkthrough

The 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.

Changes

Storage readiness qualification

Layer / File(s) Summary
Storage remediation detector
src/lib/readiness/storage-remediation.ts
Defines the storage compatibility capability and detects reports with exactly one blocking, remediable storage incompatibility.
Managed-cluster readiness consumers
src/lib/inference/serving/managed-cluster-topology.ts, src/lib/inference/serving/resolver.ts
Managed-cluster validation accepts remediable storage conflicts. Other unsupported statuses, nonzero exit codes, and blocking or fatal findings remain failures. Resolver logic uses the shared detector.
Discovery, qualification, and documentation
src/lib/inference/serving/managed-cluster-discovery.test.ts, src/lib/inference/serving/managed-cluster-topology.test.ts, docs/inference/set-up-vllm-on-two-dgx-sparks.mdx
Tests cover available and unavailable remediation and additional blocking findings. DGX Spark setup documentation describes the readiness exception.
Gateway signal-path test fixture
test/nemoclaw-start-gateway-marker.test.ts
The test fixture loads launch_openclaw_gateway_process before invoking gateway launch helpers.

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
Loading

Possibly related PRs

Suggested labels: platform: dgx-spark

Suggested reviewers: cv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The gateway marker test fixture update is unrelated to the linked storage-remediation and topology-gate objectives. Move the unrelated gateway marker test change to a separate pull request or link an issue that requires it.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy #8789 by admitting remediable storage conflicts while rejecting unavailable remediation and other blocking findings.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: allowing remediable storage conflicts to pass the inference topology gate.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/managed-cluster-storage-gate

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit c019ae8 in the fix/managed-cluster-... branch remains at 96%, unchanged from commit 250d4ab in the main branch.


Updated August 12, 2026 17:18 UTC

@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Skipped

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

3 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

  • established — storage remediation at src/lib/inference/serving/managed-cluster-topology.test.ts:384: Retain "storage remediation". Existing readiness documentation and tests use the term consistently.
  • justified — remediable storage conflict at src/lib/inference/serving/managed-cluster-topology.test.ts:408: Retain "remediable storage conflict" where the distinction between the admitted conflict and other readiness failures matters.
  • justified — sole blocking finding at docs/inference/set-up-vllm-on-two-dgx-sparks.mdx:74: Retain "sole blocking finding" because it states the bounded admission condition.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: inference-routing

Manual-only E2E: network-policy
The manual PR workflow does not run these selectors for the commit under review. Run them from reviewed code on main.

2 optional E2E recommendations
  • overlayfs-autofix
  • vllm-docker-storage

Workflow run details

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>
@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Security review for the latest PR commit: PASS

  1. Secrets and credentials: no credentials, tokens, or secret-bearing paths changed.
  2. Input validation and data sanitization: the shared predicate requires status incompatible, exit code 2, exactly one blocking or fatal finding, the expected finding ID with blocking severity and one capability reference, one unique storage_compatible capability in the absent state, and one unique remediation capability in the present state.
  3. Authentication and authorization: no authentication, authorization, identity, or privilege boundary changed.
  4. Dependencies and third-party libraries: no dependency, lockfile, install hook, generated runtime, or dynamic-loading change.
  5. Error handling and logging: no logging or exception path changed. Callers continue to return the existing readiness rejection when the predicate does not match.
  6. Cryptography and data protection: no cryptographic or protected-data path changed.
  7. Configuration and security headers: no network policy, sandbox capability, container user, TLS, header, file-mode, or policy-precedence change.
  8. Security testing: five new topology and discovery tests cover the admitted case, unavailable remediation, and an additional blocking finding. Existing resolver tests also cover an additional fatal finding and missing remediation.
  9. System security: the predicate is pure and runs in the pre-launch qualification path after schema, mutation, and age checks. It creates no alternate entry point, retry path, state mutation, TOCTOU window, or recovery bypass.

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.

@laitingsheng laitingsheng added area: inference Inference routing, serving, model selection, or outputs area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression labels Aug 11, 2026

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Readiness update

The branch now includes the current main revision through an ordinary signed merge. The effective PR diff remains six files with 163 additions and 32 deletions; this is not a large line-count increase.

Validation on the refreshed branch:

  • managed-cluster discovery and topology tests: 64 passed
  • shared gateway regression suites, run in isolation: 51 passed, 1 skipped
  • CLI and plugin builds: passed
  • CLI type checking: passed
  • applicable repository hooks: passed
  • documentation validation: 0 errors, with 2 Fern upgrade notices
  • GitHub commit verification: every commit is Verified
  • documentation review: docs-updated; the existing two-DGX-Spark page covers the remediable storage case
  • sensitive-path review remains valid because the effective PR diff did not change

The prior gateway-shard failures were inherited from the older base and are covered by the current main test-infrastructure fix. Fresh GitHub checks are running. Human review remains outstanding; no reviewer routing was changed.

@prekshivyas
prekshivyas merged commit f712516 into main Aug 12, 2026
100 of 109 checks passed
@prekshivyas
prekshivyas deleted the fix/managed-cluster-storage-gate branch August 12, 2026 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: inference Inference routing, serving, model selection, or outputs area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression

Projects

None yet

4 participants