args cleanup #1072

daniel-noland · 2025-11-22T03:46:53Z

Partially addresses #1051

This is somewhat messy due to the integration push. I intend to clean it up, but I think it should be merged as soon as possible in order to avoid bit rot on the dpdk integration which follows this.

Copilot

Pull request overview

This PR implements an "init handoff" mechanism to transfer configuration from the dataplane-init process to the dataplane worker process using sealed memory file descriptors (memfd). The changes introduce comprehensive argument parsing, hardware device scanning, and zero-copy configuration serialization using rkyv.

Key changes:

Complete rewrite of init/src/main.rs to handle device detection, VFIO binding, and configuration handoff
Extensive documentation and type refinement in args/src/lib.rs including NetworkDeviceDescription, LaunchConfiguration, and FinalizedMemFile
Addition of ProfilingConfigSection for Pyroscope integration

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
init/src/main.rs	Implements hardware scanning, device validation, VFIO binding, and configuration serialization/handoff to dataplane worker
init/Cargo.toml	Adds dependencies for args parsing, command-fds, miette, rkyv, serde, and tracing
args/src/lib.rs	Adds extensive documentation, renames `PortArg` to `NetworkDeviceDescription`, adds `ProfilingConfigSection`, refines configuration types
dataplane/Cargo.toml	Adds rkyv dependency for configuration deserialization
Dockerfile	Changes entrypoint from `/dataplane` to `/dataplane-init` and copies artifact to both paths
Cargo.lock	Updates various dependency versions (clap, cc, hashbrown, indexmap, etc.)

Copilot · 2025-11-22T06:23:22Z

args/src/lib.rs

