Skip to content

Commit

Permalink
feat(server): ensure deployed apps find shared components
Browse files Browse the repository at this point in the history 
Signed-off-by: Brooks Townsend <[email protected]>
  • Loading branch information
@brooksmtownsend
brooksmtownsend committed Oct 11, 2024
1 parent df30a4e commit ee3b618
Show file tree
Hide file tree
Showing 6 changed files with 159 additions and 106 deletions.
46 changes: 46 additions & 0 deletions crates/wadm-types/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,52 @@ impl Manifest {
self.spec.components.iter()
}

/// Helper function to find shared components that are missing from the given list of
/// deployed applications
pub fn missing_shared_components(&self, deployed_apps: &Vec<&Manifest>) -> Vec<&Component> {
self.spec
.components
.iter()
.filter_map(|shared_component| {
match &shared_component.properties {
Properties::Capability {
properties:
CapabilityProperties {
image: None,
application: Some(shared_app),
..
},
}
| Properties::Component {
properties:
ComponentProperties {
image: None,
application: Some(shared_app),
..
},
} => {
if deployed_apps.iter().filter(|a| a.shared()).any(|m| {
m.metadata.name == shared_app.name
&& m.components().any(|c| {
c.name == shared_app.component
// This compares just the enum variant, not the actual properties
// For example, if we reference a shared component that's a capability,
// we want to make sure the deployed component is a capability.
&& std::mem::discriminant(&c.properties)
== std::mem::discriminant(&shared_component.properties)
})
}) {
None
} else {
Some(shared_component)
}
}
_ => None,
}
})
.collect()
}

/// Returns only the WebAssembly components in the manifest
pub fn wasm_components(&self) -> impl Iterator<Item = &Component> {
self.components()
Expand Down
11 changes: 11 additions & 0 deletions crates/wadm-types/src/validation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ pub fn validate_component_properties(application: &Manifest) -> Vec<ValidationFa
image,
application,
config,
secrets,
..
},
}
Expand All @@ -619,6 +620,7 @@ pub fn validate_component_properties(application: &Manifest) -> Vec<ValidationFa
image,
application,
config,
secrets,
..
},
} => match (image, application) {
Expand All @@ -645,6 +647,15 @@ pub fn validate_component_properties(application: &Manifest) -> Vec<ValidationFa
),
));
}
(None, Some(shared_properties)) if !secrets.is_empty() => {
failures.push(ValidationFailure::new(
ValidationFailureLevel::Error,
format!(
"Shared component '{}' cannot specify additional 'secrets'",
shared_properties.name
),
));
}
// Shared application components already have scale properties defined in their original manifest
(None, Some(shared_properties))
if component
Expand Down
2 changes: 1 addition & 1 deletion crates/wadm/src/scaler/convert.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ fn component_scalers<S, P, L>(
);

