perf(fabric): stop rebuilding fabric-manager clients for every call#3256
perf(fabric): stop rebuilding fabric-manager clients for every call#3256chet wants to merge 1 commit into
Conversation
|
@coderabbitai full_review, thanks! |
|
🐇✨ ✅ Action performedFull review finished. |
|
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
WalkthroughThis PR adds IB fabric test support, reuses IB fabric clients in the monitor and REST manager, defers partition-controller fabric connections until needed, and refactors NMXC client pooling around endpoint caches with TLS staleness checks. ChangesConnection reuse across IB fabric, partition controller, and NMXC
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 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: 1
🧹 Nitpick comments (3)
crates/ib-partition-controller/src/handler.rs (1)
287-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for the
Ready+ active (not-deleted) path.The test suite exercises
Provisioning,Error,Ready(marked-deleted), andDeleting(live network), but never theReadybranch's "not marked as deleted" sub-branch (Lines 162-243) — the second call site ofconnect_ib_fabric. That's the primary QoS-reconciliation path this PR touches; a regression there (e.g., accidentally building an extra client, or skipping the connect) wouldn't be caught.✅ Suggested additional test
+ #[tokio::test] + async fn ready_active_builds_one_ufm_client() { + let (builds, outcome) = run_handler( + IBPartitionControllerState::Ready, + make_partition(Some(0x101), false), + ) + .await; + + assert!(outcome.is_ok()); + assert_eq!(builds, 1, "the arm that queries UFM for QoS builds one client"); + }🤖 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/ib-partition-controller/src/handler.rs` around lines 287 - 543, Add a test covering the Ready state when the partition is not deleted, since this is the untested QoS-reconciliation path in IBPartitionStateHandler::handle_object_state. Reuse the existing test helpers (make_partition, run_handler, CountingFabricManager, StubIBFabric) and assert the Ready/non-deleted branch transitions as expected while exercising the second connect_ib_fabric call site. Also verify the client build count matches the intended behavior so a regression in client creation or skipping the connect is caught.crates/libnmxc/src/lib.rs (1)
277-422: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider collapsing the fifteen delegating RPC methods into a declarative macro.
Every
Nmxcmethod here follows an identical three-line contract: invokeself.inner, feed the result throughnote_result, then return it. Beyond the ~140 lines of ceremony, the pattern is load-bearing — a future method added to theNmxctrait that omits thenote_resultcall would silently disable transport-error eviction for that RPC, with no compile-time signal. A smallmacro_rules!makes the delegation uniform and self-enforcing.♻️ Illustrative delegation macro
macro_rules! delegate_with_eviction { ($( async fn $name:ident(&mut self $(, $arg:ident: $ty:ty)*) -> $ret:ty; )*) => { $( async fn $name(&mut self $(, $arg: $ty)*) -> $ret { let result = self.inner.$name($($arg),*).await; self.note_result(&result); result } )* }; } #[async_trait::async_trait] impl Nmxc for PooledNmxc { delegate_with_eviction! { async fn hello(&mut self, gateway_id: &str) -> Result<nmxc_model::ServerHello, NmxcError>; async fn get_domain_properties(&mut self, context: Option<nmxc_model::Context>, gateway_id: &str) -> Result<nmxc_model::DomainProperties, NmxcError>; // ... remaining RPCs ... } }Please confirm the macro expands cleanly through
#[async_trait::async_trait]in this toolchain before adopting it, as the attribute macro rewrites the method signatures.🤖 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/libnmxc/src/lib.rs` around lines 277 - 422, The `impl Nmxc for PooledNmxc` block repeats the same delegate-and-evict pattern across every RPC method, so replace the manual boilerplate with a `macro_rules!` helper that expands each method to call `self.inner.<method>`, pass the `Result` through `note_result`, and return it. Keep the macro definition close to `PooledNmxc` so it can cover all the `Nmxc` trait methods consistently, and verify it works cleanly with `#[async_trait::async_trait]` before wiring the existing methods through the macro.Source: Path instructions
crates/ib-fabric/src/lib.rs (1)
614-676: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit these log sites as structured fields rather than interpolated strings.
fabric,guid,pkey, and the error are interpolated into the message across Lines 614, 618, 632 and 674-676, which defeats logfmt-based filtering. Promote them to attributes so operators can query byfabric/pkey.As per coding guidelines: "prefer placing common fields as attributes passed to tracing functions instead of using string interpolation" (e.g.
tracing::error!(%fabric, %pkey, error=%e, "...")).🤖 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/ib-fabric/src/lib.rs` around lines 614 - 676, The bind/unbind logging in the ib-fabric flow is using interpolated strings instead of structured tracing fields, which makes `fabric`, `guid`, `pkey`, and the error harder to filter. Update the `client_for_fabric` bind and unbind paths in the report-processing loop to pass these values as tracing attributes on the `tracing::error!`/`warn!` calls rather than embedding them in the message. Keep the human-readable message short, and expose the key values via fields like `%fabric`, `%guid`, `%pkey`, and `error=%e` so logfmt-based querying works reliably.Source: Coding guidelines
🤖 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/ib-fabric/src/lib.rs`:
- Around line 384-391: The error branch in the client-creation path is logging
the wrong operation name: in the match on fabric_manager.new_client(...),
replace the "IB fabric health check failed" message with one that clearly
identifies the new_client/client-construction failure, while keeping the same
fabric, endpoints, and error fields. Use the unique symbols
fabric_manager.new_client and fabric_metrics.fabric_error to locate the branch,
and leave the actual health-check failure log elsewhere unchanged.
---
Nitpick comments:
In `@crates/ib-fabric/src/lib.rs`:
- Around line 614-676: The bind/unbind logging in the ib-fabric flow is using
interpolated strings instead of structured tracing fields, which makes `fabric`,
`guid`, `pkey`, and the error harder to filter. Update the `client_for_fabric`
bind and unbind paths in the report-processing loop to pass these values as
tracing attributes on the `tracing::error!`/`warn!` calls rather than embedding
them in the message. Keep the human-readable message short, and expose the key
values via fields like `%fabric`, `%guid`, `%pkey`, and `error=%e` so
logfmt-based querying works reliably.
In `@crates/ib-partition-controller/src/handler.rs`:
- Around line 287-543: Add a test covering the Ready state when the partition is
not deleted, since this is the untested QoS-reconciliation path in
IBPartitionStateHandler::handle_object_state. Reuse the existing test helpers
(make_partition, run_handler, CountingFabricManager, StubIBFabric) and assert
the Ready/non-deleted branch transitions as expected while exercising the second
connect_ib_fabric call site. Also verify the client build count matches the
intended behavior so a regression in client creation or skipping the connect is
caught.
In `@crates/libnmxc/src/lib.rs`:
- Around line 277-422: The `impl Nmxc for PooledNmxc` block repeats the same
delegate-and-evict pattern across every RPC method, so replace the manual
boilerplate with a `macro_rules!` helper that expands each method to call
`self.inner.<method>`, pass the `Result` through `note_result`, and return it.
Keep the macro definition close to `PooledNmxc` so it can cover all the `Nmxc`
trait methods consistently, and verify it works cleanly with
`#[async_trait::async_trait]` before wiring the existing methods through the
macro.
🪄 Autofix (Beta)
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: 36f3813f-6eb3-4e9a-9c29-77805ef40c13
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/ib-fabric/Cargo.tomlcrates/ib-fabric/src/ib/mod.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/Cargo.tomlcrates/ib-partition-controller/src/handler.rscrates/libnmxc/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/ib-partition-controller/src/handler.rs (1)
287-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing positive-path test for the
Readybranch's UFM build count.The test suite verifies exactly one client build for
Deletingwith a live network (Line 533-542), but the symmetricReady(pkey present, not marked deleted) success path — which also callsconnect_ib_fabricand additionally reuses the client forupdate_partition_qos_conf— has no equivalent build-count assertion. Only the early-exit "marked deleted"Readycase is tested for 0 builds. Given this branch is central to this PR's "single client reused across query + update" change, a positive test would close the loop.🧪 Suggested additional test
+ #[tokio::test] + async fn ready_with_pkey_builds_one_ufm_client_and_reuses_it() { + let (builds, outcome) = run_handler( + IBPartitionControllerState::Ready, + make_partition(Some(0x101), false), + ) + .await; + + assert!(matches!(outcome, Ok(StateHandlerOutcome::DoNothing { .. }))); + assert_eq!( + builds, 1, + "the Ready arm builds one client and reuses it for get_ib_network + update_partition_qos_conf" + ); + }🤖 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/ib-partition-controller/src/handler.rs` around lines 287 - 543, The test coverage is missing the positive-path assertion for the Ready branch where a live partition should build exactly one UFM client and reuse it for both connect and update. Add a new test alongside provisioning_builds_no_ufm_client and deleting_with_live_network_builds_one_ufm_client that drives IBPartitionStateHandler::handle_object_state through IBPartitionControllerState::Ready with make_partition(Some(...), false), and assert the outcome is the expected transition while CountingFabricManager::build_count() is 1. Use run_handler, make_partition, and IBPartitionStateHandler as the key entry points.crates/libnmxc/src/lib.rs (1)
222-229: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftCoalesce concurrent cache misses per endpoint.
Two concurrent
create_clientcalls for the same uncached URI can both miss, both connect, and then race to insert. That preserves correctness, but it weakens the PR’s connection-reuse goal under concurrent callers. Consider caching an in-flight per-URI initialization, e.g. aOnceCell<Channel>entry, so concurrent misses share the same connect attempt.Potential direction
-type ChannelCache = Arc<Mutex<HashMap<String, Channel>>>; +type ChannelCache = Arc<Mutex<HashMap<String, Arc<tokio::sync::OnceCell<Channel>>>>>; async fn channel_for(&self, endpoint: &Endpoint, uri: &str) -> Result<Channel, NmxcError> { - if let Some(channel) = lock_channels(&self.channels).get(uri) { - return Ok(channel.clone()); - } - - let channel = self.connector.connect(endpoint).await?; - lock_channels(&self.channels).insert(uri.to_string(), channel.clone()); - Ok(channel) + let cell = { + let mut channels = lock_channels(&self.channels); + Arc::clone( + channels + .entry(uri.to_string()) + .or_insert_with(|| Arc::new(tokio::sync::OnceCell::new())), + ) + }; + + cell.get_or_try_init(|| async { self.connector.connect(endpoint).await }) + .await + .cloned() }As per path instructions,
crates/**/*.rsreviews should prioritize behavior, concurrency, resource lifetimes, and missing tests over style-only comments.🤖 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/libnmxc/src/lib.rs` around lines 222 - 229, The channel cache in channel_for still allows concurrent uncached lookups for the same URI to race, so multiple create_client calls can perform duplicate connector.connect attempts before insert. Change the per-URI caching in channel_for (and the channels map used by create_client) to store an in-flight initialization, such as a OnceCell or similar shared async entry, so the first caller performs the connect and all concurrent callers await the same result. Update the cache insertion/retrieval logic to reuse that shared initialization path and add a concurrency-focused test covering simultaneous create_client calls for the same endpoint/URI.Source: Path instructions
crates/ib-fabric/src/lib.rs (1)
614-675: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured tracing fields for the new apply-path logs.
The new warn/error logs interpolate
fabric,guid,pkey, anderrorinto message strings, making logfmt querying harder.Suggested logfmt-friendly rewrite
- tracing::warn!("Missing pkey {pkey} does not map to a Partition ID"); + tracing::warn!(%pkey, "Missing pkey does not map to a Partition ID"); @@ - tracing::warn!("Missing pkey {pkey} does not map to a Partition"); + tracing::warn!(%pkey, "Missing pkey does not map to a Partition"); @@ - tracing::error!("Failed to bind {guid} to pkey {pkey} on fabric {fabric}: {e}"); + tracing::error!( + %fabric, + %guid, + %pkey, + error = %e, + "Failed to bind GUID to pkey" + ); @@ - tracing::error!( - "Failed to unbind {guid} from pkey {pkey} on fabric {fabric}: {e}" - ); + tracing::error!( + %fabric, + %guid, + %pkey, + error = %e, + "Failed to unbind GUID from pkey" + );As per coding guidelines, “For Rust logging, use plain
tracing::macros with structured fields for ordinary log messages.” As per path instructions, review Rust code against STYLE_GUIDE.md, including structured tracing fields.🤖 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/ib-fabric/src/lib.rs` around lines 614 - 675, The new apply-path logs in the bind/unbind flow should use structured tracing fields instead of interpolated strings. Update the `tracing::warn!` and `tracing::error!` calls in the `bind_ib_ports` and `unbind_ib_ports` paths to pass `fabric`, `guid`, `pkey`, and the error as named fields with a static message, so the logs remain queryable in logfmt. Keep the existing control flow and status handling in `apply`/the surrounding loop, just change the logging style to match the Rust tracing conventions used elsewhere.Sources: Coding guidelines, 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.
Inline comments:
In `@crates/libnmxc/src/lib.rs`:
- Around line 638-647: The refused_endpoint helper in lib.rs is flaky because it
drops the TcpListener before the test uses the URI, allowing the port to be
rebound and the transport error to disappear. Update refused_endpoint (or the
related hello() test setup) to use a deterministic failure path, such as a local
accept-and-close listener whose task is awaited, or a test double that forces
PooledNmxc::note_result to observe a transport error. Keep the endpoint-building
logic in refused_endpoint, but ensure the failure mode cannot race with the
caller.
---
Nitpick comments:
In `@crates/ib-fabric/src/lib.rs`:
- Around line 614-675: The new apply-path logs in the bind/unbind flow should
use structured tracing fields instead of interpolated strings. Update the
`tracing::warn!` and `tracing::error!` calls in the `bind_ib_ports` and
`unbind_ib_ports` paths to pass `fabric`, `guid`, `pkey`, and the error as named
fields with a static message, so the logs remain queryable in logfmt. Keep the
existing control flow and status handling in `apply`/the surrounding loop, just
change the logging style to match the Rust tracing conventions used elsewhere.
In `@crates/ib-partition-controller/src/handler.rs`:
- Around line 287-543: The test coverage is missing the positive-path assertion
for the Ready branch where a live partition should build exactly one UFM client
and reuse it for both connect and update. Add a new test alongside
provisioning_builds_no_ufm_client and
deleting_with_live_network_builds_one_ufm_client that drives
IBPartitionStateHandler::handle_object_state through
IBPartitionControllerState::Ready with make_partition(Some(...), false), and
assert the outcome is the expected transition while
CountingFabricManager::build_count() is 1. Use run_handler, make_partition, and
IBPartitionStateHandler as the key entry points.
In `@crates/libnmxc/src/lib.rs`:
- Around line 222-229: The channel cache in channel_for still allows concurrent
uncached lookups for the same URI to race, so multiple create_client calls can
perform duplicate connector.connect attempts before insert. Change the per-URI
caching in channel_for (and the channels map used by create_client) to store an
in-flight initialization, such as a OnceCell or similar shared async entry, so
the first caller performs the connect and all concurrent callers await the same
result. Update the cache insertion/retrieval logic to reuse that shared
initialization path and add a concurrency-focused test covering simultaneous
create_client calls for the same endpoint/URI.
🪄 Autofix (Beta)
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: 62651a40-0b71-4dc5-9ea6-fa51fdbc6313
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/ib-fabric/Cargo.tomlcrates/ib-fabric/src/ib/mod.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/Cargo.tomlcrates/ib-partition-controller/src/handler.rscrates/libnmxc/src/lib.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
@coderabbitai full_review, thanks! |
|
🐇✨ ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/libnmxc/src/lib.rs (1)
256-274: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConcurrent cache misses on the same endpoint can trigger duplicate connects.
channel_forreads the cache, drops the lock, then connects and re-acquires the lock to insert — there is no per-endpoint synchronization across that window. Two concurrentcreate_clientcalls for the same uncached (or just-evicted) endpoint will both miss the cache, both callself.connector.connect(...), and the loser's channel is silently discarded when the winner's insert overwrites it. Functionally harmless (the discarded channel just gets dropped), but it undercuts the PR's stated goal of building at most one client per fabric per pass, especially in the eviction path where a rotation could briefly cause a connect storm from multiple waiting callers.Consider guarding the connect-and-insert path with a per-endpoint lock (e.g., an
Entry-style API usingtokio::sync::OnceCellper URI, or a dedicatedtokio::sync::Mutexper endpoint) so concurrent callers for the same endpoint await one connect instead of racing.♻️ Sketch: single-flight per-endpoint connect
-type ChannelCache = Arc<Mutex<HashMap<String, (Channel, SystemTime)>>>; +type ChannelCache = Arc<Mutex<HashMap<String, Arc<tokio::sync::OnceCell<(Channel, SystemTime)>>>>>;The exact shape depends on how staleness re-checks should interact with the single-flight cell, but the key idea is: reserve the cache slot before connecting, so concurrent callers await the same in-flight connect instead of each starting their own.
🤖 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/libnmxc/src/lib.rs` around lines 256 - 274, `channel_for` currently releases the cache lock before calling `self.connector.connect`, so concurrent callers for the same URI can both miss and race to create duplicate channels. Update the caching flow in `channel_for` (and the `self.channels`/`lock_channels` access pattern) to use per-URI single-flight synchronization so only one connect runs for a given endpoint while others await it. Preserve the existing stale-entry recheck and eviction behavior, but ensure the connect-and-insert path is guarded by a per-endpoint lock or `OnceCell`-style in-flight entry rather than a read-then-connect race.
🤖 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/libnmxc/src/lib.rs`:
- Around line 256-274: `channel_for` currently releases the cache lock before
calling `self.connector.connect`, so concurrent callers for the same URI can
both miss and race to create duplicate channels. Update the caching flow in
`channel_for` (and the `self.channels`/`lock_channels` access pattern) to use
per-URI single-flight synchronization so only one connect runs for a given
endpoint while others await it. Preserve the existing stale-entry recheck and
eviction behavior, but ensure the connect-and-insert path is guarded by a
per-endpoint lock or `OnceCell`-style in-flight entry rather than a
read-then-connect race.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 63c4b91f-1c98-44d9-b36a-86a4c78b0d63
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/ib-fabric/Cargo.tomlcrates/ib-fabric/src/ib/fakes.rscrates/ib-fabric/src/ib/mod.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/Cargo.tomlcrates/ib-partition-controller/src/handler.rscrates/libnmxc/Cargo.tomlcrates/libnmxc/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/ib-fabric/src/ib/mod.rs
- crates/ib-fabric/src/lib.rs
- crates/ib-partition-controller/src/handler.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/ib-fabric/src/lib.rs (1)
639-717: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared bind/unbind change-recording logic.
The
missing_guid_pkeysandunexpected_guid_pkeysloops duplicate the same shape (get client, invoke operation, map status, recordmetrics.applied_changes) apart from the specificIBFabriccall andUfmOperationvariant. A small helper taking the operation as a closure/enum could remove this duplication.🤖 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/ib-fabric/src/lib.rs` around lines 639 - 717, The bind and unbind loops in the report handling code duplicate the same client lookup, operation execution, status mapping, and metrics recording flow. Extract that shared logic into a small helper in this area of ib-fabric::lib (near the report-processing loops), and pass in the specific IBFabric call and UfmOperation variant as parameters or a closure so only the bind/unbind-specific parts differ. Keep the existing tracing and AppliedChange recording behavior intact, but centralize the common change-recording path to avoid maintaining two nearly identical branches.
🤖 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/ib-fabric/src/lib.rs`:
- Line 642: The tracing calls in this area are interpolating values like pkey,
guid, fabric, and e directly into the message instead of emitting them as
structured fields. Update the relevant tracing::warn! and tracing::error! sites
in lib.rs, including the Partition ID lookup and related error paths, to pass
these values as attributes on the tracing macros so they can be filtered and
aggregated by key. Keep the human-readable message short and move the variable
data into fields, using the existing symbols around the affected logging calls.
- Around line 639-663: The batch processing in apply_guid_pkey_changes is
aborting on a single fabric client creation failure because
client_for_fabric(...).await? propagates the error instead of handling it per
fabric. Change both the bind and unbind paths to catch client_for_fabric errors
locally, log the failure, record an error status if needed, and continue to the
next fabric/report just like the existing bind_ib_ports and unbind_ib_ports
error handling does.
---
Nitpick comments:
In `@crates/ib-fabric/src/lib.rs`:
- Around line 639-717: The bind and unbind loops in the report handling code
duplicate the same client lookup, operation execution, status mapping, and
metrics recording flow. Extract that shared logic into a small helper in this
area of ib-fabric::lib (near the report-processing loops), and pass in the
specific IBFabric call and UfmOperation variant as parameters or a closure so
only the bind/unbind-specific parts differ. Keep the existing tracing and
AppliedChange recording behavior intact, but centralize the common
change-recording path to avoid maintaining two nearly identical branches.
🪄 Autofix (Beta)
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: e69f10cc-a8a9-423d-b503-9dddf0a8da19
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/ib-fabric/Cargo.tomlcrates/ib-fabric/src/ib/fakes.rscrates/ib-fabric/src/ib/mod.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/Cargo.tomlcrates/ib-partition-controller/src/handler.rscrates/libnmxc/Cargo.tomlcrates/libnmxc/src/lib.rs
|
@coderabbitai full_review, thanks! |
|
🐇✨ ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/ib-fabric/src/ib/fakes.rs (1)
70-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
StubIBFabric::get_ib_networkcan't exercise the Ready-arm QoS-update reuse path.This stub always returns
qos_conf: None. Inib-partition-controller/src/handler.rs, theReady(not-deleted) arm doesibnetwork.qos_conf.as_ref().ok_or_else(...)?immediately after callingget_ib_network— with this fake it will always short-circuit into theMissingDataerror, never reaching the QoS-comparison/update_partition_qos_confcall. As a result, the handler's own reuse claim (oneconnect_ib_fabric()client serving bothget_ib_networkandupdate_partition_qos_conf) has no test coverage; the four newhandler.rstests only coverProvisioning,Error,Ready+deleted, andDeleting.Consider returning a populated
qos_conf: Some(IBQosConf::default())(or accepting it as amake_partition-style parameter) so aReady-arm test can assertbuild_count() == 1across both UFM calls.🧪 Sketch of the fix
async fn get_ib_network( &self, pkey: u16, _options: GetPartitionOptions, ) -> Result<IBNetwork, IbError> { Ok(IBNetwork { name: "stub".to_string(), pkey, ipoib: false, - qos_conf: None, + qos_conf: Some(IBQosConf { + mtu: IBMtu::default(), + rate_limit: IBRateLimit::default(), + service_level: IBServiceLevel::default(), + }), associated_guids: Some(HashSet::new()), membership: None, }) }🤖 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/ib-fabric/src/ib/fakes.rs` around lines 70 - 83, The StubIBFabric::get_ib_network fake always returns qos_conf as None, which prevents the Ready arm in ib-partition-controller/src/handler.rs from reaching the QoS comparison and update_partition_qos_conf path. Update StubIBFabric::get_ib_network in ib/fakes.rs so it can return a populated QoS config (for example a default IBQosConf, or make it configurable like make_partition) while keeping the other fields intact. This will let a Ready-state handler test exercise the reuse path and verify the single-client behavior across get_ib_network and update_partition_qos_conf, including build_count() == 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/libnmxc/src/lib.rs`:
- Around line 119-146: `NmxcTlsConfig::material_newer_than` is doing synchronous
filesystem metadata checks on the async hot path. Update this stale-check logic
to avoid blocking the executor by switching to an async-safe approach such as
`tokio::fs::metadata` or moving the sync work into `spawn_blocking`, and make
sure the call path from `is_stale`/`channel_for` and `create_client` uses that
async flow consistently with `build_https_tls_config`. Keep the existing
behavior and logging, but ensure the TLS-material freshness check no longer
performs `std::fs::metadata(...).modified()` directly during cache hits.
- Around line 256-274: The channel_for cache path currently releases the
channels lock before awaiting connector.connect, which allows concurrent
create_client calls for the same URI to race and duplicate TLS handshakes.
Update channel_for in crates/libnmxc/src/lib.rs to serialize in-flight connects
per URI by introducing a per-URI shared in-progress state (such as a OnceCell or
shared future) so cache misses and stale-entry rebuilds reuse the same connect
work instead of creating multiple channels. Keep the existing cache behavior
around Channel, created timestamps, and stale eviction, but ensure only one
connect runs per URI at a time.
---
Nitpick comments:
In `@crates/ib-fabric/src/ib/fakes.rs`:
- Around line 70-83: The StubIBFabric::get_ib_network fake always returns
qos_conf as None, which prevents the Ready arm in
ib-partition-controller/src/handler.rs from reaching the QoS comparison and
update_partition_qos_conf path. Update StubIBFabric::get_ib_network in
ib/fakes.rs so it can return a populated QoS config (for example a default
IBQosConf, or make it configurable like make_partition) while keeping the other
fields intact. This will let a Ready-state handler test exercise the reuse path
and verify the single-client behavior across get_ib_network and
update_partition_qos_conf, including build_count() == 1.
🪄 Autofix (Beta)
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: 412b93cc-cb94-414b-9135-7a395d31fd18
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/ib-fabric/Cargo.tomlcrates/ib-fabric/src/ib/fakes.rscrates/ib-fabric/src/ib/mod.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/Cargo.tomlcrates/ib-partition-controller/src/handler.rscrates/libnmxc/Cargo.tomlcrates/libnmxc/src/lib.rs
|
@coderabbitai full_review, thanks! |
|
🐇✨ ✅ Action performedFull review finished. |
|
I think we just need one client ("channel") for the whole application. Auth is credentials that can be passed to each request, so maybe you'd want to have a wrapper of That is unless we'd do mTLS and there is a different client cert per UFM endpoint we are talking to, but I don't think that's the case. |
@Matthias247 Yeah I like it. Updated! The client is now used for the life of the application (one per fabric, since each fabric can have its own manager), and [only] gets rebuilt when its credentials (or endpoint) actually change; it reads the credentials from the secret manager each pass so rotations get picked up. There are also a few tests for the behavior too (same creds → same client, rotated creds → rebuilt, etc). |
Every operation against a fabric manager built a complete client from scratch: fetch credentials from the secret store, read TLS certificates off disk, open a fresh TCP+TLS connection, make one call, throw it all away. On a busy fabric that meant a few hundred client builds per monitor pass -- three per fabric plus one per port change -- and the NVLink manager reconnected to every chassis on every tick, which drops to once per second while changes are pending. The partition controller built a client on every reconcile whether or not the state arm talks to UFM at all. The goal is to build a client once and use it for everything it's needed for. A fabric's UFM client is now cached for the life of the process and rebuilt only when its credentials -- or its endpoint -- change: every pass still reads each fabric's credentials from the secret manager, so rotations are picked up promptly, while the per-pass memo keeps that to one read per fabric and shares the client across the health check, the reads, and every port change the pass makes. A client that lives across passes keeps its connections warm in its own pool, so a steady-state pass builds nothing and re-handshakes nothing. NMX-C channels are cached per endpoint across ticks and rebuilt when the TLS material on disk changes -- the same staleness policy the repo's gRPC clients use -- while dropped connections heal on their own. The partition controller connects only in the arms that actually query UFM, so a secrets hiccup no longer fails reconciles that never needed the fabric manager. - `ib-fabric`: one client per fabric, cached across monitor passes and rebuilt only on a credential or endpoint change; within a pass it is shared across the health check, port and partition reads, and port changes. - `ib-partition-controller`: connect (and read the manager config) only where the work uses them. - `libnmxc`: per-endpoint channel cache with a file-mtime staleness check on the TLS material. Counting tests pin the build counts: a full fabric pass builds exactly one client (was three plus one per port change), partition arms that skip UFM build none, one connect serves a cached endpoint, and rotating the certificate on disk triggers a rebuild. The client cache has its own contracts under test: while credentials are unchanged every call returns the same client, a rotated credential rebuilds it, and each fabric holds a separate entry. This supports NVIDIA#3212 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇✨ ✅ Action performedFull review finished. |
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/ib-fabric/src/ib/mod.rs`:
- Around line 183-187: The concurrent build path for the fabric client is not
fully safe because an older in-flight build can overwrite a newer cached client
after credentials rotate. Update the client creation/cache insertion logic in
ib::mod so that the result is revalidated against the latest credentials
immediately before storing it, or serialize per-fabric builds to prevent stale
inserts. Also add a concurrent rotation test that exercises the new_client path
used by crates/api-core/src/handlers/ib_fabric.rs and verifies the newer
credentials win under overlap.
🪄 Autofix (Beta)
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: 33aa9ded-23ed-494b-89b5-05171dd29c0c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/ib-fabric/Cargo.tomlcrates/ib-fabric/src/ib/fakes.rscrates/ib-fabric/src/ib/mod.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/Cargo.tomlcrates/ib-partition-controller/src/handler.rscrates/libnmxc/Cargo.tomlcrates/libnmxc/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- crates/libnmxc/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/ib-partition-controller/Cargo.toml
- crates/ib-fabric/Cargo.toml
- crates/ib-fabric/src/ib/fakes.rs
- crates/ib-fabric/src/lib.rs
- crates/libnmxc/src/lib.rs
- crates/ib-partition-controller/src/handler.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/libnmxc/src/lib.rs (1)
119-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the redundant
matchin the freshness check.The
Err(e) => Err(e)arm is an identity mapping; the whole expression reduces to a single combinator, which reads more clearly.♻️ Proposed simplification
- for path in paths.into_iter().flatten() { - let modified = match tokio::fs::metadata(path).await { - Ok(metadata) => metadata.modified(), - Err(e) => Err(e), - }; - if let Ok(mtime) = modified + for path in paths.into_iter().flatten() { + let modified = tokio::fs::metadata(path).await.and_then(|m| m.modified()); + if let Ok(mtime) = modified && mtime > created {🤖 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/libnmxc/src/lib.rs` around lines 119 - 148, The freshness check in NmxcTlsConfig::material_newer_than has a redundant Err(e) => Err(e) arm that is just an identity mapping. Simplify the tokio::fs::metadata(path).await handling to use a single combinator or direct result propagation while keeping the existing mtime comparison and tracing::info! behavior unchanged. Keep the logic inside material_newer_than intact and only collapse the unnecessary match for readability.crates/ib-partition-controller/src/handler.rs (1)
143-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
Ready(not-deleted) lazy-build path.Tests exist for every branch that builds zero clients, and for
Deletingbuilding one, but not for theReady/not-deleted branch (Lines 160-165) — the other arm this layer changed to lazily connect. Worth closing the gap so a future regression (e.g. reintroducing an eager build, or dropping the lazy connect) is caught.✅ Proposed test addition
#[tokio::test] async fn ready_not_deleted_builds_one_ufm_client() { let (builds, outcome) = run_handler( IBPartitionControllerState::Ready, make_partition(Some(0x101), false), ) .await; // StubIBFabric returns `qos_conf: None`, so this arm errors out on the // missing QoS data -- but the client must still be built exactly once. assert!(matches!( outcome, Err(StateHandlerError::MissingData { missing: "qos_conf", .. }) )); assert_eq!(builds, 1, "the arm that queries UFM builds one client"); }Also applies to: 338-399
🤖 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/ib-partition-controller/src/handler.rs` around lines 143 - 248, Add a test that covers the Ready state when the partition is not marked as deleted, since this is the lazy-connect path now handled in IBPartitionControllerState::Ready. Use the existing handler test helpers around run_handler and make_partition to assert that this branch still builds exactly one UFM/IB fabric client, even if the call later returns MissingData for qos_conf. This closes the gap alongside the Deleting and zero-client cases and protects the Ready/not-deleted flow from regressions in connect_ib_fabric or the Ready match arm.
🤖 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/ib-partition-controller/src/handler.rs`:
- Around line 143-248: Add a test that covers the Ready state when the partition
is not marked as deleted, since this is the lazy-connect path now handled in
IBPartitionControllerState::Ready. Use the existing handler test helpers around
run_handler and make_partition to assert that this branch still builds exactly
one UFM/IB fabric client, even if the call later returns MissingData for
qos_conf. This closes the gap alongside the Deleting and zero-client cases and
protects the Ready/not-deleted flow from regressions in connect_ib_fabric or the
Ready match arm.
In `@crates/libnmxc/src/lib.rs`:
- Around line 119-148: The freshness check in NmxcTlsConfig::material_newer_than
has a redundant Err(e) => Err(e) arm that is just an identity mapping. Simplify
the tokio::fs::metadata(path).await handling to use a single combinator or
direct result propagation while keeping the existing mtime comparison and
tracing::info! behavior unchanged. Keep the logic inside material_newer_than
intact and only collapse the unnecessary match for readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e5c42bf1-408d-49e1-b655-711d1958e6da
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/ib-fabric/Cargo.tomlcrates/ib-fabric/src/ib/fakes.rscrates/ib-fabric/src/ib/mod.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/Cargo.tomlcrates/ib-partition-controller/src/handler.rscrates/libnmxc/Cargo.tomlcrates/libnmxc/src/lib.rs
2 participants
Every operation against a fabric manager built a complete client from scratch: fetch credentials from the secret store, read TLS certificates off disk, open a fresh TCP+TLS connection, make one call, throw it all away. On a busy fabric that meant a few hundred client builds per monitor pass -- three per fabric plus one per port change -- and the NVLink manager reconnected to every chassis on every tick, which drops to once per second while changes are pending. The partition controller built a client on every reconcile whether or not the state arm talks to UFM at all.
The goal is to build a client once and use it for everything it's needed for. A fabric's UFM client is now cached for the life of the process and rebuilt only when its credentials -- or its endpoint -- change: every pass still reads each fabric's credentials from the secret manager, so rotations are picked up promptly, while the per-pass memo keeps that to one read per fabric and shares the client across the health check, the reads, and every port change the pass makes. A client that lives across passes keeps its connections warm in its own pool, so a steady-state pass builds nothing and re-handshakes nothing. NMX-C channels are cached per endpoint across ticks and rebuilt when the TLS material on disk changes -- the same staleness policy the repo's gRPC clients use -- while dropped connections heal on their own. The partition controller connects only in the arms that actually query UFM, so a secrets hiccup no longer fails reconciles that never needed the fabric manager.
ib-fabric: one client per fabric, cached across monitor passes and rebuilt only on a credential or endpoint change; within a pass it is shared across the health check, port and partition reads, and port changes.ib-partition-controller: connect (and read the manager config) only where the work uses them.libnmxc: per-endpoint channel cache with a file-mtime staleness check on the TLS material.Counting tests pin the build counts: a full fabric pass builds exactly one client (was three plus one per port change), partition arms that skip UFM build none, one connect serves a cached endpoint, and rotating the certificate on disk triggers a rebuild. The client cache has its own contracts under test: while credentials are unchanged every call returns the same client, a rotated credential rebuilds it, and each fabric holds a separate entry.
This supports #3212