[sub intf] Port object reference count update #1712

wendani · 2021-04-17T01:08:20Z

What I did

At sub port Port object instantiation

Init sub port Port object reference count
Increase parent port Port object reference count

At sub port Port object removal

Check sub port Port object reference count drops to zero
Decrease parent port Port object reference count

At physical port removal

Check physical port Port object reference count drops to zero

Why I did it
Enhancement

How I verified it

vs test extension

Issue parent port removal at APPL_DB level before removing sub port interface. Verify that parent port persists in ASIC_DB until sub interface is removed.

Without the change, extended vs test fails:

Physical port

========================================================================== FAILURES ===========================================================================
_________________________________________________________ TestSubPortIntf.test_sub_port_intf_removal __________________________________________________________

self = <test_sub_port_intf.TestSubPortIntf object at 0x7f92a4f56748>, dvs = <conftest.DockerVirtualSwitch object at 0x7f92a4f569e8>

    def test_sub_port_intf_removal(self, dvs):
        self.connect_dbs(dvs)
    
        self._test_sub_port_intf_removal(dvs, self.SUB_PORT_INTERFACE_UNDER_TEST)
        self._test_sub_port_intf_removal(dvs, self.LAG_SUB_PORT_INTERFACE_UNDER_TEST)
    
        self._test_sub_port_intf_removal(dvs, self.SUB_PORT_INTERFACE_UNDER_TEST, vrf_name=self.VRF_UNDER_TEST)
        self._test_sub_port_intf_removal(dvs, self.LAG_SUB_PORT_INTERFACE_UNDER_TEST, vrf_name=self.VRF_UNDER_TEST)
    
        self._test_sub_port_intf_removal(dvs, self.SUB_PORT_INTERFACE_UNDER_TEST, vrf_name=self.VNET_UNDER_TEST)
        self._test_sub_port_intf_removal(dvs, self.LAG_SUB_PORT_INTERFACE_UNDER_TEST, vrf_name=self.VNET_UNDER_TEST)
    
>       self._test_sub_port_intf_removal(dvs, self.SUB_PORT_INTERFACE_UNDER_TEST, removal_seq_test=True)

test_sub_port_intf.py:1058: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test_sub_port_intf.py:945: in _test_sub_port_intf_removal
    self.check_sub_port_intf_fvs(self.state_db, state_tbl_name, sub_port_intf_name, fv_dict)
test_sub_port_intf.py:334: in check_sub_port_intf_fvs
    db.wait_for_field_match(table_name, key, fv_dict)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <dvslib.dvs_database.DVSDatabase object at 0x7f92a4ea3710>, table_name = 'PORT_TABLE', key = 'Ethernet64.10', expected_fields = {'state': 'ok'}
polling_config = PollingConfig(polling_interval=0.01, timeout=5.0, strict=True), failure_message = None

    def wait_for_field_match(
        self,
        table_name: str,
        key: str,
        expected_fields: Dict[str, str],
        polling_config: PollingConfig = PollingConfig(),
        failure_message: str = None,
    ) -> Dict[str, str]:
        """Wait for the entry stored at `key` to have the specified field/values and retrieve it.
    
        This method is useful if you only care about the contents of a subset of the fields stored
        in the specified entry.
    
        Args:
            table_name: The name of the table where the entry is stored.
            key: The key that maps to the entry being checked.
            expected_fields: The fields and their values we expect to see in the entry.
            polling_config: The parameters to use to poll the db.
            failure_message: The message to print if the call times out. This will only take effect
                if the PollingConfig is set to strict.
    
        Returns:
            The entry stored at `key`. If no entry is found, then an empty Dict is returned.
        """
    
        def access_function():
            fv_pairs = self.get_entry(table_name, key)
            return (
                all(fv_pairs.get(k) == v for k, v in expected_fields.items()),
                fv_pairs,
            )
    
        status, result = wait_for_result(
            access_function, self._disable_strict_polling(polling_config)
        )
    
        if not status:
            message = failure_message or (
                f"Expected field/value pairs not found: expected={expected_fields}, "
                f'received={result}, key="{key}", table="{table_name}"'
            )
>           assert not polling_config.strict, message
E           AssertionError: Expected field/value pairs not found: expected={'state': 'ok'}, received={}, key="Ethernet64.10", table="PORT_TABLE"

