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

[subintf] Fix kernel admin status #1873

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

wendani
Copy link
Contributor

@wendani wendani commented Aug 13, 2021

Why I did it
Fix sonic-net/SONiC#601

Sub port interface has a separate admin status control from parent port. However, at kernel side, the admin status operation of parent port is not fully decoupled from that of sub port interface: Admin down parent port also admins down sub port, and admin up parent port also admins up sub port.

Under such a behavior, one case that need additional care from user software, specifically, intfmgrd, is that sub port interface is configured admin down in CONFIG_DB, and parent port goes from admin down to admin up in CONFIG_DB. Sub port interface at kernel will go from admin down to admin up accordingly, leaving its kernel-side admin status to be different from what is configured in CONFIG_DB. This may affect behavior of some control plane packets.

Other admin status combination of {parent port, sub port interface} pair is discussed in sonic-net/SONiC#601 (comment)

The SAI/ASIC side admin status of sub port interface is not affected by this PR.

What I did
Have IntfMgr subscribe to CONFIG PORT and LAG table to listen to parent port admin status change. The particular status we are interested in is parent port admin status up and sub port interface admin status down. When such a case is captured, we will admin down sub port interface in kernel.

Because the operation of admin down parent port is issued from portmgrd or teammgrd, which is a separate process from intfmgrd, we need to make sure that parent port admin operation is completed in the first place. To this end, we will check whether parent port admin status is syncd from CONFIG DB to APPL DB. We use this as an indicator that kernel-side operation for parent port is completed as setting APPL DB is coded as the last step in *mgrd task processing by convention.

How I verified it
vs test

  1. Start with parent port admin up, sub port interface admin up in CONFIG_DB
    • Check sub port interface admin up in kernel, and admin up in SAI/ASIC
  2. Set parent port admin down, sub port interface remains admin up in CONFIG_DB
    • Check sub port interface admin down in kernel, and admin up in SAI/ASIC
  3. Set parent port admin up, sub port interface remains admin up in CONFIG_DB
    • Check sub port interface admin up in kernel, and admin up in SAI/ASIC
  4. parent port remains admin up, set sub port interface admin down in CONFIG_DB
    • Check sub port interface admin down in kernel, and admin down in SAI/ASIC
  5. Set parent port admin down, sub port interface remains admin down in CONFIG_DB
    • Check sub port interface admin down in kernel, and admin down in SAI/ASIC
  6. Set parent port admin up, sub port interface remains admin down in CONFIG_DB (What this PR fixes)
    • Check sub port interface admin down in kernel, and admin down in SAI/ASIC

Details if related

vs test failure message without the fix in this PR. Failure on line 864:

========================================================================= FAILURES ==========================================================================
____________________________________________ TestSubPortIntf.test_sub_port_intf_parent_port_admin_status_change _____________________________________________

self = <test_sub_port_intf.TestSubPortIntf object at 0x7fd6b75331d0>, dvs = <conftest.DockerVirtualSwitch object at 0x7fd6b7533da0>

    def test_sub_port_intf_parent_port_admin_status_change(self, dvs):
        self.connect_dbs(dvs)
    
>       self._test_sub_port_intf_parent_port_admin_status_change(dvs, self.SUB_PORT_INTERFACE_UNDER_TEST)

test_sub_port_intf.py:912: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test_sub_port_intf.py:864: in _test_sub_port_intf_parent_port_admin_status_change
    self.check_sub_port_intf_admin_status_kernel(dvs, sub_port_intf_name, admin_up=False)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <test_sub_port_intf.TestSubPortIntf object at 0x7fd6b75331d0>, dvs = <conftest.DockerVirtualSwitch object at 0x7fd6b7533da0>
port_name = 'Ethernet64.10', admin_up = False

    def check_sub_port_intf_admin_status_kernel(self, dvs, port_name, admin_up):
        up = "UP"
        if admin_up == True:
            up = "," + up
        (ec, out) = dvs.runcmd(['bash', '-c', "ip link show {} | grep {}".format(port_name, up)])
        if admin_up == True:
            assert ec == 0
            assert up in out
        else:
>           assert ec == 1
E           assert 0 == 1
E             +0
E             -1

test_sub_port_intf.py:337: AssertionError
================================================================== short test summary info ==================================================================
FAILED test_sub_port_intf.py::TestSubPortIntf::test_sub_port_intf_parent_port_admin_status_change - assert 0 == 1
=============================================================== 1 failed in 77.22s (0:01:17) ================================================================

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

Signed-off-by: Wenda Ni <wonda.ni@gmail.com>
Signed-off-by: Wenda Ni <wonda.ni@gmail.com>
Signed-off-by: Wenda Ni <wonda.ni@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Subport] On SONiC-201911 branch Parent interface state change is not linked with child interface
1 participant