[Nexthop][fboss2-dev] Add config and delete interface sflow sample-dest by vybhav-nexthop · Pull Request #1522 · facebook/fboss

vybhav-nexthop · 2026-08-17T07:30:00Z

Pre-submission checklist

I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running pip install -r requirements-dev.txt && pre-commit install
pre-commit run

Summary

Background: what sFlow sampling is and what these three attributes control

A switch forwards far more traffic than it could ever copy for analysis, so sFlow samples instead: the ASIC picks one in N packets on a port and exports just those. Statistically that is enough to see top talkers, flow mix, and anomalies without touching line-rate forwarding.

Three Port fields control this:

sFlowIngressRate / sFlowEgressRate: the sampling rate in each direction. Every 1/N packets is sampled; 0 disables sampling in that direction.
sampleDest: where each sampled copy goes once sampling is enabled, trading CPU cost against flexibility:
- cpu: the ASIC punts the sample to the switch CPU, and the agent wraps it into sFlow datagrams for the configured collectors. Flexible, but each sample costs CPU and punt-path bandwidth.
- mirror: the ASIC forwards the sample out the port's ingress mirror session straight to a remote collector box, encapsulated, with no CPU involvement. Line rate, but the collector does all the work, and it only exists for ingress sampling — mirror is invalid while sFlowEgressRate is nonzero.

What: Adds fboss2-dev config interface <ports> sflow <attr> <value> [<attr> <value> ...] and its delete counterpart, for three attributes. Any combination can be set (or deleted) together in one command:

Attribute	Sets	Delete resets to
`sample-dest <cpu\|mirror>`	`Port.sampleDest`	unset (agent default)
`ingress-rate <N>`	`Port.sFlowIngressRate`	`0`
`egress-rate <N>`	`Port.sFlowEgressRate`	`0`

Why: none of these three fields had CLI coverage in either direction.

How: CmdConfigInterfaceSflow / CmdDeleteInterfaceSflow each parse one or more <attr> <value> pairs (or <attr> names for delete) and apply every attribute present in the call together, the same way CmdConfigInterface handles its own attributes (mtu, description, ...) — rather than a separate leaf class and CLI11 subcommand per attribute, and rather than an if/else chain that would only let one attribute take effect per call. Adding a future sflow attribute is one more branch in the same file.

Validation: rate values must be a non-negative integer. sample-dest mirror is refused when the port's sFlowEgressRate — from this same command if given, else the port's current value — is nonzero, and symmetrically, a nonzero egress-rate is refused when the port's effective sample-dest is mirror. This is evaluated against the combined result of the whole command, so sample-dest mirror egress-rate 0 succeeds and sample-dest mirror egress-rate 50 fails regardless of which attribute is given first. It's the same constraint the agent enforces (ApplyThriftConfig: "Egress sampling to mirror destination is unsupported"), caught by the CLI before the config is touched. Hitless: sampling is (re)programmed from the port delta at runtime (SaiPortManager::addSamplePacket/removeSamplePacket).

Sample usage

$ fboss2-dev config interface eth1/1/1 sflow sample-dest cpu ingress-rate 100 egress-rate 50
Successfully set sFlow sample-dest=cpu, ingress-rate=100, egress-rate=50 for interface(s) eth1/1/1
$ fboss2-dev config interface eth1/2/1 sflow sample-dest mirror egress-rate 50
Port eth1/2/1: sample-dest mirror requires sFlowEgressRate 0 — egress sampling to a mirror destination is unsupported
$ fboss2-dev delete interface eth1/1/1 sflow sample-dest ingress-rate egress-rate
Reset sFlow sample-dest, ingress-rate, egress-rate for interface(s) eth1/1/1

Changed files

