From a70825d35fec245d0b9c92f4a31754180e50f827 Mon Sep 17 00:00:00 2001 From: Javier Viola Date: Thu, 15 Aug 2024 09:15:10 -0300 Subject: [PATCH] add check for binaries in native --- .../orchestrator/src/generators/chain_spec.rs | 1 - crates/orchestrator/src/lib.rs | 151 ++++++++++++++---- crates/orchestrator/src/network_spec.rs | 27 ++-- .../src/network_spec/parachain.rs | 9 +- crates/provider/src/docker/node.rs | 2 +- crates/provider/src/kubernetes/node.rs | 2 +- crates/provider/src/lib.rs | 2 +- crates/provider/src/native/node.rs | 2 +- 8 files changed, 151 insertions(+), 45 deletions(-) diff --git a/crates/orchestrator/src/generators/chain_spec.rs b/crates/orchestrator/src/generators/chain_spec.rs index e93c0161a..ccfef8b84 100644 --- a/crates/orchestrator/src/generators/chain_spec.rs +++ b/crates/orchestrator/src/generators/chain_spec.rs @@ -175,7 +175,6 @@ impl ChainSpec { let sanitized_cmd = if replacement_value.is_empty() { // we need to remove the `--chain` flag self.command.as_ref().unwrap().cmd().replace("--chain", "") - //.as_ref().unwrap().replace("--chain", "") } else { self.command.as_ref().unwrap().cmd().to_owned() }; diff --git a/crates/orchestrator/src/lib.rs b/crates/orchestrator/src/lib.rs index b0a782315..f72e4a9f5 100644 --- a/crates/orchestrator/src/lib.rs +++ b/crates/orchestrator/src/lib.rs @@ -12,6 +12,7 @@ pub mod shared; mod spawner; use std::{ + collections::HashSet, net::IpAddr, path::{Path, PathBuf}, time::Duration, @@ -411,36 +412,96 @@ fn validate_spec_with_provider_capabilities( network_spec: &NetworkSpec, capabilities: &ProviderCapabilities, ) -> Result<(), anyhow::Error> { - if !capabilities.requires_image { - return Ok(()); - } - - // Relaychain - if network_spec.relaychain.default_image.is_none() { - // we should check if each node have an image - let nodes = &network_spec.relaychain.nodes; - if nodes.iter().any(|node| node.image.is_none()) { - return Err(anyhow::anyhow!( - "missing image for node, and not default is set at relaychain" - )); - } - }; + let mut errs: Vec = vec![]; - // Paras - for para in &network_spec.parachains { - if para.default_image.is_none() { - let nodes = ¶.collators; + if capabilities.requires_image { + // Relaychain + if network_spec.relaychain.default_image.is_none() { + // we should check if each node have an image + let nodes = &network_spec.relaychain.nodes; if nodes.iter().any(|node| node.image.is_none()) { - return Err(anyhow::anyhow!( - "missing image for node, and not default is set at parachain {}", - para.id + errs.push(String::from( + "Missing image for node, and not default is set at relaychain", )); } + }; + + // Paras + for para in &network_spec.parachains { + if para.default_image.is_none() { + let nodes = ¶.collators; + if nodes.iter().any(|node| node.image.is_none()) { + errs.push(format!( + "Missing image for node, and not default is set at parachain {}", + para.id + )); + } + } + } + } else { + // native + // We need to get all the `cmds` and verify if are part of the path + let mut cmds: HashSet<&str> = Default::default(); + if let Some(cmd) = network_spec.relaychain.default_command.as_ref() { + cmds.insert(cmd.as_str()); + } + for node in network_spec.relaychain().nodes.iter() { + cmds.insert(node.command()); + } + + // Paras + for para in &network_spec.parachains { + if let Some(cmd) = para.default_command.as_ref() { + cmds.insert(cmd.as_str()); + } + + for node in para.collators.iter() { + cmds.insert(node.command()); + } } + + // now check the binaries + let path = std::env::var("PATH").unwrap_or_default(); // path should always be set + println!("PATH es: {path}"); + let parts: Vec<_> = path.split(":").collect(); + for cmd in cmds { + let missing = if cmd.contains('/') { + std::fs::metadata(cmd).is_err() + } else { + // should be in the PATH + !parts + .iter() + .any(|part| std::fs::metadata(format!("{}/{}", part, cmd)).is_ok()) + }; + + if missing { + errs.push(help_msg(cmd)); + } + } + } + + if !errs.is_empty() { + let msg = errs.join("\n"); + return Err(anyhow::anyhow!(format!("Invalid configuration: \n {msg}"))); } Ok(()) } + +fn help_msg(cmd: &str) -> String { + match cmd { + "parachain-template-node" | "solochain-template-node" | "minimal-template-node" => { + format!("Missing binary {cmd}, compile by running: \n\tcargo build --package {cmd} --release") + }, + "polkadot" => { + format!("Missing binary {cmd}, compile by running: \n\t cargo build --locked --release --features fast-runtime --bin {cmd} --bin polkadot-prepare-worker --bin polkadot-execute-worker") + }, + _ => { + format!("Missing binary {cmd}, please compile it.") + }, + } +} + // TODO: get the fs from `DynNamespace` will make this not needed // but the FileSystem trait isn't object-safe so we can't pass around // as `dyn FileSystem`. We can refactor or using some `erase` techniques @@ -543,12 +604,15 @@ mod tests { use super::*; - fn generate(with_image: bool) -> Result> { + fn generate( + with_image: bool, + with_cmd: Option<&'static str>, + ) -> Result> { NetworkConfigBuilder::new() .with_relaychain(|r| { let mut relay = r .with_chain("rococo-local") - .with_default_command("polkadot"); + .with_default_command(with_cmd.unwrap_or("polkadot")); if with_image { relay = relay.with_default_image("docker.io/parity/polkadot") } @@ -559,7 +623,9 @@ mod tests { }) .with_parachain(|p| { p.with_id(2000).cumulus_based(true).with_collator(|n| { - let node = n.with_name("collator").with_command("polkadot-parachain"); + let node = n + .with_name("collator") + .with_command(with_cmd.unwrap_or("polkadot-parachain")); if with_image { node.with_image("docker.io/paritypr/test-parachain") } else { @@ -572,7 +638,7 @@ mod tests { #[tokio::test] async fn valid_config_with_image() { - let network_config = generate(true).unwrap(); + let network_config = generate(true, None).unwrap(); let spec = NetworkSpec::from_config(&network_config).await.unwrap(); let caps = ProviderCapabilities { requires_image: true, @@ -586,8 +652,8 @@ mod tests { } #[tokio::test] - async fn invalid_config() { - let network_config = generate(false).unwrap(); + async fn invalid_config_without_image() { + let network_config = generate(false, None).unwrap(); let spec = NetworkSpec::from_config(&network_config).await.unwrap(); let caps = ProviderCapabilities { requires_image: true, @@ -599,4 +665,35 @@ mod tests { let valid = validate_spec_with_provider_capabilities(&spec, &caps); assert!(valid.is_err()) } + + #[tokio::test] + async fn invalid_config_missing_cmd() { + let network_config = generate(false, Some("other")).unwrap(); + let spec = NetworkSpec::from_config(&network_config).await.unwrap(); + let caps = ProviderCapabilities { + requires_image: false, + has_resources: false, + prefix_with_full_path: false, + use_default_ports_in_cmd: false, + }; + + let valid = validate_spec_with_provider_capabilities(&spec, &caps); + assert!(valid.is_err()) + } + + #[tokio::test] + async fn valid_config_present_cmd() { + let network_config = generate(false, Some("cargo")).unwrap(); + let spec = NetworkSpec::from_config(&network_config).await.unwrap(); + let caps = ProviderCapabilities { + requires_image: false, + has_resources: false, + prefix_with_full_path: false, + use_default_ports_in_cmd: false, + }; + + let valid = validate_spec_with_provider_capabilities(&spec, &caps); + println!("{:?}", valid); + assert!(valid.is_ok()) + } } diff --git a/crates/orchestrator/src/network_spec.rs b/crates/orchestrator/src/network_spec.rs index 3c8285cf7..1bbeecbb3 100644 --- a/crates/orchestrator/src/network_spec.rs +++ b/crates/orchestrator/src/network_spec.rs @@ -49,16 +49,25 @@ impl NetworkSpec { } } - Ok(NetworkSpec { - relaychain, - parachains, - hrmp_channels: network_config - .hrmp_channels() + if errs.is_empty() { + Ok(NetworkSpec { + relaychain, + parachains, + hrmp_channels: network_config + .hrmp_channels() + .into_iter() + .cloned() + .collect(), + global_settings: network_config.global_settings().clone(), + }) + } else { + let errs_str = errs .into_iter() - .cloned() - .collect(), - global_settings: network_config.global_settings().clone(), - }) + .map(|e| e.to_string()) + .collect::>() + .join("\n"); + Err(OrchestratorError::InvalidConfig(errs_str)) + } } pub async fn populate_nodes_available_args( diff --git a/crates/orchestrator/src/network_spec/parachain.rs b/crates/orchestrator/src/network_spec/parachain.rs index 3b38d25ac..4bea46b7f 100644 --- a/crates/orchestrator/src/network_spec/parachain.rs +++ b/crates/orchestrator/src/network_spec/parachain.rs @@ -77,14 +77,15 @@ impl ParachainSpec { cmd } else if let Some(first_node) = config.collators().first() { let Some(cmd) = first_node.command() else { - return Err(OrchestratorError::InvalidConfig("Parachain, either default_command or command in the first node needs to be set.".to_string())); + return Err(OrchestratorError::InvalidConfig(format!("Parachain {}, either default_command or command in the first node needs to be set.", config.id()))); }; cmd } else { - return Err(OrchestratorError::InvalidConfig( - "Parachain without nodes and default_command isn't set.".to_string(), - )); + return Err(OrchestratorError::InvalidConfig(format!( + "Parachain {}, without nodes and default_command isn't set.", + config.id() + ))); }; // TODO: internally we use image as String diff --git a/crates/provider/src/docker/node.rs b/crates/provider/src/docker/node.rs index 4828b6e26..50c1a69b7 100644 --- a/crates/provider/src/docker/node.rs +++ b/crates/provider/src/docker/node.rs @@ -392,7 +392,7 @@ where .map_err(|err| { ProviderError::RunCommandError( format!("sh -c {}", &command.join(" ")), - self.name.to_string(), + format!("in pod {}", self.name), err.into(), ) }) diff --git a/crates/provider/src/kubernetes/node.rs b/crates/provider/src/kubernetes/node.rs index 0fa419c66..709e2bf53 100644 --- a/crates/provider/src/kubernetes/node.rs +++ b/crates/provider/src/kubernetes/node.rs @@ -512,7 +512,7 @@ where .map_err(|err| { ProviderError::RunCommandError( format!("sh -c {}", &command.join(" ")), - self.name.to_string(), + format!("in pod {}", self.name), err.into(), ) }) diff --git a/crates/provider/src/lib.rs b/crates/provider/src/lib.rs index 2e87cfb19..d52ec7ef4 100644 --- a/crates/provider/src/lib.rs +++ b/crates/provider/src/lib.rs @@ -34,7 +34,7 @@ pub enum ProviderError { #[error("Failed to spawn node '{0}': {1}")] NodeSpawningFailed(String, anyhow::Error), - #[error("Error running command '{0}' in pod {1}: {2}")] + #[error("Error running command '{0}' {1}: {2}")] RunCommandError(String, String, anyhow::Error), #[error("Error running script'{0}': {1}")] diff --git a/crates/provider/src/native/node.rs b/crates/provider/src/native/node.rs index 7faee9550..f4dc44d24 100644 --- a/crates/provider/src/native/node.rs +++ b/crates/provider/src/native/node.rs @@ -442,7 +442,7 @@ where .map_err(|err| { ProviderError::RunCommandError( format!("{} {}", &options.program, &options.args.join(" ")), - options.program, + "locally".to_string(), err.into(), ) })?;