perf(nvlink-manager): process partition monitor groups concurrently - #4695
perf(nvlink-manager): process partition monitor groups concurrently#4695rtamma-nv wants to merge 6 commits into
Conversation
- Batch chassis NMX-C endpoint lookups instead of one query per chassis serial - Pre-split machine_nvlink_info into per-group disjoint shards before the concurrent fan-out; groups are mutually exclusive so no locking is needed - Add a configurable concurreny limit (default 16) Signed-off-by: Roopesh Tamma <rtamma@nvidia.com> Signed-off-by: Roopesh Tamma <rtamma@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe NVLink monitor now performs batched endpoint lookup and bounded concurrent machine-group processing. Group-local results and metrics are merged after processing. Configuration sets a non-zero concurrency limit with a default of 16. ChangesNVLink monitor concurrency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MonitorIteration
participant NvlinkNmxcEndpoints
participant ProcessMachineGroupInput
participant NvlPartitionMonitorMetrics
MonitorIteration->>NvlinkNmxcEndpoints: find_by_chassis_serials(chassis_serials)
NvlinkNmxcEndpoints-->>MonitorIteration: matching endpoint rows
MonitorIteration->>ProcessMachineGroupInput: process owned group input under semaphore limit
ProcessMachineGroupInput-->>MonitorIteration: GroupResult
MonitorIteration->>NvlPartitionMonitorMetrics: merge_from(group metrics)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/nvlink-manager/src/lib.rs (1)
1630-1668: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEvery group repeats the same global logical-partition cleanup, now concurrently.
db_nvl_logical_partitionsis the complete, iteration-wide list. Lines 1374 and 1400 pass that same list to every group input, and Line 1636 forwards it intoCheckPartitionsInput.update_db_with_nmx_c_operationsthen walks the full list and callsdb::nvl_logical_partition::final_deletefor each partition marked as deleted.Before this change the groups ran sequentially, so the first group deleted the rows and later groups deleted nothing. Now up to
partition_monitor_max_concurrent_groupstransactions issue the identicalDELETEfor the same rows at the same time. At best this is duplicated work plus row-lock contention. Iffinal_deletetouches several tables, the concurrent transactions can also acquire locks in different orders and deadlock, which would fail an otherwise healthy group.Perform the deleted-logical-partition cleanup once per iteration in
run_single_iteration_inner, after the fan-in, instead of once per group.#!/bin/bash # Description: Inspect final_delete for logical partitions and any cascading writes. set -euo pipefail fd -t f 'nvl_logical_partition.rs' crates/api-db --exec cat -n {}🤖 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 1630 - 1668, Move deleted logical-partition cleanup out of per-group processing and perform it once per iteration in run_single_iteration_inner after all groups have been joined. Stop passing the complete db_nvl_logical_partitions list through each CheckPartitionsInput and remove its per-group handling from update_db_with_nmx_c_operations, then invoke the existing final_delete flow once using the iteration-wide list.
🧹 Nitpick comments (4)
crates/api-db/src/nvlink_nmxc_endpoints.rs (1)
110-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a table for the three
find_by_chassis_serialstests.The three tests invoke the same operation with different serial inputs and expectations. A single fixture plus a table of
(input_serials, expected_serials)cases would reduce duplication and keep one database round of setup. Keep the standalone form if per-case row seeding must differ.As per coding guidelines: "Use a table whenever two or more tests invoke the same operation with different inputs, but keep genuinely distinct tests standalone."
🤖 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/api-db/src/nvlink_nmxc_endpoints.rs` around lines 110 - 168, Consolidate the three find_by_chassis_serials tests into one table-driven test with shared fixture rows and cases containing input serials and expected serials. Iterate over each case and assert the returned results, preserving the empty-input and unknown-serial behaviors; keep separate setup only if a case genuinely requires different seeded rows.Source: Coding guidelines
crates/nvlink-manager/src/lib.rs (3)
3837-3851: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test re-implements the production fan-in instead of exercising it.
The comment states that the block mirrors the fold in
run_single_iteration_inner. The test therefore validates a copy of the logic, not the logic itself. If the production fold changes, this test still passes.Extract the fold into a small function, for example
fn fold_group_results(metrics: &mut NvlPartitionMonitorMetrics, results: Vec<GroupResult>) -> (usize, Vec<PendingNullNvlinkObservation>), and call it from bothrun_single_iteration_innerand this test.🤖 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 3837 - 3851, Extract the duplicated group-result fan-in logic from `run_single_iteration_inner` and this test into a shared helper such as `fold_group_results`, returning the completed-operation count and pending null observations while updating `NvlPartitionMonitorMetrics`. Replace both existing folds with calls to this helper, preserving the current metric initialization and merge behavior.
1471-1486: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
early_return!macro with an explicit helper.The macro expands to a
returnfrom the enclosing function. This hides control flow from the reader and from tooling. A small associated function that builds the failureGroupResultkeeps the control flow explicit at each call site while removing the same duplication.As per coding guidelines: "Prefer simple, explicit Rust code over clever or heavily abstracted code; add abstractions only for real requirements."♻️ Proposed refactor
- macro_rules! early_return { - ($reason:expr) => {{ - Self::queue_null_nvlink_status_observation( - &mut null_observations, - &group_id, - group_type, - snapshots, - $reason, - ); - return GroupResult { - completed_operations: 0, - null_observations, - partial_metrics: group_metrics, - }; - }}; - } + // Builds the terminal `GroupResult` for a group that cannot reach NMX-C. + let unreachable = |mut null_observations: Vec<PendingNullNvlinkObservation>, + group_metrics: NvlPartitionMonitorMetrics, + reason: ChassisNmxCUnreachableReason| { + Self::queue_null_nvlink_status_observation( + &mut null_observations, + &group_id, + group_type, + snapshots, + reason, + ); + GroupResult { + completed_operations: 0, + null_observations, + partial_metrics: group_metrics, + } + };Each site then reads
return unreachable(null_observations, group_metrics, ChassisNmxCUnreachableReason::NoEndpoint);.🤖 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 1471 - 1486, Replace the early_return! macro with a small associated helper that records the null NVLink status observation and constructs the failure GroupResult without performing the return itself. Update every macro call site to explicitly return the helper’s result, preserving the existing completed_operations, null_observations, and partial_metrics values and each call’s reason.Source: Coding guidelines
1404-1419: 🚀 Performance & Scalability | 🔵 TrivialReserve database pool capacity for the partition monitor.
max_database_connectionsdefaults to 1000 and is 64 in development, while the monitor limit independently defaults to 16. The group transactions run sequentially, so each active group uses at most one connection from these operations. If an operator sets the pool below 16, the monitor can still consume the entire pool. Document and validate a limit that reserves connections for other components.🤖 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 1404 - 1419, Document and validate the relationship between max_database_connections and partition_monitor_max_concurrent_groups so the monitor cannot consume the entire database pool. Require enough reserved capacity for other components, rejecting invalid configuration during startup while preserving the existing semaphore-based group processing.
🤖 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 `@crates/nvlink-manager/src/config.rs`:
- Around line 57-72: Prevent zero from reaching the partition monitor semaphore:
update NvLinkConfig::partition_monitor_max_concurrent_groups to use a non-zero
representation and adjust serde defaults accordingly, then pass its validated
value via get() when constructing the semaphore in run_single_iteration_inner.
Preserve the default of 16 and ensure deserialization rejects or otherwise
cannot produce zero.
In `@crates/nvlink-manager/src/metrics.rs`:
- Line 304: Update the release notes to document that
carbide_nvlink_partition_monitor_machine_status_updates_count now reports the
total machine NVLink status updates across all groups per iteration, rather than
the final group’s count. Confirm that existing dashboards and alert thresholds
account for the increased multi-group values.
---
Outside diff comments:
In `@crates/nvlink-manager/src/lib.rs`:
- Around line 1630-1668: Move deleted logical-partition cleanup out of per-group
processing and perform it once per iteration in run_single_iteration_inner after
all groups have been joined. Stop passing the complete db_nvl_logical_partitions
list through each CheckPartitionsInput and remove its per-group handling from
update_db_with_nmx_c_operations, then invoke the existing final_delete flow once
using the iteration-wide list.
---
Nitpick comments:
In `@crates/api-db/src/nvlink_nmxc_endpoints.rs`:
- Around line 110-168: Consolidate the three find_by_chassis_serials tests into
one table-driven test with shared fixture rows and cases containing input
serials and expected serials. Iterate over each case and assert the returned
results, preserving the empty-input and unknown-serial behaviors; keep separate
setup only if a case genuinely requires different seeded rows.
In `@crates/nvlink-manager/src/lib.rs`:
- Around line 3837-3851: Extract the duplicated group-result fan-in logic from
`run_single_iteration_inner` and this test into a shared helper such as
`fold_group_results`, returning the completed-operation count and pending null
observations while updating `NvlPartitionMonitorMetrics`. Replace both existing
folds with calls to this helper, preserving the current metric initialization
and merge behavior.
- Around line 1471-1486: Replace the early_return! macro with a small associated
helper that records the null NVLink status observation and constructs the
failure GroupResult without performing the return itself. Update every macro
call site to explicitly return the helper’s result, preserving the existing
completed_operations, null_observations, and partial_metrics values and each
call’s reason.
- Around line 1404-1419: Document and validate the relationship between
max_database_connections and partition_monitor_max_concurrent_groups so the
monitor cannot consume the entire database pool. Require enough reserved
capacity for other components, rejecting invalid configuration during startup
while preserving the existing semaphore-based group processing.
🪄 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: CHILL
Plan: Enterprise
Run ID: 33e2b7d9-2112-4033-b577-af45726e117d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/api-db/src/nvlink_nmxc_endpoints.rscrates/nvlink-manager/Cargo.tomlcrates/nvlink-manager/src/config.rscrates/nvlink-manager/src/lib.rscrates/nvlink-manager/src/metrics.rscrates/nvlink-manager/src/nmx_c_endpoint.rs
💤 Files with no reviewable changes (1)
- crates/nvlink-manager/src/nmx_c_endpoint.rs
…r_mzx_concurrent_groups Signed-off-by: Roopesh Tamma <rtamma@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/nvlink-manager/src/config.rs (3)
70-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the default helper’s visibility.
default_partition_monitor_max_concurrent_groupshas no external callers. Changepub const fntoconst fn; the serde default and local tests remain valid.🤖 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/config.rs` at line 70, Change the visibility of default_partition_monitor_max_concurrent_groups from pub const fn to const fn, leaving its const behavior and existing serde default and local test usage unchanged.Source: Coding guidelines
71-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unnecessary
unsafeconstructor.
NonZeroUsize::new(16).expect("16 is non-zero")is const-compatible with Rust 1.96.0 and preserves the invariant without an unsafe block.🤖 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/config.rs` around lines 71 - 72, Replace the unsafe NonZeroUsize::new_unchecked(16) construction with the const-compatible safe NonZeroUsize::new(16).expect(...) form, preserving the existing value and invariant without an unsafe block.Source: Coding guidelines
60-62: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the default helper private and remove the unnecessary
unsafeconstruction. Only local configuration code and tests usedefault_partition_monitor_max_concurrent_groups;pubis not required. Use safeNonZeroUsize::new(16)construction. The semaphore consumer already calls.get().🤖 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/config.rs` around lines 60 - 62, Update NvLinkConfig::default_partition_monitor_max_concurrent_groups to be private, since it is only used locally and by tests. Replace its unsafe NonZeroUsize construction with the safe NonZeroUsize::new(16) approach, preserving the existing default value and semaphore consumer’s .get() usage.Source: Path instructions
🤖 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/config.rs`:
- Line 70: Change the visibility of
default_partition_monitor_max_concurrent_groups from pub const fn to const fn,
leaving its const behavior and existing serde default and local test usage
unchanged.
- Around line 71-72: Replace the unsafe NonZeroUsize::new_unchecked(16)
construction with the const-compatible safe NonZeroUsize::new(16).expect(...)
form, preserving the existing value and invariant without an unsafe block.
- Around line 60-62: Update
NvLinkConfig::default_partition_monitor_max_concurrent_groups to be private,
since it is only used locally and by tests. Replace its unsafe NonZeroUsize
construction with the safe NonZeroUsize::new(16) approach, preserving the
existing default value and semaphore consumer’s .get() usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e14a452c-dffc-45d5-a269-bad90a1ff81e
📒 Files selected for processing (2)
crates/nvlink-manager/src/config.rscrates/nvlink-manager/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/nvlink-manager/src/lib.rs
…to NvLinkConfig table Signed-off-by: Roopesh Tamma <rtamma@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/api-core/src/cfg/README.md`:
- Line 353: Update the partition_monitor_max_concurrent_groups entry in the
README configuration table to explicitly document that omitted values default to
16, positive usize values are accepted, and 0 is rejected during configuration
deserialization; then inspect the generated admin-UI table to ensure the
corresponding documentation is updated consistently.
🪄 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: CHILL
Plan: Enterprise
Run ID: 47446d1e-9665-46a1-8ffe-5f3b01676610
📒 Files selected for processing (1)
crates/api-core/src/cfg/README.md
Signed-off-by: Roopesh Tamma <rtamma@nvidia.com>
|
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. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4695.docs.buildwithfern.com/infra-controller |
Signed-off-by: Roopesh Tamma <rtamma@nvidia.com>
Signed-off-by: Roopesh Tamma <rtamma@nvidia.com>
1 participant
Nvlink partition monitor currently works by fetching all MNNVL capable managed hosts, groups them by rack/cluster_chassis_serial and works on each group serially.
For sites with a large number of managed hosts, each iteration of partition monitor incurs significant memory footprint, latency, and DB query load.
Need to parallelize and optimize nvlink partition monitor handling to work at scale.
Related issues
#4279
Type of Change
Breaking Changes
Testing
Additional Notes