Sanity Test for nodes running in k8s (BFT-394) #63
Conversation
Co-authored-by: Bruno França <[email protected]>
node/tests/src/main.rs
Outdated
| // This way we can run the test from every directory and also inside kubernetes pod. | ||
| let manifest_path = std::env::var("CARGO_MANIFEST_DIR"); | ||
| if let Ok(manifest) = manifest_path { | ||
| format!("{}/config.txt", manifest) |
There was a problem hiding this comment.
nit: Path::new(manifest_path.unwrap_or("")).join("config.txt")
node/tests/src/main.rs
Outdated
| .open(&config_file_path)?; | ||
| for addr in pods_ip { | ||
| config_file | ||
| .write_all(format!("{}\n", addr.to_string()).as_bytes()) |
There was a problem hiding this comment.
write!(config_file,"{addr}\n").context("...")?;
| for ip in node_ips.lines() { | ||
| let url: String = format!("http://{}:3154", ip); | ||
| let nodes_socket = fs::read_to_string(config_file_path).unwrap(); | ||
| for socket in nodes_socket.lines() { |
There was a problem hiding this comment.
first parse the ips, rather than blindly turning them into urls
node/tools/src/k8s.rs
Outdated
| let pod_running_container = pod_spec | ||
| .containers | ||
| .first() | ||
| .context("Failed to get pod container") |
There was a problem hiding this comment.
context().ok() doesn't make sense, because ok() is dropping the error altogether. In this case, it actually made more sense to first filter() and then map(), where map might return an error (now we just silently drop all the errors here).
There was a problem hiding this comment.
Okay, you're correct that returning a Result from this function is no longer useful when using filter_map. I made the change using filter followed by map, but the errors that arise in the filter closure are still being ignored because it requires a boolean and using ? to return a Result is not possible. Let me know if this is acceptable. In the map closure, the errors should now be handled correctly.
There was a problem hiding this comment.
if you need to return an error from filter, then just use a regular loop over the elements instead.
There was a problem hiding this comment.
I ended up including everything in the for loop instead of having the for loop only for filtering and then mapping. I chose this solution mainly because:
- I can reuse some pod variables instead of retrieving them twice, once for filtering and then again for mapping and address parsing.
- This way, I only iterate over all the pods once. With the separate map approach, I had to create an empty vector, push all the filtered pods into it, and then convert it to an iterator for the address parsing part.
Let me know what you think.
node/tools/src/k8s.rs
Outdated
| ensure!( | ||
| !pod_addresses.is_empty(), | ||
| "No consensus pods found in the k8s cluster" | ||
| ); |
There was a problem hiding this comment.
nit: that should be rather checked by the caller, who will inspect the result
node/tools/src/k8s.rs
Outdated
| let mut node_rpc_addresses: Vec<SocketAddr> = Vec::new(); | ||
| for pod in pods.into_iter() { | ||
| let pod_container = pod | ||
| .clone() |
node/tools/src/k8s.rs
Outdated
| .context("Failed to get pod spec")? | ||
| .containers | ||
| .first() | ||
| .cloned() |
There was a problem hiding this comment.
first().as_ref().context()
What ❔
Add a first sanity test to check node liveness running with the new Kubernetes script.
Why ❔
We need this basic test to see how to address the test suite development and have a layer used to communicate a local environment (like CI) with the nodes running in kubernetes.