[mpls][sonic-vpp] Enable MPLS data-plane tests on the VPP t1-lag testbed - #26619
[mpls][sonic-vpp] Enable MPLS data-plane tests on the VPP t1-lag testbed#26619augusdn wants to merge 3 commits into
Conversation
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@StormLiangMS @wangxin @yxieca A user wants to merge changes to the conditional mark files into |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR enables MPLS data-plane test coverage on the SONiC-VPP KVM t1-lag-vpp testbed by unblocking previously over-broad topology gating, adapting the tests to PortChannel-based topologies, and wiring the test module into the VPP PR CI selection.
Changes:
- Fix MPLS testbed gating to rely on topology type (
t1) and resolve PortChannel interfaces to underlying PTF member port indices. - Update MPLS tests to pass a list of destination PTF port IDs directly into
verify_packet_any_port, and remove a duplicatemask.Mask()wrapper. - Enable the
mplsfeature for VPP golden configs, narrow the VPP conditional skip totest_push_labelonly, and addmpls/test_mpls.pyto thet1-lag-vppPR test set.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/mpls/test_mpls.py | Adjust packet verification to support multi-egress (PortChannel member) destination ports; minor mask cleanup. |
| tests/mpls/conftest.py | Fix topology gating and map PortChannel DUT interfaces to PTF member port IDs for t1-lag. |
| tests/common/plugins/conditional_mark/tests_mark_conditions_sonic_vpp.yaml | Replace blanket MPLS skip on VPP with a targeted skip of only test_push_label and a documented reason. |
| ansible/library/generate_golden_config_db.py | Enable mpls feature in generated golden config for VPP testbeds so MPLS tests aren’t skipped by feature gating. |
| .azure-pipelines/pr_test_scripts.yaml | Add mpls/test_mpls.py into the t1-lag-vpp CI test selection. |
| portchannels = mg_facts.get('minigraph_portchannels', {}) | ||
| if dut_port in portchannels: | ||
| members = portchannels[dut_port]['members'] | ||
| else: | ||
| members = [dut_port] | ||
| return [mg_facts['minigraph_port_indices'][member] for member in members] |
5969ce6 to
e90f906
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
CI status on build 1178163 (commit
|
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Pushed What changed1. The This is the important one. Registering it as
The conditional_mark gate is 2. Those guards only existed because the feature was enabled. Both loops already skip features whose state isn't 3. Skip cleanly when the topology has no T2-facing or no T0-facing interface. Gating on topology type correctly admits 4. LogAnalyzer ignore for the kernel-MPLS error — see (a) below. Three things whose root cause is upstream of this PRFlagging these explicitly rather than leaving them to be discovered. (a) On sonic-vpp this is genuinely benign — MPLS forwarding is done by VPP in userspace via the SAI INSEG entries, so the kernel data path is unused. That's why the ignore is gated on (b) (c)
VerificationFull suite on A/B against a clean This stays a draft until #2008 merges and the submodule bump reaches the sonic-vpp image — until then |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Pushed Replaced the LogAnalyzer ignore with a root-cause fixThe previous revision suppressed this error with a LogAnalyzer ignore: That was treating the symptom. The actual cause is that So the fixture now loads the module instead of hiding the error. SONiC's own Verified from a clean baseline (module The module is deliberately left loaded on teardown — The durable fix still belongs upstream (sonic-buildimage loading the module, or sonic-swss not logging an Local validationThe dev VM's build environment was repaired so the full sairedis build and test suite could be run locally against the pre-requisite backend before pushing:
Still a draft until sonic-net/sonic-sairedis#2008 merges and the submodule bump reaches the sonic-vpp image — until then |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Marking this ready for review. Summary of the current CI state so the one red check isn't a surprise: 22 checks pass, 1 fails. The single failure is That job runs the whole The same tests pass once that backend is present - ElasticTest plan Everything else is green, including all eight other topology jobs ( Worth noting for review scope: this PR touches no shared global test files. An earlier revision needed guards in the pre-test/post-test/ |
Enable tests/mpls on the sonic-vpp KVM testbed (t1-lag-vpp):
- conftest: support t1-lag by gating on the topology *type* ('t1') and
resolving PortChannel spine/tor interfaces to their PTF member port
ids (a physical interface resolves to a single-element list). The old
check was `name not in ('t1')`, which compares against a string
rather than a tuple, so anything other than a plain 't1' topology was
skipped.
- test_mpls: pass the dst PTF port-id list directly to
verify_packet_any_port (ports=dst_pid), and drop a duplicate
mask.Mask() wrap in the pop expected-packet helper.
- conditional_mark: replace the blanket mpls/test_mpls.py vpp skip with
a skip for test_push_label only. pop/swap/swap_labelstack run and pass
on VPP; push is blocked by an orchagent ROUTE_TABLE MPLS-push install
gap (tracked separately), not the VPP SAI backend.
- golden_config: enable the 'mpls' feature on sonic-vpp testbeds so the
tests are not skipped by the "'mpls' not in feature_status" condition.
The feature is absent from both init_cfg.json and the minigraph-derived
config, so it has to be turned on somewhere for the tests to run.
- pre/posttest: skip 'mpls' when walking enabled features to toggle the
rsyslog rate limit. Those loops assume every enabled feature has a
container of the same name and run 'docker exec -i <feature>', but
mpls is a config flag rather than a containerized service, so the
exec fails with "No such container: mpls" and aborts the run. This
mirrors the existing frr_bmp skip, which is there for the same reason.
- pr_test_scripts: add mpls/test_mpls.py to the t1-lag-vpp set so the
vpp PR checker actually exercises it.
Note that mpls/test_mpls.py is already listed for t1-lag, where the
topology check above silently skipped it. With that check fixed the
tests run wherever the mpls feature is enabled; on images without the
feature they stay skipped by the existing feature_status condition.
Verified on vms-kvm-vpp-t1-lag: 3 passed, 1 skipped (push).
Signed-off-by: Augustine Lee <augustinelee@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…pology guard Register the mpls FEATURE with state "disabled" instead of "enabled", and drop the shared-file workarounds that only existed because it was enabled. mpls is a config flag, not a containerized service, so there is no "mpls" docker. With state "enabled" the image's own monit container_checker reports "Expected containers not running: mpls", which fails the pre-test sanity check for every test on this platform. The frr_bmp exemption for the same class of containerless feature lives in sonic-buildimage's /usr/bin/container_checker, so it cannot be worked around from sonic-mgmt. The tests are gated by "'mpls' not in feature_status", which is a key-membership test, so "disabled" is enough to un-skip them. Verified on a live DUT: the feature is still listed by "show feature status", container_checker exits 0 and monit reports container_checker OK. Because the feature is no longer enabled, the guards added to test_pretest.py and test_posttest.py are dead code - both loops already skip features whose state is not enabled - so they are reverted. That removes this change's footprint on shared, global test files entirely. Also skip the module when the topology has no T2-facing or no T0-facing interface. Gating on topology type instead of name correctly admits t1-lag, but it also admits t1 variants that have no T2 peer at all (t1-backend, whose neighbors are all BT0, plus t1-isolated-d128/d32 and the v6 variant). Those would have failed with an IndexError from random.choice([]). Ignore the kernel-MPLS error intfmgrd logs on sonic-vpp. Configuring MPLS on an interface makes intfmgrd run "sysctl -w net.mpls.conf.<intf>.input=1", which needs the mpls_router kernel module. No SONiC image loads that module, so the command fails and is logged as an ERR. On sonic-vpp it is benign because MPLS forwarding is done by VPP in userspace via the SAI INSEG entries, not by the Linux kernel data path. Signed-off-by: Augustine Lee <augustinelee@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 251d571f-a53b-47f8-9eb4-293dbc6ff7a4
…rror Enabling MPLS on an interface makes intfmgrd run "sysctl -w net.mpls.conf.<intf>.input=1", which needs the mpls_router kernel module. The module ships in the image but nothing loads it, so the sysctl fails and is logged as an ERR. The previous revision suppressed that error with a LogAnalyzer ignore. Load the module in the setup fixture instead, which removes the cause rather than the symptom: the sysctl then succeeds and SONiC's own "config interface mpls add" completes as designed. sonic-swss's own MPLS test (tests/test_mpls.py setup_mpls) loads it the same way, and sonic-mgmt already modprobes in tests/console/conftest.py and tests/test_pktgen.py. Verified from a clean baseline (module rmmod'ed first): 3 passed, 1 skipped, zero setIntfMpls errors, with no LogAnalyzer ignore in place. The module is deliberately left loaded: modprobe is idempotent, the module ships in the image, and unloading could disrupt anything else using MPLS. It touches no CONFIG_DB state. Signed-off-by: Augustine Lee <augustinelee@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 251d571f-a53b-47f8-9eb4-293dbc6ff7a4
8971943 to
5e2f795
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Description of PR
Summary:
Fixes sonic-net/sonic-buildimage#25782
Enables the MPLS data-plane tests (
tests/mpls/test_mpls.py) on the sonic-vpp KVMtestbed (
t1-lag-vpp).test_pop_label,test_swap_labelandtest_swap_labelstackpass;
test_push_labelstays skipped for the reason described below.Important
Depends on sonic-net/sonic-sairedis#2008, which adds the VPP SAI MPLS backend
(INSEG disposition + IP-route label imposition). Merge only after that change has
landed and been picked up into the sonic-vpp image - otherwise the newly enabled
tests have no backend to talk to and
mpls/test_mpls.pyfails by design.Type of change
Back port request
Tracking issue/work item for backport/cherry-pick request (GitHub issue or Microsoft ADO): N/A - no backport requested.
Failure type: other (new platform enablement, not a regression)
Tested branch
Test result
master: image
SONiC.master-28652.1177040-4e2ccc26c(a throwaway sonic-buildimagebuild carrying [vpp] Add MPLS data-plane support (INSEG + IP-route push) sonic-sairedis#2008) on the converged
vms-kvm-vpp-t1-lagtestbed, ElasticTest plan
6a69da68f481df03c4e59c5e- SUCCESS, 22 tests, 17 passed /5 skipped / 0 failed / 0 errors:
That run proved PR A's backend and PR B's test code together, pre-merge.
Two changes were made to this PR after that run - the
mplsFEATURE registrationmoved from
enabledtodisabled(which let three shared global files be reverted,see below), and the
mpls_routermodprobe replaced a LogAnalyzer ignore. Both werere-validated on the same
vms-kvm-vpp-t1-lagtestbed:Approach
What is the motivation for this PR?
MPLS was never exercised on sonic-vpp -
mpls/test_mpls.pywas skipped outright for thevpp asic. With the VPP SAI backend in the pre-requisite PR the disposition path works, so
the tests can be turned on and give us real MPLS coverage on a KVM testbed.
How did you do it?
5 files, +68 / -13. No shared global test file is modified.
tests/mpls/conftest.pympls_routerkernel module in setup.tests/mpls/test_mpls.pyverify_packet_any_port(ports=dst_pid); drop a duplicatedmask.Mask()wrap in the pop expected-packet helper.tests/common/plugins/conditional_mark/tests_mark_conditions_sonic_vpp.yamlmpls/test_mpls.pyskip with a skip fortest_push_labelonly.ansible/library/generate_golden_config_db.pymplsFEATURE on sonic-vpp testbeds, withstate: disabled(the state matters - see below)..azure-pipelines/pr_test_scripts.yamlmpls/test_mpls.pyto thet1-lag-vppset so the vpp PR checker runs it.Notes on the individual changes:
name not in ('t1'), which compares against astring rather than a tuple, so every topology except a plain
t1was skipped -including
t1-lag.t1-lagthe spine/tor facing interfaces arePortChannels, which are not present in
minigraph_port_indices. A physical interfaceresolves to a single-element list.
t1variants (t1-backend, whose neighbors are allBT0) have no T2 peer, and the old name-based check happened to exclude them; withoutthis the module would fail with an
IndexErrorfromrandom.choice([]).mpls_routermodprobe. Enabling MPLS on an interface makesintfmgrdrunsysctl -w net.mpls.conf.<intf>.input=1, which needs that module. It ships in theimage but nothing loads it, so the sysctl fails and is logged as an
ERR. Loading itremoves the cause rather than suppressing the symptom, and lets SONiC's own
config interface mpls addcomplete as designed. sonic-swss's own MPLS test(
tests/test_mpls.py,setup_mpls) loads it the same way, and sonic-mgmt alreadymodprobes in
tests/console/conftest.pyandtests/test_pktgen.py. The module isleft loaded on teardown:
modprobeis idempotent, the module ships in the image,unloading could disrupt anything else using MPLS, and it touches no CONFIG_DB state.
Why the
mplsFEATURE is registered asdisabledand notenabled:the tests are gated by the
'mpls' not in feature_statuscondition intests_mark_conditions.yaml, andmplsis present in neitherinit_cfg.jsonnor theminigraph-derived config, so the key has to exist somewhere for the tests to run at all.
That condition is a key-membership test, so
disabledis enough to un-skip them.Registering it as
enabledis actively wrong here:mplsis a config flag, not acontainerized service, and there is no
mplsdocker. Withstate: enabledthe image'sown monit
container_checkerreportswhich fails the pre-test sanity check for every test on this platform. The
frr_bmpexemption for the same class of containerless feature lives insonic-buildimage's
/usr/bin/container_checker, so it cannot be worked around fromsonic-mgmt. Using
disabledkeeps monit healthy, keeps the tests un-skipped, and needsno image change. Verified on a live DUT: feature still listed by
show feature status,container_checkerexits 0, monit reportscontainer_checker OK.Choosing
disabledalso let an earlier revision of this PR be simplified: thepre-test/post-test/
dut_utilsguards it previously needed became dead code and werereverted, so this PR now touches no shared global test files.
On
test_push_label: it exercises IP-to-label imposition and does not work end toend. The test injects the push route straight into
ROUTE_TABLE, and orchagent does notinstall that route into ASIC_DB, so it never reaches the SAI backend. That is a route
management gap rather than anything specific to VPP, so the test is skipped with that
reason recorded.
One thing worth flagging for reviewers:
mpls/test_mpls.pyis already listed undert1-lag(andonboarding_t1_multi_asic) inpr_test_scripts.yaml, but the topologycheck described above silently skipped it there. Fixing that check means the tests now
run wherever the
mplsfeature is registered. On images without the feature they stayskipped by the existing
feature_statuscondition, so the practical blast radius islimited to vpp. The
impacted-area-kvmtest-t1-lagandmulti-asic-t1PR checks bothpassed with this change in place.
How did you verify/test it?
See the Test result section above for the image version, ElasticTest plan id and
counts. In summary: the combined pre-merge ElasticTest run was green (17 passed /
5 skipped / 0 failed / 0 errors), and the two later changes were re-validated on the
same testbed, including an A/B against a clean
masterworktree on the same DUT withidentical scope, which produced identical results (14 passed / 4 skipped / 0 failed on
both sides) - i.e. no regression to the surrounding framework tests.
The dependency on sonic-net/sonic-sairedis#2008 was verified rather than assumed: the
green run above used a throwaway image built with that PR in it, and the
t1-lag-vppPRcheck on an image without it fails
mpls/test_mpls.py, which is the expected merge-orderbehaviour described in the callout at the top.
DUT was healthy throughout: VPP bonds up with
l34-inner, all 24 BGP sessionsestablished,
container_checkerOK.Any platform specific information?
sonic-vpp only. The conditional_mark change is confined to
tests_mark_conditions_sonic_vpp.yaml, and the golden-config change is gated on thesonic-vpp testbeds, so no other ASIC or platform is affected. The one cross-platform
effect is the
tests/mpls/conftest.pytopology-gate fix described above, which is acorrectness fix to a comparison that was silently skipping
t1-lag; on images that donot register the
mplsfeature the tests remain skipped by the existingfeature_statuscondition.Supported testbed topology if it's a new test case?
Not a new test case - this enables an existing module on an additional platform.
Verified on
t1-lag-vpp. The conftest gate acceptst1topology types generally.Documentation
No documentation change needed - this enables an existing test module on an additional
platform.