config_names.append(&mut secret_names.clone());
// TODO: Consider a way to report on status of a shared component
// TODO(#451): Consider a way to report on status of a shared component
match (trt.trait_type.as_str(), &trt.properties, &properties.image) {
// Shared application components already have their own spread/daemon scalers, you
// cannot modify them from another manifest
Expand Down
129 changes: 63 additions & 66 deletions crates/wadm/src/server/handlers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ impl<P: Publisher> Handler<P> {
self.send_error(
msg.reply,
format!(
"Manifest name {} contains invalid characters. Manifest names can only contain alphanumeric characters, dashes, and underscores.",
manifest_name
"Manifest name {manifest_name} contains invalid characters. Manifest names can only contain alphanumeric characters, dashes, and underscores.",
),
)
.await;
Expand All @@ -89,66 +88,46 @@ impl<P: Publisher> Handler<P> {
}
};

if let Some(error_message) = validate_manifest(manifest.clone()).await.err() {
if let Some(error_message) = validate_manifest(&manifest).await.err() {
self.send_error(msg.reply, error_message.to_string()).await;
return;
}

let all_manifests: Vec<Manifest> = self
let all_stored_manifests = self
.store
.list(account_id, lattice_id)
.await
.unwrap_or_default()
.into_iter()
.filter(|manifest| manifest.get_current().shared())
// TODO: Do we need to just get deployed here? I assume that would be a warning if not deployed
.map(|manifest| manifest.get_current().clone())
.unwrap_or_default();
let deployed_shared_apps: Vec<&Manifest> = all_stored_manifests
.iter()
// Only keep deployed, shared applications
.filter(|manifest| {
manifest.deployed_version().is_some() && manifest.get_current().shared()
})
.map(|manifest| manifest.get_current())
.collect();

// TODO: We are also going to have to ensure that links don't clobber other links. Specifically
// this relates to the uniqueness properties of a source/target/interface/etc for a link.

// Ensure that all shared components are present in other (deployed?) manifests that are
// marked as shared applications
let mut shared_components = manifest
.components()
// Find all shared components
.filter_map(|c| match &c.properties {
Properties::Capability {
properties:
CapabilityProperties {
image: None,
application: Some(manifest_properties),
..
},
} => Some(manifest_properties),
Properties::Component {
properties:
ComponentProperties {
image: None,
application: Some(manifest_properties),
..
},
} => Some(manifest_properties),
_ => None,
});

// Ensure all shared components point to a component that is deployed in another manifest
// TODO: would be better to find matching bad shared components
if !shared_components.all(|properties| {
// TODO: ensure that the type of the property is preserved, e.g. component -> component
// TODO: ensure that the manifest is deployed
all_manifests.iter().any(|m| {
m.metadata.name == properties.name
&& m.components().any(|c| c.name == properties.component)
})
}) {
self.send_error(msg.reply, "shared component not found".to_string())
.await;
return;
}
// Explicitly drop to let the compiler know we're done with the manifest borrow
drop(shared_components);
// NOTE(brooksmtownsend): You can put an application with missing shared components, because
// the time where you truly need them is when you deploy the application. This can cause a bit
// of friction when it comes to deploy, but it avoids the frustrating race condition where you
// - Put the application looking for a deployed shared component
// - Undeploy the application with the shared component
// - Deploy the new application looking for the shared component (error)
let missing_shared_components = manifest.missing_shared_components(&deployed_shared_apps);
let message = if missing_shared_components.is_empty() {
format!(
"Successfully put manifest {} {}",
manifest_name,
current_manifests.current_version().to_owned()
)
} else {
format!(
"Successfully put manifest {} {}, but some shared components are not deployed: {:?}",
manifest_name,
current_manifests.current_version().to_owned(),
missing_shared_components
)
};

let incoming_version = manifest.version().to_owned();
if !current_manifests.add_version(manifest) {
Expand All @@ -170,11 +149,7 @@ impl<P: Publisher> Handler<P> {
},
name: manifest_name.clone(),
total_versions: current_manifests.count(),
message: format!(
"Successfully put manifest {} {}",
manifest_name,
current_manifests.current_version()
),
message,
};

trace!(total_manifests = %resp.total_versions, "Storing manifests");
Expand Down Expand Up @@ -437,6 +412,7 @@ impl<P: Publisher> Handler<P> {
}
}
};
// TODO(#451): if shared and deployed, make sure that no other shared apps are using it
let reply_data = if let Some(version) = req.version {
match self.store.get(account_id, lattice_id, name).await {
Ok(Some((mut current, current_revision))) => {
Expand Down Expand Up @@ -729,6 +705,26 @@ impl<P: Publisher> Handler<P> {
}
}

// TODO(#451): If this app is shared, or the previous version was, make sure that shared
// components that have dependent applications are still present

let missing_shared_components = staged_model.missing_shared_components(
&stored_models
.iter()
.filter(|a| a.deployed_version().is_some() && a.get_current().shared())
.map(|a| a.get_current())
.collect(),
);

// Ensure all shared components point to a valid component that is deployed in another application
if !missing_shared_components.is_empty() {
self.send_error(
msg.reply,
format!("Application contains shared components that are not deployed in other applications: {:?}", missing_shared_components.iter().map(|c| &c.name).collect::<Vec<_>>())
).await;
return;
}

if !manifests.deploy(req.version.clone()) {
trace!("Requested version does not exist");
self.send_reply(
Expand Down Expand Up @@ -857,6 +853,7 @@ impl<P: Publisher> Handler<P> {
return;
}
};
// TODO(#451): if shared, make sure that no other shared apps are using it

let reply = if manifests.undeploy() {
trace!("Manifest undeployed. Storing updated manifest");
Expand Down Expand Up @@ -1019,7 +1016,7 @@ fn summary_from_manifest_status(manifest: StoredManifest, status: Status) -> Mod
}

// Manifest validation
pub(crate) async fn validate_manifest(manifest: Manifest) -> anyhow::Result<()> {
pub(crate) async fn validate_manifest(manifest: &Manifest) -> anyhow::Result<()> {
let failures = wadm_types::validation::validate_manifest(&manifest).await?;
for failure in failures {
if matches!(
Expand Down Expand Up @@ -1055,12 +1052,12 @@ mod test {
let correct_manifest = deserialize_yaml("../../tests/fixtures/manifests/simple.yaml")
.expect("Should be able to parse");

assert!(validate_manifest(correct_manifest).await.is_ok());
assert!(validate_manifest(&correct_manifest).await.is_ok());

let manifest = deserialize_yaml("../../tests/fixtures/manifests/incorrect_component.yaml")
.expect("Should be able to parse");

match validate_manifest(manifest).await {
match validate_manifest(&manifest).await {
Ok(()) => panic!("Should have detected incorrect component"),
Err(e) => {
assert!(e
Expand All @@ -1072,7 +1069,7 @@ mod test {
let manifest = deserialize_yaml("../../tests/fixtures/manifests/duplicate_component.yaml")
.expect("Should be able to parse");

match validate_manifest(manifest).await {
match validate_manifest(&manifest).await {
Ok(()) => panic!("Should have detected duplicate component"),
Err(e) => assert!(e
.to_string()
Expand All @@ -1082,7 +1079,7 @@ mod test {
let manifest = deserialize_yaml("../../tests/fixtures/manifests/duplicate_id1.yaml")
.expect("Should be able to parse");

match validate_manifest(manifest).await {
match validate_manifest(&manifest).await {
Ok(()) => {
panic!("Should have detected duplicate component ID in provider properties")
}
Expand All @@ -1094,7 +1091,7 @@ mod test {
let manifest = deserialize_yaml("../../tests/fixtures/manifests/duplicate_id2.yaml")
.expect("Should be able to parse");

match validate_manifest(manifest).await {
match validate_manifest(&manifest).await {
Ok(()) => panic!("Should have detected duplicate component ID in component properties"),
Err(e) => assert!(e
.to_string()
Expand All @@ -1105,7 +1102,7 @@ mod test {
deserialize_yaml("../../tests/fixtures/manifests/missing_capability_component.yaml")
.expect("Should be able to parse");

match validate_manifest(manifest).await {
match validate_manifest(&manifest).await {
Ok(()) => panic!("Should have detected missing capability component"),
Err(e) => assert!(e
.to_string()
Expand All @@ -1118,7 +1115,7 @@ mod test {
#[tokio::test]
async fn manifest_name_long_image_ref() -> Result<()> {
validate_manifest(
deserialize_yaml("../../tests/fixtures/manifests/long_image_refs.yaml")
&deserialize_yaml("../../tests/fixtures/manifests/long_image_refs.yaml")
.context("failed to deserialize YAML")?,
)
.await
Expand Down
1 change: 0 additions & 1 deletion tests/e2e.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ use wadm_types::{
use wasmcloud_control_interface::HostInventory;

const LOG_DIR: &str = "tests/e2e_log";
pub const DEFAULT_LATTICE_ID: &str = "default";
// Due to download times and the time needed to stabilize, we still need to wait for just a little bit
// This number comes from 30s (max backoff time) + 5s of wiggle room
pub const DEFAULT_WAIT_TIME: Duration = Duration::from_secs(35);
Expand Down
Loading

0 comments on commit ee3b618

Please sign in to comment.