dvslib/dvs_database.py:203: AssertionError
=================================================================== short test summary info ===================================================================
FAILED test_sub_port_intf.py::TestSubPortIntf::test_sub_port_intf_removal - AssertionError: Expected field/value pairs not found: expected={'state': 'ok'}, ...
================================================================ 1 failed in 90.46s (0:01:30) =================================================================
root@5bf4ef40dc91:/var/wendani/src/sonic-buildimage/src/sonic-swss/tests#

LAG

========================================================================== FAILURES ===========================================================================
_________________________________________________________ TestSubPortIntf.test_sub_port_intf_removal __________________________________________________________

self = <test_sub_port_intf.TestSubPortIntf object at 0x7fe7bc392278>, dvs = <conftest.DockerVirtualSwitch object at 0x7fe7c07cf7b8>

    def test_sub_port_intf_removal(self, dvs):
        self.connect_dbs(dvs)
    
        self._test_sub_port_intf_removal(dvs, self.SUB_PORT_INTERFACE_UNDER_TEST)
        self._test_sub_port_intf_removal(dvs, self.LAG_SUB_PORT_INTERFACE_UNDER_TEST)
    
        self._test_sub_port_intf_removal(dvs, self.SUB_PORT_INTERFACE_UNDER_TEST, vrf_name=self.VRF_UNDER_TEST)
        self._test_sub_port_intf_removal(dvs, self.LAG_SUB_PORT_INTERFACE_UNDER_TEST, vrf_name=self.VRF_UNDER_TEST)
    
        self._test_sub_port_intf_removal(dvs, self.SUB_PORT_INTERFACE_UNDER_TEST, vrf_name=self.VNET_UNDER_TEST)
        self._test_sub_port_intf_removal(dvs, self.LAG_SUB_PORT_INTERFACE_UNDER_TEST, vrf_name=self.VNET_UNDER_TEST)
    
        #self._test_sub_port_intf_removal(dvs, self.SUB_PORT_INTERFACE_UNDER_TEST, removal_seq_test=True)
>       self._test_sub_port_intf_removal(dvs, self.LAG_SUB_PORT_INTERFACE_UNDER_TEST, removal_seq_test=True)

test_sub_port_intf.py:1059: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test_sub_port_intf.py:992: in _test_sub_port_intf_removal
    self.check_sub_port_intf_profile_removal(rif_oid)
test_sub_port_intf.py:272: in check_sub_port_intf_profile_removal
    self.asic_db.wait_for_deleted_keys(ASIC_RIF_TABLE, [rif_oid])
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <dvslib.dvs_database.DVSDatabase object at 0x7fe7bc3d6cc0>, table_name = 'ASIC_STATE:SAI_OBJECT_TYPE_ROUTER_INTERFACE'
deleted_keys = ['oid:0x6000000000616'], polling_config = PollingConfig(polling_interval=0.01, timeout=5.0, strict=True), failure_message = None

    def wait_for_deleted_keys(
        self,
        table_name: str,
        deleted_keys: List[str],
        polling_config: PollingConfig = PollingConfig(),
        failure_message: str = None,
    ) -> List[str]:
        """Wait for the specfied keys to no longer exist in the table.
    
        Args:
            table_name: The name of the table from which to fetch the keys.
            deleted_keys: The keys we expect to be removed from the table.
            polling_config: The parameters to use to poll the db.
            failure_message: The message to print if the call times out. This will only take effect
                if the PollingConfig is set to strict.
    
        Returns:
            The keys stored in the table. If no keys are found, then an empty List is returned.
        """
    
        def access_function():
            keys = self.get_keys(table_name)
            return (all(key not in keys for key in deleted_keys), keys)
    
        status, result = wait_for_result(
            access_function, self._disable_strict_polling(polling_config)
        )
    
        if not status:
            expected = [key for key in result if key not in deleted_keys]
            message = failure_message or (
                f"Unexpected keys found: expected={expected}, received={result}, "
                f'table="{table_name}"'
            )
>           assert not polling_config.strict, message
E           AssertionError: Unexpected keys found: expected=['oid:0x60000000005a7'], received=('oid:0x60000000005a7', 'oid:0x6000000000616'), table="ASIC_STATE:SAI_OBJECT_TYPE_ROUTER_INTERFACE"

