[Nexthop][fboss2-dev] Add config and delete interface sflow sample-dest - #1522
[Nexthop][fboss2-dev] Add config and delete interface sflow sample-dest#1522vybhav-nexthop wants to merge 3 commits into
Conversation
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
left a comment
There was a problem hiding this comment.
Let's follow how we handle multiple interface attributes in CmdConfigInterface and do the same thing for CmdConfigInterfaceSflow rather than create three different classes.
| using RetType = std::string; | ||
| }; | ||
|
|
||
| class CmdConfigInterfaceSflowSampleDest |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Added ingress-rate and egress-rate too, in 0b89a7e: All three attributes now go through one class each on the config/delete side, per your original comment. |
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runSummary
Background: what sFlow sampling is and what sample-dest chooses
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 (N is
Port.sFlowIngressRate/sFlowEgressRate) and exports just those. Statistically that is enough to see top talkers, flow mix, and anomalies without touching line-rate forwarding.Port.sampleDestchooses where each sampled copy goes, and the two options trade 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.The field is a modifier: it takes effect once a port's sampling rate is nonzero, and an unset field leaves the ASIC default behavior.
What: Adds
fboss2-dev config interface <ports> sflow sample-dest <cpu|mirror>and its delete counterpartdelete interface <ports> sflow sample-dest.Port.sampleDest: sFlow samples are processed on-box (cpu) or sent to the port's ingress mirror (mirror).Why:
Port.sampleDesthad no CLI coverage in either direction.How: New
sflowsubcommand branch underconfig interface/delete interface, in the same tree shape as ipv6/switchport. The CLI refusesmirrorwhen the port has a non-zerosFlowEgressRate. That is the constraint the agent enforces (ApplyThriftConfig: "Egress sampling to mirror destination is unsupported"), but the CLI catches it first with a targeted message, before the config is touched. Hitless: sampling is (re)programmed from the port delta at runtime (SaiPortManager::addSamplePacket/removeSamplePacket).Sample usage
Test Plan
Unit tests (13 new, config + delete):
Full config suite:
//fboss/cli/fboss2/test/config:cmd_config_test PASSED in 53.1s.Integration test on an Nexthop DUT: set cpu, verify it lands in the running config, delete, verify the field is unset again.
All of the above ran against the internal tree this commit was cherry-picked from. The upstream tree was not built locally; this PR's CI is the build validation for the rebased commit.
Review Findings
Pre-publication review (5 reviewers + verifier) raised three findings, all fixed before push: mixed interface lists now report L3-only names as skipped in both commands (a 13th unit test covers it), and the integration test gained a best-effort TearDown that clears a committed sampleDest on failure.