File	Change
`commands/config/interface/sflow/CmdConfigInterfaceSflow.{h,cpp}`	Parses one or more `<attr> <value>` pairs, applies all attributes present together
`commands/delete/interface/sflow/CmdDeleteInterfaceSflow.{h,cpp}`	Parses one or more `<attr>` names, resets all present together
`CmdListConfig.cpp`	`sflow` is now a leaf command (was a stub + one nested subcommand per attribute)
`test/config/CmdConfigInterfaceSflowSampleDestTest.cpp`	Unit tests for all three attributes, both commands, combined-attribute and cross-validation cases
`test/integration_test/ConfigSflowSampleDestTest.cpp`	End-to-end: `sample-dest` set/delete round trip
`test/integration_test/ConfigSflowRateTest.cpp`	End-to-end: combined `ingress-rate`+`egress-rate` set/delete round trip

Test Plan

Unit tests (29, config + delete, covering all three attributes individually, combined in one call, and the order-independent cross-attribute mirror/egress-rate validation): all pass.

Full config suite: //fboss/cli/fboss2/test/config:cmd_config_test PASSED.

Integration tests on a hardware DUT — sample-dest (set cpu, verify running config, delete, verify unset) and combined ingress-rate+egress-rate (set both in one command, verify running config, delete both in one command, verify reset to 0):

[ RUN      ] ConfigSflowRateTest.SetThenDeleteIngressAndEgressRateCombined
[       OK ] ConfigSflowRateTest.SetThenDeleteIngressAndEgressRateCombined
[ RUN      ] ConfigSflowSampleDestTest.SetThenDeleteSampleDest
[       OK ] ConfigSflowSampleDestTest.SetThenDeleteSampleDest
[  PASSED  ] 2 tests.

Agents stayed active throughout; DUT config verified restored to its pre-test state.

config interface <ports> sflow sample-dest <cpu|mirror> sets Port.sampleDest, choosing whether sFlow samples are processed on-box (cpu) or sent to the port's ingress mirror (mirror). The CLI refuses mirror while the port has a non-zero sFlowEgressRate, mirroring the agent's own validation (egress sampling to a mirror destination is unsupported) with a targeted message. delete interface <ports> sflow sample-dest clears the optional field, returning the port to its unset default. Both are hitless: sampling is (re)programmed from the port delta at runtime (SaiPortManager add/removeSamplePacket). Mixed interface lists report L3-only names as skipped (sampleDest is a Port attribute), and the integration test carries a best-effort TearDown that clears a committed sampleDest on failure.

joseph5wu

Let's follow how we handle multiple interface attributes in CmdConfigInterface and do the same thing for CmdConfigInterfaceSflow rather than create three different classes.

joseph5wu · 2026-08-22T04:49:28Z

+  using RetType = std::string;
+};
+
+class CmdConfigInterfaceSflowSampleDest


Instead of creating a standalone CmdConfigInterfaceSflowSampleDest, can we have CmdConfigInterfaceSflow to handle all sflow related attributes in the same class:

sample_dest

ingress_rate

egress_rate?

will add these 3 here in this PR itself, since they seemed to be simple enough to implement

…eSflow Per review, collapse the standalone CmdConfigInterfaceSflowSampleDest / CmdDeleteInterfaceSflowSampleDest leaf classes into their CmdConfigInterfaceSflow / CmdDeleteInterfaceSflow parents, dispatching on attribute name the same way CmdConfigInterface handles its own attributes. This keeps future sflow attributes (ingress-rate, egress-rate) to a single class each, instead of a new leaf class and CLI11 subcommand per attribute. CLI surface is unchanged: `config interface <ports> sflow sample-dest <cpu|mirror>` and `delete interface <ports> sflow sample-dest` behave identically; only the C++ class structure and the CLI11 command-tree registration changed.

