Paella of sea bugs with jamon iberico, cinnamon, maple syrup and peanut butter.#1607
Open
Fredi-raspall wants to merge 19 commits into
Open
Paella of sea bugs with jamon iberico, cinnamon, maple syrup and peanut butter.#1607Fredi-raspall wants to merge 19 commits into
Fredi-raspall wants to merge 19 commits into
Conversation
Fredi-raspall
requested review from
Copilot,
qmonnet and
sergeymatov
and removed request for
a team and
sergeymatov
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates Unix signal handling, adds an rtnetlink-backed interface monitor, wires interface events through management and routing, adds packet serialization error variants, renames a masquerade helper, and parameterizes Dockerfile artifact paths by platform. ChangesInterface Event Pipeline
Packet Serialization Errors
Masquerade Helper Rename
Dockerfile Platform Build Arg
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves dataplane operational robustness by (1) restructuring signal handling to avoid unintended process termination and enable CLI socket rebinding, (2) integrating the router control channel into the mio poll loop via a Waker, and (3) adding netlink-based interface event monitoring and router event logging. It also extends packet outcome accounting to include post-processing serialization failures and adjusts build/container ergonomics.
Changes:
- Router I/O loop: integrate control-channel processing into
mio::PollviaWaker, add CLI socket restore control message, and harden loop-exit detection. - Interface events: add an
interface-managernetlink monitor, relay events into the router control path, and log/admin+oper state transitions as router events. - Packet stats: introduce
SerializeErrorand newDoneReasonvalues to account “no headroom” / “deparse error” outcomes; minor Dockerfile and kernel worker tweaks.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| routing/src/router/rio.rs | Adds mio::Waker integration for ctl channel; adds CLI socket restore; refactors exit guard. |
| routing/src/router/ctl.rs | Adds waker-backed RouterCtlSender, new ctl messages (RebindCli, IfEvent), drains ctl channel on wake. |
| routing/src/router/revent.rs | Adds router events for interface admin/oper transitions with formatted display. |
| routing/src/interfaces/interface.rs | Removes per-set logging for admin/oper state updates (now logged via router events). |
| routing/src/interfaces/iftablerw.rs | Enables writer methods for interface state updates (removes unused allow). |
| routing/src/interfaces/iftable.rs | Narrows visibility of state setters to pub(super). |
| routing/src/cli/display.rs | Updates IfState display to use formatter padding behavior. |
| routing/Cargo.toml | Adds interface-manager dependency. |
| net/src/packet/mod.rs | Adds SerializeError, splits serialize logic, sets DoneReason on serialization failure. |
| net/src/packet/meta.rs | Adds DoneReason::DeparseError and DoneReason::NoHeadRoom. |
| net/src/packet/stats_display.rs | Adds display strings for new DoneReason values. |
| nat/src/masquerade/nf.rs | Minor doc/comment cleanup and internal helper rename. |
| mgmt/src/processor/proc.rs | Changes ConfigProcessor::new to take a runtime handle; spawns rtnetlink connection. |
| mgmt/src/processor/launch.rs | Starts interface monitor + relay task; passes router control sender; plumbs config client. |
| mgmt/src/tests/mgmt.rs | Updates test to pass tokio runtime handle to ConfigProcessor::new. |
| lifecycle/src/lib.rs | Splits signal handling into catcher (relays signals) for broader signal coverage. |
| interface-manager/src/monitor/mod.rs | Introduces netlink-based interface monitor and broadcast distribution; adds (ignored) test. |
| interface-manager/src/lib.rs | Exposes new monitor module. |
| interface-manager/Cargo.toml | Adds dependencies needed for monitor + tests. |
| dataplane/src/runtime.rs | Adds main signal-processing task (acts on SIGUSR1 to rebind CLI); uses new catcher API. |
| dataplane/src/drivers/kernel/worker.rs | Increases RX buffer size; promotes serialize failure log to warn. |
| Dockerfile | Copies binaries from target triple subdir (target/${PLATFORM}/${PROFILE}). |
| Cargo.lock | Updates lockfile for new dependencies. |
Comment on lines
+125
to
+129
| // create netlink socket and spawn background task | ||
| let Ok((connection, netlink, _)) = rtnetlink::new_connection() else { | ||
| panic!("failed to create connection"); | ||
| }; | ||
| spawn(connection); | ||
| handle.spawn(connection); |
Comment on lines
+210
to
+214
| #[wrap(with_caps([Capability::CAP_NET_ADMIN]))] | ||
| #[n_vm::in_vm] | ||
| #[cfg_attr(not(emulated), traced_test)] | ||
| #[ignore = "could not make this test to run"] | ||
| async fn test_interface_monitor() { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
net/src/packet/mod.rs (1)
65-73: 📐 Maintainability & Code Quality | 🔵 TrivialConsider adding
#[from]toDeparseErrorfor automatic error conversionAdding
#[from]to theDeparseErrorvariant allows the?operator to automatically convertDeParseError<E>intoSerializeError<E>, eliminating the need for explicit.map_err(SerializeError::DeparseError)calls indo_serialize.The
thiserrorcrate automatically implementsError::source()when#[from]is used. EnsureDeParseError<E>implementsstd::error::Error(a requirement for#[from]); if it is a standardthiserrorenum, this likely holds true.♻️ Proposed change
#[error("De-parsing error: {0}")] - DeparseError(DeParseError<E>), + DeparseError(#[from] DeParseError<E>),Then simplify the error handling in
do_serialize:- self.headers.deparse(buf).map_err(SerializeError::DeparseError)?; + self.headers.deparse(buf)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/src/packet/mod.rs` around lines 65 - 73, The SerializeError enum currently wraps DeParseError(E) without automatic conversion, forcing manual map_err handling in do_serialize. Update the DeparseError variant in SerializeError<E> to use #[from] and confirm DeParseError<E> implements std::error::Error so thiserror can derive source() automatically. Then simplify do_serialize to rely on the ? operator for converting DeParseError<E> into SerializeError<E> instead of explicit mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Around line 11-15: The Docker build path now depends on PLATFORM, but the
build invocation only forwards PROFILE, so Docker defaults to the wrong target
and can miss the built artifacts. Update the docker build wiring in the justfile
to pass PLATFORM through alongside PROFILE, and ensure the Dockerfile COPY
commands continue to consume the same PLATFORM variable used by the workflow so
quick builds and CI resolve the same target.
In `@interface-manager/src/monitor/mod.rs`:
- Around line 95-114: The `netlink_to_event` path is incorrectly treating
`carrierup` and `carrierdown` as required, causing link-state events to be
dropped when those display-only counters are missing. Update the parsing in
`netlink_to_event` so missing `LinkAttribute::CarrierUpCount` and
`LinkAttribute::CarrierDownCount` do not short-circuit the function; instead,
make them optional or default them while still constructing the `EthEvent` from
`ifindex`, `ifup`, `iflowerup`, `ifrunning`, and `carrier`.
In `@mgmt/src/processor/launch.rs`:
- Around line 116-128: Startup link events can be dropped because
InterfaceMonitor begins emitting before the router knows the interface, and
interface_event_notify/router control currently ignores events for unknown
ifindex values. Fix this by ensuring the startup path either applies an initial
interface snapshot/replay before subscribing to deltas, or delays starting
InterfaceMonitor/interface_event_notify until after ConfigProcessor has
initialized routing state; use the existing InterfaceMonitor,
interface_event_notify, and ConfigProcessor::new/startup sequence to keep early
link events from being lost.
- Around line 89-100: The interface relay loop in interface_event_notify
currently ignores the broadcast receiver closing state, causing an endless retry
spin after rx.recv().await stops yielding events. Update the loop to explicitly
handle the closed result from rx.recv() and break out of the task when the
channel is closed, while keeping the existing send_ifevent relay behavior for
valid EthEvent messages.
In `@net/src/packet/mod.rs`:
- Around line 335-340: Update the doc comments on `Packet::deparse` and
`Packet::serialize` so the `# Errors` section refers to the actual return type,
`SerializeError`, instead of the non-existent `PacketError` variants. Keep the
wording aligned with the existing method names and error cases in
`net/src/packet/mod.rs`, replacing `PacketError::NoHeadRoom` and
`PacketError::DeparseError` with the correct `SerializeError` terminology
throughout both doc blocks.
In `@routing/src/cli/display.rs`:
- Around line 480-485: The IfState Display formatting now emits variable-length
text, which breaks alignment in the CLI tables that print it from the table
rendering code in display.rs. Fix this by either restoring fixed-width padding
in IfState::fmt so it always returns a consistently padded value, or by updating
every IfState table call site in the display formatting paths to apply an
explicit width/column constraint so the output remains aligned.
In `@routing/src/router/ctl.rs`:
- Around line 38-48: LockGuard::drop currently uses ambient tokio::task::spawn,
which can panic if the guard is dropped outside an active Tokio runtime. Update
LockGuard to store a tokio::runtime::Handle when it is created, then replace the
spawn call in drop with handle.spawn(...) so the unlock task is scheduled safely
regardless of runtime context. Ensure the handle is threaded through the
LockGuard constructor and any call sites that build it.
In `@routing/src/router/rio.rs`:
- Around line 215-228: The cli_sock_restore flow in rio::Router currently
deregisters and shuts down the active CLI socket before opening the replacement,
which can leave the router without a usable socket if open_cli_sock fails.
Change the sequence so the new socket is created first, then only call
deregister and shutdown on the old self.clisock after the new socket opens
successfully, and finally assign the new socket and register it through
register(CLISOCK, ...).
- Around line 510-516: The control-channel drain in rio’s poll loop is too
tightly coupled to the CTL_CHANNEL token, which can leave queued messages
unprocessed if a wake is missed after send_and_wake succeeds. Move the
handle_ctl_msg call in router::rio::Rio’s event loop so it runs on every poll
iteration, not only inside the CTL_CHANNEL match arm, while keeping its
try_recv-based draining behavior unchanged. Use the existing handle_ctl_msg and
send_and_wake flow as the main reference points when updating the loop.
---
Nitpick comments:
In `@net/src/packet/mod.rs`:
- Around line 65-73: The SerializeError enum currently wraps DeParseError(E)
without automatic conversion, forcing manual map_err handling in do_serialize.
Update the DeparseError variant in SerializeError<E> to use #[from] and confirm
DeParseError<E> implements std::error::Error so thiserror can derive source()
automatically. Then simplify do_serialize to rely on the ? operator for
converting DeParseError<E> into SerializeError<E> instead of explicit mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 193b3e81-e417-4008-9911-61214a56e5c0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
Dockerfiledataplane/src/drivers/kernel/worker.rsdataplane/src/runtime.rsinterface-manager/Cargo.tomlinterface-manager/src/lib.rsinterface-manager/src/monitor/mod.rslifecycle/src/lib.rsmgmt/src/processor/launch.rsmgmt/src/processor/proc.rsmgmt/src/tests/mgmt.rsnat/src/masquerade/nf.rsnet/src/packet/meta.rsnet/src/packet/mod.rsnet/src/packet/stats_display.rsrouting/Cargo.tomlrouting/src/cli/display.rsrouting/src/interfaces/iftable.rsrouting/src/interfaces/iftablerw.rsrouting/src/interfaces/interface.rsrouting/src/router/ctl.rsrouting/src/router/revent.rsrouting/src/router/rio.rs
💤 Files with no reviewable changes (1)
- routing/src/interfaces/iftablerw.rs
Comment on lines
+480
to
+485
| let s = match *self { | ||
| IfState::Unknown => "unknown", | ||
| IfState::Up => "up", | ||
| IfState::Down => "down", | ||
| }; | ||
| f.pad(s) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify IfState display call sites use explicit width where table alignment matters.
rg -n 'admin_state|oper_state|IfState' routing/src/cli/display.rs routing/src -g '*.rs' -C 2Repository: githedgehog/dataplane
Length of output: 20228
Refactor IfState Display to preserve fixed-width column alignment in CLI tables.
The Display implementation for IfState currently outputs variable-length strings ("unknown", "up", "down"). Call sites in routing/src/cli/display.rs (lines 534–535, 574) insert these values into tabular output without enforcing explicit column widths. This change removes the previous implicit fixed-width padding, causing column misalignment in CLI tables.
To resolve:
- Either revert
IfState::fmtto return a consistently padded string (e.g., 9 characters), or - Update all table-formating call sites to specify explicit width constraints (e.g.,
{:9}or configure the table formatter to fix column width).
routing/src/cli/display.rs
routing/src/cli/display.rs
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@routing/src/cli/display.rs` around lines 480 - 485, The IfState Display
formatting now emits variable-length text, which breaks alignment in the CLI
tables that print it from the table rendering code in display.rs. Fix this by
either restoring fixed-width padding in IfState::fmt so it always returns a
consistently padded value, or by updating every IfState table call site in the
display formatting paths to apply an explicit width/column constraint so the
output remains aligned.
Comment on lines
38
to
48
| impl Drop for LockGuard { | ||
| fn drop(&mut self) { | ||
| let tx = self.0.take(); | ||
| if let Some(tx) = tx { | ||
| if let Some(tx) = self.tx.take() { | ||
| let waker = Arc::clone(&self.waker); | ||
| task::spawn(async move { | ||
| if let Err(e) = tx.send(RouterCtlMsg::GuardedUnlock).await { | ||
| error!("Fatal: could not send unlock request!!: {e}"); | ||
| } else { | ||
| let _ = waker.wake(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n routing/src/router/ctl.rs | head -100Repository: githedgehog/dataplane
Length of output: 3781
🏁 Script executed:
rg -A 5 -B 5 "LockGuard" routing/src/ | head -80Repository: githedgehog/dataplane
Length of output: 2393
Avoid ambient tokio::task::spawn in Drop implementation.
LockGuard::drop invokes tokio::task::spawn without capturing a Runtime handle. If LockGuard is dropped outside an active Tokio runtime context (e.g., in synchronous code or during process shutdown), spawn will panic.
Store a tokio::runtime::Handle within LockGuard at creation and use handle.spawn(...) to ensure the task is spawned safely regardless of the caller's runtime context.
Current unsafe implementation
impl Drop for LockGuard {
fn drop(&mut self) {
if let Some(tx) = self.tx.take() {
let waker = Arc::clone(&self.waker);
task::spawn(async move {
// ...
});
}
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@routing/src/router/ctl.rs` around lines 38 - 48, LockGuard::drop currently
uses ambient tokio::task::spawn, which can panic if the guard is dropped outside
an active Tokio runtime. Update LockGuard to store a tokio::runtime::Handle when
it is created, then replace the spawn call in drop with handle.spawn(...) so the
unlock task is scheduled safely regardless of runtime context. Ensure the handle
is threaded through the LockGuard constructor and any call sites that build it.
qmonnet
approved these changes
Jun 26, 2026
qmonnet
left a comment
qmonnet
left a comment
Member
There was a problem hiding this comment.
Some small comments and suggestions below, but not blocking - PR looks good from my side 🥘. Thanks!
However:
- You have one commit missing a sign-off
- AI have raised some actual concerns, it seems, please address where necessary
- The “concurrency” job failure looks like something you need to address, too
- (The Miri job on powerpc64 is currently broken though, not related to this PR.)
Fredi-raspall
force-pushed
the
pr/fredi/misc
branch
3 times, most recently
from
290325c to
d136d7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@concurrency/src/sync/loom_backend.rs`:
- Line 49: Remove the redundant manual Receiver implementation for Arc<T> in
loom_backend so the blanket core Receiver impl via Deref is used instead. Delete
the explicit impl on Arc<T> in the sync code, and remove Receiver from the
core::ops import since it is no longer needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61415c00-0a56-41e6-bddc-932682a9f294
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Dockerfileconcurrency/src/sync/loom_backend.rsdataplane/src/drivers/kernel/worker.rsdataplane/src/runtime.rsinterface-manager/Cargo.tomlinterface-manager/src/lib.rsinterface-manager/src/monitor/mod.rslifecycle/src/lib.rsmgmt/src/processor/launch.rsmgmt/src/processor/proc.rsmgmt/src/tests/mgmt.rsnat/src/masquerade/nf.rsnet/src/packet/meta.rsnet/src/packet/mod.rsnet/src/packet/stats_display.rsrouting/Cargo.tomlrouting/src/cli/display.rsrouting/src/interfaces/iftable.rsrouting/src/interfaces/iftablerw.rsrouting/src/interfaces/interface.rsrouting/src/router/ctl.rsrouting/src/router/revent.rsrouting/src/router/rio.rs
💤 Files with no reviewable changes (1)
- routing/src/interfaces/iftablerw.rs
✅ Files skipped from review due to trivial changes (3)
- interface-manager/src/lib.rs
- net/src/packet/stats_display.rs
- mgmt/src/tests/mgmt.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- routing/Cargo.toml
- routing/src/interfaces/iftable.rs
- routing/src/router/revent.rs
- Dockerfile
- interface-manager/Cargo.toml
- dataplane/src/drivers/kernel/worker.rs
- net/src/packet/meta.rs
- lifecycle/src/lib.rs
- mgmt/src/processor/launch.rs
- routing/src/interfaces/interface.rs
- routing/src/cli/display.rs
- net/src/packet/mod.rs
- mgmt/src/processor/proc.rs
- nat/src/masquerade/nf.rs
- dataplane/src/runtime.rs
- interface-manager/src/monitor/mod.rs
- routing/src/router/rio.rs
Fredi-raspall
force-pushed
the
pr/fredi/misc
branch
from
d136d7a to
deb0530
Compare
| match sig { | ||
| DpSignal::SIGTERM | DpSignal::SIGINT | DpSignal::SIGQUIT => root.cancel(), | ||
| DpSignal::SIGUSR1 | DpSignal::SIGUSR2 | DpSignal::SIGHUP | DpSignal::SIGALRM | DpSignal::SIGPIPE => {} | ||
| DpSignal::SIGUSR2 | DpSignal::SIGHUP | DpSignal::SIGALRM | DpSignal::SIGPIPE => {}, |
Collaborator
There was a problem hiding this comment.
why not use inotify to monitor the path? And maybe have a timer which reconnects if it finds the socket missing. This seems an odd choice for a k8s product
Contributor
Author
There was a problem hiding this comment.
That's a good idea. I can give it a try.
| }; | ||
|
|
||
| // we allow veths for testing | ||
| if info.is_some() && !is_veth && phy_only { |
Collaborator
There was a problem hiding this comment.
I think we have other means of determining if an interface is physical.
More, I don't understand exactly what "physical" means in many cases, especially regarding sr-iov and virtio. We may want to rethink this
Contributor
Author
There was a problem hiding this comment.
I meant not vrf or not bridge or not vlan (so no "virtual" in the generic sense or not a sub-interface). The net device representing a nic (as close as possible) so that the carrier indication could reliably indicate if it was operational.
| } | ||
|
|
||
| /// Convert a netlink message to an `EthEvent` | ||
| fn netlink_to_event(msg: NetlinkMessage<RouteNetlinkMessage>, phy_only: bool) -> Option<EthEvent> { |
Collaborator
There was a problem hiding this comment.
shouldn't this should be a method on InterfaceMonitor?
Contributor
Author
There was a problem hiding this comment.
I guess you meant associated function?
| #[wrap(with_caps([Capability::CAP_NET_ADMIN]))] | ||
| #[n_vm::in_vm] | ||
| #[cfg_attr(not(emulated), traced_test)] | ||
| #[ignore = "could not make this test to run"] |
Collaborator
There was a problem hiding this comment.
???
This needs work
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The rebinding should happen when processing a message over the control channel. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Split the current signal handler in two parts: a signal catcher and a signal handler. The former is generic and relays signals to the handler, which is dataplane-specific. This change is needed to avoid a circular dependency that would occur because the existing handler is defined in lifecycle. An alternative would be to move the prior signal handler to dataplane, but opted to keep the lifecycle crate complete with that piece. The signal catcher just registers signal listeners and relays them to dataplane, which does the actual handling. The prior handler is extended with listeners for other signals which would otherwise terminate dataplane. Dataplane can now be stopped with: SIGINT, SIGTERM, SIGQUIT. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Cause the cli to rebind to the configured path. This allows ensuring the availability of the CLI in case the filesystem path is accidentally removed (e.g. by starting another dataplane). Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
When sending a packet once processed, its buffer needs to be updated according to the changes made in its headers. This can fail when "de-parsing" the headers or in case there is no headroom in the buffer. Avoid unreachable! on a fallible method (which should not otherwise fail) and mark the packet in case of failure so that if those problems happen, they are not silently overlooked at. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Adds a small netlink listener that produces events upon parsing netlink messages sent by the kernel and can broadcast them to multiple subscribers. This will be used to monitor flapping interfaces. This code will require small adjustments when the new interface model lands. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Instead of creating the ConfigProcessor explicitly in both config drivers, create and run it before branching. This makes it clearer that the distinct config entry points just require a config client to drive the processor. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Start the interface monitor in the management subsystem and relay interface events to the router. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Handle interface events by updating the state of the interfaces as seen by the routing crate and logging any change. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The prior code logged interface status changes in methods that update the interface objects in the interface table. This is problematic since the interface table is implemented using left-right. As a result those methods are called twice (one for each copy of the interface table), causing logs to be duplicated and appear at seemingly random times. Fix this by moving the logic outside the reach of the absorb trait. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The control channel was never integrated in the router IO poller. As a result, control messages were serviced, in the worst case, every timeout interval of the poller. This is problematic now that the router also gets interface events (which may be more frequent than the prior type of messages). Solve this by waking up the poller anytime a message is queued towards the router with the aid of a Waker. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Some smart nics are known to accept frames larger than the standard jumbo size. Enlarge the reception buffer in the kernel driver to accommodate for that. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Fredi-raspall
force-pushed
the
pr/fredi/misc
branch
from
deb0530 to
4a569e6
Compare
Contributor
Author
|
Changes since prior version:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@interface-manager/src/monitor/mod.rs`:
- Around line 183-188: Remove the duplicate tracing-test macro on the test in
mod.rs: the function currently has both the unconditional #[traced_test] and the
#[cfg_attr(not(emulated), traced_test)] variant. Keep only one of them so the
test is not wrapped twice; adjust the attributes on the affected test function
to use either the unconditional form or the cfg_attr form, but not both.
In `@routing/src/router/rio.rs`:
- Around line 116-121: The CLI socket path handling in setup_clipath_watcher
currently assumes Path::parent will exist, so relative paths like cli.sock fail
later instead of being handled cleanly. Update setup_clipath_watcher in
router::rio to either treat a missing parent as the current directory (using "."
as the watch target) or enforce absolute CLI socket paths earlier during config
parsing before this watcher is called. Keep the fix centered on the
setup_clipath_watcher path resolution logic and the config/CLI path validation
flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 534b238f-887d-441f-90b2-11bf0a378000
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
Dockerfiledataplane/src/drivers/kernel/worker.rsdataplane/src/runtime.rsinterface-manager/Cargo.tomlinterface-manager/src/lib.rsinterface-manager/src/monitor/mod.rslifecycle/src/lib.rsmgmt/src/processor/launch.rsmgmt/src/processor/proc.rsmgmt/src/tests/mgmt.rsnat/src/masquerade/nf.rsnet/src/packet/meta.rsnet/src/packet/mod.rsnet/src/packet/stats_display.rsrouting/Cargo.tomlrouting/src/cli/display.rsrouting/src/interfaces/iftable.rsrouting/src/interfaces/iftablerw.rsrouting/src/interfaces/interface.rsrouting/src/router/ctl.rsrouting/src/router/revent.rsrouting/src/router/rio.rs
💤 Files with no reviewable changes (1)
- routing/src/interfaces/iftablerw.rs
✅ Files skipped from review due to trivial changes (4)
- mgmt/src/tests/mgmt.rs
- net/src/packet/stats_display.rs
- routing/src/cli/display.rs
- nat/src/masquerade/nf.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- interface-manager/src/lib.rs
- routing/src/interfaces/iftable.rs
- routing/src/router/revent.rs
- net/src/packet/meta.rs
- interface-manager/Cargo.toml
- dataplane/src/drivers/kernel/worker.rs
- mgmt/src/processor/proc.rs
- Dockerfile
- routing/src/interfaces/interface.rs
- net/src/packet/mod.rs
- lifecycle/src/lib.rs
- routing/src/router/ctl.rs
- mgmt/src/processor/launch.rs
Add an inotify watcher to the directory containing the file path for the CLI Unix socket and rebind the socket in case the file path is removed. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Fredi-raspall
force-pushed
the
pr/fredi/misc
branch
from
4a569e6 to
980ed11
Compare
Rewrite interface monitor so that arbitrary_self_types is not needed, as that feature seems unstable. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Fredi-raspall
force-pushed
the
pr/fredi/misc
branch
2 times, most recently
from
6253738 to
2f09e5c
Compare
Polish logs, remove duped methods. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Fredi-raspall
force-pushed
the
pr/fredi/misc
branch
from
2f09e5c to
2a92ba8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes several small things experienced on deployments:
(by sending SIGUSR1 to dataplane)in case the corresponding file system entry is accidentally removed.