dvslib/dvs_database.py:437: AssertionError
=================================================================== short test summary info ===================================================================
FAILED test_sub_port_intf.py::TestSubPortIntf::test_sub_port_intf_removal - AssertionError: Unexpected keys found: expected=['oid:0x60000000005a7'], receive...
================================================================ 1 failed in 89.77s (0:01:29) =================================================================

syslog

497323 Apr 17 00:44:30.911338 9faa323d761d ERR #orchagent: :- removeLag: Failed to remove LAG PortChannel1 lid:2000000000615
497324 Apr 17 00:44:30.911364 9faa323d761d ERR #orchagent: :- handleSaiRemoveStatus: Encountered failure in remove operation, exiting orchagent, SAI API: SAI_A
       PI_LAG, status: SAI_STATUS_OBJECT_IN_USE
497325 Apr 17 00:44:30.911904 9faa323d761d NOTICE #orchagent: :- uninitialize: begin
497326 Apr 17 00:44:30.911975 9faa323d761d NOTICE #orchagent: :- uninitialize: begin
497327 Apr 17 00:44:30.911990 9faa323d761d NOTICE #orchagent: :- ~RedisChannel: join ntf thread begin
497328 Apr 17 00:44:30.912075 9faa323d761d NOTICE #orchagent: :- ~RedisChannel: join ntf thread end
497329 Apr 17 00:44:30.912152 9faa323d761d NOTICE #orchagent: :- clear_local_state: clearing local state
497330 Apr 17 00:44:30.912172 9faa323d761d NOTICE #orchagent: :- meta_init_db: begin
497331 Apr 17 00:44:30.913144 9faa323d761d NOTICE #orchagent: :- meta_init_db: end
497332 Apr 17 00:44:30.913164 9faa323d761d NOTICE #orchagent: :- uninitialize: end
497333 Apr 17 00:44:30.913224 9faa323d761d NOTICE #orchagent: :- stopRecording: stopped recording
497334 Apr 17 00:44:30.913237 9faa323d761d NOTICE #orchagent: :- stopRecording: closed recording file: sairedis.rec
497335 Apr 17 00:44:30.913597 9faa323d761d NOTICE #orchagent: :- uninitialize: end
497336 Apr 17 00:44:31.980229 9faa323d761d INFO #supervisord 2021-04-17 00:44:31,979 INFO exited: orchagent (exit status 1; not expected)

Details if related
Contains, and therefore after #1642

Incremental change to #1642 base can be found at wendani#9

[vstest/subintf] Add vs test to validate sub interface ingress to a vnet #1642

lguohan · 2021-04-24T19:57:44Z

can you describe why you did this in detail? need to understand why first.

lguohan · 2021-05-07T01:48:04Z

@wendani , is this a bug fix?

prsunny · 2021-05-07T20:55:52Z

this seems to be having the similar changes in PR #1521. Is this the superset?

wendani · 2021-05-08T02:36:43Z

@wendani , is this a bug fix?

There are two parts of changes in this pr:

One is the reference count of sub port Port object itself. This part is bug fix. The motivation is that for each Port object instantiated, it should be initialized in PortsOrch::m_port_ref_count. For each Port object to be removed, its m_port_ref_count check should be zero to proceed further. These operations are generic and therefore should also apply to sub port Port object.

Currently, Port object reference count increments

When a router interface is created on top.
When an ACL table is bound to Port (i.e., becoming a member of the ACL table group associated with the Port object).

Port object reference count decrements

When a router interface on top is removed.
When an ACL table is unbound from Port (i.e, removing membership from the ACL table group associated with the Port object).

Item 1, router interface creation and removal, is of direct relevance to sub port Port object.

The other part of change is the reference count update to parent port Port object when a sub port Port object is added/removed on top. This part is motivated by change in Add reference counting to ports for ACL bindings.

For a sub port Port add/removal, I think reference count of parent port Port object should be updated accordingly. However, I do not have strong justification for this change, just following the practice in #1614

wendani · 2021-05-08T07:45:30Z

this seems to be having the similar changes in PR #1521. Is this the superset?

Yes, this is incremental development on top of #1521. When #1521 is merged, we can see the incremental part clearly.

#1521 fixes the sub interface removal sequence.

        if (getIntfIpCount(alias))
        {
            return false;
        }

