From 3d6aa458509c1a92d828e831506a4535fabeeed1 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Fri, 30 Oct 2026 16:57:32 +0000 Subject: [PATCH] telemetry: report CLI and reboot command failures Rebase onto agent configuration read-error handling. Original layer head: c91d9cfecacafa6f0980d580cf6f9edb3735e6ee Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 43cb811b-f191-4d90-852f-e3373041ca11 --- crates/trident/src/lib.rs | 4 +- crates/trident/src/main.rs | 206 ++++++++++++++++--------------- crates/trident/src/server/mod.rs | 34 ++--- crates/trident/src/validation.rs | 14 +++ 4 files changed, 143 insertions(+), 115 deletions(-) diff --git a/crates/trident/src/lib.rs b/crates/trident/src/lib.rs index 35d1430076..ea3a3d0085 100644 --- a/crates/trident/src/lib.rs +++ b/crates/trident/src/lib.rs @@ -66,8 +66,8 @@ pub use crate::{ logstream::Logstream, operation_context::{ command_name, run_command, run_command_if, run_reboot_command, - run_with_captured_operation, run_with_operation, save_reboot_operation, - take_reboot_operation, OperationSource, + run_with_captured_operation, save_reboot_operation, take_reboot_operation, + OperationSource, }, tracestream::TraceStream, }, diff --git a/crates/trident/src/main.rs b/crates/trident/src/main.rs index 5f7d97ad69..0b9d91feb9 100644 --- a/crates/trident/src/main.rs +++ b/crates/trident/src/main.rs @@ -11,9 +11,9 @@ use trident::{ cli::{self, Cli, Commands, GetKind, TridentExitCodes}, init::offline, manual_rollback::{self, utils::ManualRollbackRequestKind}, - run_with_captured_operation, run_with_operation, save_reboot_operation, take_reboot_operation, - validation, BackgroundLog, BackgroundUploader, DataStore, ExitKind, LogForwarder, Logstream, - OperationSource, TraceStream, Trident, TRIDENT_BACKGROUND_LOG_PATH, + run_command, run_reboot_command, save_reboot_operation, validation, BackgroundLog, + BackgroundUploader, DataStore, ExitKind, LogForwarder, Logstream, OperationSource, TraceStream, + Trident, TRIDENT_BACKGROUND_LOG_PATH, }; use trident_api::{ config::HostConfigurationSource, @@ -49,7 +49,17 @@ fn run_trident( proxy_status("NO_PROXY"), ); - // Catch exit fast commands + // Fast-exit commands: read-only/one-shot commands that never start a + // servicing run (validate, get, diagnose, offline-initialize, a manual + // rollback --check, start-network). These deliberately run outside + // run_command below -- no command_start/command_error telemetry is + // emitted for them. Their failures (a malformed --config, a datastore + // that can't be opened, a diagnostics bundle that can't be written, + // etc.) are operator-input or read errors, not servicing outcomes; a + // genuine underlying datastore/host problem still gets telemetry when + // the actual servicing operation (install/update/etc.) that triggered + // it runs. Handled here, before `command`/run_command are even set up, + // so none of that machinery needs to reason about them. match &args.command { Commands::Validate { config } => { return validation::validate_host_config_file(config).map(|()| ExitKind::Done); @@ -123,96 +133,97 @@ fn run_trident( _ => (), } - let res = panic::catch_unwind(move || { - match &args.command { - Commands::Install { status, error, .. } - | Commands::Update { status, error, .. } - | Commands::Commit { status, error } - | Commands::RebuildRaid { status, error, .. } - | Commands::Rollback { status, error, .. } => { - // Determined before any preflight checks below, and used - // to wrap the *entire* servicing branch (preflight checks, - // Trident::new, and the actual command) in a single - // run_with_operation call -- not just the innermost - // install/update/commit/rollback/rebuild-raid call, as - // before. That previously left Trident::new (and - // everything it does, including firing "trident_start") - // outside any operation context: every event from CLI - // startup through to just before the actual command ran - // had no operation_id/command. - // `Commands::kind()` is total over every variant - // (including ones that can never actually reach this - // arm), so it renders the same wire name the old - // per-variant match did without needing an - // `unreachable!()` fallback to prove exhaustiveness. - let kind = args.command.kind(); - - // Determined up front, before any preflight checks below, - // so a missing/nonexistent --config is rejected immediately. - let config_path = match &args.command { - Commands::Update { config, .. } | Commands::Install { config, .. } => { - Some(config.clone()) - } - Commands::RebuildRaid { config, .. } => config.clone(), - _ => None, - }; - if let Some(path) = &config_path { - if !path.exists() { - return run_with_operation(&kind, OperationSource::Cli, || { - Err(TridentError::new(InvalidInputError::ReadInputFile { - path: path.to_string_lossy().to_string(), - })) - .message("Config file does not exist") - }); - } - } + // Only servicing commands reach here: Install, Update, Commit, + // RebuildRaid, and a non-check Rollback. These get command_start/ + // command_error telemetry via run_command below; the fast-exit + // commands above already returned without any. `Commands::kind()` is + // total over every variant (including the fast-exit ones and + // Daemon/GrpcClient, none of which can actually reach here), so it + // renders the same wire name the old per-variant match did without + // needing `unreachable!()` arms to prove exhaustiveness. + let kind = args.command.kind(); + + // Determine this up front, before the pre-warm attach below: a + // multiboot install may swap to a brand-new temporary datastore + // inside `Trident::install` (see there), distinct from + // `datastore_path` here (the existing host's persistent datastore) -- + // so skip attaching an installation ID from the pre-swap datastore + // until `install` has settled on which one it actually uses, rather + // than attaching the existing host's here and having it (and every + // event emitted before the swap decision) be wrong for the rest of + // the run. See `new_deferring_installation_id`'s doc comment for the + // full rationale. + let defer_installation_id = matches!( + args.command, + Commands::Install { + multiboot: true, + .. + } + ); - // Determine this up front, before the pre-warm attach - // below: a multiboot install may swap to a brand-new - // temporary datastore inside `Trident::install` (see - // there), distinct from `datastore_path` here (the - // existing host's persistent datastore) -- so skip - // attaching an installation ID from the pre-swap datastore - // until `install` has settled on which one it actually - // uses, rather than attaching the existing host's here and - // having it (and every event emitted before the swap - // decision) be wrong for the rest of the run. See - // `new_deferring_installation_id`'s doc comment for the - // full rationale. - let defer_installation_id = matches!( - args.command, - Commands::Install { - multiboot: true, - .. - } - ); - - // Pre-warm persisted IDs for errors that occur before Trident::new can - // attach them. Startup events omit both IDs even when they are cached. - // This does not create a datastore, but may migrate a legacy installation - // ID. Multiboot defers installation-ID attachment until its staging - // datastore inherits the source identity. - // Load once and reuse the same snapshot for both the - // pre-warm attach here and the operation closure below: - // calling `AgentConfig::load()` a second time inside the - // closure could observe a different `DatastorePath` (e.g. a - // CIH bootstrap swap between the two reads), leaving the - // ID cached on `tracestream` here attributed to a - // different datastore than the one the operation actually - // runs against. - let agent_config_result = AgentConfig::load(); - if let Ok(agent_config) = &agent_config_result { - tracestream.attach_ids_if_present( - agent_config.datastore_path(), - defer_installation_id, - ); - } + // Pre-warm persisted IDs for errors that occur before Trident::new can + // attach them. Startup events omit both IDs even when they are cached. + // This does not create a datastore, but may migrate a legacy installation + // ID. Multiboot defers installation-ID attachment until its staging + // datastore inherits the source identity. + // Load once and reuse the same snapshot inside the run_command + // closure below, rather than reloading there: calling + // `AgentConfig::load()` a second time could observe a different + // `DatastorePath` (e.g. a CIH bootstrap swap between the two reads), + // leaving the ID attached to `tracestream` here attributed to a + // different datastore than the one the operation actually runs + // against. + let agent_config_result = AgentConfig::load(); + if let Ok(agent_config) = &agent_config_result { + tracestream.attach_ids_if_present(agent_config.datastore_path(), defer_installation_id); + } - run_with_operation(&kind, OperationSource::Cli, || { + // Determined up front so a missing/nonexistent --config is rejected + // immediately, before run_command below even fires command_start. + let config_path = match &args.command { + Commands::Update { config, .. } | Commands::Install { config, .. } => Some(config.clone()), + Commands::RebuildRaid { config, .. } => config.clone(), + _ => None, + }; + if let Some(path) = &config_path { + if !path.exists() { + return run_command(&kind, OperationSource::Cli, || { + Err(TridentError::new(InvalidInputError::ReadInputFile { + path: path.to_string_lossy().to_string(), + })) + .message("Config file does not exist") + }); + } + } + + // run_command itself now catches a panic from its closure (while the + // operation context is still active) and fires command_error before + // re-raising it, so a genuine panic gets the same telemetry as a + // normal Err. This outer catch_unwind remains as a safety net for a + // panic occurring outside run_command's closure (e.g. in run_command's + // own setup) and to keep converting an unwound panic into a non-zero + // exit code below. + let res = panic::catch_unwind(move || { + run_command(&kind, OperationSource::Cli, || { + match &args.command { + Commands::Install { status, error, .. } + | Commands::Update { status, error, .. } + | Commands::Commit { status, error } + | Commands::RebuildRaid { status, error, .. } + | Commands::Rollback { status, error, .. } => { // config_path was already validated (existence-checked) - // above. Reuse the same `AgentConfig` snapshot loaded - // just above rather than reloading -- see the comment - // there. + // above. + let config_path = match &args.command { + Commands::Update { config, .. } | Commands::Install { config, .. } => { + Some(config.clone()) + } + Commands::RebuildRaid { config, .. } => config.clone(), + _ => None, + }; + + // Reuse the same `AgentConfig` snapshot loaded above + // (before this closure/panic::catch_unwind) rather + // than reloading -- see the comment there. let agent_config = agent_config_result?; // For commands that cannot themselves stage a new // install/update (see @@ -316,7 +327,7 @@ fn run_trident( // Capture this operation's identity while its context // is still installed (this closure runs entirely - // inside `run_with_operation`'s scope), so the reboot + // inside `run_command`'s scope), so the reboot // requested below by the caller can be tagged with the // *original* install/update/etc.'s `operation_id`/ // `command` instead of a disconnected fresh one -- see @@ -326,10 +337,10 @@ fn run_trident( } res.message(format!("Failed to execute '{}' command", args.command)) - }) + } + _ => unreachable!(), } - _ => unreachable!(), - } + }) }); match res { @@ -561,10 +572,7 @@ fn main() -> ExitCode { return TridentExitCodes::Failed.into(); } Ok(ExitKind::NeedsReboot) => { - if let Err(e) = run_with_captured_operation( - take_reboot_operation(), - trident::request_reboot_with_wait, - ) { + if let Err(e) = run_reboot_command(trident::request_reboot_with_wait) { error!("Failed to reboot: {e:?}"); return TridentExitCodes::RebootUnsuccessful.into(); } diff --git a/crates/trident/src/server/mod.rs b/crates/trident/src/server/mod.rs index f8a0f723ba..d24c7279c1 100644 --- a/crates/trident/src/server/mod.rs +++ b/crates/trident/src/server/mod.rs @@ -32,6 +32,8 @@ use trident_proto::v1preview::{ status_service_server::StatusServiceServer, validation_service_server::ValidationServiceServer, }; +use trident_api::error::{ReportError, ServicingError, TridentError}; + use crate::{ agentconfig::AgentConfig, cli::TridentExitCodes, @@ -180,21 +182,25 @@ fn reboot(signals: ShutdownSignals) -> ExitCode { // time this function runs, the whole daemon event loop // (`server_main_inner`/`main_task`) has already returned, so there is // no thread-local operation context active here at all. - let request_result = operation_context::run_with_captured_operation( - operation_context::take_reboot_operation(), - reboot::request_reboot, - ); - if let Err(e) = request_result { - error!("Failed to request reboot: {e:?}"); - return TridentExitCodes::RebootUnsuccessful.into(); - } + // + // Routed through `run_reboot_command` (not + // `run_with_captured_operation` directly) so a failed reboot fires + // `command_error` here exactly like the CLI's reboot path does, + // making a failed daemon-driven reboot (systemctl unreachable, or the + // shutdown signal never arriving) distinguishable in telemetry from a + // successful one. + let reboot_result: Result<(), TridentError> = operation_context::run_reboot_command(|| { + reboot::request_reboot().structured(ServicingError::Reboot)?; + + // Wait for either a shutdown signal or the reboot timeout. + signals + .exit_receiver() + .recv_timeout(Duration::from_secs(REBOOT_WAIT_DURATION_SECS)) + .structured(ServicingError::RebootTimeout) + }); - // Wait for either a shutdown signal or the reboot timeout - if let Err(e) = signals - .exit_receiver() - .recv_timeout(Duration::from_secs(REBOOT_WAIT_DURATION_SECS)) - { - error!("Reboot wait timed out: {e:?}"); + if let Err(e) = reboot_result { + error!("Failed to reboot: {e:?}"); return TridentExitCodes::RebootUnsuccessful.into(); } diff --git a/crates/trident/src/validation.rs b/crates/trident/src/validation.rs index ffa59f6564..38125801b3 100644 --- a/crates/trident/src/validation.rs +++ b/crates/trident/src/validation.rs @@ -38,6 +38,20 @@ pub(crate) fn parse_host_config( parsed } +/// Read and parse (but do not semantically validate) a Host Configuration +/// file at the given path -- the same parse step `Trident::new` performs +/// internally before it does anything else with the file, exposed so +/// callers can perform it as a side-effect-free preflight check (e.g. +/// before creating a datastore on the strength of the path merely +/// existing) without duplicating the read+parse logic themselves. +pub fn parse_host_config_file(path: impl AsRef) -> Result { + let contents = + fs::read_to_string(path.as_ref()).structured(InvalidInputError::ReadInputFile { + path: path.as_ref().display().to_string(), + })?; + parse_host_config(&contents, Some(path.as_ref())) +} + /// Validate a Host Configuration file at the given path. pub fn validate_host_config_file(path: impl AsRef) -> Result<(), TridentError> { info!(