Per review, CmdConfigInterfaceSflow / CmdDeleteInterfaceSflow should handle all sflow attributes: sample_dest, ingress_rate, egress_rate. Adds the two rate attributes as new branches in the same dispatch already added for sample-dest, mapping to Port.sFlowIngressRate/sFlowEgressRate: config interface <ports> sflow ingress-rate <N> config interface <ports> sflow egress-rate <N> delete interface <ports> sflow ingress-rate (resets to 0) delete interface <ports> sflow egress-rate (resets to 0) Rate values must be a non-negative integer. egress-rate carries the same MIRROR/sFlowEgressRate constraint sample-dest already enforces (ApplyThriftConfig: "Egress sampling to mirror destination is unsupported"), checked from the other direction: a port whose sample-dest is already MIRROR refuses a non-zero egress-rate. Unit tests: 6 new (setIngressRate, setEgressRate, rateValueInvalid, egressRateRefusedWhenMirror, deleteClearsIngressRate, deleteClearsEgressRate), plus fixed two existing tests that used ingress-rate as a stand-in for an unrecognized attribute (now a real one) to use a bogus attribute name instead. Integration test (new file, minimal): ConfigSflowRateTest covers the config/delete round trip for both rate attributes in one DUT run, mirroring ConfigSflowSampleDestTest's shape. Run on an Nexthop DUT (NH-4010-F): PASSED.

vybhav-nexthop · 2026-08-24T07:09:42Z

Added ingress-rate and egress-rate too, in 0b89a7e: config/delete interface <ports> sflow ingress-rate|egress-rate <N>, mapping to Port.sFlowIngressRate/sFlowEgressRate, both handled by the same CmdConfigInterfaceSflow/CmdDeleteInterfaceSflow class as sample-dest. egress-rate carries the same MIRROR/egress-rate constraint sample-dest already enforces, checked from the other direction (refuses a non-zero egress-rate on a port whose sample-dest is already MIRROR).

All three attributes now go through one class each on the config/delete side, per your original comment.

joseph5wu

Overall it looks pretty good to me. Just make sure you can support setting multiple attributes at the same time like CmdConfigInterface.cpp

joseph5wu · 2026-08-24T17:32:27Z

+    if (attr == kAttrSampleDest) {
+      // The agent rejects egress sampling to a mirror destination
+      // (ApplyThriftConfig throws for MIRROR + sFlowEgressRate > 0); fail
+      // here with a targeted message before touching the config.
+      if (dest == cfg::SampleDestination::MIRROR &&
+          *port->sFlowEgressRate() > 0) {
+        throw std::invalid_argument(
+            fmt::format(
+                "Port {}: sample-dest {} requires sFlowEgressRate 0 — egress "
+                "sampling to a mirror destination is unsupported",
+                *port->name(),
+                kSampleDestMirror));
+      }
+      port->sampleDest() = dest;
+    } else if (attr == kAttrIngressRate) {
+      port->sFlowIngressRate() = rate;
+    } else {
+      // Same MIRROR/egress-rate constraint as above, checked from the other
+      // side: refuse a non-zero egress-rate on a port whose sampleDest is
+      // already MIRROR.
+      if (rate > 0 && port->sampleDest().has_value() &&
+          *port->sampleDest() == cfg::SampleDestination::MIRROR) {
+        throw std::invalid_argument(
+            fmt::format(
+                "Port {}: egress-rate must be 0 while sample-dest is {} — "
+                "egress sampling to a mirror destination is unsupported",
+                *port->name(),
+                kSampleDestMirror));
+      }
+      port->sFlowEgressRate() = rate;
+    }


Instead of using if-else, can you support just if independently for the three attributes?
This way we can support a combined command like fboss2-dev config interface XXX sflow sample-dest CPU ingress-rate XXX egress-rate YYY?

joseph5wu · 2026-08-24T17:33:24Z

+  std::string attrLabel = attr == kAttrSampleDest
+      ? "sample destination"
+      : (attr == kAttrIngressRate ? "ingress-rate" : "egress-rate");
+  std::string message = fmt::format(
+      "Successfully set sFlow {} for interface(s) {} to {}",
+      attrLabel,
+      folly::join(", ", updatedNames),
+      displayValue);
+  if (!skippedNames.empty()) {
+    message +=
+        fmt::format("; skipped (no port): {}", folly::join(", ", skippedNames));
+  }


The benefit to have the same CmdConfigInterfaceSflow.cpp to control three attributes is to program the three attributes at the same time. Let's make sure we don't make it exclusive

got it , will do it the next commit . Thanks

