feat(switch): persist and expose NVLink domain UUID - #4473
Conversation
Signed-off-by: Jay Zhu <jayzhu@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 ignored due to path filters (1)
📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThe change adds nullable NVLink domain UUID tracking to switches. Rack monitoring performs NMX-C Hello operations, persists valid domain UUIDs for active rack switches, preserves prior values for nil or failed observations, and exposes the value through RPC. ChangesNVLink domain tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Monitor as NVLink monitor
participant Endpoint as NMX-C endpoint
participant Database as Switch database
Monitor->>Endpoint: Resolve rack endpoint and send Hello
Endpoint-->>Monitor: Return domain UUID
Monitor->>Database: Persist UUID for active rack switches
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-08-01 06:06:42 UTC | Commit: a8a5a81 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/nvlink-manager/src/lib.rs (2)
1385-1393: 🚀 Performance & Scalability | 🔵 TrivialConsider bounding and parallelising the standalone Hello fan-out.
This loop contacts each rack endpoint serially. The iteration duration therefore grows linearly with the number of racks that have no managed hosts, while the iteration still holds
ITERATION_WORK_KEY. The existing partition loops are also serial, so this matches current behavior and is not a regression.If the rack count grows, run these observations concurrently with a bounded degree, for example a
JoinSetcapped by a semaphore, so a large site does not stretch the monitor cadence. A per-call timeout would also stop one unresponsive NMX-C endpoint from delaying every remaining rack.🤖 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 `@crates/nvlink-manager/src/lib.rs` around lines 1385 - 1393, Update the standalone rack observation loop around observe_and_record_rack_switch_domain_uuid to perform endpoint calls concurrently with a bounded degree of parallelism, such as a semaphore-limited JoinSet. Add a per-call timeout so an unresponsive endpoint cannot delay the iteration indefinitely, while preserving the existing filter that skips racks present in managed_host_snapshots_by_rack_id.
1638-1701: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared Hello-and-parse sequence.
Lines 1643-1697 repeat the four-step sequence already implemented in
process_nmx_c_partition_monitor_groupat lines 1443-1523: buildEndpoint, create the client, callhello, then parse the domain UUID. The two copies differ only in the failure reaction. The group path queues aPendingNullNvlinkObservationper reason and records metrics. This path only logs.Extract one helper that returns the failure reason, then let each caller apply its own policy. The reason enum already exists, so the group path keeps its metric and queueing behavior and this path maps the reason to a warn.
♻️ Proposed shape for the shared helper
/// Resolves the NMX-C domain UUID for one endpoint. /// /// Returns the reason on failure so callers can choose between queueing a /// null observation and logging only. async fn observe_domain_uuid( &self, endpoint_url: &str, ) -> Result<NvLinkDomainId, ChassisNmxCUnreachableReason> { let endpoint = Endpoint::new(endpoint_url) .map_err(|_| ChassisNmxCUnreachableReason::InvalidEndpointUri)?; let mut client = self .nmxc_client_pool .create_client(endpoint) .await .map_err(|_| ChassisNmxCUnreachableReason::ClientCreateFailed)?; let hello = client .hello(NMX_C_GATEWAY_ID) .await .map_err(|_| ChassisNmxCUnreachableReason::HelloFailed)?; domain_uuid_from_nmx_c_hello(&hello) .map_err(|_| ChassisNmxCUnreachableReason::DomainUuidParseFailed) }Keep the per-step
errorfield in the logs. Either return the error alongside the reason, or log inside the helper before mapping.🤖 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 `@crates/nvlink-manager/src/lib.rs` around lines 1638 - 1701, Extract the repeated Endpoint creation, NMX-C client creation, hello call, and domain UUID parsing from observe_and_record_rack_switch_domain_uuid and process_nmx_c_partition_monitor_group into a shared observe_domain_uuid helper returning Result<NvLinkDomainId, ChassisNmxCUnreachableReason>. Preserve each caller’s existing failure policy: the group path must retain metrics and PendingNullNvlinkObservation queueing, while observe_and_record_rack_switch_domain_uuid logs a warning; retain per-step error details by returning or otherwise preserving the underlying errors.
🤖 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.
Nitpick comments:
In `@crates/nvlink-manager/src/lib.rs`:
- Around line 1385-1393: Update the standalone rack observation loop around
observe_and_record_rack_switch_domain_uuid to perform endpoint calls
concurrently with a bounded degree of parallelism, such as a semaphore-limited
JoinSet. Add a per-call timeout so an unresponsive endpoint cannot delay the
iteration indefinitely, while preserving the existing filter that skips racks
present in managed_host_snapshots_by_rack_id.
- Around line 1638-1701: Extract the repeated Endpoint creation, NMX-C client
creation, hello call, and domain UUID parsing from
observe_and_record_rack_switch_domain_uuid and
process_nmx_c_partition_monitor_group into a shared observe_domain_uuid helper
returning Result<NvLinkDomainId, ChassisNmxCUnreachableReason>. Preserve each
caller’s existing failure policy: the group path must retain metrics and
PendingNullNvlinkObservation queueing, while
observe_and_record_rack_switch_domain_uuid logs a warning; retain per-step error
details by returning or otherwise preserving the underlying errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b48adb77-32fe-4dba-9e26-7a3951f86299
📒 Files selected for processing (6)
crates/api-db/migrations/20260801001826_switch_nvlink_domain_uuid.sqlcrates/api-db/src/switch.rscrates/api-model/src/switch/mod.rscrates/nvlink-manager/src/lib.rscrates/rpc/proto/forge.protocrates/rpc/src/model/switch.rs
chet
left a comment
chet
left a comment
There was a problem hiding this comment.
Clean! But yeah [for now] we need to sync up forge.proto for REST too; working on converging down to one.
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Ahh, got it. Thanks! |
Yeah, and then the PR gets huge, haha. |
2 participants
Switch health metrics and logs need the NVLink domain UUID to identify failures at the cluster level. This PR persists the NVLink domain UUID reported by NMX-C and exposes it through the NICo API.
Related issues
Supports #4397
Type of Change
Breaking Changes
Testing