userspace: add bounded port mirror runtime slice#1408
Conversation
|
@copilot review |
Adversarial review complete on |
There was a problem hiding this comment.
Pull request overview
Adds initial userspace-dp port-mirroring support (#1376) by implementing a bounded “forwarded-path” mirror clone path, wiring mirror configs into runtime forwarding state, and surfacing mirror counters through Rust/Go status and CLI formatting (while intentionally keeping the userspace capability gate in place).
Changes:
- Implement bounded mirror cloning in the pending-forward TX path with per-binding sampling and lossy backpressure controls.
- Wire mirror configs into
ForwardingStateand add per-binding mirror counters in live state, status snapshots, and protocol structs. - Extend Go status formatting/tests and update docs to reflect partial runtime support and remaining gating.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| userspace-dp/src/protocol.rs | Adds mirror counters to BindingStatus + BindingCountersSnapshot and tests for wire round-trip/projection. |
| userspace-dp/src/main_tests.rs | Extends wire-key serialization tests to include mirror counter keys. |
| userspace-dp/src/afxdp/worker/mod.rs | Adds per-binding mirror sampling counter and a test-only BindingWorker constructor for mirror tests; extends live snapshot struct. |
| userspace-dp/src/afxdp/umem/mod.rs | Adds atomic mirror counters to BindingLiveState and includes them in snapshots. |
| userspace-dp/src/afxdp/types/forwarding.rs | Adds mirror_configs to ForwardingState and defines MirrorRuntimeConfig. |
| userspace-dp/src/afxdp/tx/dispatch.rs | Hooks mirror selection + clone enqueue into the forwarded pending-TX path. |
| userspace-dp/src/afxdp/mod.rs | Wires the new mirror module into the AF_XDP dataplane module tree. |
| userspace-dp/src/afxdp/mirror.rs | New module implementing sampling, clone enqueue, and mirror counter recording + unit tests. |
| userspace-dp/src/afxdp/forwarding_build.rs | Populates runtime mirror config map from the config snapshot. |
| userspace-dp/src/afxdp/coordinator/mod.rs | Threads mirror counters into coordinator-maintained binding status and reset paths. |
| pkg/dataplane/userspace/statusfmt.go | Includes mirror counters in the CLI status summary aggregation/output. |
| pkg/dataplane/userspace/statusfmt_test.go | Updates summary output expectations for mirror counters. |
| pkg/dataplane/userspace/protocol.go | Adds mirror counter fields to Go protocol types for status decoding/encoding. |
| pkg/dataplane/userspace/protocol_test.go | Adds JSON wire-key and round-trip tests for mirror counters in Go types. |
| docs/userspace-dataplane-gaps.md | Updates capability-gap doc to reflect “gated; partial runtime” port-mirroring support. |
| docs/pr/1373-retire-ebpf-dataplane/plan-1376-port-mirroring.md | Documents the current runtime slice, counters, and expanded test expectations. |
| /// #1376: mirror clone dropped because the output binding did not | ||
| /// have enough reserved TX frames available. |
Round-1 dual-review consolidated synthesis on
|
| Reviewer | Verdict |
|---|---|
| Codex | MERGE-NEEDS-MAJOR (2 MAJOR + 1 MEDIUM) |
| Gemini Pro 3 | MERGE-NEEDS-MAJOR (3 MAJOR) |
Both converge MERGE-NEEDS-MAJOR. 4 distinct issues across reviewers.
Codex MAJOR 1 — VLAN/logical interface mirror handling broken
pkg/dataplane/userspace/snapshot.go:153 keys mirrors by configured interface ifindex. VLAN/logical units carry child/synthetic Ifindex + ParentIfindex (snapshot.go:660). Runtime lookup uses raw request.meta.ingress_ifindex (afxdp/tx/dispatch.rs:184) and output binding uses config.output_ifindex directly (afxdp/mirror.rs:51). Never resolves (parent_ifindex, vlan_id) → logical_ifindex or output egress_ifindex → bind_ifindex.
Worked failure: parent-bound VLAN/reth mirror inputs miss entirely; logical outputs drop as NoBinding.
Codex MAJOR 2 — Flow-cache fast path bypasses mirroring
Cached forward hits rewrite and queue PreparedTxRequest directly at poll_descriptor.rs:221-315, skipping enqueue_pending_forwards which is the ONLY mirror hook (tx/dispatch.rs:184-201).
Worked failure: Established session mirrors the MISS packet, then stops mirroring all cache-hit packets. Operator sees first packet of every flow mirrored but nothing else.
Gemini MAJOR 1 — Cross-worker blackholing
userspace-dp/src/afxdp/mirror.rs:51-61:
let target_binding_index = binding_lookup.target_index(
ingress_index, ingress_binding.ifindex, ingress_queue_id, config.output_ifindex,
);
let Some(target_binding) = target_binding_index.and_then(...) else {
return MirrorCloneResult::NoBinding;
};WorkerBindingLookup is per-coordinator and only indexes BindingWorker slices on the LOCAL thread. If output_ifindex is on a different worker, target_index = None → silently dropped as NoBinding.
Worked failure: Mirror ingress on worker-0, output on worker-3 → every clone dropped. No telemetry signal beyond NoBinding counter.
Gemini MAJOR 2 — CoS bypass + telemetry contamination
userspace-dp/src/afxdp/mirror.rs:105 hardcodes cos_queue_id: None. Mirror clones with cos_queue_id=None skip token bucket queues and dump directly into TX ring via drain_pending_tx post-CoS backup loop.
Worked failure: Mirror under pressure bypasses #760 CoS limits, saturates physical link, AND fires binding.live.owner_profile_owner.post_drain_backup_bytes metric (designed to hunt cap-bypass bugs) → operator-visible false alarm masking the real bypass.
Gemini MAJOR 3 — Plan-doc claims unmet
- Plan: "commit validation rejects duplicate ingress mirror entries" — not implemented;
compiler.goblindly invokesdp.SetMirrorConfig()which silently overwrites - Plan: lists
mirror::duplicate_ingress_ifindex_rejected+mirror::ipv4_ipv6_full_frame_preservationtests — NEITHER exists
Codex MEDIUM — Tests don't pin end-to-end
Mostly DTO round-trips + enqueue_mirror_clone helper tests. No test that builds snapshot → ForwardingState → runs enqueue_pending_forwards → asserts clone output/counters. No VLAN/logical-interface test. No flow-cache bypass test.
Wins (both reviewers)
- ✓ Simple physical/unit-0 pending-forward path is wired
- ✓ Backwards-compat for absent
mirror_configsand missing status counters - ✓ L2 frame preservation (Gemini: copies full L2 frame including 802.1Q tags)
- ✓ Capability gate still rejects port mirroring → production fail-closed
Recommendation
Block on:
- Resolve VLAN/logical ifindex via parent+vlan→logical lookup (Codex MAJOR 1)
- Wire mirror enqueue into flow-cache hit fast path (Codex MAJOR 2)
- Handle cross-worker mirror via shared queue/IPC or document the limitation (Gemini MAJOR 1)
- Respect CoS via proper cos_queue_id assignment OR document mirror-bypasses-CoS as a stated design choice (Gemini MAJOR 2)
- Implement compile-time duplicate-ingress validation; add the two tests the plan committed to (Gemini MAJOR 3)
- Add end-to-end snapshot→ForwardingState→clone-output test pinning the actual production shape
Codex task-mpa6l22e-qa1tu4; Gemini task-mpa6lmrq-oll8cz. Not merging — author's decision.
|
@copilot review |
Addressed in
Adversarial re-review complete on latest head ( |
Round-2 Codex review on
|
Round-2 dual-review consolidated synthesis on
|
| Reviewer | Verdict |
|---|---|
| Codex | MERGE-NEEDS-MAJOR (2 NEW MAJORs from r2 changes) |
| Gemini Pro 3 | MERGE-NEEDS-MAJOR (2 MAJOR: flow-cache double-allocation) |
Both converge MERGE-NEEDS-MAJOR. Both independently caught the flow-cache pre-sampling allocation.
r1 wins (both reviewers confirm)
- ✓ VLAN/logical ingress lookup:
resolve_mirror_configresolves(ingress_ifindex, vlan)before parent fallback - ✓ Cross-worker mirror delivery: functionally works
- ✓ CoS egress/default queue + backup escape: addressed (mirror clones now bound by
mirror_cos_queue_id) - ✓
Codex #784mixed-head invariant honored in CoS fallback scans
Codex MAJOR 1 + Gemini MAJOR 1 — Flow-cache mirror copies before sampling
userspace-dp/src/afxdp/poll_descriptor.rs:263-268:
let mirror_frame = resolve_mirror_config(
worker_ctx.forwarding,
meta.ingress_ifindex as i32,
meta.ingress_vlan_id,
).map(|_| packet_frame.to_vec());Worked failure (both reviewers): mirror with rate = 10000. Every flow-cache hit on mirror-configured interface allocates+copies the full frame BEFORE select_mirror_config() evaluates sampling. 9,999/10,000 allocations are discarded post-fact.
Fix: sample first (cheap predicate), to_vec() only if admitted.
Gemini MAJOR 2 — Double-allocation of admitted clones
When admitted, system pays allocation tax TWICE:
poll_descriptor.rscreatesmirror_frame = Vec<u8>and passes as&[u8]toenqueue_sampled_mirror_clone_to_liveenqueue_mirror_clone_to_livethen doesTxRequest { bytes: frame.to_vec(), ... }— second allocation
Fix: either check sample rate before the first allocation, OR construct TxRequest to take ownership of the buffer (move semantics).
Codex MAJOR 2 — Cross-worker mirror lacks isolation
Same-worker path uses MIRROR_PENDING_LIMIT at mirror.rs:95-101. Cross-worker uses target_live.try_enqueue_tx_owned() at :156-168, bounded ONLY by shared redirect inbox (umem/mod.rs:1123-1133).
Worked failure: mirror clones occupy worker-B's normal TX inbox. Heavy mirror on worker-A → worker-B primary forwarding stalls.
Fix: add MIRROR_PENDING_LIMIT-equivalent reserve/cap on cross-worker mirror path.
Tests + Plan-doc (both reviewers)
- Tests still helper-level; don't drive production through flow-cache branch or prove sampled mirrors avoid unsampled allocation, or prove cross-worker mirror pressure can't harm primary forwarding
- Plan still lists
mirror::duplicate_ingress_ifindex_rejected+mirror::ipv4_ipv6_full_frame_preservationtests that don't exist - Plan claims "small pending-backlog limit" but cross-worker output bypasses it
Recommendation
Block on:
- Sample-before-allocate in
poll_descriptor.rs:263(moveselect_mirror_config()ahead ofpacket_frame.to_vec()) - Eliminate double-allocation by passing owned
Vec<u8>directly intoTxRequest - Add
MIRROR_PENDING_LIMIT-equivalent reserve on cross-worker mirror path
Codex task-mpa7x6pf-3z63lf; Gemini task-mpa7xgdb-lvvimk. Not merging — author's decision.
Round-3 Codex review on
|
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
userspace-dp/src/afxdp/umem/mod.rs:1106
try_admit_tx_owned()recordsrecord_redirect_inbox_overflow()(incrementingtx_errors+redirect_inbox_overflow_drops) even though it hasn't actually attempted an enqueue yet. When used as a preflight (e.g. mirror admission), this can double-count drops once the subsequent enqueue also fails, and it also attributes mirror pressure to the generic redirect overflow counters. Consider making this a pure capacity check (no counter side effects) and leaving counter bumps to the actual push path, or folding admission+push into a single method that can returnQueueFullwithout touching redirect overflow counters for mirror clones.
pub(super) fn try_admit_tx_owned(&self) -> Result<(), ()> {
let pending_len = self.pending_tx.len();
let max_pending = self.max_pending_tx.load(Ordering::Relaxed) as usize;
if (max_pending > 0 && pending_len >= max_pending)
|| pending_len >= self.pending_tx.capacity()
{
self.record_redirect_inbox_overflow();
return Err(());
}
Ok(())
| pub(super) fn try_admit_tx_owned(&self) -> Result<(), ()> { | ||
| let pending_len = self.pending_tx.len(); | ||
| let max_pending = self.max_pending_tx.load(Ordering::Relaxed) as usize; | ||
| if (max_pending > 0 && pending_len >= max_pending) | ||
| || pending_len >= self.pending_tx.capacity() | ||
| { | ||
| self.record_redirect_inbox_overflow(); | ||
| return Err(()); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub(super) fn try_admit_mirror_tx_owned(&self, mirror_pending_limit: usize) -> Result<(), ()> { | ||
| let pending_len = self.pending_tx.len(); | ||
| let mut admission_cap = self.pending_tx.capacity(); | ||
| let max_pending = self.max_pending_tx.load(Ordering::Relaxed) as usize; | ||
| if max_pending > 0 { | ||
| admission_cap = admission_cap.min(max_pending); | ||
| } | ||
| if mirror_pending_limit > 0 { | ||
| admission_cap = admission_cap.min(mirror_pending_limit); | ||
| } | ||
| if pending_len >= admission_cap { | ||
| return Err(()); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
| pub(in crate::afxdp) fn admit_mirror_clone_to_live( | ||
| mirror_targets: &MirrorTargetMap, | ||
| mirror_tx_ifindex: i32, | ||
| ingress_queue_id: u32, | ||
| frame_len: usize, | ||
| ) -> Result<Arc<BindingLiveState>, MirrorCloneResult> { | ||
| if frame_len > tx_frame_capacity() { | ||
| return Err(MirrorCloneResult::NoFrame); | ||
| } | ||
| let Some(target_live) = mirror_targets.target_live(mirror_tx_ifindex, ingress_queue_id) | ||
| else { | ||
| return Err(MirrorCloneResult::NoBinding); | ||
| }; | ||
| if target_live | ||
| .try_admit_mirror_tx_owned(MIRROR_PENDING_LIMIT) | ||
| .is_err() | ||
| { |
Addressed in
Fixed in
Verified and addressed in
Adversarial re-review on |
Claude r4 review on
|
Round-4 triple-review synthesis on
|
| Reviewer | Verdict |
|---|---|
| Claude | MERGE-READY |
| Codex | MERGE-NEEDS-MAJOR |
| Gemini Pro 3 | MERGE-READY |
Sample-before-allocate — CLOSED (all three agree)
Codex walked poll_descriptor.rs:263-295: resolve → sample (mirror_sample_allows) → admit → packet_frame.to_vec() happens last. Gemini quoted the same chain. Closed.
Cross-worker isolation — Codex MAJOR
Codex argues the cross-worker preflight is racy AND lacks a TX-frame reserve:
"
try_admit_mirror_tx_owned()is not a reservation.MpscInbox::len()is explicitly approximate/non-linearizable atmpsc_inbox.rs:86-95, and the final push goes throughtry_enqueue_tx_owned()/try_push_redirect_inbox()atumem/mod.rs:1093-1095and1152-1162, which only re-checkmax_pending_tx/hard capacity, notMIRROR_PENDING_LIMIT. Concurrent producers can all pass admission before any one producer publishes the push."
"Cross-worker never checks target
free_tx_frames, and the eventual owner-side TX path still allocates withfree_tx_frames.pop_front()intx/transmit.rs:116-118with no mirror reserve. Cross-worker mirrorTxRequests are also indistinguishable from primary redirect requests once queued."
Gemini counterpoint:
"The same-worker path checks
MIRROR_TX_FRAME_RESERVEbecause it pulls directly from the localfree_tx_framesring at enqueue time. The cross-worker path cannot safely evaluatefree_tx_framesacross threads; instead, it caps heap allocations toMIRROR_PENDING_LIMIT. The egress worker evaluates itsfree_tx_framesreserve upon dequeuing."
My read
Codex's race claim is sharp and substantiated. Whether it rises to MAJOR depends on threat model:
- Heap OOM protection: bound holds (heap can't grow unbounded — TX_BATCH_SIZE caps cross-worker queue per target via
try_push_redirect_inboxhard cap). - Strict mirror-vs-primary isolation: bound does NOT hold under contention. N concurrent producers each see
len < MIRROR_PENDING_LIMIT, all push, queue reachesMIRROR_PENDING_LIMIT + N - 1. Then non-mirror TX waits behind that backlog. - Egress TX reserve: confirmed absent (
tx/transmit.rs:116-118free_tx_frames.pop_front()has no mirror gate). - Observability: both same-worker-full and cross-worker-full route to
mirror_drops_queue_full— operator can't distinguish.
Gemini's argument that "cross-thread reserve isn't possible" understates it — you CAN reserve via an atomic counter on the target binding, just not via direct ring observation. The egress worker could decrement a mirror_frames_in_flight AtomicU32 on transmit and the producer could check mirror_frames_in_flight < limit atomically.
Observability gap — VERIFIED
record_mirror_clone_result() at mirror.rs:299-300 maps all QueueFull cases to one counter. Operator can't tell "my mirror destination worker is backed up" from "my own worker is backed up". Distinct counters would unblock triage.
Recommendation
Block candidates (Codex MAJORs):
- Cross-worker race: add atomic in-flight counter on target binding, or restructure to do reservation+commit pattern.
- Add
mirror_drops_cross_worker_queue_fullcounter distinct from same-worker. - Consider cross-worker mirror TX-frame reserve (decrement on transmit, gate at admit).
Strongly consider: add a concurrent-producer test that asserts MIRROR_PENDING_LIMIT is honored under contention — current test is serial.
Sample-before-allocate is closed; that's a real win this round. Author's call on whether the residual cross-worker gaps rise to block this PR or get their own issue.
Codex task: task-mpaahj6y-407546. Gemini task: task-mpaahy4h-g5h2p8. Not merging — author's decision.
|
@copilot review |
Addressed by
Adversarial re-review on head ( |
| "TX completions: 5", | ||
| "Mirrored packets: 10", | ||
| "Mirrored bytes: 1280", | ||
| "Mirror drops: no-frame=1 tx-frame-reserve=0 no-binding=2 queue-full=3", |
Claude r8 review on
|
Follow-up hardening on
|
Round-8 synthesis on
|
| Reviewer | Verdict |
|---|---|
| Claude | MERGE-READY |
| Codex | MERGE-NEEDS-MINOR (hardening recommendation) |
| Gemini Pro 3 | (still tool-calling at 11m35s — partial result will follow) |
r7 third-strip MAJOR — CLOSED (both Claude + Codex agree)
Codex confirmed:
"Both cited functions now propagate
req.mirror_clone:cross_binding.rs:211andcross_binding.rs:269."
"Tests pin both paths:
redirect_prepared_cos_request_to_owner_preserves_mirror_clone_on_worker_commandconstructs a prepared mirror request and asserts theWorkerCommand::EnqueueShapedLocalTxRequesthasmirror_clone == true.redirect_prepared_cos_request_to_owner_binding_preserves_mirror_clone_on_live_queueasserts the owner-live queuedTxRequesthasmirror_clone == true."
Hostile fourth-site search — clean
Codex independently grepped all TxRequest {} / PreparedTxRequest {} construction sites in userspace-dp/src/afxdp/ at 7a382d10:
"Runtime
mirror_clone: falsesites remain in origin/forwarding paths:coordinator/inject.rs,neighbor_dispatch.rs,poll_descriptor.rs,tunnel.rs,tx/dispatch.rs, andtx/tcp_segmentation.rs. I do not see another prepared/local mirror conversion that strips an existing mirror clone. Mirror producers inmirror.rssettrue; CoS conversion helpers and the two r8 sites now propagate."
Claude grep matched exactly: all mirror_clone: true sites are mirror enqueue creation; all mirror_clone: false outside tests are non-mirror originator paths (forward/inject/tunnel/ICMP/segmentation/redirect/dispatch); the two r8 sites correctly use req.mirror_clone.
Codex MINOR — structural hardening recommendation
"Still explicit-field discipline.
TxRequestandPreparedTxRequestare plain structs with a rawmirror_clone: bool; there is no builder,From, or typed constructor forcing clone identity propagation. The compiler forces the field to be mentioned, but it does not force the correct value."
"This is just the cited sites, plus targeted tests. It is not systematic. Given this is round 3 of the same failure mode, I'd require a minor hardening patch before calling it merge-ready: centralize prepared-to-local cloning into one helper or constructor and make the mirror/origin distinction impossible to casually lose."
Codex's framing is sharp. Three rounds of the same bug pattern (r6 clone_prepared_request_for_cos, r7 CoS drain reserve gates, r8 cross_binding redirect) suggest the bug isn't a one-off — it's a structural risk that recurs whenever a new prepared-to-local conversion is added. A From<&PreparedTxRequest> for TxRequest impl (or PreparedTxRequest::to_local_tx_request() method) that mechanically propagates mirror_clone removes the failure mode entirely.
Recommendation
Merge-ready functionally — the three flagged sites across r6/r7/r8 are all closed. Strongly consider the From-impl / centralized-helper hardening before merge (or as immediate follow-up issue), per Codex's MINOR. This is the third recurrence of the same class.
Gemini Pro 3 task task-mpamcdp4-72kk5y is still tool-calling at 11m35s. Will update synthesis if Gemini surfaces a new finding.
Codex task: task-mpamc0pa-2dz07u. Not merging — author's decision.
Claude r9 review on
|
Round-9 synthesis on
|
| Reviewer | Verdict |
|---|---|
| Claude | MERGE-READY |
| Codex | MERGE-READY (no code findings) |
| Gemini Pro 3 | (still tool-calling at 5m42s — second prolonged session in a row) |
Codex confirmation
"The helpers preserve all identity fields, including
mirror_clone,flow_key, CoS/DSCP, expected metadata, and egress ifindex in tx.rs:29 and tx.rs:97. The four target sites are converted: cross_binding.rs:202, cross_binding.rs:250, cos_classify.rs:456, cos_classify.rs:524."
"I found no remaining manual prepared-to-local or local-to-prepared identity-copy sites. Production
TxRequest { ... }/PreparedTxRequest { ... }constructors outside these helpers are origin builders, not clones from the opposite request type."
"No default-value or field-shadowing trap found. The helpers enumerate fields explicitly, so future field additions fail compile instead of silently defaulting."
"
to_local_request(&self, bytes)is the right shape here because callers still need the original prepared request to recycle or return on failure."
Independent grep verification (Claude)
mirror_clone: field assignments on 82704447 (7-PR Rust files):
mirror.rs:224, 315—mirror_clone: true(mirror enqueue creation, correct)mirror.rs:447, 1264, 1311— test fixturestypes/tx.rs:24, 92— field definitionstypes/tx.rs:45, 107—mirror_clone: self.mirror_clonein the two new helpers
The 4 prior strip-risk sites (cross_binding.rs:211,269 + cos_classify.rs:467,545) no longer have explicit mirror_clone: literals — they use the helpers.
Why this closes the recurrence class
Three rounds of the same bug pattern (r6/r7/r8 mirror_clone strip at three different sites) → r9 makes propagation the default rather than a discipline. Adding a 5th conversion site would now require either:
- Using the helpers (structural propagation) — easy
- Manually listing all 10 fields explicitly — strictly more code than the helper
The economic incentive flips. Code review still matters but the failure mode is no longer "casual oversight" — it's "deliberate inversion of a clearer pattern."
Recommendation
Merge-ready. Four-round mirror PR arc concluded with a structural hardening commit. The three flagged sites across r6/r7/r8 are functionally closed, and r9 prevents the next site from recurring.
Will update if Gemini surfaces a finding after its long tool-call session resolves. Per feedback_gemini_infra_outage_merge_policy: not blocking on Gemini given Claude + Codex have converged with quoted-line evidence.
Codex task: task-mpamv35p-lh6dms. Gemini task: task-mpamvi8p-kn95gq (pending). Not merging — author's decision.
Round-9 final synthesis on
|
| Reviewer | Verdict |
|---|---|
| Claude | MERGE-READY |
| Codex | MERGE-READY |
| Gemini Pro 3 | MERGE-READY |
All three converge with quoted evidence. Updating prior partial synthesis.
Gemini confirmation (with quoted code at 82704447)
"A.
PreparedTxRequest::to_local_request(&self, bytes)— does it propagatemirror_clone? Yes. The helper correctly propagates the field:" (with verbatim quote ofto_local_requestshowingmirror_clone: self.mirror_clone)
"B.
TxRequest::into_prepared_request(self, offset, recycle)— does it propagatemirror_clone? Yes. The field is cleanly mapped:" (verbatim quote)
"C. All 4 callsites converted to use the helpers? Yes. The diff confirms exact application across all four targeting sites. A repository-wide
grepon production code confirms no other cross-conversions were missed."
"D. Any subtle field omission risk in the helpers? No. Because the helpers do not use the
..struct update syntax (e.g...Default::default()), Rust's struct initialization rules mandate that every field is explicitly provided. Any future fields added to eitherTxRequestorPreparedTxRequestwill trigger a hard compiler error inside these helpers until explicitly accounted for. This eliminates the risk of silent field omission across the codebase."
Net
- The three-round bug pattern (r6 strip → r7 strip → r8 strip) is structurally closed by r9's centralized helpers.
- All 4 prior strip-risk sites collapse to helper calls.
- The compiler now enforces field completeness on the conversion boundary — silent omission impossible.
Recommendation
Merge-ready. Four-round port-mirror runtime PR concluded with structural hardening.
Codex task: task-mpamv35p-lh6dms. Gemini task: task-mpamvi8p-kn95gq. Not merging — author's decision.
psaab
force-pushed
the
codex/1403-1376-port-mirror-runtime
branch
from
8270444 to
8cc149d
Compare
Agent-Logs-Url: https://github.com/psaab/xpf/sessions/34d87e52-34a1-4f86-8ac5-7c2de55ed631 Co-authored-by: psaab <196946+psaab@users.noreply.github.com>
Agent-Logs-Url: https://github.com/psaab/xpf/sessions/19717d1e-17c3-4f6a-ad60-a2abb75aaa75 Co-authored-by: psaab <196946+psaab@users.noreply.github.com>
Agent-Logs-Url: https://github.com/psaab/xpf/sessions/f938fef1-f48e-447e-85c5-2f4ca5cdc753 Co-authored-by: psaab <196946+psaab@users.noreply.github.com>
psaab
force-pushed
the
codex/1403-1376-port-mirror-runtime
branch
from
8cc149d to
8a43bad
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/dataplane/userspace/statusfmt_test.go:83
- The status summary output now prints mirror drops with
same-workerandcross-workerfields, but this test only asserts the prefix up throughqueue-full=.... Consider updating the expected substring to includesame-worker=... cross-worker=...so regressions in the newly added fields are caught.
"Bound bindings: 2/2",
"XSK-registered bindings: 1/2",
"Zerocopy bindings: 1/2",
"Shared UMEM bindings: 1/2",
"Armed queues: 0/2",
| - Counters exposed through Rust status, Go protocol, CLI, and Prometheus: | ||
| - Counters exposed through Rust status, Go protocol, and CLI summary: | ||
| `mirrored_packets`, `mirrored_bytes`, `mirror_drops_no_frame`, | ||
| `mirror_drops_no_binding`, and `mirror_drops_queue_full`. |
Adds bounded port mirror runtime slice (#1408) in userspace. - Fix port mirror runtime forwarding paths - userspace-dp: fix mirror logical output bind resolution - userspace-dp: harden mirror admission isolation - userspace-dp: sample-first mirror and isolate cross-worker backlog - userspace-dp: reserve cross-worker mirror inbox slots - userspace-dp: preserve tx frame reserve for mirrors Touches userspace-dp/src/afxdp, userspace-dp/src/protocol.rs, pkg/dataplane/userspace, and docs/pr/1373-retire-ebpf-dataplane across documentation, tests, userspace dataplane, and dataplane integration. The largest file deltas are userspace-dp/src/afxdp/mirror.rs, userspace-dp/src/afxdp/umem/mod.rs, and userspace-dp/src/afxdp/worker/mod.rs. The diff is 2622 additions and 86 deletions across 46 files.
Summary
Refs #1376.
Adds a bounded forwarded-path port-mirror runtime slice for the userspace dataplane:
ForwardingStateThis intentionally leaves the userspace capability gate in place. Full #1376 closure still needs ingress/transmit-path parity beyond pending-forward TX, prebuilt/deferred path coverage, tcpdump validation on the mirror output, and pressure evidence proving mirror load does not hurt primary forwarding.
Validation
go test ./pkg/dataplane/userspacecargo test --manifest-path userspace-dp/Cargo.toml mirror::cargo test --manifest-path userspace-dp/Cargo.toml binding_counters_snapshot_serializes_with_expected_wire_keyscargo test --manifest-path userspace-dp/Cargo.toml config_snapshot_mirror_configs_roundtripcargo test --manifest-path userspace-dp/Cargo.toml mirror_counters_binding_status_wire_roundtrip_and_projectiongit diff --check