-
Notifications
You must be signed in to change notification settings - Fork 453
globally unique port allocation for unittests #5865
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
globally unique port allocation for unittests #5865
Conversation
let bind_ip_addr = IpAddr::V4(Ipv4Addr::UNSPECIFIED); | ||
let port_range = (MIN_PORT_RANGE, MAX_PORT_RANGE); | ||
|
||
let bind_ip_addr = IpAddr::V4(Ipv4Addr::LOCALHOST); |
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.
do we only use TestValidator
locally? never across machines?
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.
It can run remotely but was never intended to. We use it in tests for SVM programs in CI (which run locally). I believe some devs use it for testing too, but also in a local context. Finally, this is just the default config for it anyway, one can override it with CLA if needed.
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.
looks like solana-test-validator cli defauls bind_address to 0.0.0.0. See:
Lines 860 to 868 in f0032eb
.arg( | |
Arg::with_name("bind_address") | |
.long("bind-address") | |
.value_name("HOST") | |
.takes_value(true) | |
.validator(solana_net_utils::is_host) | |
.default_value("0.0.0.0") | |
.help("IP address to bind the validator ports [default: 0.0.0.0]"), | |
) |
agave/validator/src/bin/solana-test-validator.rs
Lines 175 to 180 in f0032eb
let bind_address = matches.value_of("bind_address").map(|bind_address| { | |
solana_net_utils::parse_host(bind_address).unwrap_or_else(|err| { | |
eprintln!("Failed to parse --bind-address: {err}"); | |
exit(1); | |
}) | |
}); |
and set again here:
agave/validator/src/bin/solana-test-validator.rs
Lines 555 to 557 in f0032eb
if let Some(bind_address) = bind_address { | |
genesis.bind_ip_addr(bind_address); | |
} |
Would it make sense to make the default cli bind to localhost since bindip_addr
in TestValidatorNodeConfig::Default()
gets overwritten?
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.
Yes, you are on point there, already PR'd that one #5862
#[cfg(not(debug_assertions))] | ||
let port_range = solana_net_utils::VALIDATOR_PORT_RANGE; | ||
#[cfg(debug_assertions)] | ||
let port_range = localhost_port_range_for_tests(); |
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.
maybe a dumbn question, but when we run tests in CI, are they compiled with --release or --debug?
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.
CI runs with debug assertions. That is kinda the point:) This is unrelated to whether optimizer is enabled or not. I guess we could gate it on dev-context-only-utils as well, I am honestly not sure what the best way here is.
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.
ya tbh I think gating on dev-context-only-utils would be preferable. That way you have to explicitly set the feature to get the desired, limited port range. not 100% sure though, maybe @bw-solana has a better insight here?
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.
Yeah that is possible but will be more invasive as we would need to manually activate the feature in all crates that pull test-validator for their unittests. It would also be mighty unobvious as to why the feature is enabled if it does not gate any functions.
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.
ahh good point. ok i am fine leaving it as is then
Co-authored-by: Greg Cusack <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5865 +/- ##
=========================================
- Coverage 83.0% 83.0% -0.1%
=========================================
Files 828 829 +1
Lines 375858 375910 +52
=========================================
+ Hits 312113 312153 +40
- Misses 63745 63757 +12 🚀 New features to boost your workflow:
|
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.
lgtm!
Problem
We can not rely on port ranges not intersecting during test runs, and we need bindings with fixed offsets for QUIC ports.
This PR ensures all bindings use non-overlapping port ranges.
Summary of Changes