-
Notifications
You must be signed in to change notification settings - Fork 15
chore: refine testing infrastructure and tests exec time comparison #426
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
0f3177a
8a4edac
bc8b91d
b8db956
502b6a9
0474bf1
1f60535
9c37ca5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| use std::env; | ||
| use std::fmt; | ||
| use std::io::Write; | ||
| use std::net::TcpStream; | ||
| use std::path::PathBuf; | ||
| use std::process::{Command, Output}; | ||
| use std::time::{Duration, Instant}; | ||
|
|
||
| pub struct DockerComposeCmd { | ||
| pub root_path: PathBuf, | ||
|
|
@@ -40,6 +42,21 @@ pub fn format_output(output: &Output) -> OutputWrapper<'_> { | |
| OutputWrapper(output) | ||
| } | ||
|
|
||
| /// Wait until all given TCP ports on localhost are no longer bound. | ||
| /// This prevents "address already in use" errors when Docker Compose retries | ||
| /// start before the OS has released ports from the previous run. | ||
| fn wait_for_ports_free(ports: &[u16], timeout: Duration) { | ||
| let deadline = Instant::now() + timeout; | ||
| for &port in ports { | ||
| while Instant::now() < deadline { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this mean that if one of the ports in loop {
if ports.iter().all(|&p| port_is_bindable(DOCKER_ADDR, p) { return Ok(()) }
// Now check deadline, bail with error if reached
if Instant::now() >= deadline { … … … }
thread::sleep(sleep);
}Overkill I guess but we should be able to check the ports in parallel too yeah? |
||
| if TcpStream::connect(("127.0.0.1", port)).is_err() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| break; // port is free | ||
| } | ||
| std::thread::sleep(Duration::from_millis(500)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl DockerComposeCmd { | ||
| pub fn new(mode: KMSMode) -> Self { | ||
| let manifest_dir = std::env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR not set"); | ||
|
|
@@ -51,8 +68,26 @@ impl DockerComposeCmd { | |
| DockerComposeCmd { root_path, mode } | ||
| } | ||
|
|
||
| fn ports_for_mode(&self) -> &'static [u16] { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this function could be |
||
| match self.mode { | ||
| KMSMode::ThresholdTestParameterNoInitSixParty => &[ | ||
| 50100, 50200, 50300, 50400, 50500, 50600, 50001, 50002, 50003, 50004, 50005, 50006, | ||
| ], | ||
| KMSMode::ThresholdDefaultParameter | ||
| | KMSMode::ThresholdTestParameter | ||
| | KMSMode::ThresholdTestParameterNoInit | ||
| | KMSMode::ThresholdCustodianTestParameter => { | ||
| &[50100, 50200, 50300, 50400, 50001, 50002, 50003, 50004] | ||
| } | ||
| KMSMode::Centralized | KMSMode::CentralizedCustodian => &[50100], | ||
| } | ||
| } | ||
|
|
||
| pub fn up(&self) { | ||
| self.down(); // Make sure that no container is running | ||
| // Wait for the OS to release ports before starting new containers. | ||
| // Without this, Docker Compose retries fail with "address already in use". | ||
| wait_for_ports_free(self.ports_for_mode(), Duration::from_secs(30)); | ||
| let build_docker = env::var("DOCKER_BUILD_TEST_CORE_CLIENT").unwrap_or("".to_string()); | ||
|
|
||
| // set the FHE params based on mode | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using
k8sinstead of the fullkubernetes. :)