From c1a7d093c1374afc4a0b4473d1369bd82322e9b6 Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 26 Oct 2023 16:18:06 -0700 Subject: [PATCH] [update-engine] move terminal ExecutionStatus into its own type (#4364) I found myself repeating the completed/failed/aborted branches very often, and making this its own type simplifies that quite nicely. Also track the total elapsed time, and show it in the wicket UI. Note the "after 2.93ms" in this screenshot: ![image](https://github.com/oxidecomputer/omicron/assets/180618/dd202abc-41f9-4722-a4ac-1ac99e0f9c8a) Depends on #4361. --- update-engine/src/buffer.rs | 135 ++++++++++++++++++++++++++-------- wicket/src/ui/panes/update.rs | 111 ++++++++++++++++------------ 2 files changed, 166 insertions(+), 80 deletions(-) diff --git a/update-engine/src/buffer.rs b/update-engine/src/buffer.rs index 2dd216c7ab..3e7db63cb9 100644 --- a/update-engine/src/buffer.rs +++ b/update-engine/src/buffer.rs @@ -1307,14 +1307,61 @@ impl ExecutionSummary { StepStatus::Running { .. } => { execution_status = ExecutionStatus::Running { step_key }; } - StepStatus::Completed { .. } => { - execution_status = ExecutionStatus::Completed { step_key }; + StepStatus::Completed { info } => { + let (root_total_elapsed, leaf_total_elapsed) = match info { + Some(info) => ( + Some(info.root_total_elapsed), + Some(info.leaf_total_elapsed), + ), + None => (None, None), + }; + + let terminal_status = ExecutionTerminalInfo { + kind: TerminalKind::Completed, + root_total_elapsed, + leaf_total_elapsed, + step_key, + }; + execution_status = + ExecutionStatus::Terminal(terminal_status); } - StepStatus::Failed { .. } => { - execution_status = ExecutionStatus::Failed { step_key }; + StepStatus::Failed { reason } => { + let (root_total_elapsed, leaf_total_elapsed) = + match reason.info() { + Some(info) => ( + Some(info.root_total_elapsed), + Some(info.leaf_total_elapsed), + ), + None => (None, None), + }; + + let terminal_status = ExecutionTerminalInfo { + kind: TerminalKind::Failed, + root_total_elapsed, + leaf_total_elapsed, + step_key, + }; + execution_status = + ExecutionStatus::Terminal(terminal_status); } - StepStatus::Aborted { .. } => { - execution_status = ExecutionStatus::Aborted { step_key }; + StepStatus::Aborted { reason, .. } => { + let (root_total_elapsed, leaf_total_elapsed) = + match reason.info() { + Some(info) => ( + Some(info.root_total_elapsed), + Some(info.leaf_total_elapsed), + ), + None => (None, None), + }; + + let terminal_status = ExecutionTerminalInfo { + kind: TerminalKind::Aborted, + root_total_elapsed, + leaf_total_elapsed, + step_key, + }; + execution_status = + ExecutionStatus::Terminal(terminal_status); } StepStatus::WillNotBeRun { .. } => { // Ignore steps that will not be run -- a prior step failed. @@ -1352,7 +1399,7 @@ impl StepSortKey { /// Status about a single execution ID. /// /// Part of [`ExecutionSummary`]. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Debug, Eq, PartialEq)] pub enum ExecutionStatus { /// This execution has not been started yet. NotStarted, @@ -1365,27 +1412,50 @@ pub enum ExecutionStatus { step_key: StepKey, }, - /// This execution completed running. - Completed { - /// The last step that completed. - step_key: StepKey, - }, + /// Execution has finished. + Terminal(ExecutionTerminalInfo), +} - /// This execution failed. - Failed { - /// The step key that failed. - /// - /// Use [`EventBuffer::get`] to get more information about this step. - step_key: StepKey, - }, +/// Terminal status about a single execution ID. +/// +/// Part of [`ExecutionStatus`]. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct ExecutionTerminalInfo { + /// The way in which this execution reached a terminal state. + pub kind: TerminalKind, + + /// Total elapsed time (root) for this execution. + /// + /// The total elapsed time may not be available if execution was interrupted + /// and we inferred that it was terminated. + pub root_total_elapsed: Option, + + /// Total elapsed time (leaf) for this execution. + /// + /// The total elapsed time may not be available if execution was interrupted + /// and we inferred that it was terminated. + pub leaf_total_elapsed: Option, + /// The step key that was running when this execution was terminated. + /// + /// * For completed executions, this is the last step that completed. + /// * For failed or aborted executions, this is the step that failed. + /// * For aborted executions, this is the step that was running when the + /// abort happened. + pub step_key: StepKey, +} + +/// The way in which an execution was terminated. +/// +/// Part of [`ExecutionStatus`]. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum TerminalKind { + /// This execution completed running. + Completed, + /// This execution failed. + Failed, /// This execution was aborted. - Aborted { - /// The step that was running when the abort happened. - /// - /// Use [`EventBuffer::get`] to get more information about this step. - step_key: StepKey, - }, + Aborted, } /// Keys for the event tree. @@ -1971,8 +2041,9 @@ mod tests { if is_last_event { ensure!( matches!( - summary[&root_execution_id].execution_status, - ExecutionStatus::Completed { .. }, + &summary[&root_execution_id].execution_status, + ExecutionStatus::Terminal(info) + if info.kind == TerminalKind::Completed ), "this is the last event so ExecutionStatus must be completed" ); @@ -1987,8 +2058,9 @@ mod tests { .expect("this is the first nested engine"); ensure!( matches!( - nested_summary.execution_status, - ExecutionStatus::Failed { .. }, + &nested_summary.execution_status, + ExecutionStatus::Terminal(info) + if info.kind == TerminalKind::Failed ), "for this engine, the ExecutionStatus must be failed" ); @@ -1998,8 +2070,9 @@ mod tests { .expect("this is the second nested engine"); ensure!( matches!( - nested_summary.execution_status, - ExecutionStatus::Completed { .. }, + &nested_summary.execution_status, + ExecutionStatus::Terminal(info) + if info.kind == TerminalKind::Completed ), "for this engine, the ExecutionStatus must be succeeded" ); diff --git a/wicket/src/ui/panes/update.rs b/wicket/src/ui/panes/update.rs index cf6ac255b6..2819b3ddda 100644 --- a/wicket/src/ui/panes/update.rs +++ b/wicket/src/ui/panes/update.rs @@ -29,7 +29,8 @@ use ratatui::widgets::{ use slog::{info, o, Logger}; use tui_tree_widget::{Tree, TreeItem, TreeState}; use update_engine::{ - AbortReason, ExecutionStatus, FailureReason, StepKey, WillNotBeRunReason, + AbortReason, ExecutionStatus, FailureReason, StepKey, TerminalKind, + WillNotBeRunReason, }; use wicket_common::update_events::{ EventBuffer, EventReport, ProgressEvent, StepOutcome, StepStatus, @@ -1009,9 +1010,7 @@ impl UpdatePane { } ExecutionStatus::NotStarted - | ExecutionStatus::Completed { .. } - | ExecutionStatus::Failed { .. } - | ExecutionStatus::Aborted { .. } => None, + | ExecutionStatus::Terminal(_) => None, } } else { None @@ -1041,9 +1040,7 @@ impl UpdatePane { associated with it", ); match summary.execution_status { - ExecutionStatus::Completed { .. } - | ExecutionStatus::Failed { .. } - | ExecutionStatus::Aborted { .. } => { + ExecutionStatus::Terminal(_) => { // If execution has reached a terminal // state, we can clear it. self.popup = @@ -1888,7 +1885,7 @@ impl ComponentUpdateListState { "root execution ID should have a summary associated with it", ); - match summary.execution_status { + match &summary.execution_status { ExecutionStatus::NotStarted => { status_text.push(Span::styled( "Update not started", @@ -1913,47 +1910,63 @@ impl ComponentUpdateListState { )); Some(ComponentUpdateShowHelp::Running) } - ExecutionStatus::Completed { .. } => { - status_text - .push(Span::styled("Update ", style::plain_text())); - status_text.push(Span::styled( - "completed", - style::successful_update_bold(), - )); - Some(ComponentUpdateShowHelp::Completed) - } - ExecutionStatus::Failed { step_key } => { - status_text - .push(Span::styled("Update ", style::plain_text())); - status_text.push(Span::styled( - "failed", - style::failed_update_bold(), - )); - status_text.push(Span::styled( - format!( - " at step {}/{}", - step_key.index + 1, - summary.total_steps, - ), - style::plain_text(), - )); - Some(ComponentUpdateShowHelp::Completed) - } - ExecutionStatus::Aborted { step_key } => { - status_text - .push(Span::styled("Update ", style::plain_text())); - status_text.push(Span::styled( - "aborted", - style::failed_update_bold(), - )); - status_text.push(Span::styled( - format!( - " at step {}/{}", - step_key.index + 1, - summary.total_steps, - ), - style::plain_text(), - )); + ExecutionStatus::Terminal(info) => { + match info.kind { + TerminalKind::Completed => { + status_text.push(Span::styled( + "Update ", + style::plain_text(), + )); + status_text.push(Span::styled( + "completed", + style::successful_update_bold(), + )); + } + TerminalKind::Failed => { + status_text.push(Span::styled( + "Update ", + style::plain_text(), + )); + status_text.push(Span::styled( + "failed", + style::failed_update_bold(), + )); + status_text.push(Span::styled( + format!( + " at step {}/{}", + info.step_key.index + 1, + summary.total_steps, + ), + style::plain_text(), + )); + } + TerminalKind::Aborted => { + status_text.push(Span::styled( + "Update ", + style::plain_text(), + )); + status_text.push(Span::styled( + "aborted", + style::failed_update_bold(), + )); + status_text.push(Span::styled( + format!( + " at step {}/{}", + info.step_key.index + 1, + summary.total_steps, + ), + style::plain_text(), + )); + } + } + + if let Some(total_elapsed) = info.root_total_elapsed { + status_text.push(Span::styled( + format!(" after {:.2?}", total_elapsed), + style::plain_text(), + )); + } + Some(ComponentUpdateShowHelp::Completed) } }