Merged
Conversation
Fredi-raspall
force-pushed
the
pr/fredi/dataplane-state-cli
branch
from
90bc3f1 to
b0efe04
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Extends the dataplane troubleshooting CLI (Issue #1317) by plumbing additional runtime/config state into the routing CLI handler and adding display/providers for flow/NAT/filter data, while also consolidating common CLI formatting/provider utilities into a new shared crate.
Changes:
- Add new
show ...commands/actions for VPC summary/peerings, gateway groups/communities, flow-table, flow-filter, NAT (static/port-forwarding/masquerade), and config summary. - Introduce
commoncrate withCliDataProvider+ sharedHeading/Frame/lineformatting, and migrate existing per-crate helpers to it. - Track and publish applied
GwConfig+ config history from mgmt to routing over the router control channel for CLI rendering.
Reviewed changes
Copilot reviewed 48 out of 50 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| routing/src/router/rio.rs | Store latest GwConfig + config history in RIO; pass CliSources into CLI request handling. |
| routing/src/router/mod.rs | Add CliSources and thread it through Router::new → start_rio. |
| routing/src/router/ctl.rs | Add new control messages + sender helpers to deliver config and history into routing. |
| routing/src/rib/vrftable.rs | Switch tests to shared Frame helper. |
| routing/src/rib/vrf.rs | Switch tests to shared Frame helper. |
| routing/src/lib.rs | Export CliSources; stop exporting old CLI formatting types. |
| routing/src/frr/test.rs | Update router constructor call signature. |
| routing/src/cli/pretty_utils.rs | Remove local formatting helpers (migrated to common). |
| routing/src/cli/mod.rs | Remove pretty_utils module from routing CLI. |
| routing/src/cli/handler.rs | Add handlers for new CLI actions; render from GwConfig, history, and external providers. |
| routing/src/cli/display.rs | Switch to shared Heading/line formatting. |
| routing/Cargo.toml | Add dependency on common crate. |
| nat/src/stateless/setup/display.rs | Use shared Heading; implement CliDataProvider for NatTables. |
| nat/src/stateless/natrw.rs | Expose inner() read handle for CLI provider plumbing. |
| nat/src/stateful/apalloc/display.rs | Use shared Heading; implement CliDataProvider for masquerade allocator. |
| nat/src/stateful/allocator_writer.rs | Expose inner() for CLI provider plumbing. |
| nat/src/portfw/portfwtable/display.rs | Use shared Heading; implement CliDataProvider for port-forwarding table. |
| nat/src/portfw/portfwtable/access.rs | Expose inner() read handle for CLI provider plumbing. |
| nat/src/portfw/flow_state.rs | Reduce log noise when flow expiry extension is not applied. |
| nat/Cargo.toml | Add dependency on common crate. |
| mgmt/src/tests/mgmt.rs | Update VPC display usage + router constructor signature. |
| mgmt/src/processor/proc.rs | Rework config apply/rollback + history updates; send config + history to routing control channel. |
| mgmt/src/processor/mod.rs | Remove old mgmt display module. |
| mgmt/src/processor/mgmt_client.rs | Change mgmt client API to always return Arc<GwConfig>/GenId (blank default). |
| mgmt/src/processor/k8s_less_client.rs | Adjust generation logic for new “always present” config/genid behavior. |
| mgmt/src/processor/k8s_client.rs | Adjust status building to read apply time from ArcSwap meta. |
| mgmt/src/processor/gwconfigdb.rs | Store applied config as Arc<GwConfig> (blank default) and centralize history logging. |
| mgmt/src/processor/display.rs | Remove old config history display (moved to config crate display). |
| mgmt/src/processor/confbuild/router.rs | Accept Arc<GwConfig> in router config generation. |
| mgmt/src/processor/confbuild/internal.rs | Remove warning for configs without BGP (behavioral tweak). |
| flow-filter/src/tables.rs | Make internal tables accessible to display module; move Display impls out. |
| flow-filter/src/lib.rs | Add display module. |
| flow-filter/src/filter_rw.rs | Expose inner() read handle for CLI provider plumbing. |
| flow-filter/src/display.rs | New: Display + CliDataProvider for flow-filter table dumps. |
| flow-filter/Cargo.toml | Add dependency on common crate. |
| flow-entry/src/flow_table/display.rs | Use shared Heading; implement CliDataProvider for flow-table dumps. |
| flow-entry/Cargo.toml | Add dependency on common crate. |
| dataplane/src/packet_processor/mod.rs | Wire real dataplane state into routing CLI via CliSources. |
| config/src/lib.rs | Re-export ConfigSummary. |
| config/src/gwconfig.rs | Store GwConfigMeta in ArcSwap; add rollback flag. |
| config/src/external/overlay/tests.rs | Update to new VPC summary/detailed display helpers. |
| config/src/display.rs | Add new summary/peerings views and config history summary display. |
| config/Cargo.toml | Add common and arc-swap dependencies; adjust chrono features. |
| common/src/lib.rs | New crate root for shared utilities. |
| common/src/cliprovider.rs | New: shared CLI provider trait + formatting helpers. |
| common/Cargo.toml | New workspace crate for shared CLI utilities. |
| cli/src/cliproto.rs | Add new CLI actions for troubleshooting. |
| cli/bin/cmdtree_dp.rs | Add new show ... command tree nodes mapping to new actions. |
| Cargo.toml | Add common to workspace members and workspace deps. |
| Cargo.lock | Lockfile updates for new crate and dependency versions. |
You can also share your feedback on Copilot code review. Take the survey.
Fredi-raspall
force-pushed
the
pr/fredi/dataplane-state-cli
branch
from
b0efe04 to
3f8c686
Compare
This crate is meant to include miscellaneous types or traits that other traits may refer to, with the goal of avoiding circular dependencies. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Tidy up start_router() and reorder declarations as this will be needed to pass extra info to the router so that it is accessible from the cli handler. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The trait will be implemented by types capable of providing data to the CLI. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Fredi-raspall
force-pushed
the
pr/fredi/dataplane-state-cli
branch
from
a86252b to
5005e47
Compare
qmonnet
requested changes
Mar 16, 2026
Member
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Great to have these new commands!! Thanks for this.
Some high-level comments:
-
Unless you plan to add more code to the
commoncrate, I'd rename it for now, because its contents are specific to the CLI. Maybecli_commonorcli_display? -
Regarding the new
ArcSwap-based rollback system: in some upcoming work I'd like to do some processing of theVpcExposeat validation time, and store it somewhere as part of the internal config to avoid doing it again at NAT set-up time. I don't expect it to be an issue after your changes, right? Just checking to be sure. -
Please let's try to be accurate with regards to commit title prefixes, something like “clean up X” or “move Y to Z” should probably use
refactor(...):rather thanfeat(...).
I also have some minor comments or questions inline below.
Contributor
Author
The intent is to put miscellaneous stuff there, not just cli stuff. Hence the "common" name. I'd not rename it because of this and we should probably remember that the "common" crate is just for that: store/declare things that are needed by multiple crates, without the risk of introducing circular dependencies.
I don't think that'd be an issue. The ArcSwap affects only the metadata and the config is Arc'ed once the internal config has been built, which happens only if validation was successful. So, you still have a chance to mutate it if that's the question.
Ok! |
Adds new commands to the cli and remove some that were never implemented in the end. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The CliSources contains type-erased CliDataProviders for the Cli, currently living in the routing crate, to have access to them. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Implement CliDataProvider trait for some NFs and add method inner() to provide a ReadHandle<T> from the left-right wrapper types we use. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
This commit is a small refactor of the configuration database that simplifies the code and: - avoids &mut when possible - removes unnecessary methods - uses Arc<> for the GW config to avoid expensive clones and further simplifies the code. - registers rollbacks in the configuration history - uses arcswap to keep the configuration metadata These changes simplify the code and make it easier to export the configuration. The use of arcswap is motivated by the fact that, once a config is applied, we want to make it immutable. However, the metadata should be allowed to change. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Clean-up and simplify existing code to deal with the history of configuration changes and augment the cli handler to display them. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The current flow timer implementation relies on AtomicInstant, which cannot go backwards. Do not show errors if we can't shrink a flow lifetime. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Simplify the mgmt code given that there is always an applied configuration (even if blank). Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Move the declaration of tracing target "port-frwarding" to the modules' root so that it governs all port-forwarding logs. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The types represent the values that the user entered in the API. So, if nothing is specified, None should be the value. We could set defaults in the config, but the default values should come from the NF implementing the functionality. Since we don't want to add circular dependencies, let's make those optional and let the NF set the default as appropriate. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
The port-forwarding uses two timeouts, idle and established, while the API currently only exposes one (idle). Map the API idle timeout to the port-forwarding established timeout, which is the one that users may care the most. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Fredi-raspall
force-pushed
the
pr/fredi/dataplane-state-cli
branch
from
5005e47 to
8ad4837
Compare
qmonnet
requested changes
Mar 17, 2026
Member
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
The changes look good, and I have no newer comment. Please make sure to address or reply to all comments from the first review 😕
Remove some logs that are not anymore needed now that the state can be inspected with the cli, as well as logs that are produced periodically and add little information. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Add custom tracing targets to, by default, disable debug logs from several 3rd party crates that clutter the logs. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
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: #1317
@pau-hedgehog @edipascale you may find these commands useful for troubleshooting.
SAMPLES