sonic-swss icon indicating copy to clipboard operation
sonic-swss copied to clipboard

[mirror] Update session status at port oper status change for mirror destination in directly connected subnet case

Open wendani opened this issue 4 years ago • 1 comments

Why I did it In the case of mirror destination in directly connected subnet, reflect actual mirror session status in STATE_DB at port oper status change.

What I did

  • In current code, MirrorOrch::getNeighborInfo returns true in the following two scenarios:
    // 1) If session destination IP is directly connected, and the neighbor
    //    information is retrieved successfully, then continue.
    // 2) If session has next hop, and the next hop's neighbor information is
    //    retrieved successfully, then continue.

In case 1) directly connected subnet, we refine the return-true condition, as in case 2), to check that session has a valid next hop, which corresponds to directly connected subnet advertisement by frr. This is to distinguish from the opposite, directly connected subnet withdrawal case at port oper status down event, where neighbor info can still be retrieved successfully.

  • A generic change: Remove data member NeighborEntry neighbor from MirrorEntry as its usage is only of local scope inside member function getNeighborInfo. As so, there is no need to track it as metadata for a mirror session.

How I verified it Newly added vs test for physical port;

Extension to test_MirrorToLagAddRemove for LAG

Also, added neighbor mac update, add, and remove vs test.

Details if related A next hop object update on directly connected subnet come in two forms:

  • In directly connected subnet advertisement, the corresponding next hop object comes in the form of {"0.0.0.0", alias}, where alias = "Ethernet49", "PortChannel0001", or "Vlan1000".
  • In directly connected subnet withdrawal, the corresponding next hop object comes in the form of {"0.0.0.0", ""}

Use alias string empty check to distinguish between advertisement and withdrawal.

vs test failure output without the change

Physical port
========================================================================== FAILURES ==========================================================================
_________________________________________________ TestMirror.test_MirrorToPortDestDirectSubnetSessionStatus __________________________________________________

self = <test_mirror.TestMirror object at 0x7fb35a09a710>, dvs = <conftest.DockerVirtualSwitch object at 0x7fb35a080d68>
testlog = <function testlog at 0x7fb35a0c9268>

    def test_MirrorToPortDestDirectSubnetSessionStatus(self, dvs, testlog):
        self.setup_db(dvs)
    
        session = "TEST_SESSION"
        src_ip = "1.1.1.1"
        dst_ip = "2.2.2.2"
        gre_type= "0x6558"
        dscp = "8"
        ttl = "100"
        queue = "0"
    
        PORT_UNDER_TEST = "Ethernet16"
        PORT_ADDR_UNDER_TEST = "2.2.2.1/30"
        DIRECT_SUBNET_UNDER_TEST = "2.2.2.0/30"
    
        # Create mirror session
        self.create_mirror_session(session, src_ip, dst_ip, gre_type, dscp, ttl, queue)
        assert self.get_mirror_session_status(session) == INACTIVE
    
        self.set_interface_status(dvs, PORT_UNDER_TEST, "up")
        assert self.get_mirror_session_status(session) == INACTIVE
    
        self.add_ip_address(PORT_UNDER_TEST, PORT_ADDR_UNDER_TEST)
        assert self.get_mirror_session_status(session) == INACTIVE
    
        self.add_neighbor(PORT_UNDER_TEST, dst_ip, "02:04:06:08:10:12")
        assert self.get_mirror_session_status(session) == ACTIVE
        tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_MIRROR_SESSION")
        assert len(tbl.getKeys()) == 1
    
        # Mimic host interface oper status down that causes frr to withdraw
        # directly connected subnet prefix
        self.remove_route_appl_db(dvs, DIRECT_SUBNET_UNDER_TEST)
>       assert self.get_mirror_session_status(session) == INACTIVE
E       AssertionError: assert 'active' == 'inactive'
E         - inactive
E         ? --
E         + active

test_mirror.py:933: AssertionError
================================================================== short test summary info ===================================================================
FAILED test_mirror.py::TestMirror::test_MirrorToPortDestDirectSubnetSessionStatus - AssertionError: assert 'active' == 'inactive'
================================================================ 1 failed in 64.72s (0:01:04) ================================================================
LAG
=========================================================================== FAILURES ============================================================================
_____________________________________________________________ TestMirror.test_MirrorToLagAddRemove ______________________________________________________________

self = <test_mirror.TestMirror object at 0x7f4e99789940>, dvs = <conftest.DockerVirtualSwitch object at 0x7f4e997ae710>
testlog = <function testlog at 0x7f4e99ab3378>

    def test_MirrorToLagAddRemove(self, dvs, testlog):
        """
        This test covers basic mirror session creation and removal operations
        with destination port sits in a LAG
        Operation flow:
        1. Create mirror sesion
        2. Create LAG; assign IP; create directly connected neighbor
           The session shoudl be up only at this time.
        3. Remove neighbor; remove IP; remove LAG
        4. Remove mirror session
    
        """
        self.setup_db(dvs)
    
        session = "TEST_SESSION"
    
        marker = dvs.add_log_marker()
        # create mirror session
        self.create_mirror_session(session, "10.10.10.10", "11.11.11.11", "0x6558", "8", "100", "0")
        assert self.get_mirror_session_state(session)["status"] == "inactive"
        self.check_syslog(dvs, marker, "Attached next hop observer .* for destination IP 11.11.11.11", 1)
    
        # create port channel; create port channel member
        self.create_port_channel(dvs, "008")
        self.create_port_channel_member("008", "Ethernet88")
    
        # bring up port channel and port channel member
        self.set_interface_status(dvs, "PortChannel008", "up")
        self.set_interface_status(dvs, "Ethernet88", "up")
    
        # add ip address to port channel 008
        self.add_ip_address("PortChannel008", "11.11.11.0/24")
        assert self.get_mirror_session_state(session)["status"] == "inactive"
    
        # create neighbor to port channel 008
        self.add_neighbor("PortChannel008", "11.11.11.11", "88:88:88:88:88:88")
        assert self.get_mirror_session_state(session)["status"] == "active"
    
        # check asic database
        tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_MIRROR_SESSION")
        assert len(tbl.getKeys()) == 1
    
        (status, fvs) = tbl.get(tbl.getKeys()[0])
        assert status == True
        for fv in fvs:
            if fv[0] == "SAI_MIRROR_SESSION_ATTR_MONITOR_PORT":
                assert dvs.asicdb.portoidmap[fv[1]] == "Ethernet88"
            elif fv[0] == "SAI_MIRROR_SESSION_ATTR_DST_MAC_ADDRESS":
                assert fv[1] == "88:88:88:88:88:88"
    
        # Oper down lag
        self.set_lag_oper_status(dvs, "PortChannel008", "down")
>       assert self.get_mirror_session_status(session) == INACTIVE
E       AssertionError: assert 'active' == 'inactive'
E         - inactive
E         ? --
E         + active

test_mirror.py:462: AssertionError
==================================================================== short test summary info ====================================================================
FAILED test_mirror.py::TestMirror::test_MirrorToLagAddRemove - AssertionError: assert 'active' == 'inactive'
================================================================= 1 failed in 70.00s (0:01:09) ==================================================================

wendani avatar Apr 26 '21 19:04 wendani

This pull request introduces 1 alert when merging 5592cb57c30998209eb32feac50bb5b79f1a9afb into d9f28b64255db54310d3398119f13dfb3203f311 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Apr 26 '21 20:04 lgtm-com[bot]