Per review, CmdConfigInterfaceSflow / CmdDeleteInterfaceSflow should be able to program sample-dest, ingress-rate, and egress-rate together in a single command, not mutually exclusive: config interface <ports> sflow sample-dest cpu ingress-rate 100 egress-rate 50 delete interface <ports> sflow sample-dest ingress-rate egress-rate SflowAttrArgs / SflowDeleteAttrArgs now parse repeated <attr> <value> pairs (config) / repeated <attr> names (delete) instead of exactly one, validating attribute names at construction (last occurrence of a repeated attribute wins). queryClient extracts an optional value for each of the three attributes up front, then applies whichever are present together per port. The mirror/egress-rate cross-check now evaluates the combined effective state -- the new value if given this call, else the port's existing value -- so "sample-dest mirror egress-rate 0" succeeds and "sample-dest mirror egress-rate 50" fails, regardless of token order. Unit tests: added configArgsMultiplePairsValid, deleteArgsMultipleValid, combinedAttributesAppliedTogether, combinedMirrorWithZeroEgressRateSucceeds, combinedMirrorWithNonzeroEgressRateFails, combinedRejectionIsOrderIndependent, repeatedAttributeLastValueWins, deleteClearsMultipleAttributesTogether. Renamed the two arity tests for clarity; unknownAttr/deleteUnknownAttrThrows now assert directly on the arg-type constructor (validation moved there). Integration test: ConfigSflowRateTest now issues one combined config command (ingress-rate + egress-rate) and one combined delete, instead of two separate commands each, to actually exercise the new capability end-to-end. Run on a hardware DUT: PASSED.

meta-codesync · 2026-08-24T18:33:23Z

@joseph5wu has imported this pull request. If you are a Meta employee, you can view this in D117229194.

meta-codesync · 2026-08-24T21:15:57Z

@joseph5wu merged this pull request in 2c207c4.

vybhav-nexthop requested review from a team as code owners August 17, 2026 07:30

meta-cla Bot added the CLA Signed label Aug 17, 2026

github-actions Bot added the nexthop label Aug 17, 2026

joseph5wu requested changes Aug 22, 2026

View reviewed changes

vybhav-nexthop added 2 commits August 24, 2026 06:24

joseph5wu reviewed Aug 24, 2026

View reviewed changes

meta-codesync Bot closed this in 2c207c4 Aug 24, 2026

meta-codesync Bot added the Merged label Aug 24, 2026

Navigation Menu

Uh oh!

[Nexthop][fboss2-dev] Add config and delete interface sflow sample-dest - #1522

[Nexthop][fboss2-dev] Add config and delete interface sflow sample-dest#1522
vybhav-nexthop wants to merge 4 commits into
facebook:mainfrom
nexthop-ai:sflow-sample-dest

vybhav-nexthop commented Aug 17, 2026 •

edited

Loading

Uh oh!

joseph5wu left a comment

Uh oh!

joseph5wu Aug 22, 2026

Uh oh!

vybhav-nexthop Aug 24, 2026

Uh oh!

vybhav-nexthop commented Aug 24, 2026

Uh oh!

joseph5wu left a comment

Uh oh!

joseph5wu Aug 24, 2026

Uh oh!

joseph5wu Aug 24, 2026

Uh oh!

vybhav-nexthop Aug 24, 2026

Uh oh!

meta-codesync Bot commented Aug 24, 2026

Uh oh!

meta-codesync Bot commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

vybhav-nexthop commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background: what sFlow sampling is and what these three attributes control

Sample usage

Changed files

Test Plan

Uh oh!

joseph5wu left a comment

Choose a reason for hiding this comment

Uh oh!

joseph5wu Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

vybhav-nexthop Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

vybhav-nexthop commented Aug 24, 2026

Uh oh!

joseph5wu left a comment

Choose a reason for hiding this comment

Uh oh!

joseph5wu Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

joseph5wu Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

vybhav-nexthop Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

meta-codesync Bot commented Aug 24, 2026

Uh oh!

meta-codesync Bot commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vybhav-nexthop commented Aug 17, 2026 •

edited

Loading