fix(refactor): move endpoint refresh logic out of api handler#3868
fix(refactor): move endpoint refresh logic out of api handler#3868krish-nvidia wants to merge 1 commit into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbit
WalkthroughThe change centralizes endpoint exploration and report refreshes in ChangesEndpoint Exploration Service
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test 431a382 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 (8)
crates/api-core/src/tests/site_explorer.rs (3)
803-823: 📐 Maintainability & Code Quality | 🔵 TrivialRepeated spawn/wait/release boilerplate across contention tests.
This block, along with the near-identical setups at Lines 848-871 and Lines 1000-1024, repeats the same "block → spawn refresh → wait_until_started → release → join" sequence. See the consolidated comment for a suggested shared helper.
🤖 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-core/src/tests/site_explorer.rs` around lines 803 - 823, Extract the repeated block–spawn–wait–release–join sequence from this contention test and the analogous sections near the other refresh tests into a shared async test helper. Update each call site to use the helper while preserving the existing request parameters, error assertions, blocker lifecycle, and refresh-result propagation.
848-871: 📐 Maintainability & Code Quality | 🔵 TrivialSame spawn/wait/release pattern repeated.
Shares the boilerplate flagged at Lines 803-823; see the consolidated comment for a shared-helper suggestion.
🤖 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-core/src/tests/site_explorer.rs` around lines 848 - 871, Deduplicate the repeated spawn/wait/release setup in this test by reusing the shared helper introduced for the earlier refresh-lock scenario. Update the test around run_site_explorer_iteration to use that helper while preserving the assertion that no probe occurs during refresh and the existing refresh completion behavior.
1000-1024: 📐 Maintainability & Code Quality | 🔵 TrivialSame spawn/wait/release pattern repeated (third occurrence).
Shares the boilerplate flagged at Lines 803-823 and Lines 848-871; see the consolidated comment for a shared-helper suggestion.
🤖 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-core/src/tests/site_explorer.rs` around lines 1000 - 1024, Refactor this third spawn/wait/release sequence around refresh_endpoint_report into the shared helper requested by the consolidated review comment, reusing the existing endpoint-specific refresh synchronization helper from the earlier occurrences. Preserve the concurrent refresh of bmc_ip_a, the independent refresh and version assertion for bmc_ip_b, and the helper’s error propagation.crates/site-explorer/tests/integration/health_report.rs (1)
27-27: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated
EndpointExplorationService+SiteExplorer::newwiring.Same pattern as
crates/site-explorer/tests/integration/env.rs,crates/site-explorer/tests/integration/zero_dpu.rs, andcrates/test-harness/src/lib.rs. See the consolidated comment.Also applies to: 66-77
🤖 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/site-explorer/tests/integration/health_report.rs` at line 27, Consolidate the duplicated EndpointExplorationService and SiteExplorer::new setup in the health report integration test by reusing the shared test-harness wiring already used by env.rs, zero_dpu.rs, and crates/test-harness/src/lib.rs. Update the affected setup block (including lines 66-77) and remove the now-unnecessary direct import and construction.crates/site-explorer/tests/integration/zero_dpu.rs (1)
24-24: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated
EndpointExplorationService+SiteExplorer::newwiring.Same pattern as the other integration fixtures and
crates/test-harness/src/lib.rs. The customexplorations_per_runconfig here is exactly what the existingtest_site_explorer(test_harness, config)helper already parameterizes on — see the consolidated comment for the suggested fix.Also applies to: 57-61, 75-75
🤖 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/site-explorer/tests/integration/zero_dpu.rs` at line 24, Replace the duplicated EndpointExplorationService and SiteExplorer::new setup in the zero-DPU fixtures with the existing test_site_explorer(test_harness, config) helper from the test harness. Pass the current custom explorations_per_run configuration through that helper, and remove imports or local wiring that become unused.crates/site-explorer/tests/integration/env.rs (1)
22-22: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated
EndpointExplorationService+SiteExplorer::newwiring.Correct wiring, but identical to the same block in
crates/test-harness/src/lib.rs,crates/site-explorer/tests/integration/health_report.rs, andcrates/site-explorer/tests/integration/zero_dpu.rs. See the consolidated comment for a suggested fix.Also applies to: 63-72
🤖 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/site-explorer/tests/integration/env.rs` at line 22, Consolidate the duplicated EndpointExplorationService and SiteExplorer::new wiring used by the integration environment setup, test harness, health_report, and zero_dpu tests. Extract or reuse a shared helper for constructing this service and update the affected callers to use it, preserving the existing configuration and behavior.crates/test-harness/src/lib.rs (1)
26-26: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated
EndpointExplorationService+SiteExplorer::newwiring (canonical site).Correct, but this method duplicates the free function
carbide_site_explorer::test_support::test_site_explorerand 3 other test fixtures touched in this same PR. As per path instructions, test-support code should favor "reusable helpers" over copy-pasted fixture logic — worth consolidating now given this PR just proved how easy it is for these 5 copies to drift whenSiteExplorer::new's signature changes. See the consolidated comment for the suggested fix.Also applies to: 126-141
🤖 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/test-harness/src/lib.rs` at line 26, Replace the duplicated EndpointExplorationService and SiteExplorer::new setup in the test fixtures with the reusable carbide_site_explorer::test_support::test_site_explorer helper. Update all affected fixtures, including the canonical site setup, to call this helper and remove imports that become unused.Source: Path instructions
crates/site-explorer/src/endpoint_exploration_service.rs (1)
100-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
#[allow(clippy::too_many_arguments)].try_explore_endpointhas only five explicit parameters plusself, so it stays below Clippy’s default threshold; keeping the suppression only weakens lint hygiene.🤖 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/site-explorer/src/endpoint_exploration_service.rs` around lines 100 - 108, Remove the #[allow(clippy::too_many_arguments)] attribute immediately preceding try_explore_endpoint; leave the method signature and implementation unchanged.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/site-explorer/src/endpoint_exploration_service.rs`:
- Around line 174-192: Update the refresh probe setup in the endpoint
exploration method to honor override_target_port instead of hardcoding port 443
when constructing bmc_address. Reuse the existing periodic-path port selection
logic or pass the caller’s resolved SocketAddr through, while preserving the
existing try_explore_endpoint flow.
---
Nitpick comments:
In `@crates/api-core/src/tests/site_explorer.rs`:
- Around line 803-823: Extract the repeated block–spawn–wait–release–join
sequence from this contention test and the analogous sections near the other
refresh tests into a shared async test helper. Update each call site to use the
helper while preserving the existing request parameters, error assertions,
blocker lifecycle, and refresh-result propagation.
- Around line 848-871: Deduplicate the repeated spawn/wait/release setup in this
test by reusing the shared helper introduced for the earlier refresh-lock
scenario. Update the test around run_site_explorer_iteration to use that helper
while preserving the assertion that no probe occurs during refresh and the
existing refresh completion behavior.
- Around line 1000-1024: Refactor this third spawn/wait/release sequence around
refresh_endpoint_report into the shared helper requested by the consolidated
review comment, reusing the existing endpoint-specific refresh synchronization
helper from the earlier occurrences. Preserve the concurrent refresh of
bmc_ip_a, the independent refresh and version assertion for bmc_ip_b, and the
helper’s error propagation.
In `@crates/site-explorer/src/endpoint_exploration_service.rs`:
- Around line 100-108: Remove the #[allow(clippy::too_many_arguments)] attribute
immediately preceding try_explore_endpoint; leave the method signature and
implementation unchanged.
In `@crates/site-explorer/tests/integration/env.rs`:
- Line 22: Consolidate the duplicated EndpointExplorationService and
SiteExplorer::new wiring used by the integration environment setup, test
harness, health_report, and zero_dpu tests. Extract or reuse a shared helper for
constructing this service and update the affected callers to use it, preserving
the existing configuration and behavior.
In `@crates/site-explorer/tests/integration/health_report.rs`:
- Line 27: Consolidate the duplicated EndpointExplorationService and
SiteExplorer::new setup in the health report integration test by reusing the
shared test-harness wiring already used by env.rs, zero_dpu.rs, and
crates/test-harness/src/lib.rs. Update the affected setup block (including lines
66-77) and remove the now-unnecessary direct import and construction.
In `@crates/site-explorer/tests/integration/zero_dpu.rs`:
- Line 24: Replace the duplicated EndpointExplorationService and
SiteExplorer::new setup in the zero-DPU fixtures with the existing
test_site_explorer(test_harness, config) helper from the test harness. Pass the
current custom explorations_per_run configuration through that helper, and
remove imports or local wiring that become unused.
In `@crates/test-harness/src/lib.rs`:
- Line 26: Replace the duplicated EndpointExplorationService and
SiteExplorer::new setup in the test fixtures with the reusable
carbide_site_explorer::test_support::test_site_explorer helper. Update all
affected fixtures, including the canonical site setup, to call this helper and
remove imports that become unused.
🪄 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: a1c75efc-888f-4d5e-9a70-821450c9153d
📒 Files selected for processing (15)
crates/api-core/src/api.rscrates/api-core/src/errors.rscrates/api-core/src/handlers/site_explorer.rscrates/api-core/src/setup.rscrates/api-core/src/test_support/builder.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/site_explorer.rscrates/site-explorer/src/endpoint_exploration_service.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/src/test_support/mod.rscrates/site-explorer/tests/integration/env.rscrates/site-explorer/tests/integration/health_report.rscrates/site-explorer/tests/integration/zero_dpu.rscrates/test-harness/src/lib.rs
| let service = self.clone(); | ||
| let bmc_address = SocketAddr::new(bmc_ip, 443); | ||
| // The generated report is based on this row. Use its version for the eventual CAS so a | ||
| // concurrent report update during the probe cannot be overwritten. | ||
| let baseline_version = existing_endpoint.report_version; | ||
| let existing_report = existing_endpoint.report; | ||
| let boot_interface_mac = existing_endpoint.boot_interface_mac; | ||
|
|
||
| let join_handle = tokio::spawn(async move { | ||
| let probe = service | ||
| .try_explore_endpoint( | ||
| bmc_address, | ||
| &bmc_interface, | ||
| expected.as_ref(), | ||
| existing_report.last_exploration_error.as_ref(), | ||
| boot_interface_mac, | ||
| ) | ||
| .await | ||
| .ok_or(EndpointExplorationServiceError::AlreadyInProgress(bmc_ip))?; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how override_target_port is consumed and whether the old refresh handler used it.
rg -nP -C3 'override_target_port' crates/site-explorer crates/api-core
# Inspect the service's socket construction for the refresh probe.
rg -nP -C2 'SocketAddr::new' crates/site-explorer/src/endpoint_exploration_service.rsRepository: NVIDIA/infra-controller
Length of output: 17780
Respect override_target_port in refresh probes
The refresh path hardcodes 443, so any deployment using override_target_port will probe the wrong BMC port here. Reuse the same port selection as the periodic path, or pass the resolved SocketAddr in from the caller.
🤖 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/site-explorer/src/endpoint_exploration_service.rs` around lines 174 -
192, Update the refresh probe setup in the endpoint exploration method to honor
override_target_port instead of hardcoding port 443 when constructing
bmc_address. Reuse the existing periodic-path port selection logic or pass the
caller’s resolved SocketAddr through, while preserving the existing
try_explore_endpoint flow.
1 participant
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes