-
Notifications
You must be signed in to change notification settings - Fork 6
feat(args): completely rework args crate #971
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
96097cd to
a39ef0a
Compare
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.
Pull Request Overview
This PR refactors dataplane configuration management by moving common types and constants from their previous locations into the args crate, and introduces a new configuration serialization system using sealed memory files (memfds). The key changes enable the dataplane-init process to pass ephemeral launch-time configuration to child dataplane processes.
- Moved socket path constants (
DEFAULT_DP_UX_PATH,DEFAULT_DP_UX_PATH_CLI,DEFAULT_FRR_AGENT_PATH) fromrouting::riotoargsmodule - Moved
GrpcAddressenum frommgmt::processor::launchtoargsmodule with enhanced serialization support - Added extensive new configuration infrastructure including
MemFile,FinalizedMemFile,LaunchConfiguration, and integrity checking using SHA384 - Removed DPDK-specific command-line arguments and the unused
setup_pipelinefunction fromdataplane/src/main.rs
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| routing/src/router.rs | Updated imports to use socket path constants from args instead of crate::rio |
| routing/src/rio.rs | Removed socket path constants (moved to args), updated references to use args:: prefix |
| routing/Cargo.toml | Added dependency on args workspace crate |
| mgmt/src/processor/launch.rs | Removed GrpcAddress enum (moved to args), updated to use args::GrpcAddress, changed to_path_buf() to into(), fully qualified tokio::task::spawn |
| mgmt/Cargo.toml | Added dependency on args workspace crate, reordered dependencies alphabetically |
| dataplane/src/main.rs | Removed unused imports and setup_pipeline function, replaced method calls with new naming (grpc_address(), driver_name()), replaced DPDK driver start with todo!() |
| args/src/lib.rs | Added socket path constants, GrpcAddress enum, extensive memfd infrastructure, LaunchConfiguration types, integrity checking, conversion from CmdArgs to LaunchConfiguration, renamed methods, moved test code |
| args/Cargo.toml | Added many new dependencies (miette, sha2, memmap2, nix, bytecheck, serde, etc.), reordered and reorganized dependencies |
| Cargo.lock | Added new transitive dependencies for the new functionality |
mvachhar
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.
Please address the copilot comments and partition lib.rs into a few files as described. Otherwise, this looks good.
d583e1b to
fed2308
Compare
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.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
fed2308 to
84e5508
Compare
The args crate is now has a wider scope: it is not just responsible for parsing and validating arguments, it is (partly) responsible for handing the dataplane a memfd file with parsed and acted on data. The idea here is to 1. allow all of the highly sensitive, highely privileged, and one off jobs to be done by dataplane-init rather than dataplane 2. so that dataplane can init the DPDK eal as quickly as possible after process start. The memfd strategy (developed in this PR), is a relatively simple way to accomplish these objectives (although there is a lot of room for improvement here). Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
84e5508 to
c0a4cfd
Compare
mvachhar
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.
Delaying addressing the scope and file comments to #972 to favor fewer merge conflicts for Fredi.
This is broken out from #964 in order to help @Fredi-raspall do any needed rebase work.
Ideally we can keep getting chunks of #964 in as we progress in integration.
Note
I also significantly extended the docs vs what is currently in #964