feat: surface the swallowed errors across the fleet#3469
Conversation
|
@coderabbitai full_review, thanks! |
|
(ᵘ‿ᵘ)♡ ✅ Action performedFull review finished. |
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (17)
WalkthroughThe changes make previously ignored rollback, cleanup, filesystem, configuration, and subprocess failures observable. They add health-report persistence telemetry, warn on empty DPU serial numbers, and ensure BMC reset metrics count only successful resets. ChangesAPI transaction rollback observability
Runtime failure diagnostics
Health report persistence telemetry
BMC reset outcome accounting
Estimated code review effort: 3 (Moderate) | ~25 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: 3
🧹 Nitpick comments (2)
crates/site-explorer/src/lib.rs (1)
3938-3969: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the outcome test table-driven and cover both transports.
This test only exercises
Ipmitool, manually repeats the two cases, and does not verify the new failure log fields. Add table-driven cases for bothIpmitoolandRedfish, asserting the returned boolean, metric count, and logged method/address/error.As per coding guidelines, Rust tests for functions mapping inputs to observable results should use table-driven cases.
🤖 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 3938 - 3969, Update bmc_reset_counter_counts_successes_not_attempts to use table-driven cases covering successful and failed outcomes for both BmcResetMethod::Ipmitool and BmcResetMethod::Redfish. For each case, assert the returned boolean, bmc_reset_count, and failure log fields including method, address, and error; retain the success behavior and isolate or reset metrics/log capture per case.Source: Coding guidelines
crates/preingestion-manager/src/lib.rs (1)
2160-2191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated kill/wait/warn cleanup sequence.
The stdout-missing and stderr-missing branches duplicate the same 4-statement kill → wait → warn cleanup verbatim. Consider a small helper to avoid drift if this cleanup logic changes later.
♻️ Proposed helper extraction
+async fn kill_and_reap(cmd: &mut tokio::process::Child, address: &str, context: &str) { + if let Err(e) = cmd.kill().await { + tracing::warn!("Upgrade script {address} cleanup: failed to kill subprocess after {context}: {e}"); + } + if let Err(e) = cmd.wait().await { + tracing::warn!("Upgrade script {address} cleanup: failed to reap subprocess after {context}: {e}"); + } +}Then call
kill_and_reap(&mut cmd, &address, "STDOUT capture failure").await;andkill_and_reap(&mut cmd, &address, "STDERR capture failure").await;at the two call sites.🤖 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/preingestion-manager/src/lib.rs` around lines 2160 - 2191, Extract the duplicated subprocess cleanup from the stdout and stderr failure branches into a small async helper, such as kill_and_reap, accepting the command, address, and failure context. Preserve the existing kill-then-wait order and warning messages, then call the helper from both branches with their respective capture-failure descriptions before marking completion and returning.
🤖 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/dsx-exchange-consumer/tests/metric_exposition.rs`:
- Around line 79-84: Update the test documentation comment above the
persist-failure metric test to use the complete metric name
carbide_dsx_exchange_consumer_health_report_persist_failures_total, matching the
event definition and assertion; leave the test behavior unchanged.
In `@crates/site-explorer/src/lib.rs`:
- Around line 2565-2567: Update the documentation for
carbide_site_explorer_bmc_reset_count and all dependent dashboards or alerts to
define and interpret the metric as the number of successfully completed BMC
resets, not initiated attempts. Locate references using the metric name and
revise labels, descriptions, queries, and thresholds consistently without
changing the implementation.
- Around line 2568-2581: Separate the remote BMC reset result from timestamp
persistence errors in redfish_reset_bmc and ipmitool_reset_bmc, and update
record_bmc_reset_outcome and their callers accordingly. Treat a successful
physical reset as success for metrics and control flow even when timestamp
persistence fails, while still surfacing the bookkeeping error; ensure the
Redfish path does not fall through to a second IPMI reset after the hardware
reset succeeds.
---
Nitpick comments:
In `@crates/preingestion-manager/src/lib.rs`:
- Around line 2160-2191: Extract the duplicated subprocess cleanup from the
stdout and stderr failure branches into a small async helper, such as
kill_and_reap, accepting the command, address, and failure context. Preserve the
existing kill-then-wait order and warning messages, then call the helper from
both branches with their respective capture-failure descriptions before marking
completion and returning.
In `@crates/site-explorer/src/lib.rs`:
- Around line 3938-3969: Update bmc_reset_counter_counts_successes_not_attempts
to use table-driven cases covering successful and failed outcomes for both
BmcResetMethod::Ipmitool and BmcResetMethod::Redfish. For each case, assert the
returned boolean, bmc_reset_count, and failure log fields including method,
address, and error; retain the success behavior and isolate or reset metrics/log
capture per case.
🪄 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: dba974ad-5c4a-49e9-bfb5-3acecfab5634
📒 Files selected for processing (15)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/dhcp/src/lib.rs (1)
78-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s structured logging contract for the new warnings.
These changes introduce free-form interpolation of errors, paths, and endpoint identifiers. Use structured
tracing::warn!fields consistently.
crates/dhcp/src/lib.rs#L78-L98: replace the threelog::warn!calls with structured fields and describeVarErroraccurately.crates/preingestion-manager/src/lib.rs#L2162-L2187: emitaddressanderrorfields for cleanup warnings.crates/preingestion-manager/src/lib.rs#L2206-L2210: emitaddressanderrorfields for the stderr-task warning.As per coding guidelines and path instructions, ordinary Rust logging should use structured
tracing::macros with common values passed as 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/dhcp/src/lib.rs` around lines 78 - 98, Replace the three log::warn! calls in crates/dhcp/src/lib.rs lines 78-98 with structured tracing::warn! fields, recording the VarError accurately alongside the relevant fallback path. In crates/preingestion-manager/src/lib.rs lines 2162-2187, update cleanup warnings to emit structured address and error fields; likewise update the stderr-task warning at lines 2206-2210 to emit address and error fields. No direct changes are needed outside these three sites.Sources: Coding guidelines, Path instructions
crates/site-explorer/src/lib.rs (2)
3938-3967: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover the physical-success/bookkeeping-failure path.
This test only feeds
record_bmc_reset_outcomepre-composed results; it cannot catch a successful remote reset followed by timestamp persistence failure. Add coverage ensuring that success is counted and the IPMI fallback is not triggered.🤖 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 3938 - 3967, Add a test covering the real BMC reset flow where the remote reset succeeds but timestamp persistence fails: verify the successful reset increments bmc_reset_count and does not trigger the IPMI fallback. Exercise the higher-level reset method rather than only record_bmc_reset_outcome, and use the existing persistence-failure setup and fallback-observable symbols.
3943-3967: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s table-driven test pattern.
The test maps outcomes to a boolean and counter state but encodes cases sequentially. Convert these cases to the existing
check_cases/scenario style. As per coding guidelines, tests mapping inputs to outputs or observable results must be table-driven.🤖 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 3943 - 3967, Convert bmc_reset_counter_counts_successes_not_attempts into the repository’s table-driven check_cases/scenario pattern, defining success and failure inputs with their expected boolean result and bmc_reset_count. Iterate through the cases while preserving the existing record_bmc_reset_outcome assertions and expected counter behavior.Source: Coding guidelines
crates/dsx-exchange-consumer/src/health_updater.rs (1)
215-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a handler-level assertion for the new emission.
test_api_failure_does_not_cache_stateverifies only cache behavior, whilemetric_exposition.rsemitsHealthReportPersistFaileddirectly. Capture the failure-path log aroundhandle_value_messageand assert the rack ID and error; otherwise this test suite can pass even if this call site stops emitting the event.🤖 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/dsx-exchange-consumer/src/health_updater.rs` around lines 215 - 222, Update test_api_failure_does_not_cache_state around handle_value_message to capture the handler-level HealthReportPersistFailed emission, then assert that the captured event contains the expected rack ID and persistence error. Keep the existing cache-behavior assertions and ensure the test specifically covers this call site rather than only metric_exposition.rs.
🤖 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/health/src/sink/log_file.rs`:
- Around line 236-254: Update shift_backups to avoid using Path::exists() before
rotating or pruning files. Call fs::rename and fs::remove_file directly,
treating only ErrorKind::NotFound as an ignorable absence and logging other
filesystem errors through the existing tracing warnings.
- Around line 211-215: The rotate_if_needed() flush path must stop rotation when
file.flush() fails instead of dropping the handle and continuing. Preserve
self.current_file on failure, propagate the I/O error immediately, and only
allow the existing ownership transfer/drop behavior after a successful flush.
---
Nitpick comments:
In `@crates/dhcp/src/lib.rs`:
- Around line 78-98: Replace the three log::warn! calls in
crates/dhcp/src/lib.rs lines 78-98 with structured tracing::warn! fields,
recording the VarError accurately alongside the relevant fallback path. In
crates/preingestion-manager/src/lib.rs lines 2162-2187, update cleanup warnings
to emit structured address and error fields; likewise update the stderr-task
warning at lines 2206-2210 to emit address and error fields. No direct changes
are needed outside these three sites.
In `@crates/dsx-exchange-consumer/src/health_updater.rs`:
- Around line 215-222: Update test_api_failure_does_not_cache_state around
handle_value_message to capture the handler-level HealthReportPersistFailed
emission, then assert that the captured event contains the expected rack ID and
persistence error. Keep the existing cache-behavior assertions and ensure the
test specifically covers this call site rather than only metric_exposition.rs.
In `@crates/site-explorer/src/lib.rs`:
- Around line 3938-3967: Add a test covering the real BMC reset flow where the
remote reset succeeds but timestamp persistence fails: verify the successful
reset increments bmc_reset_count and does not trigger the IPMI fallback.
Exercise the higher-level reset method rather than only
record_bmc_reset_outcome, and use the existing persistence-failure setup and
fallback-observable symbols.
- Around line 3943-3967: Convert bmc_reset_counter_counts_successes_not_attempts
into the repository’s table-driven check_cases/scenario pattern, defining
success and failure inputs with their expected boolean result and
bmc_reset_count. Iterate through the cases while preserving the existing
record_bmc_reset_outcome assertions and expected counter behavior.
🪄 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: 6e1d2c76-325f-4155-ab95-ce22450149af
📒 Files selected for processing (15)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rs
| if let Some(mut file) = self.current_file.take() | ||
| && let Err(e) = file.flush() | ||
| { | ||
| tracing::warn!(error = %e, "failed to flush log file before rotation"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Map the file shape first, then inspect the relevant section.
ast-grep outline crates/health/src/sink/log_file.rs --view expanded || true
echo
wc -l crates/health/src/sink/log_file.rs
echo
sed -n '170,290p' crates/health/src/sink/log_file.rs | cat -nRepository: NVIDIA/infra-controller
Length of output: 7202
🏁 Script executed:
set -euo pipefail
# Inspect the call path and any rotation tests.
sed -n '1,170p' crates/health/src/sink/log_file.rs | cat -n
echo
sed -n '300,560p' crates/health/src/sink/log_file.rs | cat -n
echo
rg -n "rotate_if_needed|write_line\\(" crates/health/src/sink/log_file.rs crates/health/src -g '!target'Repository: NVIDIA/infra-controller
Length of output: 18896
Do not continue rotation after a flush error.
rotate_if_needed() takes ownership of self.current_file before checking flush(). If that flush fails, the handle is dropped and rotation still proceeds, which can strand any pending bytes and mask the underlying I/O failure. Keep the writer alive, return the error, and only drop it after a successful flush.
🤖 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/health/src/sink/log_file.rs` around lines 211 - 215, The
rotate_if_needed() flush path must stop rotation when file.flush() fails instead
of dropping the handle and continuing. Preserve self.current_file on failure,
propagate the I/O error immediately, and only allow the existing ownership
transfer/drop behavior after a successful flush.
| if from.exists() | ||
| && let Err(e) = fs::rename(&from, &to) | ||
| { | ||
| tracing::warn!(error = %e, from = %from.display(), to = %to.display(), "failed to shift rotated log file"); | ||
| } | ||
| } | ||
|
|
||
| // current -> .1 | ||
| let backup = self.rotated_path(1); | ||
| let _ = fs::rename(current_path, &backup); | ||
| if let Err(e) = fs::rename(current_path, &backup) { | ||
| tracing::warn!(error = %e, from = %current_path.display(), to = %backup.display(), "failed to rotate current log file to backup"); | ||
| } | ||
|
|
||
| // prune the oldest backup beyond the limit | ||
| let oldest = self.rotated_path(self.max_backups + 1); | ||
| if oldest.exists() { | ||
| let _ = fs::remove_file(&oldest); | ||
| if oldest.exists() | ||
| && let Err(e) = fs::remove_file(&oldest) | ||
| { | ||
| tracing::warn!(error = %e, path = %oldest.display(), "failed to prune oldest rotated log file"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='crates/health/src/sink/log_file.rs'
echo "== file size =="
wc -l "$file"
echo "== relevant slice =="
sed -n '200,290p' "$file"
echo "== search exists/try_exists in file =="
rg -n 'try_exists|\.exists\(\)|rename\(|remove_file\(' "$file"Repository: NVIDIA/infra-controller
Length of output: 4000
Avoid gating backup rotation on Path::exists()
exists() turns permission and other metadata failures into false, so shift_backups() can skip a real error without logging it. Call rename/remove_file directly and ignore only NotFound, or switch to try_exists() with explicit Err handling.
🤖 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/health/src/sink/log_file.rs` around lines 236 - 254, Update
shift_backups to avoid using Path::exists() before rotating or pruning files.
Call fs::rename and fs::remove_file directly, treating only ErrorKind::NotFound
as an ignorable absence and logging other filesystem errors through the existing
tracing warnings.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3469.docs.buildwithfern.com/infra-controller |
|
Thanks @coderabbitai! Addressed all four actionable items in the latest push:
On the reset-outcome finding — a physical reset that succeeds but whose timestamp write fails is counted as a failure and, on the Redfish path, falls through to a second IPMI reset — that's a real issue, but it's pre-existing: this PR only swapped the counting mechanism, not the reset methods' |
|
@coderabbitai full_review, thanks! |
|
Confirmed my understanding of the addressed items:
Also, good call filing ᕕ( ᐛ )ᕗ ✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes.
Your call on the reset-outcome issue (timestamp-write failure being counted as a reset failure, and the Redfish→IPMI fall-through) makes sense — since this PR only changed how successes/failures are counted rather than the underlying (๑˃̵ᴗ˂̵)و |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/dhcp/src/lib.rs (1)
78-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured
tracingfields for the new operational diagnostics.These messages interpolate errors, paths, addresses, and cleanup context into free-form text; DHCP also uses
log::warn!. Keep messages static and pass values as fields such aserror = %e,fallback = %path,address = %address, andcontext.
crates/dhcp/src/lib.rs#L78-L98: replace eachlog::warn!with structuredtracing::warn!.crates/preingestion-manager/src/lib.rs#L2168-L2176: structureaddress,context, and cleanup errors.crates/preingestion-manager/src/lib.rs#L2208-L2211: structure the endpoint and join error.As per coding guidelines and path instructions, ordinary Rust logging should use structured
tracing::macros with common values passed as 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/dhcp/src/lib.rs` around lines 78 - 98, Replace the three log::warn! calls in crates/dhcp/src/lib.rs lines 78-98 with tracing::warn! calls using static messages and structured fields for the environment-variable errors and fallback paths. In crates/preingestion-manager/src/lib.rs lines 2168-2176, structure address, context, and cleanup errors; in lines 2208-2211, structure the endpoint and join error. Preserve the existing warning content and fallback behavior while using fields such as error = %e, fallback = %path, address = %address, and context.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/dsx-exchange-consumer/src/health_updater.rs`:
- Around line 215-222: Extend test_api_failure_does_not_cache_state to capture
the HealthReportPersistFailed telemetry emitted by the Err branch, then assert
the WARN message, matching rack_id, persist error text, and expected counter
increment. Keep the existing assertion that failed state is not cached.
---
Nitpick comments:
In `@crates/dhcp/src/lib.rs`:
- Around line 78-98: Replace the three log::warn! calls in
crates/dhcp/src/lib.rs lines 78-98 with tracing::warn! calls using static
messages and structured fields for the environment-variable errors and fallback
paths. In crates/preingestion-manager/src/lib.rs lines 2168-2176, structure
address, context, and cleanup errors; in lines 2208-2211, structure the endpoint
and join error. Preserve the existing warning content and fallback behavior
while using fields such as error = %e, fallback = %path, address = %address, and
context.
🪄 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: 2475c6ec-8ada-491c-9f8f-42b7e4b781aa
📒 Files selected for processing (17)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/metrics.rsdocs/observability/core_metrics.md
|
Thanks @coderabbitai — on the structured-logging finding:
|
|
@coderabbitai full_review, thanks! |
|
ヽ(´▽`)/ ✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes.
Nothing further from me on this thread. ᕦ(ò_óˇ)ᕤ |
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/dhcp/src/lib.rs`:
- Around line 78-98: Update the TLS path configuration fields
forge_root_ca_path, forge_client_cert_path, and forge_client_key_path to reject
empty values returned by std::env::var, accepting only non-empty paths and
falling back to the corresponding tls_default constants with the existing
warnings. Apply identical validation to all three paths before they reach
build_forge_client_config.
🪄 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: 64b1080c-9343-4c87-95a2-995f6e47aab9
📒 Files selected for processing (17)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/metrics.rsdocs/observability/core_metrics.md
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/api-core/src/handlers/credential.rs
- crates/site-explorer/src/metrics.rs
- crates/api-core/src/handlers/expected_machine.rs
- crates/api-core/src/handlers/rack.rs
- crates/machine-controller/src/handler/dpf.rs
- crates/dsx-exchange-consumer/src/health_updater.rs
- crates/health/src/sink/log_file.rs
- crates/preingestion-manager/src/lib.rs
- crates/api-core/src/handlers/switch.rs
- crates/dsx-exchange-consumer/src/metrics.rs
- crates/api-core/src/handlers/instance.rs
- crates/dsx-exchange-consumer/Cargo.toml
- crates/site-explorer/src/lib.rs
| forge_root_ca_path: std::env::var("FORGE_ROOT_CAFILE_PATH").unwrap_or_else(|e| { | ||
| log::warn!( | ||
| "FORGE_ROOT_CAFILE_PATH unset or unreadable ({e}); falling back to the built-in default root CA at {}", | ||
| tls_default::ROOT_CA | ||
| ); | ||
| tls_default::ROOT_CA.to_string() | ||
| }), | ||
| forge_client_cert_path: std::env::var("FORGE_CLIENT_CERT_PATH").unwrap_or_else(|e| { | ||
| log::warn!( | ||
| "FORGE_CLIENT_CERT_PATH unset or unreadable ({e}); falling back to the built-in default client certificate at {}", | ||
| tls_default::CLIENT_CERT | ||
| ); | ||
| tls_default::CLIENT_CERT.to_string() | ||
| }), | ||
| forge_client_key_path: std::env::var("FORGE_CLIENT_KEY_PATH").unwrap_or_else(|e| { | ||
| log::warn!( | ||
| "FORGE_CLIENT_KEY_PATH unset or unreadable ({e}); falling back to the built-in default client key at {}", | ||
| tls_default::CLIENT_KEY | ||
| ); | ||
| tls_default::CLIENT_KEY.to_string() | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Treat empty TLS-path environment variables as invalid.
std::env::var returns Ok("") for an explicitly empty variable, so these branches skip the fallback and pass empty certificate/key paths to build_forge_client_config. Validate the returned paths before accepting them, and use the built-in defaults when they are empty.
Proposed approach
- forge_root_ca_path: std::env::var("FORGE_ROOT_CAFILE_PATH").unwrap_or_else(|e| {
+ forge_root_ca_path: match std::env::var("FORGE_ROOT_CAFILE_PATH") {
+ Ok(path) if !path.is_empty() => path,
+ Ok(_) | Err(_) => {
+ // log the appropriate reason, then:
+ tls_default::ROOT_CA.to_string()
+ }
+ },Apply the same validation to the client certificate and key paths.
As per path instructions, DHCP TLS-path fallbacks must be reviewed for configuration correctness and runtime behavior.
🤖 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/dhcp/src/lib.rs` around lines 78 - 98, Update the TLS path
configuration fields forge_root_ca_path, forge_client_cert_path, and
forge_client_key_path to reject empty values returned by std::env::var,
accepting only non-empty paths and falling back to the corresponding tls_default
constants with the existing warnings. Apply identical validation to all three
paths before they reach build_forge_client_config.
Source: Path instructions
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/preingestion-manager/src/lib.rs (1)
2160-2196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
start_kill()for separate kill/reap diagnostics.
tokio::process::Child::kill().awaitalready performs the kill and waits, so the followingwait().awaitis not an independent reap step. Usestart_kill()followed bywait().await, or keep onlykill().awaitand log one cleanup failure. Tokio documentskillas SIGKILL followed bywait. (docs.rs)🤖 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/preingestion-manager/src/lib.rs` around lines 2160 - 2196, Update the local kill_and_reap helper to call Child::start_kill() before Child::wait().await, preserving the separate warning diagnostics for kill initiation and process reaping. Keep the existing address, context, and error fields and cleanup call sites unchanged.
🤖 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/preingestion-manager/src/lib.rs`:
- Around line 2160-2196: Update the local kill_and_reap helper to call
Child::start_kill() before Child::wait().await, preserving the separate warning
diagnostics for kill initiation and process reaping. Keep the existing address,
context, and error fields and cleanup call sites unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 30d872a5-7572-4f7e-83f9-01cda9c528b0
📒 Files selected for processing (17)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/metrics.rsdocs/observability/core_metrics.md
A handful of fallible operations were silent in both pipelines -- no log, no metric -- so observability couldn't see them at all, including a rack health-report persist failure discarded with `Err(_) => { /* will retry */ }`. Every one now logs, and the one whose failure rate is worth watching also counts.
Primary callouts are:
- `carbide_dsx_exchange_consumer_health_report_persist_failures_total` -- the dsx health-report persist failure now emits a single framework event that both warns and counts (the rack id and error ride as log context). It's the one failure here worth a rate, so it's the one that gets a counter.
- The silently-dropped failures now warn: the read-only and error-unwinding `txn.rollback()` drops in the instance, power-shelf, and expected-machine handlers; the DHCP TLS-material reads that fall back to a built-in default cert; the empty product serial fed into DPF; the preingestion subprocess kill/wait drops; and the health log-file flush, rotate, and prune drops. Each keeps its crate's logging convention, and the error-unwinding rollbacks log without clobbering the original error being returned.
- The site-explorer `bmc_reset_count` counted attempts, not successes: the ipmitool reset dropped its result with `.ok()` while the counter ticked regardless. A shared `record_bmc_reset_outcome` now increments only on success and logs the failure, and both the ipmitool and Redfish paths route through it -- so the counter finally means what its name says, with no rename.
The MQTT state-change send drop the issue pointed at turned out to be a test mock; the production send path already logs and meters every outcome, so nothing was left silent there.
Tests added!
This supports NVIDIA#3179
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
(๑˃̵ᴗ˂̵)و ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 20 minutes. |
2 participants
A handful of fallible operations were silent in both pipelines -- no log, no metric -- so observability couldn't see them at all, including a rack health-report persist failure discarded with
Err(_) => { /* will retry */ }. Every one now logs, and the one whose failure rate is worth watching also counts.carbide_dsx_exchange_consumer_health_report_persist_failures_total-- the dsx health-report persist failure now emits a single framework event that both warns and counts (rack id and error ride as log context). It's the one failure here worth a rate.txn.rollback()drops in the instance / power-shelf / expected-machine handlers; the DHCP TLS-material reads that fall back to a built-in default cert; the empty product serial fed into DPF; the preingestion subprocess kill/wait drops; and the health log-file flush, rotate, and prune drops. Each keeps its crate's logging convention, and the error-unwinding rollbacks log without clobbering the original error being returned.bmc_reset_countcounted attempts, not successes: the ipmitool reset dropped its result with.ok()while the counter ticked regardless. A sharedrecord_bmc_reset_outcomenow increments only on success and logs the failure, and both the ipmitool and Redfish paths route through it -- so the counter finally means what its name says, with no rename.The MQTT state-change send drop the issue pointed at turned out to be a test mock; the production send path already logs and meters every outcome, so nothing was left silent there.
This supports #3179