From 01fe39e1a0d63f255f654d6f4b98d08df2701f28 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 4 Dec 2023 12:01:25 -0500 Subject: [PATCH 1/5] [wicketd] Add refresh-config subcommand instead of using curl This allows us to add some space for refreshing a wicketd that hasn't fully started (and therefore isn't reachable on its dropshot server yet). Fixes #4604. --- smf/wicketd/manifest.xml | 2 +- wicketd/Cargo.toml | 1 + wicketd/src/bin/wicketd.rs | 32 +++++++++++++++++- wicketd/src/lib.rs | 67 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 97 insertions(+), 5 deletions(-) diff --git a/smf/wicketd/manifest.xml b/smf/wicketd/manifest.xml index 778a7abf2d..b45ff1544b 100644 --- a/smf/wicketd/manifest.xml +++ b/smf/wicketd/manifest.xml @@ -32,7 +32,7 @@ it expected https). --> diff --git a/wicketd/Cargo.toml b/wicketd/Cargo.toml index 1360c28b19..19dcbdf713 100644 --- a/wicketd/Cargo.toml +++ b/wicketd/Cargo.toml @@ -58,6 +58,7 @@ sled-hardware.workspace = true tufaceous-lib.workspace = true update-engine.workspace = true wicket-common.workspace = true +wicketd-client.workspace = true omicron-workspace-hack.workspace = true [[bin]] diff --git a/wicketd/src/bin/wicketd.rs b/wicketd/src/bin/wicketd.rs index 887ac496e0..1af41d8858 100644 --- a/wicketd/src/bin/wicketd.rs +++ b/wicketd/src/bin/wicketd.rs @@ -26,7 +26,7 @@ enum Args { #[clap(name = "CONFIG_FILE_PATH", action)] config_file_path: PathBuf, - /// The address for the technician port + /// The address on which the main wicketd dropshot server should listen #[clap(short, long, action)] address: SocketAddrV6, @@ -57,6 +57,19 @@ enum Args { #[clap(long, action, conflicts_with("read_smf_config"))] rack_subnet: Option, }, + + /// Instruct a running wicketd server to refresh its config + /// + /// Mechanically, this hits a specific endpoint served by wicketd's dropshot + /// server + RefreshConfig { + #[clap(name = "CONFIG_FILE_PATH", action)] + config_file_path: PathBuf, + + /// The address of the server to refresh + #[clap(short, long, action)] + address: SocketAddrV6, + }, } #[tokio::main] @@ -140,5 +153,22 @@ async fn do_run() -> Result<(), CmdError> { .await .map_err(|err| CmdError::Failure(anyhow!(err))) } + Args::RefreshConfig { config_file_path, address } => { + let config = Config::from_file(&config_file_path) + .with_context(|| { + format!("failed to parse {}", config_file_path.display()) + }) + .map_err(CmdError::Failure)?; + + let log = config + .log + .to_logger("wicketd") + .context("failed to initialize logger") + .map_err(CmdError::Failure)?; + + Server::refresh_config(log, address) + .await + .map_err(CmdError::Failure) + } } } diff --git a/wicketd/src/lib.rs b/wicketd/src/lib.rs index ada1902654..146ae5f616 100644 --- a/wicketd/src/lib.rs +++ b/wicketd/src/lib.rs @@ -16,11 +16,12 @@ mod preflight_check; mod rss_config; mod update_tracker; -use anyhow::{anyhow, Result}; +use anyhow::{anyhow, Context, Result}; use artifacts::{WicketdArtifactServer, WicketdArtifactStore}; use bootstrap_addrs::BootstrapPeers; pub use config::Config; pub(crate) use context::ServerContext; +use display_error_chain::DisplayErrorChain; use dropshot::{ConfigDropshot, HandlerTaskMode, HttpServer}; pub use installinator_progress::{IprUpdateTracker, RunningUpdateState}; use internal_dns::resolver::Resolver; @@ -34,6 +35,7 @@ use preflight_check::PreflightCheckerHandler; use sled_hardware::Baseboard; use slog::{debug, error, o, Drain}; use std::sync::{Mutex, OnceLock}; +use std::time::Duration; use std::{ net::{SocketAddr, SocketAddrV6}, sync::Arc, @@ -259,11 +261,70 @@ impl Server { res = self.artifact_server => { match res { Ok(()) => Err("artifact server exited unexpectedly".to_owned()), - // The artifact server returns an anyhow::Error, which has a `Debug` impl that - // prints out the chain of errors. + // The artifact server returns an anyhow::Error, which has a + // `Debug` impl that prints out the chain of errors. Err(err) => Err(format!("running artifact server: {err:?}")), } } } } + + /// Instruct a running server at the specified address to reload its config + /// parameters + pub async fn refresh_config( + log: slog::Logger, + address: SocketAddrV6, + ) -> Result<()> { + // It's possible we're being told to refresh a server's config before + // it's ready to receive such a request, so we'll give it a healthy + // amount of time before we give up: we'll set a client timeout and also + // retry a few times. See + // https://github.com/oxidecomputer/omicron/issues/4604. + const CLIENT_TIMEOUT: Duration = Duration::from_secs(5); + const SLEEP_BETWEEN_RETRIES: Duration = Duration::from_secs(10); + const NUM_RETRIES: usize = 3; + + let client = reqwest::Client::builder() + .connect_timeout(CLIENT_TIMEOUT) + .timeout(CLIENT_TIMEOUT) + .build() + .context("failed to construct reqwest Client")?; + + let client = wicketd_client::Client::new_with_client( + &format!("http://{address}"), + client, + log, + ); + let log = client.inner(); + + let mut attempt = 0; + loop { + attempt += 1; + + // If we succeed, we're done. + let Err(err) = client.post_reload_config().await else { + return Ok(()); + }; + + // If we failed, either warn+sleep and try again, or fail. + if attempt < NUM_RETRIES { + slog::warn!( + log, + "failed to refresh wicketd config \ + (attempt {attempt} of {NUM_RETRIES}); \ + will retry after {CLIENT_TIMEOUT:?}"; + "err" => %DisplayErrorChain::new(&err), + ); + tokio::time::sleep(SLEEP_BETWEEN_RETRIES).await; + } else { + slog::error!( + log, + "failed to refresh wicketd config \ + (tried {NUM_RETRIES} times)"; + "err" => %DisplayErrorChain::new(&err), + ); + return Err(err).context("failed to contact wicketd"); + } + } + } } From 283dad7410317be085458b9a1701c34cc224fdfc Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 4 Dec 2023 13:34:52 -0500 Subject: [PATCH 2/5] fix compiler warning on illumos --- wicketd/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/wicketd/src/lib.rs b/wicketd/src/lib.rs index 146ae5f616..32188d77de 100644 --- a/wicketd/src/lib.rs +++ b/wicketd/src/lib.rs @@ -72,7 +72,6 @@ pub struct SmfConfigValues { impl SmfConfigValues { #[cfg(target_os = "illumos")] pub fn read_current() -> Result { - use anyhow::Context; use illumos_utils::scf::ScfHandle; const CONFIG_PG: &str = "config"; From 836be33e4602ddd35cbd530a67266d7d5ba99ad6 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 5 Dec 2023 08:44:38 -0500 Subject: [PATCH 3/5] use utf8pathbuf for cli args --- wicketd/src/bin/wicketd.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/wicketd/src/bin/wicketd.rs b/wicketd/src/bin/wicketd.rs index 1af41d8858..4cffd80f78 100644 --- a/wicketd/src/bin/wicketd.rs +++ b/wicketd/src/bin/wicketd.rs @@ -5,6 +5,7 @@ //! Executable for wicketd: technician port based management service use anyhow::{anyhow, Context}; +use camino::Utf8PathBuf; use clap::Parser; use omicron_common::{ address::Ipv6Subnet, @@ -24,7 +25,7 @@ enum Args { /// Start a wicketd server Run { #[clap(name = "CONFIG_FILE_PATH", action)] - config_file_path: PathBuf, + config_file_path: Utf8PathBuf, /// The address on which the main wicketd dropshot server should listen #[clap(short, long, action)] @@ -64,7 +65,7 @@ enum Args { /// server RefreshConfig { #[clap(name = "CONFIG_FILE_PATH", action)] - config_file_path: PathBuf, + config_file_path: Utf8PathBuf, /// The address of the server to refresh #[clap(short, long, action)] @@ -117,9 +118,7 @@ async fn do_run() -> Result<(), CmdError> { }; let config = Config::from_file(&config_file_path) - .with_context(|| { - format!("failed to parse {}", config_file_path.display()) - }) + .with_context(|| format!("failed to parse {config_file_path}")) .map_err(CmdError::Failure)?; let rack_subnet = match rack_subnet { @@ -155,9 +154,7 @@ async fn do_run() -> Result<(), CmdError> { } Args::RefreshConfig { config_file_path, address } => { let config = Config::from_file(&config_file_path) - .with_context(|| { - format!("failed to parse {}", config_file_path.display()) - }) + .with_context(|| format!("failed to parse {config_file_path}")) .map_err(CmdError::Failure)?; let log = config From ba9bde5ac053339840f2039cbf099affd895857e Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 5 Dec 2023 08:45:03 -0500 Subject: [PATCH 4/5] remove duplicated dependency --- wicketd/Cargo.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/wicketd/Cargo.toml b/wicketd/Cargo.toml index 19dcbdf713..97550342d0 100644 --- a/wicketd/Cargo.toml +++ b/wicketd/Cargo.toml @@ -84,4 +84,3 @@ tar.workspace = true tokio = { workspace = true, features = ["test-util"] } tufaceous.workspace = true wicket.workspace = true -wicketd-client.workspace = true From ba50b1d91365308abf5d10185fe9ffeb59058d94 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 5 Dec 2023 08:49:26 -0500 Subject: [PATCH 5/5] add comment on SMF exit codes --- wicketd/src/bin/wicketd.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/wicketd/src/bin/wicketd.rs b/wicketd/src/bin/wicketd.rs index 4cffd80f78..24fa802c79 100644 --- a/wicketd/src/bin/wicketd.rs +++ b/wicketd/src/bin/wicketd.rs @@ -163,6 +163,10 @@ async fn do_run() -> Result<(), CmdError> { .context("failed to initialize logger") .map_err(CmdError::Failure)?; + // When run via `svcadm refresh ...`, we need to respect the special + // [SMF exit codes](https://illumos.org/man/7/smf_method). Returning + // an error from main exits with code 1 (from libc::EXIT_FAILURE), + // which does not collide with any special SMF codes. Server::refresh_config(log, address) .await .map_err(CmdError::Failure)