Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[acl mirror action] Mirror session ref count fix at acl rule attachment #1761

Merged
merged 6 commits into from
Jul 3, 2021

Conversation

wendani
Copy link
Contributor

@wendani wendani commented May 31, 2021

Why I did it
Retry logic was introduced to ACL rule mirror action in #1486

This assumes that mirror session may not be ready when the associated ACL rule is processed.

If mirror entry exists (CONFIG_DB processed) but mirror session is not created in sai/asic, AclRuleMirror::create() is called twice, one at processing CONFIG_DB ACL rule config, and one at later mirror session activation notify (i.e., mirror session creation event in sai/asic). In this case, mirror session reference count is incremented twice for the same rule, and mirror session removal will fail due to non-zero ref count even if ACL rule is removed.

What I did
This PR fixes the above scenario.

How I verified it
Fix is validated by developing vs test, which is piggy-backed onto test_AclBindMirror

Details if related

vs test failure before the change
=========================================================================== FAILURES ===========================================================================
________________________________________________________________ TestMirror.test_AclBindMirror _________________________________________________________________

self = <test_mirror.TestMirror object at 0x7fd282c40ef0>, dvs = <conftest.DockerVirtualSwitch object at 0x7fd282c3def0>
testlog = <function testlog at 0x7fd282f8d378>

    def test_AclBindMirror(self, dvs, testlog):
        self.setup_db(dvs)
    
        self._test_AclBindMirror(dvs, testlog)
>       self._test_AclBindMirror(dvs, testlog, create_seq_test=True)

test_mirror.py:896: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <test_mirror.TestMirror object at 0x7fd282c40ef0>, dvs = <conftest.DockerVirtualSwitch object at 0x7fd282c3def0>
testlog = <function testlog at 0x7fd282f8d378>, create_seq_test = True

    def _test_AclBindMirror(self, dvs, testlog, create_seq_test=False):
        """
        This test tests ACL associated with mirror session with DSCP value
        The DSCP value is tested on both with mask and without mask
        """
    
        session = "MIRROR_SESSION"
        acl_table = "MIRROR_TABLE"
        acl_rule = "MIRROR_RULE"
    
        # bring up port; assign ip; create neighbor; create route
        self.set_interface_status(dvs, "Ethernet32", "up")
        self.add_ip_address("Ethernet32", "20.0.0.0/31")
        self.add_neighbor("Ethernet32", "20.0.0.1", "02:04:06:08:10:12")
        if create_seq_test == False:
            self.add_route(dvs, "4.4.4.4", "20.0.0.1")
    
        # create mirror session
        self.create_mirror_session(session, "3.3.3.3", "4.4.4.4", "0x6558", "8", "100", "0")
        assert self.get_mirror_session_state(session)["status"] == ("active" if create_seq_test == False else "inactive")
    
        # check mirror session in asic database
        tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_MIRROR_SESSION")
        assert len(tbl.getKeys()) == (1 if create_seq_test == False else 0)
        if create_seq_test == False:
            mirror_session_oid = tbl.getKeys()[0]
    
        # create acl table
        self.create_acl_table(acl_table, ["Ethernet0", "Ethernet4"])
    
        # create acl rule with dscp value 48
        self.create_mirror_acl_dscp_rule(acl_table, acl_rule, "48", session)
    
        # acl rule creation check
        tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY")
        rule_entries = [k for k in tbl.getKeys() if k not in dvs.asicdb.default_acl_entries]
        assert len(rule_entries) == (1 if create_seq_test == False else 0)
    
        if create_seq_test == True:
            self.add_route(dvs, "4.4.4.4", "20.0.0.1")
    
            assert self.get_mirror_session_state(session)["status"] == "active"
    
            # assert mirror session in asic database
            tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_MIRROR_SESSION")
            assert len(tbl.getKeys()) == 1
            mirror_session_oid = tbl.getKeys()[0]
    
            # assert acl rule is created
            tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY")
            rule_entries = [k for k in tbl.getKeys() if k not in dvs.asicdb.default_acl_entries]
            assert len(rule_entries) == 1
    
        (status, fvs) = tbl.get(rule_entries[0])
        assert status == True
        for fv in fvs:
            if fv[0] == "SAI_ACL_ENTRY_ATTR_FIELD_DSCP":
                assert fv[1] == "48&mask:0x3f"
            if fv[0] == "SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS":
                assert fv[1] == "1:" + mirror_session_oid
    
        # remove acl rule
        self.remove_mirror_acl_dscp_rule(acl_table, acl_rule)
    
        # create acl rule with dscp value 16/16
        self.create_mirror_acl_dscp_rule(acl_table, acl_rule, "16/16", session)
    
        # assert acl rule is created
        tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY")
        rule_entries = [k for k in tbl.getKeys() if k not in dvs.asicdb.default_acl_entries]
        assert len(rule_entries) == 1
    
        (status, fvs) = tbl.get(rule_entries[0])
        assert status == True
        for fv in fvs:
            if fv[0] == "SAI_ACL_ENTRY_ATTR_FIELD_DSCP":
                assert fv[1] == "16&mask:0x10"
            if fv[0] == "SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS":
                assert fv[1] == "1:" + mirror_session_oid
    
        # remove acl rule
        self.remove_mirror_acl_dscp_rule(acl_table, acl_rule)
    
        # remove acl table
        self.remove_acl_table(acl_table)
    
        # remove mirror session
        self.remove_mirror_session(session)
    
        # assert no mirror session in asic database
        tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_MIRROR_SESSION")
