address comments: update client trait, propagate errors up

README.md

@@ -177,6 +177,7 @@ curl -X POST \
     }' \
 http://<node-ip>:<external-port>
 ```
+Note: you can deploy any client through validator-lab or just completely separately and have the client send TXs or query this RPC through the `http://<node-ip>:<external-port>`. 
 
 ## Generic Clients
 Bring your own client and deploy it in a Validator Lab cluster!
@@ -189,18 +190,20 @@ Key points/steps:
 
 For example, let's assume we have a client sending spam. And it takes the following arguments:
 ```
-/home/solana/spammer-executable --target-node <ip:port> --thread-sleep-ms <ms-between-spam-batches> --spam-mode <client-specific-mode>
+/home/solana/spammer-executable --target-node <kubernetes_domain_name>:<port> --thread-sleep-ms <ms-between-spam-batches> --spam-mode <client-specific-mode>
 ```
+where `<kubernetes_domain_name>:<port>` is the domain name and port of the kubernetes service running the validator you want to target. See: [Node Naming Conventions](#kubernetes_domain_name)
+
 When we go to deploy the generic client, we deploy it in a similar manner to how we deploy the bench-tps client:
 ```
 cargo run --bin cluster -- -n <namespace>
 ...
-generic-client --docker-image <client-docker-image> --executable-path <path-to-executable-in-docker-image> --delay-start <seconds-after-cluster-is-deployed-before-deploying-client> --generic-client-args 'target-node=<ip:port> thread-sleep-ms=<ms-between-spam-batches> spam-mode=<client-specific-mode>' 
+generic-client --docker-image <client-docker-image> --executable-path <path-to-executable-in-docker-image> --delay-start <seconds-after-cluster-is-deployed-before-deploying-client> --generic-client-args 'target-node=<kubernetes_domain_name>:<port> thread-sleep-ms=<ms-between-spam-batches> spam-mode=<client-specific-mode>' 
 ```
 
 4) Any flag or value the client needs that is cluster specific should be read in from an environment variable. For example, say the client requires the following arguments:
 ```
-/home/solana/spammer-executable --target-node <ip:port> --shred-version <version>
+/home/solana/spammer-executable --target-node <kubernetes_domain_name>:<port> --shred-version <version>
 ```
 Shred-version is cluster specific; it is not known when you deploy a cluster. Modify the shred-version argument in the client code to read in the environment variable `SHRED_VERSION` from the host.
 Example:
@@ -233,8 +236,9 @@ SHRED_VERSION               # cluster shred version
 ```
 ^ More environment variables to come!
 
-5) Node naming conventions.
-Say you want to launch your client and send transactions to a specific validator. Kubernetes makes it easy to identify deployed nodes. Node naming conventions:
+<a name="kubernetes_domain_name"></a>
+### Node Naming Conventions in Kubernetes
+Say you want to launch your client and send transactions to a specific validator. Kubernetes makes it easy to identify deployed nodes via `<kubernetes_domain_name>:<port>`. Node naming conventions:
 ```
 <node-name>-service.<namespace>.svc.cluster.local:<port>
 ```

src/client_config.rs

@@ -16,6 +16,11 @@ pub struct BenchTpsConfig {
 }
 
 impl ClientTrait for BenchTpsConfig {
+    fn executable_path(&self) -> Result<Vec<String>, Box<dyn Error>> {
+        let command = vec!["/home/solana/k8s-cluster-scripts/client-startup-script.sh".to_string()];
+        Ok(command)
+    }
+
     fn generate_client_command_flags(&self) -> Vec<String> {
         let mut flags = vec![];
 
@@ -41,13 +46,6 @@ impl ClientTrait for BenchTpsConfig {
 
         flags
     }
-
-    fn build_command(&self) -> Result<Vec<String>, Box<dyn Error>> {
-        let mut command =
-            vec!["/home/solana/k8s-cluster-scripts/client-startup-script.sh".to_string()];
-        command.extend(self.generate_client_command_flags());
-        Ok(command)
-    }
 }
 
 #[derive(Default, Clone, PartialEq, Debug)]
@@ -61,12 +59,7 @@ pub struct GenericClientConfig {
 }
 
 impl ClientTrait for GenericClientConfig {
-    fn generate_client_command_flags(&self) -> Vec<String> {
-        self.args.clone()
-    }
-
-    /// Build command to run on pod deployment
-    fn build_command(&self) -> Result<Vec<String>, Box<dyn Error>> {
+    fn executable_path(&self) -> Result<Vec<String>, Box<dyn Error>> {
         let exec_path_string = self
             .executable_path
             .clone()
@@ -78,9 +71,10 @@ impl ClientTrait for GenericClientConfig {
                     format!("Invalid Unicode data in path: {:?}", err),
                 )
             })?;
-        let mut command = vec![exec_path_string];
-        command.extend(self.generate_client_command_flags());
-        Ok(command)
+        Ok(vec![exec_path_string])
+    }
+    fn generate_client_command_flags(&self) -> Vec<String> {
+        self.args.clone()
     }
 }
 
@@ -90,25 +84,34 @@ pub enum ClientConfig {
     BenchTps(BenchTpsConfig),
     #[strum(serialize = "generic")]
     Generic(GenericClientConfig),
+    None,
 }
 
 impl ClientConfig {
     pub fn num_clients(&self) -> usize {
         match self {
             ClientConfig::BenchTps(config) => config.num_clients,
             ClientConfig::Generic(config) => config.num_clients,
+            ClientConfig::None => 0,
         }
     }
 
     pub fn build_command(&self) -> Result<Vec<String>, Box<dyn Error>> {
         match self {
             ClientConfig::BenchTps(config) => config.build_command(),
             ClientConfig::Generic(config) => config.build_command(),
+            ClientConfig::None => Err("Client config is None".into()),
         }
     }
 }
 
 pub trait ClientTrait {
-    fn generate_client_command_flags(&self) -> Vec<String>; // Add this method
-    fn build_command(&self) -> Result<Vec<String>, Box<dyn Error>>;
+    fn executable_path(&self) -> Result<Vec<String>, Box<dyn Error>>;
+    fn generate_client_command_flags(&self) -> Vec<String>;
+    /// Build command to run on pod deployment
+    fn build_command(&self) -> Result<Vec<String>, Box<dyn Error>> {
+        let mut command = self.executable_path()?;
+        command.extend(self.generate_client_command_flags());
+        Ok(command)
+    }
 }

src/genesis.rs

@@ -253,7 +253,7 @@ impl Genesis {
         for child in children? {
             let output = child.wait_with_output()?;
             if !output.status.success() {
-                return Err(output.status.to_string().into());
+                return Err(String::from_utf8_lossy(&output.stderr).into());
             }
         }
 
@@ -301,7 +301,7 @@ impl Genesis {
         let child = Command::new(executable_path)
             .args(args)
             .stdout(Stdio::null())
-            .stderr(Stdio::null())
+            .stderr(Stdio::piped())
             .spawn()?;
 
         Ok(child)

src/lib.rs

@@ -323,7 +323,7 @@ pub fn check_directory(path: &Path, description: &str) -> Result<(), Box<dyn std
     Ok(())
 }
 
-pub fn validate_docker_image(image: String) -> Result<(), String> {
+pub fn validate_docker_image(image: &str) -> Result<(), String> {
     let parts: Vec<&str> = image.split('/').collect();
     if parts.len() != 2 || !parts[1].contains(':') {
         return Err(

src/main.rs

@@ -264,24 +264,24 @@ fn parse_matches() -> clap::ArgMatches {
                 .help("Number of rpc nodes")
         )
         // Client Config
-        .arg(
-            Arg::with_name("number_of_clients")
-                .long("num-clients")
-                .short('c')
-                .takes_value(true)
-                .default_value("1")
-                .help("Number of clients"),
-        )
-        .arg(
-            Arg::with_name("client_duration_seconds")
-                .long("client-duration-seconds")
-                .takes_value(true)
-                .default_value("7500")
-                .value_name("SECS")
-                .help("Seconds to run benchmark, then exit"),
-        )
         .subcommand(SubCommand::with_name("bench-tps")
             .about("Run the bench-tps client")
+            .arg(
+                Arg::with_name("number_of_clients")
+                    .long("num-clients")
+                    .short('c')
+                    .takes_value(true)
+                    .default_value("1")
+                    .help("Number of clients"),
+            )
+            .arg(
+                Arg::with_name("client_duration_seconds")
+                    .long("client-duration-seconds")
+                    .takes_value(true)
+                    .default_value("7500")
+                    .value_name("SECS")
+                    .help("Seconds to run benchmark, then exit"),
+            )
             .arg(
                 Arg::with_name("client_type")
                     .long("client-type")
@@ -332,12 +332,28 @@ fn parse_matches() -> clap::ArgMatches {
         )
         .subcommand(SubCommand::with_name("generic-client")
             .about("Run a generic client")
+            .arg(
+                Arg::with_name("number_of_clients")
+                    .long("num-clients")
+                    .short('c')
+                    .takes_value(true)
+                    .default_value("1")
+                    .help("Number of clients"),
+            )
+            .arg(
+                Arg::with_name("client_duration_seconds")
+                    .long("client-duration-seconds")
+                    .takes_value(true)
+                    .default_value("7500")
+                    .value_name("SECS")
+                    .help("Seconds to run benchmark, then exit"),
+            )
             .arg(
                 Arg::with_name("docker_image")
                     .long("docker-image")
                     .takes_value(true)
                     .value_name("<repository>/<client-name>:<tag>")
-                    .validator(|s| validate_docker_image(s.to_string()))
+                    .validator(validate_docker_image)
                     .required(true)
                     .help("Name of docker image to pull and run"),
             )
@@ -356,7 +372,7 @@ fn parse_matches() -> clap::ArgMatches {
                     .default_value("0")
                     .help("Wait for `delay-start` seconds after all validators are deployed to deploy client.
                     Use case: If client needs to connect to a specific node, but that node hasn't fully deployed yet
-                    the client may no be able to resolve the node's endpoint. `--delay-start` is used to wait so
+                    the client may not be able to resolve the node's endpoint. `--delay-start` is used to wait so
                     validators can deploy fully before launching the client. Currently only used for generic clients
                     since similar functionality is built into bench-tps-client"),
             )
@@ -451,12 +467,10 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
 
     let num_validators = value_t_or_exit!(matches, "number_of_validators", usize);
     let num_rpc_nodes = value_t_or_exit!(matches, "number_of_rpc_nodes", usize);
-    let num_clients = value_t_or_exit!(matches, "number_of_clients", usize);
-    let client_duration_seconds = value_t_or_exit!(matches, "client_duration_seconds", u64);
     let client_config = if let Some(matches) = matches.subcommand_matches("bench-tps") {
         let bench_tps_config = BenchTpsConfig {
-            num_clients,
-            client_duration_seconds,
+            num_clients: value_t_or_exit!(matches, "number_of_clients", usize),
+            client_duration_seconds: value_t_or_exit!(matches, "client_duration_seconds", u64),
             client_type: matches.value_of("client_type").unwrap().to_string(),
             client_to_run: matches.value_of("client_to_run").unwrap().to_string(),
             bench_tps_args: parse_and_format_transparent_args(matches.value_of("bench_tps_args")),
@@ -473,8 +487,8 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
         ClientConfig::BenchTps(bench_tps_config)
     } else if let Some(matches) = matches.subcommand_matches("generic-client") {
         let generic_config = GenericClientConfig {
-            num_clients,
-            client_duration_seconds,
+            num_clients: value_t_or_exit!(matches, "number_of_clients", usize),
+            client_duration_seconds: value_t_or_exit!(matches, "client_duration_seconds", u64),
             image: matches.value_of("docker_image").unwrap().to_string(),
             args: parse_and_format_transparent_args(matches.value_of("generic_client_args")),
             executable_path: value_t_or_exit!(matches, "executable_path", PathBuf),
@@ -483,8 +497,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
 
         ClientConfig::Generic(generic_config)
     } else {
-        // return empty client config. will not deploy a client
-        ClientConfig::Generic(GenericClientConfig::default())
+        ClientConfig::None
     };
 
     let deploy_method = if let Some(local_path) = matches.value_of("local_path") {
@@ -742,6 +755,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
             ClientConfig::Generic(ref config) => {
                 Node::new(DockerImage::new_from_string(config.image.clone())?)
             }
+            ClientConfig::None => unreachable!(),
         };
         cluster_images.set_item(client);
     }