feat(spdm-controller): count unexpected evidence task states#4118
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbit
WalkthroughSPDM evidence collection polling now classifies task states, instruments unexpected states with retry or failure actions, and validates the resulting events and counters. The new counter is documented and test-only dependencies are added. ChangesSPDM evidence collection observability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PollEvidenceCollection
participant TaskStateClassifier
participant Instrumentation
participant StateMachine
PollEvidenceCollection->>TaskStateClassifier: classify task state
TaskStateClassifier-->>PollEvidenceCollection: Completed, InProgress, or Unexpected
PollEvidenceCollection->>Instrumentation: emit unexpected state and next_action
PollEvidenceCollection->>StateMachine: retry collection or transition to Failed
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4118.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/spdm-controller/src/handler.rs (1)
242-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid cloning each task message.
task.messages.iter().map(|t| t.message.clone())clones every message string just to join them;itertools::joinonly needsDisplay, so borrowing avoids the allocation.♻️ Proposed fix
- let err = task.messages.iter().map(|t| t.message.clone()).join("\n"); + let err = task.messages.iter().map(|t| t.message.as_str()).join("\n");As per path instructions, "avoiding needless clones" for
crates/**/*.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/spdm-controller/src/handler.rs` around lines 242 - 254, Update the error construction in the EvidenceCollectionTaskStateClass::Unexpected branch to join borrowed task message values rather than cloning each t.message. Preserve the existing newline-separated output and error field behavior while using itertools::join with a Display-compatible borrowed iterator.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/spdm-controller/src/handler.rs`:
- Around line 242-254: Update the error construction in the
EvidenceCollectionTaskStateClass::Unexpected branch to join borrowed task
message values rather than cloning each t.message. Preserve the existing
newline-separated output and error field behavior while using itertools::join
with a Display-compatible borrowed iterator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 50ed5ba1-f1b2-47aa-9425-58ac04e746a8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/spdm-controller/Cargo.tomlcrates/spdm-controller/src/handler.rsdocs/observability/core_metrics.md
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/spdm-controller/src/handler.rs (2)
242-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAggregated error string embeds raw newlines into a structured log field.
task.messages.iter().map(|t| t.message.clone()).join("\n")joins multiple messages with"\n"and stores the result in theerror#[context]field. Embedded newlines inside a single structured field undermine the logfmt-friendly single-line assumption this codebase's tracing convention relies on. Consider joining with a single-line-safe separator instead.Based on coding guidelines: "Use logfmt-compatible structured logging."
♻️ Proposed fix
- let err = task.messages.iter().map(|t| t.message.clone()).join("\n"); + let err = task.messages.iter().map(|t| &t.message).join("; ");🤖 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/spdm-controller/src/handler.rs` around lines 242 - 256, Update the error aggregation in the EvidenceCollectionTaskStateClass::Unexpected branch to join task.messages with a single-line-safe separator instead of a raw newline before assigning the result to the structured error field. Preserve the existing message order and emit behavior.Source: Coding guidelines
405-414: 🧹 Nitpick | 🔵 TrivialEvent always logs at
error, even on retry.
SpdmEvidenceCollectionTaskStateUnexpectedis declared with a fixedlog = errorregardless of the computednext_action. SincePending/transient states routinely fall into this branch and retry successfully, every retry attempt is logged as anERROR, which may generate log noise disproportionate to actual severity (reservingerrorfor the terminalFailoutcome andwarnforRetrywould better reflect operational impact). Worth confirming whethercarbide_instrument::Eventsupports level selection based on a field value, or whether this is an accepted tradeoff for this 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/spdm-controller/src/handler.rs` around lines 405 - 414, Update SpdmEvidenceCollectionTaskStateUnexpected and its emission path so log severity reflects the computed next_action: use warn for Retry and error for terminal Fail outcomes, if carbide_instrument::Event supports field-based level selection. If dynamic selection is unsupported, confirm and document retaining the fixed error level rather than changing unrelated 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.
Nitpick comments:
In `@crates/spdm-controller/src/handler.rs`:
- Around line 242-256: Update the error aggregation in the
EvidenceCollectionTaskStateClass::Unexpected branch to join task.messages with a
single-line-safe separator instead of a raw newline before assigning the result
to the structured error field. Preserve the existing message order and emit
behavior.
- Around line 405-414: Update SpdmEvidenceCollectionTaskStateUnexpected and its
emission path so log severity reflects the computed next_action: use warn for
Retry and error for terminal Fail outcomes, if carbide_instrument::Event
supports field-based level selection. If dynamic selection is unsupported,
confirm and document retaining the fixed error level rather than changing
unrelated behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0dcf907e-18c4-46ba-8cb4-0dff3cc10d3d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/spdm-controller/Cargo.tomlcrates/spdm-controller/src/handler.rsdocs/observability/core_metrics.md
SPDM evidence polling now counts Redfish task states that already enter the retry-or-fail branch. Bounded task-state and next-action labels distinguish each encounter, while machine, device, task, retry count, and task error stay log-only. The existing error message and retry threshold are unchanged. This supports NVIDIA#4109 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
2 participants
SPDM evidence polling already logs and retries any Redfish task state outside
Completed,New,Starting, andRunning, then transitions to failure after the retry budget is exhausted. Those encounters were visible per device but could not be counted or separated by state.So, the retry-or-fail branch now emits
SpdmEvidenceCollectionTaskStateUnexpected. Boundedtask_stateandnext_actionlabels make the failure path trendable, while machine, device, task, retry count, and task error stay log-only. The same computed action drives both telemetry and control flow, and the existing error message and retry threshold are unchanged.The table-driven test enumerates every Redfish
TaskStatevariant plus a missing state, covering normal states, all retrying states, and the retry-to-fail boundary.Tests added!
Related issues
This supports #4109
Part of #3169
Type of Change
Breaking Changes
Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)
cargo test -p carbide-spdm-controller evidence_collection_task_states_log_and_count_when_unexpected --libcargo make format-nightlycargo make clippycargo make carbide-lintscargo xtask check-event-namescargo xtask check-metric-docscargo xtask check-workspace-depscargo make check-licensescargo make check-bansgit diff --check origin/mainAdditional Notes
The counter measures unexpected-state encounters rather than unique devices.
Pendingremains in the existing retry-or-fail branch to keep this PR behavior-preserving; changing its state-machine treatment should be a separate policy decision. The exhaustive classifier intentionally makes future Redfish task-state additions a compile-time decision instead of silently assigning them to the failure path.