diff --git a/crates/app/src/lib.rs b/crates/app/src/lib.rs index e030d7b82..1fbc7cdf7 100644 --- a/crates/app/src/lib.rs +++ b/crates/app/src/lib.rs @@ -7,6 +7,7 @@ #![deny(missing_docs)] mod host_component; +use serde_json::Value; pub use spin_locked_app::locked; pub use spin_locked_app::values; pub use spin_locked_app::{Error, MetadataKey, Result}; @@ -207,6 +208,36 @@ impl<'a, L> App<'a, L> { .map(|locked| AppTrigger { app: self, locked }) } + /// Returns the trigger metadata for a specific trigger type. + pub fn get_trigger_metadata<'this, T: Deserialize<'this> + Default>( + &'this self, + trigger_type: &'a str, + ) -> Result> { + let Some(value) = self.get_trigger_metadata_value(trigger_type) else { + return Ok(None); + }; + let metadata = T::deserialize(value).map_err(|err| { + Error::MetadataError(format!( + "invalid metadata value for {trigger_type:?}: {err:?}" + )) + })?; + Ok(Some(metadata)) + } + + fn get_trigger_metadata_value(&self, trigger_type: &str) -> Option { + if let Some(trigger_configs) = self.locked.metadata.get("triggers") { + // New-style: `{"triggers": {"": {...}}}` + trigger_configs.get(trigger_type).cloned() + } else if self.locked.metadata["trigger"]["type"] == trigger_type { + // Old-style: `{"trigger": {"type": "", ...}}` + let mut meta = self.locked.metadata["trigger"].clone(); + meta.as_object_mut().unwrap().remove("type"); + Some(meta) + } else { + None + } + } + /// Returns an iterator of [`AppTrigger`]s defined for this app with /// the given `trigger_type`. pub fn triggers_with_type( diff --git a/crates/http/src/trigger.rs b/crates/http/src/trigger.rs index a8c713d00..ca53b3354 100644 --- a/crates/http/src/trigger.rs +++ b/crates/http/src/trigger.rs @@ -7,8 +7,6 @@ pub const METADATA_KEY: MetadataKey = MetadataKey::new("trigger"); #[derive(Clone, Debug, Default, Deserialize, Serialize)] #[serde(deny_unknown_fields)] pub struct Metadata { - // The type of trigger which should always been "http" in this case - pub r#type: String, // The based url #[serde(default = "default_base")] pub base: String, diff --git a/crates/redis/src/lib.rs b/crates/redis/src/lib.rs index 3fb789098..10d032b90 100644 --- a/crates/redis/src/lib.rs +++ b/crates/redis/src/lib.rs @@ -8,14 +8,11 @@ use anyhow::{anyhow, Context, Result}; use futures::{future::join_all, StreamExt}; use redis::{Client, ConnectionLike}; use serde::{de::IgnoredAny, Deserialize, Serialize}; -use spin_app::MetadataKey; use spin_core::async_trait; use spin_trigger::{cli::NoArgs, TriggerAppEngine, TriggerExecutor}; use crate::spin::SpinRedisExecutor; -const TRIGGER_METADATA_KEY: MetadataKey = MetadataKey::new("trigger"); - pub(crate) type RuntimeData = (); pub(crate) type Store = spin_core::Store; @@ -44,7 +41,6 @@ pub struct RedisTriggerConfig { #[derive(Clone, Debug, Default, Deserialize, Serialize)] #[serde(deny_unknown_fields)] struct TriggerMetadata { - r#type: String, address: String, } @@ -56,7 +52,10 @@ impl TriggerExecutor for RedisTrigger { type RunConfig = NoArgs; async fn new(engine: TriggerAppEngine) -> Result { - let address = engine.app().require_metadata(TRIGGER_METADATA_KEY)?.address; + let address = engine + .trigger_metadata::()? + .unwrap_or_default() + .address; let mut channel_components: HashMap> = HashMap::new(); diff --git a/crates/trigger-http/src/lib.rs b/crates/trigger-http/src/lib.rs index ddabf57af..a414d6a76 100644 --- a/crates/trigger-http/src/lib.rs +++ b/crates/trigger-http/src/lib.rs @@ -95,9 +95,10 @@ impl TriggerExecutor for HttpTrigger { async fn new(engine: TriggerAppEngine) -> Result { let mut base = engine - .app() - .require_metadata(spin_http::trigger::METADATA_KEY)? + .trigger_metadata::()? + .unwrap_or_default() .base; + if !base.starts_with('/') { base = format!("/{base}"); } diff --git a/crates/trigger/src/lib.rs b/crates/trigger/src/lib.rs index 83897e16b..058a13294 100644 --- a/crates/trigger/src/lib.rs +++ b/crates/trigger/src/lib.rs @@ -287,6 +287,10 @@ impl TriggerAppEngine { self.app.borrowed() } + pub fn trigger_metadata(&self) -> spin_app::Result> { + self.app().get_trigger_metadata(Executor::TRIGGER_TYPE) + } + /// Returns AppTriggers and typed TriggerConfigs for this executor type. pub fn trigger_configs(&self) -> impl Iterator { self.app() diff --git a/examples/spin-timer/src/lib.rs b/examples/spin-timer/src/lib.rs index 729a7c3df..b7750d0ab 100644 --- a/examples/spin-timer/src/lib.rs +++ b/examples/spin-timer/src/lib.rs @@ -29,11 +29,16 @@ pub struct TimerTrigger { component_timings: HashMap, } -// Application settings (raw serialization format) +// Picks out the timer entry from the application-level trigger settings +#[derive(Clone, Debug, Default, Deserialize, Serialize)] +struct TriggerMetadataParent { + timer: Option, +} + +// Application-level settings (raw serialization format) #[derive(Clone, Debug, Default, Deserialize, Serialize)] #[serde(deny_unknown_fields)] struct TriggerMetadata { - r#type: String, speedup: Option, } @@ -45,7 +50,7 @@ pub struct TimerTriggerConfig { interval_secs: u64, } -const TRIGGER_METADATA_KEY: MetadataKey = MetadataKey::new("trigger"); +const TRIGGER_METADATA_KEY: MetadataKey = MetadataKey::new("triggers"); #[async_trait] impl TriggerExecutor for TimerTrigger { @@ -61,6 +66,8 @@ impl TriggerExecutor for TimerTrigger { let speedup = engine .app() .require_metadata(TRIGGER_METADATA_KEY)? + .timer + .unwrap_or_default() .speedup .unwrap_or(1); diff --git a/examples/spin-timer/trigger-timer.json b/examples/spin-timer/trigger-timer.json index b55d43191..78f718657 100644 --- a/examples/spin-timer/trigger-timer.json +++ b/examples/spin-timer/trigger-timer.json @@ -3,7 +3,7 @@ "description": "Run Spin components at timed intervals", "homepage": "https://github.com/fermyon/spin/tree/main/examples/spin-timer", "version": "0.1.0", - "spinCompatibility": ">=2.0", + "spinCompatibility": ">=2.2", "license": "Apache-2.0", "packages": [ { diff --git a/src/commands/up.rs b/src/commands/up.rs index 046ac06b2..2cbd81814 100644 --- a/src/commands/up.rs +++ b/src/commands/up.rs @@ -8,6 +8,7 @@ use std::{ use anyhow::{anyhow, bail, Context, Result}; use clap::{CommandFactory, Parser}; +use itertools::Itertools; use reqwest::Url; use spin_app::locked::LockedApp; use spin_common::ui::quoted_path; @@ -16,6 +17,8 @@ use spin_oci::OciLoader; use spin_trigger::cli::{SPIN_LOCAL_APP_DIR, SPIN_LOCKED_URL, SPIN_WORKING_DIR}; use tempfile::TempDir; +use futures::StreamExt; + use crate::opts::*; use self::app_source::{AppSource, ResolvedAppSource}; @@ -128,9 +131,11 @@ impl UpCommand { if app_source == AppSource::None { if self.help { - return self - .run_trigger(trigger_command(HELP_ARGS_ONLY_TRIGGER_TYPE), None) - .await; + let mut child = self + .start_trigger(trigger_command(HELP_ARGS_ONLY_TRIGGER_TYPE), None) + .await?; + let _ = child.wait().await?; + return Ok(()); } else { bail!("Default file '{DEFAULT_MANIFEST_FILE}' not found. Run `spin up --from `, or `spin up --help` for usage."); } @@ -150,11 +155,15 @@ impl UpCommand { let resolved_app_source = self.resolve_app_source(&app_source, &working_dir).await?; - let trigger_cmd = trigger_command_for_resolved_app_source(&resolved_app_source) + let trigger_cmds = trigger_command_for_resolved_app_source(&resolved_app_source) .with_context(|| format!("Couldn't find trigger executor for {app_source}"))?; if self.help { - return self.run_trigger(trigger_cmd, None).await; + for cmd in trigger_cmds { + let mut help_process = self.start_trigger(cmd.clone(), None).await?; + _ = help_process.wait().await; + } + return Ok(()); } let mut locked_app = self @@ -162,16 +171,35 @@ impl UpCommand { .await?; self.update_locked_app(&mut locked_app); + let locked_url = self.write_locked_app(&locked_app, &working_dir).await?; let local_app_dir = app_source.local_app_dir().map(Into::into); let run_opts = RunTriggerOpts { - locked_app, + locked_url, working_dir, local_app_dir, }; - self.run_trigger(trigger_cmd, Some(run_opts)).await + let mut trigger_processes = self.start_trigger_processes(trigger_cmds, run_opts).await?; + + set_kill_on_ctrl_c(&trigger_processes)?; + + let mut trigger_tasks = trigger_processes + .iter_mut() + .map(|ch| ch.wait()) + .collect::>(); + + let first_to_finish = trigger_tasks.next().await; + + if let Some(process_result) = first_to_finish { + let status = process_result?; + if !status.success() { + return Err(crate::subprocess::ExitStatusError::new(status).into()); + } + } + + Ok(()) } fn get_canonical_working_dir(&self) -> Result { @@ -190,24 +218,40 @@ impl UpCommand { Ok(working_dir_holder) } - async fn run_trigger( + async fn start_trigger_processes( self, + trigger_cmds: Vec>, + run_opts: RunTriggerOpts, + ) -> anyhow::Result> { + let mut trigger_processes = Vec::with_capacity(trigger_cmds.len()); + + for cmd in trigger_cmds { + let child = self + .start_trigger(cmd.clone(), Some(run_opts.clone())) + .await + .context("Failed to start trigger process")?; + trigger_processes.push(child); + } + + Ok(trigger_processes) + } + + async fn start_trigger( + &self, trigger_cmd: Vec, opts: Option, - ) -> Result<(), anyhow::Error> { + ) -> Result { // The docs for `current_exe` warn that this may be insecure because it could be executed // via hard-link. I think it should be fine as long as we aren't `setuid`ing this binary. - let mut cmd = std::process::Command::new(std::env::current_exe().unwrap()); + let mut cmd = tokio::process::Command::new(std::env::current_exe().unwrap()); cmd.args(&trigger_cmd); if let Some(RunTriggerOpts { - locked_app, + locked_url, working_dir, local_app_dir, }) = opts { - let locked_url = self.write_locked_app(&locked_app, &working_dir).await?; - cmd.env(SPIN_LOCKED_URL, locked_url) .env(SPIN_WORKING_DIR, &working_dir) .args(&self.trigger_args); @@ -215,32 +259,16 @@ impl UpCommand { if let Some(local_app_dir) = local_app_dir { cmd.env(SPIN_LOCAL_APP_DIR, local_app_dir); } + + cmd.kill_on_drop(true); } else { cmd.arg("--help-args-only"); } tracing::trace!("Running trigger executor: {:?}", cmd); - let mut child = cmd.spawn().context("Failed to execute trigger")?; - - // Terminate trigger executor if `spin up` itself receives a termination signal - #[cfg(not(windows))] - { - // https://github.com/nix-rust/nix/issues/656 - let pid = nix::unistd::Pid::from_raw(child.id() as i32); - ctrlc::set_handler(move || { - if let Err(err) = nix::sys::signal::kill(pid, nix::sys::signal::SIGTERM) { - tracing::warn!("Failed to kill trigger handler process: {:?}", err) - } - })?; - } - - let status = child.wait()?; - if status.success() { - Ok(()) - } else { - Err(crate::subprocess::ExitStatusError::new(status).into()) - } + let child = cmd.spawn().context("Failed to execute trigger")?; + Ok(child) } fn app_source(&self) -> AppSource { @@ -358,8 +386,31 @@ impl UpCommand { } } +#[cfg(windows)] +fn set_kill_on_ctrl_c(trigger_processes: &Vec) -> Result<(), anyhow::Error> { + Ok(()) +} + +#[cfg(not(windows))] +fn set_kill_on_ctrl_c(trigger_processes: &[tokio::process::Child]) -> Result<(), anyhow::Error> { + // https://github.com/nix-rust/nix/issues/656 + let pids = trigger_processes + .iter() + .flat_map(|child| child.id().map(|id| nix::unistd::Pid::from_raw(id as i32))) + .collect_vec(); + ctrlc::set_handler(move || { + for pid in &pids { + if let Err(err) = nix::sys::signal::kill(*pid, nix::sys::signal::SIGTERM) { + tracing::warn!("Failed to kill trigger handler process: {:?}", err) + } + } + })?; + Ok(()) +} + +#[derive(Clone)] struct RunTriggerOpts { - locked_app: LockedApp, + locked_url: String, working_dir: PathBuf, local_app_dir: Option, } @@ -424,16 +475,20 @@ fn trigger_command(trigger_type: &str) -> Vec { vec!["trigger".to_owned(), trigger_type.to_owned()] } -fn trigger_command_for_resolved_app_source(resolved: &ResolvedAppSource) -> Result> { - let trigger_type = resolved.trigger_type()?; - - match trigger_type { - "http" | "redis" => Ok(trigger_command(trigger_type)), - _ => { - let cmd = resolve_trigger_plugin(trigger_type)?; - Ok(vec![cmd]) - } - } +fn trigger_command_for_resolved_app_source( + resolved: &ResolvedAppSource, +) -> Result>> { + let trigger_type = resolved.trigger_types()?; + trigger_type + .iter() + .map(|&t| match t { + "http" | "redis" => Ok(trigger_command(t)), + _ => { + let cmd = resolve_trigger_plugin(t)?; + Ok(vec![cmd]) + } + }) + .collect() } #[cfg(test)] diff --git a/src/commands/up/app_source.rs b/src/commands/up/app_source.rs index 3a6b7ae23..88431ce76 100644 --- a/src/commands/up/app_source.rs +++ b/src/commands/up/app_source.rs @@ -83,7 +83,7 @@ pub enum ResolvedAppSource { } impl ResolvedAppSource { - pub fn trigger_type(&self) -> anyhow::Result<&str> { + pub fn trigger_types(&self) -> anyhow::Result> { let types = match self { ResolvedAppSource::File { manifest, .. } => { manifest.triggers.keys().collect::>() @@ -96,7 +96,6 @@ impl ResolvedAppSource { }; ensure!(!types.is_empty(), "no triggers in app"); - ensure!(types.len() == 1, "multiple trigger types not yet supported"); - Ok(types.into_iter().next().unwrap()) + Ok(types.into_iter().map(|t| t.as_str()).collect()) } }