-
Notifications
You must be signed in to change notification settings - Fork 20
expand Manifest with pool_size and client_timeout #584
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
Conversation
and use it in Reaper, also expand qos_client with the cli argument for it
|
Generally looks good just a few comments |
6b3fe05 to
07173dd
Compare
also fix small nits
07173dd to
bff3355
Compare
r-n-o
left a comment
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.
Awesome! Some minor minor suggestions below (feel free to punt / address in a followup PR!); flawless diff otherwise 👑
| let enclave_pool = | ||
| StreamPool::new(SocketAddress::new_unix(&usock), 1).unwrap(); | ||
| StreamPool::single(SocketAddress::new_unix(&usock)).unwrap(); |
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.
💯
| /// Initial client timeout for the processor until the Manifest says otherwise | ||
| pub const INITIAL_CLIENT_TIMEOUT: Duration = Duration::from_secs(5); |
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.
Where is the "until the Manifest says otherwise" part? Maybe worth an extra bit of comment? (I think a simple "see reaper.rs" could be fine; that's where set_client_timeout is called from)
| fn get_pool_size_from_pivot_args(handles: &Handles) -> (bool, Option<u32>) { | ||
| if let Ok(envelope) = handles.get_manifest_envelope() { | ||
| (true, extract_pool_size_arg(&envelope.manifest.pivot.args)) | ||
| } else { | ||
| (false, None) | ||
| } | ||
| } | ||
|
|
||
| // find the u32 value of --pool-size argument passed to the pivot if present | ||
| fn extract_pool_size_arg(args: &[String]) -> Option<u32> { | ||
| if let Some((i, _)) = | ||
| args.iter().enumerate().find(|(_, a)| *a == "--pool-size") | ||
| { | ||
| if let Some(pool_size_str) = args.get(i + 1) { | ||
| match pool_size_str.parse::<u32>() { | ||
| Ok(pool_size) => Some(pool_size), | ||
| Err(_) => None, | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } |
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.
❤️
| /// Sets the client timeout value for the `app_client` | ||
| /// Sets the client timeout value for the `app_client`, maximum allowed value is `u16::MAX` milliseconds | ||
| pub fn set_client_timeout(&mut self, timeout: Duration) { | ||
| assert!(timeout.as_millis() < u16::MAX.into(), "client timeout > 65s"); |
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.
I actually think this will likely be too restrictive (e.g. zk proof gen). But we can cross that bridge when we get there
Summary & Motivation (Problem vs. Solution)
Expands the
Manifestformat and it's handlingqos-clientwithpool-sizeandclient-timeoutparameters.pool-sizeallows us to specify the number of internal USOCK/VSOCK connections for concurrency without impedeing app params list. It gets translated to an env var for the app.client-timeoutcan now be used to specify the timeout forcalls done bySocketClientinside the enclave.There's also a minor refactor moving away from
TimeValtoDurationin aroundSocketClienttimeout type and params.NOTE: hosts and proxy still need the timeout value handled explicitly of course, but that's out of scope for this PR.
WARNING: Please do NOT merge yet, as I want to test with a deploy in preprod first. Reviews are welcome.
How I Tested These Changes
Locally so far