Before change in #1521, getIntfIpCount(alias) checks parent port ip count not sub port ip count, for sub port interface removal.

neethajohn · 2021-05-24T18:44:45Z

@wendani , please rebase so that incremental changes are clearly visible

neethajohn · 2021-05-25T18:26:07Z

@wendani , can you rebase, exclude #1642 and push only the changes relevant for this PR

Signed-off-by: Wenda Ni <wonda.ni@gmail.com>

wendani · 2021-05-25T22:12:15Z

@wendani , can you rebase, exclude #1642 and push only the changes relevant for this PR

done

tests/test_sub_port_intf.py

neethajohn · 2021-05-26T22:52:56Z

/azp run

azure-pipelines · 2021-05-26T22:53:10Z

Azure Pipelines successfully started running 1 pipeline(s).

neethajohn · 2021-05-26T22:53:32Z

LGTM. @prsunny, can you take a look?

neethajohn · 2021-06-01T16:19:34Z

/azp run

azure-pipelines · 2021-06-01T16:19:50Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Wenda Ni <wonda.ni@gmail.com> There are two parts of changes in this pr: The motivation is that for each Port object instantiated, it should be initialized in PortsOrch::m_port_ref_count. For each Port object to be removed, its m_port_ref_count check should be zero to proceed further. These operations are generic and therefore should also apply to sub port Port object. Currently, Port object reference count increments - When a router interface is created on top. - When an ACL table is bound to Port (i.e., becoming a member of the ACL table group associated with the Port object). Port object reference count decrements - When a router interface on top is removed. - When an ACL table is unbound from Port (i.e, removing membership from the ACL table group associated with the Port object). Item 1, router interface creation and removal, is of direct relevance to sub port Port object. The other part of change is the reference count update to parent port Port object when a sub port Port object is added/removed on top. This part is motivated by change in Add reference counting to ports for ACL bindings. What I did At sub port Port object instantiation - Init sub port Port object reference count - Increase parent port Port object reference count At sub port Port object removal - Check sub port Port object reference count drops to zero - Decrease parent port Port object reference count At physical port removal - Check physical port Port object reference count drops to zero How I verified it vs test extension Issue parent port removal at APPL_DB level before removing sub port interface. Verify that parent port persists in ASIC_DB until sub interface is removed. Without the change, extended vs test fails:

Advance submodule head for sonic-swss on 202012 bb383be2 [Dynamic Buffer Calc][Mellanox] Bug fixes and enhancements for the lua plugins for buffer pool calculation and headroom checking (sonic-net/sonic-swss#1781) f949dfe9 [Dynamic Buffer Calc] Avoid creating lossy PG for admin down ports during initialization (sonic-net/sonic-swss#1776) def0a914 Fix config prompt question issue (sonic-net/sonic-swss#1799) 21f97506 [ci]: Merge azure pipelines from master to 202012 branch (sonic-net/sonic-swss#1764) a83a2a42 [vstest]: add dvs_route fixture 849bdf9c [Mux] Add support for mux metrics to State DB (sonic-net/sonic-swss#1757) 386de717 [qosorch] Dot1p map list initialization fix (sonic-net/sonic-swss#1746) f99abdca [sub intf] Port object reference count update (sonic-net/sonic-swss#1712) 4a00042d [vstest/nhg]: use dvs_route fixture to make test_nhg more robust Signed-off-by: Stephen Sun <stephens@nvidia.com>

Signed-off-by: Wenda Ni <wonda.ni@gmail.com> There are two parts of changes in this pr: The motivation is that for each Port object instantiated, it should be initialized in PortsOrch::m_port_ref_count. For each Port object to be removed, its m_port_ref_count check should be zero to proceed further. These operations are generic and therefore should also apply to sub port Port object. Currently, Port object reference count increments - When a router interface is created on top. - When an ACL table is bound to Port (i.e., becoming a member of the ACL table group associated with the Port object). Port object reference count decrements - When a router interface on top is removed. - When an ACL table is unbound from Port (i.e, removing membership from the ACL table group associated with the Port object). Item 1, router interface creation and removal, is of direct relevance to sub port Port object. The other part of change is the reference count update to parent port Port object when a sub port Port object is added/removed on top. This part is motivated by change in Add reference counting to ports for ACL bindings. What I did At sub port Port object instantiation - Init sub port Port object reference count - Increase parent port Port object reference count At sub port Port object removal - Check sub port Port object reference count drops to zero - Decrease parent port Port object reference count At physical port removal - Check physical port Port object reference count drops to zero How I verified it vs test extension Issue parent port removal at APPL_DB level before removing sub port interface. Verify that parent port persists in ASIC_DB until sub interface is removed. Without the change, extended vs test fails:

Introduce a new function `load_db_config()`. This function will load the global database file or database config file based on the platform.

wendani marked this pull request as ready for review April 17, 2021 03:53

neethajohn requested a review from prsunny April 22, 2021 21:51

lguohan added the subport label Apr 24, 2021

yxieca added the Request for 202012 Branch label May 7, 2021

wendani marked this pull request as draft May 25, 2021 19:00

wendani added 7 commits May 25, 2021 21:35

reference count update

d6fe3ad

Signed-off-by: Wenda Ni <wonda.ni@gmail.com>

removal sequence test

c448410

Signed-off-by: Wenda Ni <wonda.ni@gmail.com>

lag test

60689b7

Signed-off-by: Wenda Ni <wonda.ni@gmail.com>

Check asic db parent port oid

8f54804

Signed-off-by: Wenda Ni <wonda.ni@gmail.com>

physical port test

6d8b7ea

Signed-off-by: Wenda Ni <wonda.ni@gmail.com>

Update

70cf6da

Signed-off-by: Wenda Ni <wonda.ni@gmail.com>

Correction

503d983

Signed-off-by: Wenda Ni <wonda.ni@gmail.com>

wendani force-pushed the sub_intf_removal branch from b596b03 to 503d983 Compare May 25, 2021 22:10

wendani marked this pull request as ready for review May 25, 2021 22:11

neethajohn reviewed May 26, 2021

View reviewed changes

tests/test_sub_port_intf.py Show resolved Hide resolved

tests/test_sub_port_intf.py Show resolved Hide resolved

prsunny approved these changes May 27, 2021

View reviewed changes

neethajohn approved these changes Jun 1, 2021

View reviewed changes

neethajohn merged commit 9bf83df into sonic-net:master Jun 1, 2021

qiluo-msft added the Included in 202012 Branch label Jun 11, 2021

shlomibitton mentioned this pull request Jun 14, 2021

[submodule] Update sonic-swss submodule sonic-net/sonic-buildimage#7861

Merged

stephenxs mentioned this pull request Jun 29, 2021

[sonic-swss][202012] submodule update sonic-net/sonic-buildimage#8011

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sub intf] Port object reference count update #1712

[sub intf] Port object reference count update #1712

wendani commented Apr 17, 2021 •

edited

Loading

lguohan commented Apr 24, 2021

lguohan commented May 7, 2021

prsunny commented May 7, 2021 •

edited

Loading

wendani commented May 8, 2021 •

edited

Loading

wendani commented May 8, 2021

neethajohn commented May 24, 2021

neethajohn commented May 25, 2021

wendani commented May 25, 2021

neethajohn commented May 26, 2021

azure-pipelines bot commented May 26, 2021

neethajohn commented May 26, 2021

neethajohn commented Jun 1, 2021

azure-pipelines bot commented Jun 1, 2021

[sub intf] Port object reference count update #1712

[sub intf] Port object reference count update #1712

Conversation

wendani commented Apr 17, 2021 • edited Loading

At sub port Port object instantiation

At sub port Port object removal

At physical port removal

vs test extension

Physical port

LAG

lguohan commented Apr 24, 2021

lguohan commented May 7, 2021

prsunny commented May 7, 2021 • edited Loading

wendani commented May 8, 2021 • edited Loading

wendani commented May 8, 2021

neethajohn commented May 24, 2021

neethajohn commented May 25, 2021

wendani commented May 25, 2021

neethajohn commented May 26, 2021

azure-pipelines bot commented May 26, 2021

neethajohn commented May 26, 2021

neethajohn commented Jun 1, 2021

azure-pipelines bot commented Jun 1, 2021

wendani commented Apr 17, 2021 •

edited

Loading

prsunny commented May 7, 2021 •

edited

Loading

wendani commented May 8, 2021 •

edited

Loading