+)]
+#[rkyv(attr(derive(PartialEq, Eq, Debug)))]
+pub struct ProfilingConfigSection {
+    /// The URL of the pryroscope url


Corrected spelling of 'pryroscope' to 'pyroscope'.

Suggested change

/// The URL of the pryroscope url

/// The URL of the pyroscope server

init/src/main.rs

Copilot · 2025-11-22T06:23:23Z

args/src/lib.rs

    pub tracing: TracingConfigSection,
+    /// Metrics collection configuration
    pub metrics: MetricsConfigSection,
+    /// Profileing collection configuration


Corrected spelling of 'Profileing' to 'Profiling'.

Suggested change

/// Profileing collection configuration

/// Profiling collection configuration

args/src/lib.rs

Dockerfile

args/src/lib.rs

Fredi-raspall · 2025-11-22T19:17:01Z

init/src/main.rs

+
+    let launch_config = launch_config.to_owned_fd();
+
+    let io_err = std::process::Command::new("/bin/dataplane")


I wonder if the path to the dataplane should be configurable via cmd line of init itself.

Why? It is fixed in the container?

The location of dataplane will be fix in image, yes. Both things (init and dataplane) will be part of the image, so it would be okay to hardcode it. OTH, making it configurable just gives you more flexibility and makes things less implicit. But we can address this later if needed. We just need to make sure that the image builder ensures that dataplane is at /bin/dataplane (or change the init to just "dataplane" and make sure /bin is in the PATH?)

I really don't agree with this logic. I think that flexibility is an active hazard in that the process executing the dataplane is highly privileged. Giving a lever to control what is executed is a useless hazard.

Fredi-raspall

Looks good to me Daniel. Thanks!
Please address the small typos detected by copilot.

qmonnet

Please split your first commit. Description says “update documentation”, but it does also a number of other modifications to the code. They look minor, but the size of the commit makes it hard to follow what it does.

I think we should also consider mentioning when some code or docs is AI-generated (it looks like some of you docs are).

args/src/lib.rs

qmonnet · 2025-11-24T11:39:59Z

args/src/lib.rs

+    /// Number of dataplane worker threads / cores
+    pub dataplane_workers: usize,


This doesn't seem to be used in this commit.

The goal is to fill out the file. Not consume the file. That is for another PR

OK, in that case it would be worth mentioning it in the commit description, in my opinion. At the moment the title says docs(args): basic docs for LaunchConfiguration, which implies we only add docs in that commit, and it's not clear at all why we also add new fields to the struct.

qmonnet · 2025-11-24T11:41:45Z

args/src/lib.rs

+    /// Metrics collection configuration
    pub metrics: MetricsConfigSection,
+    /// Profileing collection configuration
+    pub profiling: ProfilingConfigSection,


Same, I see where you set the field but I can't find where you read it. I may have missed it, which is also a good indicator this commit contains too many things.

I don't read it in this PR. The goal is to fill out the file, not consume it

Same discussion as above #1072 (comment)

qmonnet · 2025-11-24T14:46:42Z

Now that we've done GW R1, I updated target milestone to GW R2.

These are largely ai generated and then human edited. Co-authored-by: claude sonnet 4.5 Signed-off-by: Daniel Noland <[email protected]>

The lints in this crate were too permissive. Signed-off-by: Daniel Noland <[email protected]>

Signed-off-by: Daniel Noland <[email protected]>

New config file section to set up and manage pyroscope. Signed-off-by: Daniel Noland <[email protected]>

This allows a child process to inherit configuration from a parent process. Signed-off-by: Daniel Noland <[email protected]>

Signed-off-by: Daniel Noland <[email protected]>

Introduce a trait which describes things which may be finalized into truly read only memory. Signed-off-by: Daniel Noland <[email protected]>

Signed-off-by: Daniel Noland <[email protected]>

Assert is preferred to if + panic. Signed-off-by: Daniel Noland <[email protected]>

Fixes for clippy pedantic and general style. Signed-off-by: Daniel Noland <[email protected]>

This commit series introduces new functionality which broke the prior tests. This commit amends the tests to account for that. Signed-off-by: Daniel Noland <[email protected]>

Signed-off-by: Daniel Noland <[email protected]>

Clippy doesn't like the match, so switch to if. Signed-off-by: Daniel Noland <[email protected]>

Signed-off-by: Daniel Noland <[email protected]>

This is needed for consistency with the DPDK driver. Signed-off-by: Daniel Noland <[email protected]>

In addition to being needed for future work, this is needed to make cargo doc pass. Signed-off-by: Daniel Noland <[email protected]>

We never use deserialize, and it doesn't play well with unsafe. Just remove. Signed-off-by: Daniel Noland <[email protected]>

These are needed for upcoming DPDK functionality. Signed-off-by: Daniel Noland <[email protected]>

This is duplicate with PortArgs. Likely PortArgs will need to be renamed to NetworkDeviceDescription to keep git history intact in a future commit. Failing to do so will likely require a massive rework of commit history. For now, just deleting one of them should suffice. Signed-off-by: Daniel Noland <[email protected]>

Failure to consume self here leaves you open to the chance that the immutable criteria on the lifetime will expire and the user will attempt to mutate sealed memory (which will fail). Signed-off-by: Daniel Noland <[email protected]>

daniel-noland force-pushed the pr/daniel-noland/init-handoff branch 4 times, most recently from 2357024 to 67620ec Compare November 22, 2025 06:09

daniel-noland marked this pull request as ready for review November 22, 2025 06:16

daniel-noland requested a review from a team as a code owner November 22, 2025 06:16

daniel-noland requested review from Fredi-raspall and removed request for a team November 22, 2025 06:16

daniel-noland self-assigned this Nov 22, 2025

daniel-noland added Integration Branch that should NOT be merged but kept stable as other branches depend on it. and removed Integration Branch that should NOT be merged but kept stable as other branches depend on it. labels Nov 22, 2025

daniel-noland added this to the GW R1 milestone Nov 22, 2025

daniel-noland linked an issue Nov 22, 2025 that may be closed by this pull request

Ordered recovery of dpdk integration work #1051

Open

daniel-noland added the enhancement New feature or request label Nov 22, 2025

daniel-noland requested a review from Copilot November 22, 2025 06:20

Copilot started reviewing on behalf of daniel-noland November 22, 2025 06:21 View session

Copilot finished reviewing on behalf of daniel-noland November 22, 2025 06:22

Copilot AI reviewed Nov 22, 2025

View reviewed changes

daniel-noland force-pushed the pr/daniel-noland/init-handoff branch 3 times, most recently from f6a5cf8 to 33ede97 Compare November 22, 2025 07:25

Fredi-raspall reviewed Nov 22, 2025

View reviewed changes

args/src/lib.rs Outdated Show resolved Hide resolved

Fredi-raspall reviewed Nov 22, 2025

View reviewed changes

Fredi-raspall approved these changes Nov 22, 2025

View reviewed changes

qmonnet requested changes Nov 24, 2025

View reviewed changes

qmonnet modified the milestones: GW R1, GW R2 Nov 24, 2025

daniel-noland force-pushed the pr/daniel-noland/init-handoff branch from 33ede97 to 4d1cab8 Compare November 25, 2025 01:25

daniel-noland enabled auto-merge November 25, 2025 02:00

daniel-noland force-pushed the pr/daniel-noland/init-handoff branch 2 times, most recently from f089606 to ccf7366 Compare December 4, 2025 20:37

daniel-noland changed the base branch from pr/daniel-noland/just-docs to main December 4, 2025 20:38

daniel-noland force-pushed the pr/daniel-noland/init-handoff branch from ccf7366 to d061f9f Compare December 4, 2025 20:40

daniel-noland added 24 commits December 4, 2025 21:33

docs(args): basic docs

2fe3271

These are largely ai generated and then human edited. Co-authored-by: claude sonnet 4.5 Signed-off-by: Daniel Noland <[email protected]>

fix(args): tighten lints

6ae5490

The lints in this crate were too permissive. Signed-off-by: Daniel Noland <[email protected]>

fix(args): must_use annotations for CmdArgs

c8470c6

Signed-off-by: Daniel Noland <[email protected]>

fix(args): remove unused type alias

29a2468

Signed-off-by: Daniel Noland <[email protected]>

fix(args): remove unsed WorkerStackSize

93a95fb

Signed-off-by: Daniel Noland <[email protected]>

feat(args): intro ProfilingConfigSection

0400ba4

New config file section to set up and manage pyroscope. Signed-off-by: Daniel Noland <[email protected]>

feat(args): introduce inherit method

83291cd

This allows a child process to inherit configuration from a parent process. Signed-off-by: Daniel Noland <[email protected]>

feat(args): add defensive annotations on inherit

d6c3c28

Signed-off-by: Daniel Noland <[email protected]>

feat(args): intro AsFinalizedMemFd trait

de1c25b

Introduce a trait which describes things which may be finalized into truly read only memory. Signed-off-by: Daniel Noland <[email protected]>

feat(args): add must_use annotation on to_owned_fd

1656c05

Signed-off-by: Daniel Noland <[email protected]>

chore(args): style cleanup for from_fd

2e6966d

Assert is preferred to if + panic. Signed-off-by: Daniel Noland <[email protected]>

chore(args): cleanup LaunchConfiguration

f64747c

Fixes for clippy pedantic and general style. Signed-off-by: Daniel Noland <[email protected]>

test(args): cleanup tests

80dede5

This commit series introduces new functionality which broke the prior tests. This commit amends the tests to account for that. Signed-off-by: Daniel Noland <[email protected]>

chore(args): remove unused import

b4cfe32

Signed-off-by: Daniel Noland <[email protected]>

fix(args): add muse_use annotation to MemFile::new

7f943ae

Signed-off-by: Daniel Noland <[email protected]>

fix(args): follow clippy pedantic style

58ebb26

Clippy doesn't like the match, so switch to if. Signed-off-by: Daniel Noland <[email protected]>

fix(args): rename use_nics to interfaces for consistency

59bcde6

Signed-off-by: Daniel Noland <[email protected]>

feat(args): switch KernelDriverConfigSection to use InterfaceArg

e36fdcd

This is needed for consistency with the DPDK driver. Signed-off-by: Daniel Noland <[email protected]>

fix(args): make INTEGRITY_CHECK_BYTE_LEN public

feb6090

In addition to being needed for future work, this is needed to make cargo doc pass. Signed-off-by: Daniel Noland <[email protected]>

fix(args): don't derive serde::Deserialize

64e97ef

We never use deserialize, and it doesn't play well with unsafe. Just remove. Signed-off-by: Daniel Noland <[email protected]>

feat(args): derive rkyv traits for PortArg and InterfaceArg

66a7eb8

These are needed for upcoming DPDK functionality. Signed-off-by: Daniel Noland <[email protected]>

fix(args): AsFinalizedMemFile should consume self

ce7eb2c

Failure to consume self here leaves you open to the chance that the immutable criteria on the lifetime will expire and the user will attempt to mutate sealed memory (which will fail). Signed-off-by: Daniel Noland <[email protected]>

scratch to check ci

a3ff7d9

daniel-noland force-pushed the pr/daniel-noland/init-handoff branch from d061f9f to a3ff7d9 Compare December 4, 2025 21:35

scratch to check ci

8146157

	/// The URL of the pryroscope url
	/// The URL of the pyroscope server

	/// Profileing collection configuration
	/// Profiling collection configuration


		let launch_config = launch_config.to_owned_fd();

		let io_err = std::process::Command::new("/bin/dataplane")

		/// Number of dataplane worker threads / cores
		pub dataplane_workers: usize,

args cleanup #1072

Are you sure you want to change the base?

args cleanup #1072

Uh oh!

Conversation

daniel-noland commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fredi-raspall left a comment

Choose a reason for hiding this comment

Uh oh!

qmonnet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qmonnet commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

daniel-noland commented Nov 22, 2025 •

edited

Loading