feat: nscale api change - #449
Conversation
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
Greptile SummaryThe PR updates the Nscale provider to obtain instance-to-hostname mappings from the placement-servers endpoint instead of the instances endpoint.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the review scope. No blocking failure remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant Engine as Slurm engine
participant Provider as Nscale provider
participant Placement as Placement Servers API
participant Radar as Radar API
Engine->>Provider: Resolve node mappings
Provider->>Placement: "GET /api/v2/placements/{placementId}/servers"
Placement-->>Provider: metadata.id and metadata.name
Provider-->>Engine: instance ID to hostname map
Engine->>Provider: Generate topology
Provider->>Radar: GET /v1/topology
Radar-->>Provider: topology paths by instance ID
Provider-->>Engine: generated topology graph
Reviews (2): Last reviewed commit: "Merge branch 'main' into rs-nscale" | Re-trigger Greptile |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe NScale provider replaces regional instance discovery with placement-server discovery. It requires ChangesNScale placement discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Instances2NodeMap
participant NScaleClient
participant PlacementServersAPI
Instances2NodeMap->>NScaleClient: PlacementServers(placementID)
NScaleClient->>PlacementServersAPI: GET /api/v2/placements/{placementID}/servers
PlacementServersAPI-->>NScaleClient: Placement-server metadata
NScaleClient-->>Instances2NodeMap: ID-to-name map
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 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 `@docs/providers/nscale.md`:
- Around line 8-10: The documentation describing instanceApiUrl should identify
it as the base URL for the Placement Servers API rather than the Instance API.
Keep the existing configuration key and update its description wherever it
appears in the nscale provider documentation.
- Around line 186-190: Update the verification step in the nscale provider
documentation to compare Placement Server hostnames with Slurm’s node list,
using scontrol show nodes -o and failing when the names differ; alternatively,
revise the step’s wording to describe inspection only rather than verification.
Ensure the instructions accurately reflect the Instances2NodeMap name filtering
behavior.
In `@pkg/providers/nscale/provider_test.go`:
- Around line 194-223: Expand TestInstances2NodeMap and add focused
PlacementServers test cases covering servers missing metadata.id or
metadata.name, asserting they are excluded while valid servers remain mapped.
Add malformed JSON coverage that expects an error, and canceled-context coverage
that verifies the request returns promptly without retries, using the existing
test server/provider helpers.
🪄 Autofix
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: ASSERTIVE
Plan: Enterprise
Run ID: a61b42f5-6538-45c6-ad9e-86e046d88701
📒 Files selected for processing (4)
docs/providers/nscale.mdpkg/providers/nscale/provider.gopkg/providers/nscale/provider_sim.gopkg/providers/nscale/provider_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Rungo fmt ./...; Go formatting is authoritative and code should not be hand-formatted.
Every new Go file must include the NVIDIA copyright header followed by the matching Apache 2.0 boilerplate.
Files:
pkg/providers/nscale/provider_sim.gopkg/providers/nscale/provider_test.gopkg/providers/nscale/provider.go
pkg/providers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/providers/**/*.go: Providers must return a*topology.Graphand*httperr.Error; plainerroris not acceptable at the provider interface boundary.
Providers discover topology and return the canonicaltopology.Graph; they must not emit scheduler-specific output.
When usingClusterTopology, populate fabric tiers closest-first, preserve optional accelerator domain and sub-domain fields, and callToGraph.
Files:
pkg/providers/nscale/provider_sim.gopkg/providers/nscale/provider_test.gopkg/providers/nscale/provider.go
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Do not publicly disclose suspected security vulnerabilities; report them privately through NVIDIA PSIRT using the channels documented inSECURITY.md.
Every commit must include aSigned-off-by:trailer for DCO compliance.
Use Conventional Commits with an allowed type such asfeat,fix,docs,chore,refactor,test,build, orci.
Before pushing, runmake qualify; all CI checks, including Go build/test/lint, Codecov, and DCO, must be green before merge.
Changes to provider, engine, chart, API, configuration schema, labels, annotations, or user-facing behavior must include the corresponding documentation updates described in the documentation impact table.
Files:
pkg/providers/nscale/provider_sim.godocs/providers/nscale.mdpkg/providers/nscale/provider_test.gopkg/providers/nscale/provider.go
{cmd,pkg,internal}/**/*.go
⚙️ CodeRabbit configuration file
{cmd,pkg,internal}/**/*.go: Focus on correctness, robustness, and failure paths:
- Check error handling and propagation, including whether callers
receive enough context to diagnose failures.- Verify context propagation, cancellation, timeouts, resource cleanup,
and goroutine lifecycle.- Look for races, deadlocks, unsafe shared state, leaks, and partial
updates that can leave state inconsistent.- Check nil, empty, malformed, duplicate, boundary, and partial inputs.
- Verify deterministic and idempotent behavior where operations may be
retried or repeated.- Preserve public contracts and the architectural boundaries documented
in AGENTS.md.- Flag behavior changes that lack meaningful regression coverage.
Files:
pkg/providers/nscale/provider_sim.gopkg/providers/nscale/provider_test.gopkg/providers/nscale/provider.go
docs/providers/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Provider additions or changes require the corresponding provider documentation, including prerequisites, credentials, parameters, operation, and verification.
Files:
docs/providers/nscale.md
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
New or changed public behavior and new code paths should be covered by tests.
Files:
pkg/providers/nscale/provider_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Flag missing negative, cancellation, concurrency, malformed-input,
boundary, and regression cases. Check that assertions would fail if
the behavior under test were broken.
Files:
pkg/providers/nscale/provider_test.go
pkg/providers/*/provider.go
📄 CodeRabbit inference engine (AGENTS.md)
A provider must expose
func NamedLoader() (string, providers.Loader)and be registered inpkg/registry/registry.go.
Files:
pkg/providers/nscale/provider.go
🔇 Additional comments (4)
pkg/providers/nscale/provider.go (1)
25-26: LGTM!Also applies to: 38-38, 50-50, 73-80, 107-132, 179-181, 212-218
pkg/providers/nscale/provider_sim.go (1)
70-76: LGTM!pkg/providers/nscale/provider_test.go (1)
32-103: LGTM!Also applies to: 130-192
docs/providers/nscale.md (1)
12-34: LGTM!Also applies to: 58-58, 81-81, 101-102, 127-128, 167-185
| - **Placement Servers API**: returns server metadata via `GET /api/v2/placements/{placementID}/servers` | ||
|
|
||
| The Radar response supplies the provider instance ID, switch path, and optional block ID. The Instance API response maps provider instance IDs to hostnames using `metadata.id` and `metadata.name`; this is used by the Slurm engine when Topograph discovers Slurm nodes automatically. | ||
| The Radar response supplies the provider instance ID, switch path, and optional block ID. The Placement Servers API response maps provider instance IDs to hostnames using `metadata.id` and `metadata.name`; this is used by the Slurm engine when Topograph discovers Slurm nodes automatically. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the base URL for placement-server discovery.
This section now documents the Placement Servers API, but instanceApiUrl is still described as the base URL for the Instance API. Keep the configuration key required by pkg/providers/nscale/provider.go, but describe its value as the base URL for the Placement Servers API. This prevents operators from selecting the old endpoint for the new discovery request.
🤖 Prompt for 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.
In `@docs/providers/nscale.md` around lines 8 - 10, The documentation describing
instanceApiUrl should identify it as the base URL for the Placement Servers API
rather than the Instance API. Keep the existing configuration key and update its
description wherever it appears in the nscale provider documentation.
| First verify that the Placement Servers API returns the hostnames Slurm knows: | ||
|
|
||
| ```bash | ||
| curl -s -H "Authorization: Bearer $TOKEN" \ | ||
| "$INSTANCE_API_URL/v2/instances?organizationID=$ORG®ionID=$REGION" \ | ||
| "$INSTANCE_API_URL/api/v2/placements/$PLACEMENT_ID/servers" \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the verification step compare with Slurm.
This command only prints metadata.id and metadata.name from the Placement Servers API. It does not read or compare the Slurm node list. Therefore, it cannot verify that the returned hostnames match Slurm nodes, although Instances2NodeMap filters mappings by those names. Either change the text to describe inspection only, or add a comparison with scontrol show nodes -o that fails on a mismatch.
🤖 Prompt for 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.
In `@docs/providers/nscale.md` around lines 186 - 190, Update the verification
step in the nscale provider documentation to compare Placement Server hostnames
with Slurm’s node list, using scontrol show nodes -o and failing when the names
differ; alternatively, revise the step’s wording to describe inspection only
rather than verification. Ensure the instructions accurately reflect the
Instances2NodeMap name filtering behavior.
| func TestInstances2NodeMap(t *testing.T) { | ||
| ctx := context.Background() | ||
|
|
||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| require.Equal(t, http.MethodGet, r.Method) | ||
| require.Equal(t, "/api/v2/placements/placement-1/servers", r.URL.Path) | ||
| require.Equal(t, "Bearer token", r.Header.Get("Authorization")) | ||
|
|
||
| i2n, err := provider.(*Provider).Instances2NodeMap(ctx, []string{"node-1", "node-2"}) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, err := w.Write([]byte(placementServersResponse)) | ||
| require.NoError(t, err) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| p := newTestProvider(t, ctx, server.URL) | ||
|
|
||
| i2n, err := p.Instances2NodeMap(ctx, []string{"training-workers-0", "training-workers-1"}) | ||
| require.NoError(t, err) | ||
| require.Equal(t, map[string]string{ | ||
| "instance-1": "node-1", | ||
| "instance-2": "node-2", | ||
| "psrv-7f3d9d5d2a7c4e32": "training-workers-0", | ||
| "psrv-950ab3259a1443da": "training-workers-1", | ||
| }, i2n) | ||
|
|
||
| i2n, err = provider.(*Provider).Instances2NodeMap(ctx, nil) | ||
| i2n, err = p.Instances2NodeMap(ctx, nil) | ||
| require.NoError(t, err) | ||
| require.Equal(t, map[string]string{ | ||
| "instance-1": "node-1", | ||
| "instance-2": "node-2", | ||
| "instance-3": "outside-node", | ||
| "psrv-7f3d9d5d2a7c4e32": "training-workers-0", | ||
| "psrv-950ab3259a1443da": "training-workers-1", | ||
| }, i2n) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover incomplete responses, malformed JSON, and cancellation.
Lines 126-128 in pkg/providers/nscale/provider.go skip servers without metadata.id or metadata.name, but no test executes those branches. The tests also do not cover invalid JSON or a canceled context for PlacementServers.
Add cases that assert incomplete servers are excluded, valid servers remain mapped, malformed JSON returns an error, and a canceled context returns without a retry loop.
As per coding guidelines, “New or changed public behavior and new code paths should be covered by tests.” As per path instructions, “Flag missing negative, cancellation, concurrency, malformed-input, boundary, and regression cases.”
Also applies to: 225-274
🤖 Prompt for 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.
In `@pkg/providers/nscale/provider_test.go` around lines 194 - 223, Expand
TestInstances2NodeMap and add focused PlacementServers test cases covering
servers missing metadata.id or metadata.name, asserting they are excluded while
valid servers remain mapped. Add malformed JSON coverage that expects an error,
and canceled-context coverage that verifies the request returns promptly without
retries, using the existing test server/provider helpers.
Sources: Coding guidelines, Path instructions
2 participants
Description
Updates the nscale provider to use the
list-placement-serversend point instead of theinstancesapi.Checklist
git commit -s).