>       assert len(tbl.getKeys()) == 0
E       assert 1 == 0
E         +1
E         -0

test_mirror.py:884: AssertionError
=================================================================== short test summary info ====================================================================
FAILED test_mirror.py::TestMirror::test_AclBindMirror - assert 1 == 0
================================================================= 1 failed in 87.26s (0:01:27) =================================================================

@wendani wendani changed the title [mirror] Mirror session ref count fix at acl rule attachment [acl mirror action] Mirror session ref count fix at acl rule attachment Jun 1, 2021
Copy link
Collaborator

@prsunny prsunny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, request @qiluo-msft to sign-off as original reviewer of #1486

@prsunny prsunny requested a review from qiluo-msft June 2, 2021 17:23
@prsunny
Copy link
Collaborator

prsunny commented Jun 2, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prsunny
Copy link
Collaborator

prsunny commented Jun 14, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prsunny
Copy link
Collaborator

prsunny commented Jun 14, 2021

@wendani , can you force-push?

@prsunny prsunny merged commit d594c47 into sonic-net:master Jul 3, 2021
qiluo-msft pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jul 5, 2021
[flex-counters] Delay flex counters stats init for faster boot time (sonic-net/sonic-swss#1803)
[mirror] Detach session dst ip from route orch LPM calculation regardless of session status at session CONFIG DB removal (sonic-net/sonic-swss#1800)
[Dynamic Buffer Calc] Support dynamic buffer calculation on top of port auto negotiation (sonic-net/sonic-swss#1762)
[neighorch] VOQ encap index change handling (sonic-net/sonic-swss#1729)
[neighorch] Mac for voq neighbors in VS platforms (sonic-net/sonic-swss#1724)
[acl mirror action] Mirror session ref count fix at acl rule attachment (sonic-net/sonic-swss#1761)
judyjoseph pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Aug 7, 2021
[flex-counters] Delay flex counters stats init for faster boot time (sonic-net/sonic-swss#1803)
[mirror] Detach session dst ip from route orch LPM calculation regardless of session status at session CONFIG DB removal (sonic-net/sonic-swss#1800)
[Dynamic Buffer Calc] Support dynamic buffer calculation on top of port auto negotiation (sonic-net/sonic-swss#1762)
[neighorch] VOQ encap index change handling (sonic-net/sonic-swss#1729)
[neighorch] Mac for voq neighbors in VS platforms (sonic-net/sonic-swss#1724)
[acl mirror action] Mirror session ref count fix at acl rule attachment (sonic-net/sonic-swss#1761)
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
[flex-counters] Delay flex counters stats init for faster boot time (sonic-net/sonic-swss#1803)
[mirror] Detach session dst ip from route orch LPM calculation regardless of session status at session CONFIG DB removal (sonic-net/sonic-swss#1800)
[Dynamic Buffer Calc] Support dynamic buffer calculation on top of port auto negotiation (sonic-net/sonic-swss#1762)
[neighorch] VOQ encap index change handling (sonic-net/sonic-swss#1729)
[neighorch] Mac for voq neighbors in VS platforms (sonic-net/sonic-swss#1724)
[acl mirror action] Mirror session ref count fix at acl rule attachment (sonic-net/sonic-swss#1761)
judyjoseph pushed a commit that referenced this pull request Aug 10, 2021
…nt (#1761)

* Fix mirror session ref count at acl rule attachement
Signed-off-by: wenda.ni <[email protected]>
qiluo-msft pushed a commit that referenced this pull request Sep 1, 2021
…nt (#1761)

* Fix mirror session ref count at acl rule attachement
Signed-off-by: wenda.ni <[email protected]>
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-swss that referenced this pull request Oct 5, 2021
…nt (sonic-net#1761)

* Fix mirror session ref count at acl rule attachement
Signed-off-by: wenda.ni <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants