[sai_test] Add opt-in port and BFD notification test cases - #2336
[sai_test] Add opt-in port and BFD notification test cases#2336nicholasching wants to merge 10 commits into
Conversation
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hi @kcudnik @tjchadaga, when you have a chance, could you please invoke azure pipelines to run checks. Thanks! |
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
| from sai_utils import sai_ipaddress, sai_ipprefix | ||
|
|
||
|
|
||
| PORT_NOTIFICATION_TYPE = 0 |
There was a problem hiding this comment.
This should come from the enum defined in PR 2335
| self.start_session() | ||
| self.bfd_event(SAI_BFD_SESSION_STATE_UP) | ||
| self.assert_bfd_state(SAI_BFD_SESSION_STATE_UP) | ||
| self.assert_vpp_bfd_state("up") |
There was a problem hiding this comment.
Please check on asic type before calling vpp specific check.
There was a problem hiding this comment.
Good catch; thanks Fred. All done in f7c395d, now check self.platform == VPP_PLATFORM before call VPP checks.
| def _run(self, interface_name): | ||
| from scapy.all import sniff | ||
|
|
||
| while not self.stop_event.is_set(): |
There was a problem hiding this comment.
AI points out a gap between sniff calls that may cause loss of bfd message because between sniff calls, there is no BPF socket to receive the bfd packet. It suggests to use AsyncSniffer.
def start(self):
import functools
from scapy.all import AsyncSniffer
for name in self.interface_names:
ready = threading.Event()
sniffer = AsyncSniffer(
iface=name,
# Narrow the match in BPF rather than in Python. The filter is
# now compiled once instead of twice a second, and the responder
# never sees its own replies looped back by AF_PACKET.
filter="udp dst port {} and src host {}".format(
self.udp_port, self.local_ip
),
store=False,
# functools.partial, not a lambda: a lambda closing over the
# loop variable would bind every sniffer to the last interface.
prn=functools.partial(self._respond, name),
started_callback=ready.set,
)
sniffer.start()
self.sniffers.append(sniffer)
# start() returns before the capture socket is bound. Without this
# the first control packets are missed exactly as before.
if not ready.wait(timeout=SNIFFER_START_TIMEOUT):
self.stop()
raise AssertionError(
"BFD responder failed to start capture on {}".format(name)
)
def stop(self):
for sniffer in self.sniffers:
try:
sniffer.stop()
except Exception:
# stop() raises if the sniffer never reached the run loop.
pass
self.sniffers = []
There was a problem hiding this comment.
Oh that is very true! Implemented in 2a0aa35, BFD responder now uses one continuously running AsyncSniffer per peer interface and waits for capture startup before creating the session, avoiding packet-loss windows between polling sniff calls.
|
|
||
| def setUp(self): | ||
| params = test_params_get() or {} | ||
| if params.get(PLATFORM_PARAM) != VPP_PLATFORM: |
There was a problem hiding this comment.
do we need this check? what is required for another platform to enable notification test? There is also notification test opt-in flag. If it is not supported in certain flag, they can opt it out.
Our goal is making this test suitable for other platforms. They can add platform specific check. But without it, it still works.
There was a problem hiding this comment.
do we need this check? what is required for another platform to enable notification test?
Hi Fred, yes, unfortunately, unlike the port tests, the current BFD test implementation is VPP-specific. Since it constructs the complete working BFD environment inside the VPP test container, it uses the VPP LAG topology, VPP-specific addressing and routing, Linux peer interfaces, a Scapy BFD responder, and VPP state inspection. Other platforms would need equivalent platform-specific BFD fixtures, which have not been implemented yet.
Our goal is making this test suitable for other platforms. They can add platform specific check. But without it, it still works.
I've done some more research; it is possible to move the VPP-specific topology and observability from the shared SAI test suite into the SAI VPP UT harness. For a final structure that looks something like this:
SAI PR #2336:
Generic notification tests and BFD fixture interface
sonic-sairedis Phase 4 PR:
VPP BFD fixture
VPP Docker image injection
VPP test parameters
VPP-specific CI invocation
I'm making this change now. This should make the test fully generic and require the platform under test to provide their platform-specific fixture implementation instead.
There was a problem hiding this comment.
Hi Fred, I’ve pushed a couple more commits to generalize the notification tests. The harness is now responsible for supplying the platform-specific BFD fixture to SAI notification tests, moving the VPP-specific code from SAI into sonic-sairedis. Other platforms will be responsible for providing their own fixture when enabling the BFD cases.
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
| self.start_session() | ||
| self.bfd_event(SAI_BFD_SESSION_STATE_UP) | ||
| self.assert_bfd_state(SAI_BFD_SESSION_STATE_UP) | ||
| self.assert_vpp_bfd_state("up") |
There was a problem hiding this comment.
Let's move the platform check to the caller. If other platforms are added to this test, we don't want this function calls assert_vpp_bfd_state, assert_cisco_8000_bfd_state etc. It should call the function corresponding to the platform.
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Context / motivation
Part of the SAIVPP unit-test framework.
sai_testcurrently has no coverage for asynchronous SAI notifications: nothing exercisesSAI_SWITCH_ATTR_PORT_STATE_CHANGE_NOTIFYorSAI_SWITCH_ATTR_BFD_SESSION_STATE_CHANGE_NOTIFY. This adds the first tests for that path, using the saithrift notification RPCs added separately.What this change does
test/sai_test/sai_notification_test.py. New module with five classes:PortStateChangeTest,PortStateRecoveryTest,BfdSessionUpTest,BfdSessionDownTestandBfdMultihopTest.vpp_notification_testPTF parameter; the BFD classes additionally requireSIMULATE_SONIC=1. With neither set the module is inert.NO-CARRIERwhilevppctl show hardware-interfacesstill reports the link up and no interface event is ever raised.vppctloutput rather than only re-reading the SAI attribute that the notification path itself writes. All SAI objects and link state are restored infinally/tearDown.Scope / risk
PORT_COUNT=32with per-test isolation, which reported 90 observed selectors and zero regressions against the existing stable baseline.Dependencies
Requires the saithrift notification bridge PR, #2335; must be merged before this PR can be merged.