-
Notifications
You must be signed in to change notification settings - Fork 6
dataplane init system part 1 #897
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?
Conversation
7658bd3
to
4b3877d
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 pull request introduces the foundation for a dataplane initialization system with new NIC management capabilities. The changes focus on creating infrastructure for binding network cards to appropriate drivers and setting up the system initialization flow.
Key changes:
- Created a new
init
crate with PCI device and driver management functionality - Added comprehensive documentation and configuration for the initialization system
- Updated various crates to use embedded README documentation and improved error handling
Reviewed Changes
Copilot reviewed 35 out of 39 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
init/ |
New crate for dataplane initialization with PCI device management |
the-rules.md |
Comprehensive documentation of DPDK + async threading rules |
start-up.mmd |
Mermaid diagram for proposed startup sequence |
tracectl/ |
Updated to use embedded README documentation |
dpdk/ |
Improved error handling and memory management |
dataplane/ |
Disabled some components and added build dependencies |
Various config files | Updated dependencies, build settings, and documentation |
4b3877d
to
43ffbfe
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 35 out of 39 changed files in this pull request and generated no new comments.
a2b236f
to
77bdba9
Compare
The init package (more properly `dataplane-init`), is responsible for starting up the dataplane and forwarding computed or augmented command line arguments to it. This commit only includes basic documentation and mission statement. This commit series focuses on initializing the network card in a manor suitable for use by DPDK. Signed-off-by: Daniel Noland <[email protected]>
dba8f78
to
11c261e
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 4 comments.
Comments suppressed due to low confidence (1)
init/src/nic.rs:1
- Redundant error handling: the
?
operator is unnecessary afterErr(...)
sinceErr
is already being returned. Remove the?
operator.
// SPDX-License-Identifier: Apache-2.0
11c261e
to
96b6a25
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 2 comments.
Comments suppressed due to low confidence (1)
init/src/nic.rs:1
- The
?
operator is unnecessary here since this is already the final expression in the match arm. The expression should be simplified to justErr(InitErr::IoError(e.into()))
.
// SPDX-License-Identifier: Apache-2.0
45f5d60
to
51bc4f9
Compare
Introduce a package wide application error type. This is very different from our more typical library level error types in that it is intended to be a catch-all error type for fatal errors. Almost nothing which can go wrong in this crate is expected to be recoverable without human intervention. Signed-off-by: Daniel Noland <[email protected]>
This will require extension to customize log level. Signed-off-by: Daniel Noland <[email protected]>
d9b6290
to
5803d40
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 3 comments.
fn driver(&self) -> Result<Option<PciDriver>, InitErr> { | ||
let device_path = self.device_path()?; | ||
info!("found device {self} under device path {:?}", device_path); | ||
let driver_path = device_path.relative("driver")?; |
Copilot
AI
Oct 10, 2025
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.
The code attempts to get the driver path but doesn't handle the case where the device has no driver bound (symlink doesn't exist). This will cause relative()
to fail with an IO error instead of returning Ok(None)
as the function signature suggests.
Copilot generated this review using guidance from repository custom instructions.
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.
true. Will fix in the next PR
Basic guard rails which discourage us from making some mistakes. Features: - SysfsPath type ensuring paths are under sysfs mount - UTF-8 validation to prevent log injection attacks - Filesystem type verification using statfs - Path canonicalization to handle symlinks safely Signed-off-by: Daniel Noland <[email protected]>
This function finds our sysfs mount and panics if it is missing. It also sets up tracing. Signed-off-by: Daniel Noland <[email protected]>
Introduces PCI NIC abstraction for driver management with: - Driver detection and identification - Unbinding from kernel drivers - Binding to vfio-pci for DPDK - Support for i40e, mlx5 (partial), virtio-net, and vfio-pci drivers Implements traits for safe driver manipulation through sysfs. Signed-off-by: Daniel Noland <[email protected]>
Implements basic NIC driver binding for DPDK compatibility. Binds PCI network devices to vfio-pci driver as required by DPDK for device control. This is part 1 of the dataplane initialization system, focusing on minimal viable functionality for team testing. Future work: - Add proper argument parsing - Implement privilege dropping (especially CAP_SYS_ADMIN) - Add exec to dataplane process after success Signed-off-by: Daniel Noland <[email protected]>
5803d40
to
0b5aad6
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.
I've got concerns about the name for the crate. init
sounds like we initialise the dataplane itself, not like a preliminary step to set up the NICs etc. before launching the dataplane
executable. I also fear it might lead to confusion (if not for us, for users/costumers) with the init
process on some Linux distribs. How about bootstrap
, maybe?
Great work otherwise!
init/src/main.rs
Outdated
#![doc = include_str!("../README.md")] | ||
#![deny(clippy::pedantic, missing_docs)] | ||
|
||
fn main() {} |
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.
Your process will be named init
(unless I'm mistaken). I think it would maybe make sense to find another name instead to avoid any confusion with the init
process on some Linux distributions. Even if it might be clear to us in the context, it will become a source of confusion when debugging customer issues if we mention an init
process.
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.
Perhaps hwinit is a better name?
/// Some [`std::io::Error`] error occurred | ||
#[error(transparent)] | ||
IoError(#[from] std::io::Error), | ||
/// Invalid UTF-8 in a path under sysfs is an absolutely wild error case I expect to |
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.
This is a rustdoc comment for API docs. Who is “I”? 🙂
/// Invalid UTF-8 in a path under sysfs is an absolutely wild error case I expect to | |
/// Invalid UTF-8 in a path under sysfs is an absolutely wild error case we expect to |
/// | ||
/// Note that the NIC may or may not be visible to the OS, depending on the state of | ||
/// the system. | ||
pub struct PciNic { |
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 can't help but read this “PicNic” every time I see it 😎
This is a first installment of the init package.
It will eventually be responsible for starting the dataplane, but is not currently and will not be even if this is merged.
That will require a few extra steps.
As such, this code, merged or not, will have no impact on the rest of the development for this workspace just yet. I'm only submitting this part early to try to keep the PRs as byte size and manageable chunks.