[test gap] Add two test cases to cover lldp neighbors or interfaces incomplete scenarios#22420
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ncompete scenarios Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
082f763 to
b8102e8
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR updates the LLDP test suite in sonic-mgmt to close a test gap for scenarios where LLDP interface/neighbor discovery is incomplete (notably after config reload), aligning coverage with the failure mode described in issue #22376.
Changes:
- Replaces the prior
test_lldp_after_config_reloadwith two new tests: one validating LLDP interface/chassis correctness without reload and one validating persistence after config reload. - Adds reusable helper functions to validate
show lldp table,lldpcli show interfaces,lldpctl_facts, and chassis output consistency. - Adds syslog checking via loganalyzer match/ignore patterns for LLDP-specific errors.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
- Replace 'if loganalyzer else None' with 'contextlib.nullcontext()' to avoid crash when loganalyzer is disabled (None is not a context manager) - Use DEVICE_METADATA['localhost']['mac'] for T2 topology chassis-id instead of always using mgmt interface MAC, consistent with existing check_lldp_neighbor() logic Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
32643f0 to
b11d285
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
On dualtor testbeds, some admin-up interfaces may not have LLDP neighbors, causing false assertion failures. Change strict equality check to only fail when LLDP/lldpctl table has unexpected extra interfaces that are not admin-up or are PortChannels. Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
87295e2 to
b6397d5
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…-reload baseline - Override match_regex for ALL DUTs (not just selected one) to only match LLDP-specific errors, since fixture teardown analyzes all DUTs - Remove per-DUT ignore_regex for config reload (no longer needed with the override approach) - Use pre-reload LLDP neighbor count as baseline for post-reload wait instead of computing expected count from interface status - Improve error message in verify_lldp_table for clarity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
On KVM testbeds (asic_type='vs'), eth0 is a virtual management interface with no LLDP neighbor on the other end, so it won't appear in LLDP table, lldpcli interfaces, or lldpctl_facts. Make the eth0 assertions conditional: - Physical testbeds: assert eth0 must be present (unchanged behavior) - Virtual testbeds: log info message instead of failing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
SonicAsic has 'asic_index' as a property, not 'get_asic_index()' method. This caused AttributeError on T2 multi-ASIC testbeds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
On multi-ASIC systems, 'docker exec lldp{N} lldpcli show interfaces' only
returns interfaces for ASIC N, but 'show interface status' returns all
interfaces across all ASICs. Filter intf_status_output to only include
ports belonging to the current ASIC before comparing with lldpcli output.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
On multi-ASIC T2, each ASIC's lldp container reports its own ASIC-level MAC from DEVICE_METADATA, not the global device MAC. Use asic.config_facts() instead of duthost.config_facts() to get the correct expected chassis MAC for multi-ASIC systems. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@StormLiangMS @yxieca could you please help review? |
StormLiangMS
left a comment
There was a problem hiding this comment.
Overall: Good quality — a few suggestions
The refactoring from a monolithic test into 4 reusable helpers (verify_lldp_table, verify_lldpcli_interfaces, verify_lldpctl_facts, verify_chassis_info) is well done. Multi-ASIC, virtual/KVM, and loganalyzer handling all look solid. KVM CI passed cleanly.
Feedback:
-
LLDP table parsing (verify_lldp_table): Parsing
show lldp tableby splitting on whitespace is fragile — if a neighbor name contains spaces,parts[0]won't be the interface. Consider usingduthost.show_and_parse("show lldp table")for more robust parsing, consistent with howintf_status_outputis obtained. -
Duplicated chassis MAC logic: The chassis MAC computation block (T2 multi-ASIC vs single-ASIC vs non-T2) is duplicated in both
test_lldp_interfacesandtest_lldp_interfaces_config_reload. Consider extracting it into a helper likeget_expected_chassis_mac(duthost, asic, tbinfo)to reduce duplication. -
Hardcoded capability assertions: The test asserts
Wlan: off, Station: offfor all platforms. Are there any SONiC deployments where these would differ? If so, consider making these configurable or conditional.
1. Use show_and_parse for LLDP table parsing instead of fragile whitespace splitting (verify_lldp_table) 2. Extract chassis MAC logic into get_expected_chassis_mac() helper to eliminate duplication between test_lldp_interfaces and test_lldp_interfaces_config_reload 3. Make Wlan/Station capability checks conditional - only assert 'off' if the capability is reported (not all platforms list them) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…nd_parse
show_and_parse('show lldp table') may include separator lines ('-----')
and footer lines ('Total entries...') as parsed entries. Filter these out
by checking the localport value.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thanks @StormLiangMS for the thorough review! All 3 suggestions have been addressed in commit 802efe6:
Verified all 5 tests pass on T1 physical testbed ( |
…ncomplete scenarios (sonic-net#22420) What is the motivation for this PR? To add 2 new test cases to check if all lldp neighbors exist and if all interfaces exists in lldpcli show interfaces. We don't have a test case to cover the issue we found in PR: sonic-net/sonic-buildimage#25436 sonic-net#22562 cover some scenarios, but not all of them. So I removed the test case in sonic-net#22562 and add 2 test cases with more checks in this PR. How did you do it? 1. test_lldp_interfaces Purpose: Verify LLDP functionality across all interfaces without config reload Key Features: Validates LLDP table completeness Verifies lldpcli interface discovery Checks lldpctl_facts consistency Validates chassis ID and capabilities Active syslog monitoring via loganalyzer Test Steps: Recording Phase: Capture all interfaces from show interface status LLDP Table Verification: Compare LLDP table with interface status (admin up, no PortChannels) lldpcli Verification: Compare lldpcli output with interface status (all interfaces, no PortChannels) lldpctl_facts Verification: Compare lldpctl_facts with interface status (admin up, no PortChannels) Consistency Check: Verify all lldpctl_facts interfaces exist in lldpcli Chassis Verification: Validate chassis MAC address and capabilities (Bridge: on, Router: on, Wlan: off, Station: off) 2. test_lldp_interface_config_reload Purpose: Verify LLDP functionality persists correctly after config reload Key Features: Tests LLDP recovery after config reload Validates interface persistence Ensures neighbor rediscovery Monitors for LLDP-specific errors while ignoring expected reload errors Addresses the core issue from GitHub [test gap] check lldp neighbors after config reload sonic-net#22376 Test Steps: Pre-Reload Recording: Capture all interfaces before config reload Config Reload: Perform safe config reload with interface checks Stabilization Wait: Wait for critical services and LLDP convergence LLDP Table Verification: Compare LLDP table with pre-reload interfaces (admin up, no PortChannels) lldpcli Verification: Compare lldpcli output with pre-reload interfaces (all interfaces, no PortChannels) lldpctl_facts Verification: Compare lldpctl_facts with pre-reload interfaces (admin up, no PortChannels) Consistency Check: Verify all lldpctl_facts interfaces exist in lldpcli Chassis Verification: Validate chassis MAC and capabilities remain correct How did you verify/test it? run it on testbed. Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Venkata Gouri Rajesh Etla <vrajeshe@cisco.com>
…ncomplete scenarios (sonic-net#22420) What is the motivation for this PR? To add 2 new test cases to check if all lldp neighbors exist and if all interfaces exists in lldpcli show interfaces. We don't have a test case to cover the issue we found in PR: sonic-net/sonic-buildimage#25436 sonic-net#22562 cover some scenarios, but not all of them. So I removed the test case in sonic-net#22562 and add 2 test cases with more checks in this PR. How did you do it? 1. test_lldp_interfaces Purpose: Verify LLDP functionality across all interfaces without config reload Key Features: Validates LLDP table completeness Verifies lldpcli interface discovery Checks lldpctl_facts consistency Validates chassis ID and capabilities Active syslog monitoring via loganalyzer Test Steps: Recording Phase: Capture all interfaces from show interface status LLDP Table Verification: Compare LLDP table with interface status (admin up, no PortChannels) lldpcli Verification: Compare lldpcli output with interface status (all interfaces, no PortChannels) lldpctl_facts Verification: Compare lldpctl_facts with interface status (admin up, no PortChannels) Consistency Check: Verify all lldpctl_facts interfaces exist in lldpcli Chassis Verification: Validate chassis MAC address and capabilities (Bridge: on, Router: on, Wlan: off, Station: off) 2. test_lldp_interface_config_reload Purpose: Verify LLDP functionality persists correctly after config reload Key Features: Tests LLDP recovery after config reload Validates interface persistence Ensures neighbor rediscovery Monitors for LLDP-specific errors while ignoring expected reload errors Addresses the core issue from GitHub [test gap] check lldp neighbors after config reload sonic-net#22376 Test Steps: Pre-Reload Recording: Capture all interfaces before config reload Config Reload: Perform safe config reload with interface checks Stabilization Wait: Wait for critical services and LLDP convergence LLDP Table Verification: Compare LLDP table with pre-reload interfaces (admin up, no PortChannels) lldpcli Verification: Compare lldpcli output with pre-reload interfaces (all interfaces, no PortChannels) lldpctl_facts Verification: Compare lldpctl_facts with pre-reload interfaces (admin up, no PortChannels) Consistency Check: Verify all lldpctl_facts interfaces exist in lldpcli Chassis Verification: Validate chassis MAC and capabilities remain correct How did you verify/test it? run it on testbed. Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description of PR
Summary:
Fixes # (issue)
Address #22376
Type of change
Back port request
Approach
What is the motivation for this PR?
To add 2 new test cases to check if all lldp neighbors exist and if all interfaces exists in lldpcli show interfaces.
We don't have a test case to cover the issue we found in PR: sonic-net/sonic-buildimage#25436
#22562 cover some scenarios, but not all of them. So I removed the test case in #22562 and add 2 test cases with more checks in this PR.
How did you do it?
1. test_lldp_interfaces
Purpose: Verify LLDP functionality across all interfaces without config reload
Key Features:
Test Steps:
show interface status2. test_lldp_interface_config_reload
Purpose: Verify LLDP functionality persists correctly after config reload
Key Features:
Test Steps:
How did you verify/test it?
run it on testbed.
Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation