feat: Site explorer skips MACs marked as ignored - #4143
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. |
|
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
WalkthroughSite Explorer now acknowledges SiteExplorer BMC suppressions and excludes suppressed BMCs from periodic exploration and ingestion. Manual refreshes return ChangesSite Explorer suppression handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SiteExplorer
participant SuppressionDB
participant EndpointPlanner
SiteExplorer->>SuppressionDB: acknowledge SiteExplorer suppressions
SuppressionDB-->>SiteExplorer: return affected BMC MAC addresses
SiteExplorer->>SuppressionDB: load SiteExplorer suppressions
SuppressionDB-->>SiteExplorer: return suppression records
SiteExplorer->>EndpointPlanner: exclude suppressed BMC candidates
EndpointPlanner-->>SiteExplorer: return eligible endpoints
sequenceDiagram
participant Client
participant APIHandler
participant EndpointExplorationService
participant SuppressionDB
Client->>APIHandler: request endpoint refresh
APIHandler->>EndpointExplorationService: refresh endpoint report
EndpointExplorationService->>SuppressionDB: check SiteExplorer suppression
SuppressionDB-->>EndpointExplorationService: return suppressed status
EndpointExplorationService-->>APIHandler: return Suppressed error
APIHandler-->>Client: return FailedPrecondition
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
0cebd55 to
da2358a
Compare
72243c0 to
51d51bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/site-explorer/src/lib.rs (1)
2258-2262: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider tracking suppressed-BMC skips as a count, consistent with the file's existing candidate-class metrics.
Every other candidate bucket in this function (
priority_update_candidates,routine_update_candidates,stale_delete_candidates,unexplored_candidates,scannable_interfaces) is recorded viametrics.record_update_explored_endpoints_count, feeding the existing observable-gauge/SharedMetricsHolderpattern. The new suppression-skip paths only log atinfowith no corresponding count, losing observability parity for what is presumably an important operational signal during mass decommissioning.♻️ Suggested addition
+let mut suppressed_skip_count = 0usize; ... if ignored_bmc_macs.contains(&iface.mac_address) { tracing::info!(...); + suppressed_skip_count += 1; continue; } ... +metrics.record_update_explored_endpoints_count("suppressed_bmc_skips", suppressed_skip_count);As per coding guidelines, "Use the existing observable-gauge /
SharedMetricsHolderpattern for point-in-time state rather than occurrence Events."Also applies to: 2306-2320
🤖 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/lib.rs` around lines 2258 - 2262, Track suppressed-BMC skips using the existing point-in-time metrics pattern rather than logging them only as events. Update the relevant suppression branches around the ignored_bmc_macs checks to maintain a count and record it through metrics.record_update_explored_endpoints_count using the existing SharedMetricsHolder flow, consistent with the other candidate buckets in the surrounding function.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/api-core/src/handlers/site_explorer.rs`:
- Around line 295-309: Consolidate suppression error formatting around
EndpointExplorationServiceError::Suppressed: in
crates/api-core/src/handlers/site_explorer.rs lines 295-309, construct the
Suppressed variant with bmc_ip and bmc_interface.mac_address and convert it via
.into(); in crates/api-core/src/errors.rs lines 358-363, bind the Suppressed
variant and use suppressed_error.to_string() when creating
CarbideError::FailedPrecondition, matching the existing BackgroundTaskFailed
pattern.
In `@crates/site-explorer/src/lib.rs`:
- Around line 1996-2019: Update acknowledge_site_explorer_suppressions to
acknowledge only SiteExplorer suppression rows whose acknowledged_at is NULL,
using a single batched database update that returns the acknowledged BMC MAC
addresses instead of issuing one sequential acknowledge call per MAC. Preserve
the returned HashSet behavior while eliminating repeat writes for
already-acknowledged suppressions.
- Around line 2021-2027: Move suppression acknowledgment in explore_site so it
runs before check_preconditions and therefore still executes when preconditions
fail. Remove the acknowledge_site_explorer_suppressions call from
update_explored_endpoints, add an ignored_bmc_macs parameter using the
appropriate HashSet<MacAddress> type, and pass the acknowledged MAC set into
that method.
---
Nitpick comments:
In `@crates/site-explorer/src/lib.rs`:
- Around line 2258-2262: Track suppressed-BMC skips using the existing
point-in-time metrics pattern rather than logging them only as events. Update
the relevant suppression branches around the ignored_bmc_macs checks to maintain
a count and record it through metrics.record_update_explored_endpoints_count
using the existing SharedMetricsHolder flow, consistent with the other candidate
buckets in the surrounding function.
🪄 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: 8c19e4e4-31d1-4c5f-bb5d-c478b2f60530
📒 Files selected for processing (8)
crates/api-core/src/errors.rscrates/api-core/src/handlers/site_explorer.rscrates/api-core/src/tests/site_explorer.rscrates/api-web/src/explored_endpoint.rscrates/api-web/src/tests/explored_endpoint.rscrates/site-explorer/src/endpoint_exploration_service.rscrates/site-explorer/src/lib.rscrates/site-explorer/tests/integration/site_explorer.rs
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/site-explorer/src/lib.rs`:
- Around line 2014-2022: Update the suppression handling around the SiteExplorer
candidate-loading flow to collect every SiteExplorer suppression into a
HashSet<MacAddress>, removing the acknowledged_at filter. Thread this set
through the later refresh, discovery, and identify_machines_to_ingest paths, and
exclude interfaces whose BMC MAC is present so suppressed BMCs cannot be
scheduled or ingest cached data.
🪄 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: 0fc4a010-4ea6-48fb-aac6-ea91d9c30a9c
📒 Files selected for processing (4)
crates/api-core/src/errors.rscrates/api-core/src/handlers/site_explorer.rscrates/api-db/src/bmc_suppression.rscrates/site-explorer/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/api-core/src/handlers/site_explorer.rs
- crates/api-core/src/errors.rs
Signed-off-by: Eric Wetzel <ewetzel@nvidia.com>
Signed-off-by: Eric Wetzel <ewetzel@nvidia.com>
Signed-off-by: Eric Wetzel <ewetzel@nvidia.com>
Signed-off-by: Eric Wetzel <ewetzel@nvidia.com>
Signed-off-by: Eric Wetzel <ewetzel@nvidia.com>
04bfbf3 to
85f43ff
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4143.docs.buildwithfern.com/infra-controller |
Signed-off-by: Eric Wetzel <ewetzel@nvidia.com>
Signed-off-by: Eric Wetzel <ewetzel@nvidia.com>
Signed-off-by: Eric Wetzel <ewetzel@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/site-explorer/src/lib.rs (1)
1335-1347: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winExtract the repeated suppression-check-and-log pattern; stop logging one INFO line per suppressed BMC on every tick. The same
if run_context.suppressed_bmc_macs.contains(&mac) { tracing::info!(...); continue/skip; }block is duplicated at 8 sites. A suppression row persists for as long as a machine stays decommissioned, so each site re-emits its INFO log for the same MAC on everyrun_intervaltick, forever. Across 8 sites and a fleet undergoing bulk decommissioning, this becomes a steady, unbounded stream of log output for state that never changes.
crates/site-explorer/src/lib.rs#L1335-L1347: extract a sharedfn is_suppressed(mac, run_context) -> bool(or similar) helper and call it here instead of the inline check.crates/site-explorer/src/lib.rs#L1975-L1988: use the same shared helper.crates/site-explorer/src/lib.rs#L2020-L2034: use the same shared helper inside the.filter()closure.crates/site-explorer/src/lib.rs#L2191-L2201: use the same shared helper.crates/site-explorer/src/lib.rs#L2229-L2239: use the same shared helper.crates/site-explorer/src/lib.rs#L2281-L2291: use the same shared helper.crates/site-explorer/src/lib.rs#L2389-L2393: use the same shared helper.crates/site-explorer/src/lib.rs#L2437-L2454: use the same shared helper inside the.filter()closure.At every site, downgrade the per-item log from
tracing::info!totracing::debug!, or replace it with one aggregatemetrics.record_update_explored_endpoints_count("suppressed_<phase>", count)call per phase (the pattern this function already uses for other aggregate stats), so operators keep visibility into suppression activity without per-BMC log spam on every tick.🤖 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/lib.rs` around lines 1335 - 1347, Extract the repeated suppressed-MAC check into a shared helper and use it at crates/site-explorer/src/lib.rs lines 1335-1347, 1975-1988, 2020-2034, 2191-2201, 2229-2239, 2281-2291, 2389-2393, and 2437-2454, including both filter closures. Replace each per-item tracing::info! with tracing::debug! (or record one aggregate suppressed phase metric) while preserving each site’s skip/filter behavior.
🧹 Nitpick comments (1)
crates/site-explorer/src/lib.rs (1)
2063-2117: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch the BMC-IP lookup instead of querying per suppression.
acknowledge_site_explorer_suppressionsissues onelookup_bmc_ip_by_mac_addressdatabase round trip per unacknowledged suppression inside thefor suppression in ...loop (Line 2083). During a burst of newly-suppressed BMCs (the decommissioning scenario this PR targets), this becomes N sequential round trips in a single iteration.Add a batched lookup (e.g.
lookup_bmc_ips_by_mac_addresses(db, &[MacAddress]) -> DatabaseResult<HashMap<MacAddress, Vec<IpAddr>>>usingWHERE mi.mac_address = ANY($1)), fetch it once for all unacknowledged MACs before the loop, then use the map inside the loop to claim guards. This collapses the per-suppression lookups into one query while keeping the existing per-suppression guard-claiming logic.♻️ Proposed refactor sketch
+ // In crates/api-db/src/machine_interface.rs + pub async fn lookup_bmc_ips_by_mac_addresses( + db: impl DbReader<'_>, + mac_addresses: &[MacAddress], + ) -> DatabaseResult<HashMap<MacAddress, Vec<IpAddr>>> { + let query = r"SELECT mi.mac_address, mia.address FROM machine_interfaces mi + INNER JOIN machine_interface_addresses mia ON (mia.interface_id = mi.id) + WHERE mi.mac_address = ANY($1)"; + let rows: Vec<(MacAddress, IpAddr)> = sqlx::query_as(query) + .bind(mac_addresses) + .fetch_all(db) + .await + .map_err(|e| DatabaseError::query(query, e))?; + Ok(rows.into_iter().fold(HashMap::new(), |mut map, (mac, ip)| { + map.entry(mac).or_default().push(ip); + map + })) + }🤖 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/lib.rs` around lines 2063 - 2117, Update acknowledge_site_explorer_suppressions to collect unacknowledged suppression MAC addresses, fetch their BMC IPs once through a new batched database helper such as lookup_bmc_ips_by_mac_addresses using an ANY query, and store the results keyed by MacAddress. Replace the per-suppression lookup_bmc_ip_by_mac_address call with map access while preserving the existing endpoint guard-claiming and acknowledgement behavior.
🤖 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.
Outside diff comments:
In `@crates/site-explorer/src/lib.rs`:
- Around line 1335-1347: Extract the repeated suppressed-MAC check into a shared
helper and use it at crates/site-explorer/src/lib.rs lines 1335-1347, 1975-1988,
2020-2034, 2191-2201, 2229-2239, 2281-2291, 2389-2393, and 2437-2454, including
both filter closures. Replace each per-item tracing::info! with tracing::debug!
(or record one aggregate suppressed phase metric) while preserving each site’s
skip/filter behavior.
---
Nitpick comments:
In `@crates/site-explorer/src/lib.rs`:
- Around line 2063-2117: Update acknowledge_site_explorer_suppressions to
collect unacknowledged suppression MAC addresses, fetch their BMC IPs once
through a new batched database helper such as lookup_bmc_ips_by_mac_addresses
using an ANY query, and store the results keyed by MacAddress. Replace the
per-suppression lookup_bmc_ip_by_mac_address call with map access while
preserving the existing endpoint guard-claiming and acknowledgement behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5be5cb60-d879-4601-a808-aea07aea2d3f
📒 Files selected for processing (5)
crates/api-db/src/bmc_suppression.rscrates/site-explorer/src/endpoint_exploration_service.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/test_support/test_site_explorer.rscrates/site-explorer/tests/integration/site_explorer.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/site-explorer/src/endpoint_exploration_service.rs
- crates/api-db/src/bmc_suppression.rs
When NICo is uningesting/decommissioning a machine, we need a way to prevent site explorer from exploring and re-ingesting its BMC(s). In this PR we modify site explorer to load the site-explorer-suppressed BMCs from the
suppressed_macstable before exploring. Site explorer will skip over these MAC addresses and log them accordingly. This includes both scheduled and manually requested explorations.A nice to have follow-up task would be to make this skipping behavior more visible with a "Skipped Endpoints" metric or similar.
Related issues
Closes #3817
Type of Change
Breaking Changes
Testing