From 16d4b2e006826bb5feebd74aa180d3a400685ab0 Mon Sep 17 00:00:00 2001 From: Tom Kirchner Date: Tue, 17 Nov 2020 22:47:35 +0000 Subject: [PATCH 1/2] apiclient: add high-level update subcommands --- sources/Cargo.lock | 3 + sources/api/apiclient/Cargo.toml | 7 +- sources/api/apiclient/src/lib.rs | 45 +++- sources/api/apiclient/src/main.rs | 286 +++++++++++++++++++- sources/api/apiclient/src/update.rs | 394 ++++++++++++++++++++++++++++ 5 files changed, 711 insertions(+), 24 deletions(-) create mode 100644 sources/api/apiclient/src/update.rs diff --git a/sources/Cargo.lock b/sources/Cargo.lock index a0b22918553..a687d38c50b 100644 --- a/sources/Cargo.lock +++ b/sources/Cargo.lock @@ -334,6 +334,9 @@ dependencies = [ "http", "hyper", "hyper-unix-connector", + "log", + "serde_json", + "simplelog", "snafu", "tokio", "unindent", diff --git a/sources/api/apiclient/Cargo.toml b/sources/api/apiclient/Cargo.toml index 1cc541b6619..f538a4f6c43 100644 --- a/sources/api/apiclient/Cargo.toml +++ b/sources/api/apiclient/Cargo.toml @@ -13,10 +13,13 @@ exclude = ["README.md"] http = "0.2" hyper = { version = "0.13", default-features = false } hyper-unix-connector = "0.1" +log = "0.4" +serde_json = "1.0" +simplelog = "0.8" snafu = "0.6" -tokio = { version = "0.2", default-features = false, features = ["macros", "rt-threaded"] } +tokio = { version = "0.2", default-features = false, features = ["macros", "rt-threaded", "time"] } # When hyper updates to tokio 0.3: -#tokio = { version = "0.3", default-features = false, features = ["macros", "rt-multi-thread"] } +#tokio = { version = "0.3", default-features = false, features = ["macros", "rt-multi-thread", "time"] } unindent = "0.1" [build-dependencies] diff --git a/sources/api/apiclient/src/lib.rs b/sources/api/apiclient/src/lib.rs index 834b4f188b4..a897b4260d8 100644 --- a/sources/api/apiclient/src/lib.rs +++ b/sources/api/apiclient/src/lib.rs @@ -18,6 +18,8 @@ use hyper_unix_connector::{UnixClient, Uri}; use snafu::{ensure, ResultExt}; use std::path::Path; +pub mod update; + mod error { use snafu::Snafu; @@ -34,7 +36,7 @@ mod error { ResponseStatus { method: String, code: http::StatusCode, - uri: http::uri::Uri, + uri: String, body: String, }, @@ -69,6 +71,36 @@ pub async fn raw_request( method: S2, data: Option, ) -> Result<(http::StatusCode, String)> +where + P: AsRef, + S1: AsRef, + S2: AsRef, +{ + let (status, body) = raw_request_unchecked(&socket_path, &uri, &method, data).await?; + + // Error if the response status is in not in the 2xx range. + ensure!( + status.is_success(), + error::ResponseStatus { + method: method.as_ref(), + code: status, + uri: uri.as_ref(), + body, + } + ); + + Ok((status, body)) +} + +/// Works exactly like raw_request in making an HTTP request over a Unix-domain socket, but doesn't +/// check that the returned status code represents success. This can be useful if you have to +/// handle specific error codes, rather than inspecting the Error type of raw_request. +pub async fn raw_request_unchecked( + socket_path: P, + uri: S1, + method: S2, + data: Option, +) -> Result<(http::StatusCode, String)> where P: AsRef, S1: AsRef, @@ -103,16 +135,5 @@ where .context(error::ResponseBodyRead)?; let body = String::from_utf8(body_bytes.to_vec()).context(error::NonUtf8Response)?; - // Error if the response status is in not in the 2xx range. - ensure!( - status.is_success(), - error::ResponseStatus { - method, - code: status, - uri, - body, - } - ); - Ok((status, body)) } diff --git a/sources/api/apiclient/src/main.rs b/sources/api/apiclient/src/main.rs index 3a18e84720c..29619e01c65 100644 --- a/sources/api/apiclient/src/main.rs +++ b/sources/api/apiclient/src/main.rs @@ -1,5 +1,14 @@ +//! The apiclient binary provides some high-level, synchronous methods of interacting with the +//! API, for example an `update` subcommand that wraps the individual API calls needed to update +//! the host. There's also a low-level `raw` subcommand for direct interaction. + +use apiclient::update; +use log::{info, log_enabled, trace, warn}; +use simplelog::{ConfigBuilder as LogConfigBuilder, LevelFilter, TermLogger, TerminalMode}; +use snafu::ResultExt; use std::env; use std::process; +use std::str::FromStr; use unindent::unindent; const DEFAULT_API_SOCKET: &str = "/run/api.sock"; @@ -8,31 +17,62 @@ const DEFAULT_METHOD: &str = "GET"; /// Stores user-supplied global arguments. #[derive(Debug)] struct Args { - verbosity: usize, + log_level: LevelFilter, socket_path: String, } impl Default for Args { fn default() -> Self { Self { - verbosity: 3, + log_level: LevelFilter::Info, socket_path: DEFAULT_API_SOCKET.to_string(), } } } /// Stores the usage mode specified by the user as a subcommand. +#[derive(Debug)] enum Subcommand { Raw(RawArgs), + Reboot(RebootArgs), + Update(UpdateSubcommand), } /// Stores user-supplied arguments for the 'raw' subcommand. +#[derive(Debug)] struct RawArgs { method: String, uri: String, data: Option, } +/// Stores user-supplied arguments for the 'reboot' subcommand. +#[derive(Debug)] +struct RebootArgs {} + +/// Stores the 'update' subcommand specified by the user. +#[derive(Debug)] +enum UpdateSubcommand { + Check(CheckArgs), + Apply(ApplyArgs), + Cancel(CancelArgs), +} + +/// Stores user-supplied arguments for the 'update check' subcommand. +#[derive(Debug)] +struct CheckArgs {} + +/// Stores user-supplied arguments for the 'update apply' subcommand. +#[derive(Debug)] +struct ApplyArgs { + check: bool, + reboot: bool, +} + +/// Stores user-supplied arguments for the 'update cancel' subcommand. +#[derive(Debug)] +struct CancelArgs {} + /// Informs the user about proper usage of the program and exits. fn usage() -> ! { let msg = &format!( @@ -40,16 +80,35 @@ fn usage() -> ! { Global options: -s, --socket-path PATH Override the server socket path. Default: {socket} - -v, --verbose Print extra information like HTTP status code. + --log-level Desired amount of output; trace|debug|info|warn|error + -v, --verbose Sets log level to 'debug'. This prints extra info, + like HTTP status code to stderr in 'raw' mode. Subcommands: - raw Makes an HTTP request to the server. + raw Makes an HTTP request and prints the response on stdout. 'raw' is the default subcommand and may be omitted. + update check Prints information about available updates. + update apply Applies available updates. + update cancel Deactivates an applied update. + reboot Reboots the host. raw options: -u, --uri URI Required; URI to request from the server, e.g. /tx -m, -X, --method METHOD HTTP method to use in request. Default: {method} - -d, --data DATA Data to include in the request body. Default: empty", + -d, --data DATA Data to include in the request body. Default: empty + + reboot options: + None. + + update check options: + None. + + update apply options: + -c, --check Automatically `update check` and apply whatever is found. + -r, --reboot Automatically reboot if an update was found and applied. + + update cancel options: + None.", socket = DEFAULT_API_SOCKET, method = DEFAULT_METHOD, ); @@ -63,6 +122,8 @@ fn usage_msg>(msg: S) -> ! { usage(); } +// =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= + /// Parses user arguments into an Args structure. fn parse_args(args: env::Args) -> (Args, Subcommand) { let mut global_args = Args::default(); @@ -75,7 +136,17 @@ fn parse_args(args: env::Args) -> (Args, Subcommand) { "-h" | "--help" => usage(), // Global args - "-v" | "--verbose" => global_args.verbosity += 1, + "--log-level" => { + let log_level_str = iter + .next() + .unwrap_or_else(|| usage_msg("Did not give argument to --log-level")); + global_args.log_level = + LevelFilter::from_str(&log_level_str).unwrap_or_else(|_| { + usage_msg(format!("Invalid log level '{}'", log_level_str)) + }); + } + + "-v" | "--verbose" => global_args.log_level = LevelFilter::Debug, "-s" | "--socket-path" => { global_args.socket_path = iter @@ -84,7 +155,9 @@ fn parse_args(args: env::Args) -> (Args, Subcommand) { } // Subcommands - "raw" if subcommand.is_none() && !arg.starts_with('-') => subcommand = Some(arg), + "raw" | "reboot" | "update" if subcommand.is_none() && !arg.starts_with('-') => { + subcommand = Some(arg) + } // Other arguments are passed to the subcommand parser _ => subcommand_args.push(arg), @@ -94,10 +167,14 @@ fn parse_args(args: env::Args) -> (Args, Subcommand) { match subcommand.as_deref() { // Default subcommand is 'raw' None | Some("raw") => return (global_args, parse_raw_args(subcommand_args)), + Some("reboot") => return (global_args, parse_reboot_args(subcommand_args)), + Some("update") => return (global_args, parse_update_args(subcommand_args)), _ => usage_msg("Missing or unknown subcommand"), } } +/// Parses arguments for the 'raw' subcommand, which is also the default if no subcommand is +/// provided. fn parse_raw_args(args: Vec) -> Subcommand { let mut method = None; let mut uri = None; @@ -138,21 +215,181 @@ fn parse_raw_args(args: Vec) -> Subcommand { }) } -async fn run() -> apiclient::Result<()> { +/// Parses arguments for the 'reboot' subcommand. +fn parse_reboot_args(args: Vec) -> Subcommand { + if !args.is_empty() { + usage_msg(&format!("Unknown arguments: {}", args.join(", "))); + } + Subcommand::Reboot(RebootArgs {}) +} + +/// Parses the desired subcommand of 'update'. +fn parse_update_args(args: Vec) -> Subcommand { + let mut subcommand = None; + let mut subcommand_args = Vec::new(); + + let mut iter = args.into_iter(); + while let Some(arg) = iter.next() { + match arg.as_ref() { + // Subcommands + "check" | "apply" | "cancel" if subcommand.is_none() && !arg.starts_with('-') => { + subcommand = Some(arg) + } + + // Other arguments are passed to the subcommand parser + _ => subcommand_args.push(arg), + } + } + + let update = match subcommand.as_deref() { + Some("check") => parse_check_args(subcommand_args), + Some("apply") => parse_apply_args(subcommand_args), + Some("cancel") => parse_cancel_args(subcommand_args), + _ => usage_msg("Missing or unknown subcommand for 'update'"), + }; + + Subcommand::Update(update) +} + +/// Parses arguments for the 'update check' subcommand. +fn parse_check_args(args: Vec) -> UpdateSubcommand { + if !args.is_empty() { + usage_msg(&format!("Unknown arguments: {}", args.join(", "))); + } + UpdateSubcommand::Check(CheckArgs {}) +} + +/// Parses arguments for the 'update apply' subcommand. +fn parse_apply_args(args: Vec) -> UpdateSubcommand { + let mut check = false; + let mut reboot = false; + + let mut iter = args.into_iter(); + while let Some(arg) = iter.next() { + match arg.as_ref() { + "-c" | "--check" => check = true, + "-r" | "--reboot" => reboot = true, + + x => usage_msg(&format!("Unknown argument '{}'", x)), + } + } + + UpdateSubcommand::Apply(ApplyArgs { check, reboot }) +} + +/// Parses arguments for the 'update cancel' subcommand. +fn parse_cancel_args(args: Vec) -> UpdateSubcommand { + if !args.is_empty() { + usage_msg(&format!("Unknown arguments: {}", args.join(", "))); + } + UpdateSubcommand::Cancel(CancelArgs {}) +} + +// =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= + +/// Requests a reboot through the API. +async fn reboot(args: &Args) -> Result<()> { + let uri = "/actions/reboot"; + let method = "POST"; + let (_status, _body) = apiclient::raw_request(&args.socket_path, uri, method, None) + .await + .context(error::Request { uri, method })?; + + info!("Rebooting, goodbye..."); + Ok(()) +} + +/// Requests an update status check through the API, printing the updated status, in a pretty +/// format if possible. +async fn check(args: &Args) -> Result { + let output = update::check(&args.socket_path) + .await + .context(error::Check)?; + + match serde_json::from_str::(&output) { + Ok(value) => println!("{:#}", value), + Err(e) => { + warn!("Unable to deserialize response (invalid JSON?): {}", e); + println!("{}", output); + } + } + + Ok(output) +} + +/// Main entry point, dispatches subcommands. +async fn run() -> Result<()> { let (args, subcommand) = parse_args(env::args()); + trace!("Parsed args for subcommand {:?}: {:?}", subcommand, args); + + // We use TerminalMode::Stderr because apiclient users expect server response data on stdout. + TermLogger::init( + args.log_level, + LogConfigBuilder::new() + .add_filter_allow_str("apiclient") + .build(), + TerminalMode::Stderr, + ) + .context(error::Logger)?; match subcommand { Subcommand::Raw(raw) => { let (status, body) = - apiclient::raw_request(args.socket_path, raw.uri, raw.method, raw.data).await?; + apiclient::raw_request(&args.socket_path, &raw.uri, &raw.method, raw.data) + .await + .context(error::Request { + uri: &raw.uri, + method: &raw.method, + })?; - if args.verbosity > 3 { + // In raw mode, the user is expecting only the server response on stdout, so we more + // carefully control other output and only write it to stderr. + if log_enabled!(log::Level::Debug) { eprintln!("{}", status); } if !body.is_empty() { println!("{}", body); } } + + Subcommand::Reboot(_reboot) => { + reboot(&args).await?; + } + + Subcommand::Update(subcommand) => match subcommand { + UpdateSubcommand::Check(_check) => { + check(&args).await?; + } + + UpdateSubcommand::Apply(apply) => { + if apply.check { + let output = check(&args).await?; + // Exit early if no update is required, either because none is available or one + // is already applied and ready. + if !update::required(&output) { + return Ok(()); + } + } + + update::apply(&args.socket_path) + .await + .context(error::Apply)?; + + // If the user requested it, and if we applied an update, reboot. (update::apply + // will fail if no update was available or it couldn't apply the update.) + if apply.reboot { + reboot(&args).await?; + } else { + info!("Update has been applied and will take effect on next reboot."); + } + } + + UpdateSubcommand::Cancel(_cancel) => { + update::cancel(&args.socket_path) + .await + .context(error::Cancel)?; + } + }, } Ok(()) @@ -168,3 +405,32 @@ async fn main() { process::exit(1); } } + +mod error { + use apiclient::update; + use snafu::Snafu; + + #[derive(Debug, Snafu)] + #[snafu(visibility = "pub(super)")] + pub enum Error { + #[snafu(display("Failed to apply update: {}", source))] + Apply { source: update::Error }, + + #[snafu(display("Failed to cancel update: {}", source))] + Cancel { source: update::Error }, + + #[snafu(display("Failed to check for updates: {}", source))] + Check { source: update::Error }, + + #[snafu(display("Logger setup error: {}", source))] + Logger { source: simplelog::TermLogError }, + + #[snafu(display("Failed {} request to '{}': {}", method, uri, source))] + Request { + method: String, + uri: String, + source: apiclient::Error, + }, + } +} +type Result = std::result::Result; diff --git a/sources/api/apiclient/src/update.rs b/sources/api/apiclient/src/update.rs new file mode 100644 index 00000000000..c83199cda59 --- /dev/null +++ b/sources/api/apiclient/src/update.rs @@ -0,0 +1,394 @@ +/*! +This module simplifies the usage of the update APIs, and makes them synchronous by waiting for +updated status after commands are started. + +The update API forks off a thar-be-updates process to do the long-running work that affects the +system. So that users can know when that's complete, the /updates/status call includes a +`most_recent_command` structure giving details about the time, type, and result of the last issued +command. So, this is our basic process: +* Check initial status, saving the timestamp. +* Issue our request. +* Poll the status API (with timeout and failure checks) until the timestamp changes. +* Confirm that the status shows that our requested command was successful. + +Note: there's some potential for conflict with other callers, even with the API locking. If +someone else issues a command after ours completes, but before we can check, the status could be +lost. The update API's state machine ensures we only move in expected directions, and we don't +want to reinvent its logic here, or be too strict about timing. We can just time out, or perhaps +sync up if their request was the same. If it becomes a problem, we could perhaps use something +like transactions, or timed lock files, to avoid it. +*/ + +use super::{raw_request, raw_request_unchecked}; +use http::StatusCode; +use log::{debug, info, trace, warn}; +use snafu::{ensure, ResultExt}; +use std::path::Path; +use std::thread; +use std::time::Duration; +use tokio::time; + +/// Refresh the list of available updates and return the current status. +pub async fn check

(socket_path: P) -> Result +where + P: AsRef, +{ + info!("Refreshing updates..."); + let (_body, status) = wait_request( + socket_path, + "/actions/refresh-updates", + "POST", + None, + "refresh", + &WaitPolicy::new(Duration::from_millis(100), 10 * 10), + ) + .await?; + + Ok(status) +} + +/// Checks whether an update is required given the current update status, and informs the user +/// about the specific state. +// Note: we shouldn't reimplement the update API's state tracking or knowledge here, but giving +// a useful message rather than "failed to prepare update" in common scenarios is critical. +pub fn required(check_output: &str) -> bool { + let state = + response_field(&["update_state"], check_output).unwrap_or_else(|| "unknown".to_string()); + match state.as_ref() { + "Idle" => { + info!("No updates available."); + false + } + "Ready" => { + info!("Update already applied; reboot for it to take effect, or request a cancel."); + false + } + _ => { + debug!("Saw state '{}', assuming update is required", state); + true + } + } +} + +/// Applies the update shown as selected in the output of check(), and makes it active. +pub async fn apply

(socket_path: P) -> Result<()> +where + P: AsRef, +{ + info!("Downloading and applying update to disk..."); + let (_body, _status) = wait_request( + &socket_path, + "/actions/prepare-update", + "POST", + None, + "prepare", + &WaitPolicy::new(Duration::from_millis(500), 2 * 60 * 10), + ) + .await + .context(error::PrepareUpdate)?; + + info!("Setting the update active so it will apply on the next reboot..."); + let (_body, _status) = wait_request( + &socket_path, + "/actions/activate-update", + "POST", + None, + "activate", + &WaitPolicy::new(Duration::from_millis(100), 10 * 5), + ) + .await?; + + Ok(()) +} + +/// Cancels an applied update so another can be applied. +pub async fn cancel

(socket_path: P) -> Result +where + P: AsRef, +{ + info!("Canceling update..."); + let (_body, status) = wait_request( + socket_path, + "/actions/deactivate-update", + "POST", + None, + "deactivate", + &WaitPolicy::new(Duration::from_millis(100), 10 * 5), + ) + .await?; + + Ok(status) +} + +// =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= + +/// Pulls a nested field out of a JSON string. The input is a list of strings representing the +/// nested structures, e.g. ["a", "b"] to select the 42 from {"a": {"b": 42}}. +/// +/// Returns None if requested key doesn't exist or we can't fetch it for any reason. +// This deserializes the JSON each time, but we know our input is small, and it's worth the +// convenience. Same reason for not using a Result. +fn response_field( + field: impl IntoIterator, + response_str: &str, +) -> Option { + let response: serde_json::Value = match serde_json::from_str(&response_str) { + Ok(json) => json, + Err(_) => return None, + }; + + let mut result = &response; + for component in field { + match result.get(component) { + Some(x) => result = x, + None => break, + } + } + result.as_str().map(|s| s.to_string()) +} + +/// Represents how a caller wants wait_request to handle waiting for status updates after +/// requesting an action from an API call. After `max_attempts` checks with `between_attempts` +/// duration between them, the call will be timed out and fail. +#[derive(Debug)] +struct WaitPolicy { + between_attempts: Duration, + max_attempts: u32, +} + +impl WaitPolicy { + fn new(between_attempts: Duration, max_attempts: u32) -> Self { + Self { + between_attempts, + max_attempts, + } + } +} + +/// This synchronously wraps a call to the update API, waiting for asynchronous status updates to +/// be complete before returning. The parameters are the same as for raw_request, plus a few +/// required to understand how we should wait: +/// * The name of the command, as given in "cmd_type" of the most_recent_command structure of +/// update API responses, e.g. "refresh" for calls to /action/refresh-updates. +/// * A WaitPolicy that describes how long we should wait for the action to complete. +async fn wait_request( + socket_path: P, + url: S1, + method: S2, + data: Option, + command_name: &'static str, + wait: &WaitPolicy, +) -> Result<(String, String)> +where + P: AsRef, + S1: AsRef, + S2: AsRef, +{ + // Fetch the initial status of the API so we know when it's changed. + let (code, initial_body) = raw_request_unchecked(&socket_path, "/updates/status", "GET", None) + .await + .context(error::GetStatus)?; + + // The timestamp is the primary field we use to notice a change. + // The first call to the update API on a new system will return a 404, which is fine. + let before_timestamp = if code == StatusCode::NOT_FOUND { + "first call".to_string() + } else if code.is_success() { + response_field(&["most_recent_command", "timestamp"], &initial_body) + .unwrap_or_else(|| "first call".to_string()) + } else { + return error::MissingStatus { + code, + body: initial_body, + } + .fail(); + }; + debug!("Found initial timestamp '{}'", before_timestamp); + + // Make the real request the user wanted. + let (_code, response_body) = raw_request(&socket_path, &url, &method, data) + .await + .context(error::Request { command_name })?; + + // Note: we've now made the real request the user asked for, and the rest is our bookkeeping to + // wait for it to finish. We're more careful with retries and don't want to early-exit with ?. + + let mut attempt: u32 = 0; + let mut failures: u32 = 0; + // How many times we'll retry our bookkeeping checks, e.g. if the status API fails. + let max_failures: u32 = 5; + // How often to let the user know we're still waiting. + let notify_every = Duration::from_secs(5); + // Counter so we can tell whether we should notify. + let mut waited = Duration::from_millis(0); + + loop { + // Check if we've timed out or failed too many requests. + attempt += 1; + ensure!( + attempt < wait.max_attempts, + error::TimedOut { + waited: format!("{:?}", wait.between_attempts * wait.max_attempts), + method: method.as_ref(), + url: url.as_ref(), + } + ); + ensure!( + failures < max_failures, + error::StatusCheck { + failures, + method: method.as_ref(), + url: url.as_ref(), + } + ); + + // Let the user know what's going on every once in a while, as we wait. + if attempt > 1 && waited >= notify_every { + waited = Duration::from_millis(0); + info!( + "Still waiting for updated status, will wait up to {:?} longer...", + (wait.max_attempts * wait.between_attempts) - (attempt * wait.between_attempts) + ); + } + time::delay_for(wait.between_attempts).await; // time::sleep in tokio v0.3 + waited += wait.between_attempts; + + // Get updated status to see if anything's changed. + let response = raw_request_unchecked(&socket_path, "/updates/status", "GET", None).await; + let (code, status_body) = match response { + Ok((code, status_body)) => (code, status_body), + Err(e) => { + failures += 1; + warn!( + "Unable to check for update status, failure #{}: {}", + failures, e + ); + continue; + } + }; + + // Mutating actions will return a LOCKED status if they're not yet complete. + if code == StatusCode::LOCKED { + trace!("Lock still held, presumably by our request..."); + continue; + } else if !code.is_success() { + failures += 1; + warn!( + "Got code {} when checking update status, failure #{}: {}", + code, failures, status_body + ); + continue; + } + + // Get the specific status fields we check. + let after_timestamp = response_field(&["most_recent_command", "timestamp"], &status_body) + .unwrap_or_else(|| "missing".to_string()); + let after_command = response_field(&["most_recent_command", "cmd_type"], &status_body) + .unwrap_or_else(|| "missing".to_string()); + debug!("Found timestamp '{}' and command '{}' (looking for command '{}' and a change from initial timestamp '{}')", + after_timestamp, after_command, command_name, before_timestamp); + + // If we have a new timestamp and see our expected command, we're done. + // Note: we keep waiting if the timestamp changed but the command isn't what we expected; + // see module docstring. + if after_command == command_name && before_timestamp != after_timestamp { + let after_status = response_field(&["most_recent_command", "cmd_status"], &status_body) + .unwrap_or_else(|| "missing".to_string()); + // If the command wasn't successful, give as much info as we can from the status. + ensure!( + after_status == "Success", + error::Command { + command_name, + stderr: response_field(&["most_recent_command", "stderr"], &status_body) + .unwrap_or_else(|| "".to_string()), + status_name: after_status, + exit_status: response_field( + &["most_recent_command", "exit_status"], + &status_body + ) + .map(|s| s.parse().ok()) + .flatten() + .unwrap_or(-1), + } + ); + return Ok((response_body, status_body)); + } + } +} + +mod error { + use snafu::Snafu; + + #[derive(Debug, Snafu)] + #[snafu(visibility = "pub(super)")] + pub enum Error { + #[snafu(display( + "{} attempt failed with status '{}' ({}): {}", + command_name, + status_name, + exit_status, + stderr + ))] + Command { + command_name: String, + status_name: String, + exit_status: i32, + stderr: String, + }, + + #[snafu(display("Failed getting update status: {}", source))] + GetStatus { source: crate::Error }, + + #[snafu(display("Unable to check initial update status, got code '{}': {}", code, body))] + MissingStatus { + code: http::StatusCode, + body: String, + }, + + // This is likely to be a common source of issues, so we have an extra-clear error message + // wrapper. + #[snafu(display( + "Failed to prepare update. This could mean that we don't have a list of updates yet \ + or that an update is already applied. Running 'apiclient update check' will help \ + you find out. You can cancel an applied ('Ready') update with 'apiclient update \ + cancel' if desired. Detail: {}", + source + ))] + PrepareUpdate { + #[snafu(source(from(Error, Box::new)))] + source: Box, + }, + + #[snafu(display("Failed to make {} request: {}", command_name, source))] + Request { + command_name: String, + source: crate::Error, + }, + + #[snafu(display( + "Failed to check status {} times after {} to {}, unsure of result", + failures, + method, + url + ))] + StatusCheck { + failures: u32, + method: String, + url: String, + }, + + #[snafu(display( + "Timed out after waiting {} seconds after {} to {}, unsure of result", + waited, + method, + url + ))] + TimedOut { + waited: String, + method: String, + url: String, + }, + } +} +pub use error::Error; +pub type Result = std::result::Result; From dd1f8797d4b9d354b36ac470c19d658a5d3070fb Mon Sep 17 00:00:00 2001 From: Tom Kirchner Date: Tue, 17 Nov 2020 22:49:06 +0000 Subject: [PATCH 2/2] apiserver: show reboot failure stderr as string, not bytes --- sources/api/apiserver/src/server/error.rs | 8 ++------ sources/api/apiserver/src/server/mod.rs | 2 +- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/sources/api/apiserver/src/server/error.rs b/sources/api/apiserver/src/server/error.rs index ccd8fd76171..522e9788bd2 100644 --- a/sources/api/apiserver/src/server/error.rs +++ b/sources/api/apiserver/src/server/error.rs @@ -120,12 +120,8 @@ pub enum Error { #[snafu(display("Unable to start shutdown: {}", source))] Shutdown { source: io::Error }, - #[snafu(display( - "Failed to reboot, exit code: {}, stderr: {}", - exit_code, - String::from_utf8_lossy(stderr) - ))] - Reboot { exit_code: i32, stderr: Vec }, + #[snafu(display("Failed to reboot, exit code: {}, stderr: {}", exit_code, stderr))] + Reboot { exit_code: i32, stderr: String }, // =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= diff --git a/sources/api/apiserver/src/server/mod.rs b/sources/api/apiserver/src/server/mod.rs index f63d7c2191d..72121174f5a 100644 --- a/sources/api/apiserver/src/server/mod.rs +++ b/sources/api/apiserver/src/server/mod.rs @@ -413,7 +413,7 @@ async fn reboot() -> Result { Some(code) => code, None => output.status.signal().unwrap_or(1), }, - stderr: output.stderr + stderr: String::from_utf8_lossy(&output.stderr), } ); Ok(HttpResponse::NoContent().finish())