Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to latest Spin version with Factors #189

Merged
merged 10 commits into from
Oct 9, 2024
Merged

Update to latest Spin version with Factors #189

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6b93351
Move to using latest version of Spin (factors).
rylev Sep 13, 2024
9be82cc
A bit of cleanup
rylev Sep 13, 2024
2601a0c
Add back Redis trigger
rylev Sep 13, 2024
9dd1caf
Add back SQS, Command, and MQTT triggers
rylev Sep 17, 2024
f22129f
PR feedback
rylev Sep 20, 2024
23c6e12
Update trigger dependencies
rylev Sep 23, 2024
ef32b84
Fix CI
rylev Sep 24, 2024
0c6943f
Fix CI issues
rylev Sep 24, 2024
9be07d5
Use HOSTNAME env as app id
rylev Oct 8, 2024
6919450
Update mqtt trigger with fix
rylev Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add back Redis trigger 
Signed-off-by: Ryan Levick <[email protected]>
  • Loading branch information
@rylev
rylev committed Oct 8, 2024

Verified

This commit was signed with the committer’s verified signature.
codebytere Shelley Vohr
GPG key ID: F13993A75599653C
Verified
Learn about vigilant mode
commit 2601a0c94fba4edaddf823a824c590758a4f53a8
3 changes: 2 additions & 1 deletion containerd-shim-spin/Cargo.toml
View file
Original file line number Diff line number Diff line change
@@ -22,7 +22,7 @@ spin-trigger = { git = "https://github.com/fermyon/spin", rev = "e7e69e5d7d719e5
"unsafe-aot-compilation",
] }
spin-trigger-http = { git = "https://github.com/fermyon/spin", rev = "e7e69e5d7d719e532bcbd6a903476c430466f422" }
# spin-trigger-redis = { git = "https://github.com/fermyon/spin", rev = "e7e69e5d7d719e532bcbd6a903476c430466f422" }
spin-trigger-redis = { git = "https://github.com/fermyon/spin", rev = "e7e69e5d7d719e532bcbd6a903476c430466f422" }
# trigger-mqtt = { git = "https://github.com/spinkube/spin-trigger-mqtt", tag = "v0.2.0" }
# trigger-sqs = { git = "https://github.com/fermyon/spin-trigger-sqs", rev = "v0.7.0" }
# trigger-command = { git = "https://github.com/fermyon/spin-trigger-command", tag = "v0.1.0" }
@@ -34,6 +34,7 @@ spin-expressions = { git = "https://github.com/fermyon/spin", rev = "e7e69e5d7d7
spin-factors-executor = { git = "https://github.com/fermyon/spin", rev = "e7e69e5d7d719e532bcbd6a903476c430466f422" }
spin-telemetry = { git = "https://github.com/fermyon/spin", rev = "e7e69e5d7d719e532bcbd6a903476c430466f422" }
spin-runtime-factors = { git = "https://github.com/fermyon/spin", rev = "e7e69e5d7d719e532bcbd6a903476c430466f422" }
spin-factors = { git = "https://github.com/fermyon/spin", rev = "e7e69e5d7d719e532bcbd6a903476c430466f422" }
wasmtime = "22.0"
tokio = { version = "1.38", features = ["rt"] }
openssl = { version = "*", features = ["vendored"] }
47 changes: 9 additions & 38 deletions containerd-shim-spin/src/engine.rs
View file
Original file line number Diff line number Diff line change
@@ -2,7 +2,6 @@ use std::{
collections::{hash_map::DefaultHasher, HashSet},
env,
hash::{Hash, Hasher},
path::Path,
};

use anyhow::{Context, Result};
@@ -14,26 +13,21 @@ use containerd_shim_wasm::{
use futures::future;
use log::info;
use spin_app::locked::LockedApp;
use spin_trigger::cli::{FactorsConfig, TriggerAppBuilder};
use spin_trigger_http::HttpTrigger;
use tokio::runtime::Runtime;
// use spin_trigger_redis::RedisTrigger;
// use trigger_command::CommandTrigger;
// use trigger_mqtt::MqttTrigger;
// use trigger_sqs::SqsTrigger;

use crate::{
constants::{self, RUNTIME_CONFIG_PATH},
constants,
source::Source,
trigger::get_supported_triggers,
trigger::{self, get_supported_triggers, HTTP_TRIGGER_TYPE, REDIS_TRIGGER_TYPE},
utils::{
configure_application_variables_from_environment_variables, initialize_cache,
is_wasm_content, parse_addr,
},
};

use super::trigger::HTTP_TRIGGER_TYPE;

#[derive(Clone)]
pub struct SpinEngine {
pub(crate) wasmtime_engine: wasmtime::Engine,
@@ -168,30 +162,21 @@ impl SpinEngine {
Source::File(_) => {}
};
for trigger_type in trigger_types.iter() {
let app = spin_app::App::new("TODO", app.clone());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is app creation avoided in the Spin CLI? By directly invoking the triggers via commandline? How is app ID used? Should we just set the ID to be the locked app name for now?

Copy link
Contributor Author

@rylev rylev Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be something we might be able to change in upstream as that app id is rarely actually used. Currently, in the Spin code base it's only being used in the Redis trigger for telemetry purposes. It's used by some other runtimes simply as a unique identifier.

I've opened an issue for this: spinframework/spin#2852

In the meantime, do we have any sort of identifier we can use to globally identified the running application?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By globally do you mean unique to the cluster too? Container name may be a good option

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The uniqueness isn't necessary (as far as I can tell) for everything to work properly - it is just needed for proper diagnostics as this string might be exposed to the user in telemetry. Therefore, I think we want something that makes sense to users if they see it.

I'm not sure what the best thing to use is - if you say container name works well that's fine by me. How do I access that information?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to use the HOSTNAME env var to get the container name. This is not a unique container id; rather just the container name (ie busybox). Furthermore, if the container is using hostNetwork, the hostname becomes the node hostname, but in production, that is rarely done. For now HOSTNAME sounds like the best bet

Copy link
Collaborator

@kate-goldenring kate-goldenring Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a quick check locally, printing the value of HOSTNAME from the shim and looks like it is the fully unique container name: "container name: simple-spinapp-59c4d9668-gz8dr". If you just run locally with ctr run it will use the node/machine hostname which is fine as that is not the normal execution path.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rylev what are your thoughts on using HOSTNAME?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a better idea. I suppose this isn't hard to change in the future and since nothing is really relying on the id being a particular format (it's just to help humans), we don't really even need to worry about backwards compatibility.

let f = match trigger_type.as_str() {
HTTP_TRIGGER_TYPE => {
info!(" >>> running spin http trigger");
let app = spin_app::App::new("TODO", app.clone());
let address_str = env::var(constants::SPIN_HTTP_LISTEN_ADDR_ENV)
.unwrap_or_else(|_| constants::SPIN_ADDR_DEFAULT.to_string());
let listen_address = parse_addr(&address_str)?;
let trigger = spin_trigger_http::HttpTrigger::new(&app, listen_address, None)?;
let builder: TriggerAppBuilder<
HttpTrigger,
spin_runtime_factors::FactorsBuilder,
> = TriggerAppBuilder::new(trigger);

let future = builder
.run(app, factors_config(), Default::default(), &loader)
.await?;
Box::pin(future)
trigger::run(trigger, app, &loader).await?
}
REDIS_TRIGGER_TYPE => {
info!(" >>> running spin redis trigger");
let trigger = spin_trigger_redis::RedisTrigger;
trigger::run(trigger, app, &loader).await?
}
// RedisTrigger::TRIGGER_TYPE => {
// let redis_trigger =
// build_trigger::<RedisTrigger>(app.clone(), app_source.clone()).await?;
// info!(" >>> running spin redis trigger");
// redis_trigger.run(spin_trigger::cli::NoArgs)
// }
// SqsTrigger::TRIGGER_TYPE => {
// let sqs_trigger =
// build_trigger::<SqsTrigger>(app.clone(), app_source.clone()).await?;
@@ -236,20 +221,6 @@ impl SpinEngine {
}
}

/// Configuration for the factors.
fn factors_config() -> FactorsConfig {
// Load in runtime config if one exists at expected location
let runtime_config_file = Path::new(RUNTIME_CONFIG_PATH)
.exists()
.then(|| RUNTIME_CONFIG_PATH.into());
let factors_config = FactorsConfig {
working_dir: constants::SPIN_TRIGGER_WORKING_DIR.into(),
runtime_config_file,
..Default::default()
};
factors_config
}

#[cfg(test)]
mod tests {
use oci_spec::image::MediaType;
41 changes: 38 additions & 3 deletions containerd-shim-spin/src/trigger.rs
View file
Original file line number Diff line number Diff line change
@@ -1,18 +1,53 @@
use std::collections::HashSet;
use std::{collections::HashSet, future::Future, path::Path, pin::Pin};

use anyhow::anyhow;
use spin_app::locked::LockedApp;
use spin_trigger::Trigger;
// use spin_trigger::{loader, RuntimeConfig, TriggerExecutor, TriggerExecutorBuilder};
use spin_trigger_http::HttpTrigger;
// use spin_trigger_redis::RedisTrigger;
use spin_trigger_redis::RedisTrigger;

use crate::constants::{RUNTIME_CONFIG_PATH, SPIN_TRIGGER_WORKING_DIR};
// use trigger_command::CommandTrigger;
// use trigger_mqtt::MqttTrigger;
// use trigger_sqs::SqsTrigger;

pub(crate) const HTTP_TRIGGER_TYPE: &str = <HttpTrigger as Trigger<
<spin_runtime_factors::FactorsBuilder as spin_trigger::cli::RuntimeFactorsBuilder>::Factors,
>>::TYPE;
pub(crate) const REDIS_TRIGGER_TYPE: &str = <RedisTrigger as Trigger<
<spin_runtime_factors::FactorsBuilder as spin_trigger::cli::RuntimeFactorsBuilder>::Factors,
>>::TYPE;

/// Run the trigger with the given `App` and `ComponentLoader`.
pub(crate) async fn run<
T: Trigger<<FactorsBuilder as RuntimeFactorsBuilder>::Factors> + 'static,
>(
trigger: T,
app: App,
loader: &spin_trigger::loader::ComponentLoader,
) -> anyhow::Result<Pin<Box<dyn Future<Output = anyhow::Result<()>>>>> {
let builder: TriggerAppBuilder<_, FactorsBuilder> = TriggerAppBuilder::new(trigger);

let future = builder
.run(app, factors_config(), Default::default(), loader)
.await?;
Ok(Box::pin(future))
}

/// Configuration for the factors.
fn factors_config() -> FactorsConfig {
// Load in runtime config if one exists at expected location
let runtime_config_file = Path::new(RUNTIME_CONFIG_PATH)
.exists()
.then(|| RUNTIME_CONFIG_PATH.into());
let factors_config = FactorsConfig {
working_dir: SPIN_TRIGGER_WORKING_DIR.into(),
runtime_config_file,
..Default::default()
};
factors_config
}

/// get the supported trigger types from the `LockedApp`.
///
@@ -29,8 +64,8 @@ pub(crate) const HTTP_TRIGGER_TYPE: &str = <HttpTrigger as Trigger<
/// Note: this function returns a `HashSet` of supported trigger types. Duplicates are removed.
pub(crate) fn get_supported_triggers(locked_app: &LockedApp) -> anyhow::Result<HashSet<String>> {
let supported_triggers: HashSet<&str> = HashSet::from([
// RedisTrigger::TRIGGER_TYPE,
HTTP_TRIGGER_TYPE,
REDIS_TRIGGER_TYPE,
// SqsTrigger::TRIGGER_TYPE,
// MqttTrigger::TRIGGER_TYPE,
// CommandTrigger::TRIGGER_TYPE,