Routed subinterface enhancements #1907

preetham-singh · 2021-09-15T08:36:56Z

What I did
Routed subinterfae enhancements HLD #833
Add support for long name and short name routed subinterfaces.
Handling Routed subinterface ADMIN status dependency with parent interface.
Handling Routed subinterface MTU dependency with parent interface.

Why I did it
Routed subinterface feature was broken for physical and portchannel subinterfaces for subinterface name exceeding 15 characters due to kernel limitation of netdev name length of 15.

How I verified it
Routed subinterface Unit tests

Details if related
This PR is dependent on swss-common PR 529.

Handling Routed subinterface ADMIN status dependency with parent interface Handling Routed subinterface MTU dependency with parent interface

hasan-brcm · 2021-09-23T22:23:13Z

/azpw run

adyeung · 2021-09-29T18:01:17Z

/AzurePipelines run

azure-pipelines · 2021-09-29T18:01:23Z

Commenter does not have sufficient privileges for PR 1907 in repo Azure/sonic-swss

zhangyanzhao · 2021-09-29T21:13:44Z

/AzurePipelines run

azure-pipelines · 2021-09-29T21:13:55Z

Azure Pipelines successfully started running 1 pipeline(s).

venkatmahalingam · 2021-10-29T22:26:06Z

orchagent/portsorch.cpp

-    string parentAlias = alias.substr(0, found);
-    string vlanId = alias.substr(found + 1);
+    subIntf subIf(alias);
+    string subport = subIf.longName();


Where do we use this subport?

Fixed as part of next commit

venkatmahalingam · 2021-10-29T22:32:34Z

cfgmgr/intfmgr.cpp

+    auto appConsumer = new Consumer(subscriberAppTable, this, APP_LAG_TABLE_NAME);
+    Orch::addExecutor(appConsumer);
+
+    auto subscriberStateTable = new swss::SubscriberStateTable(stateDb,


why do we need the lines 43 to 52?

APPL_DB & STATE_DB tables in orch is not subscribed for runtime table change events rather only consumer object(only for get operations). Hence creating intfmgrd(from Orch) object with APPL_DB & STATE_DB tables, intfmgrd does not get events at runtime for APPL_DB table changes.
Hence we need to explicitly register with subscriberStateTable in constructor for APPL_DB and STATE_DB tables to get runtime table add/del/update events.

@preetham-singh, APPL_DB object is intended only for one consumer. There are other implications if there are multiple subscribers. We can discuss if you want more info. It cannot be subscribed here as orchagent is the consumer for this table. As I mentioned, this is a config change that you are interested in. So suggest to subscribe for config_db table rather than any other tables.

@prsunny, this is not config change instead the event when portmgr actually sets the mtu and admin state on the interface in kernel. Otherwise, intfmgr tries to set the mtu or admin state on subinterface netdev in kernel and it fails if parent netdev is having lower mtu or is admin-down, respectively. The fix is that portmgr populates mtu/admin fields in the port table in state db, and intfmgr updates the kernel subintf netdevice afterwards. Similar fix is required for LAG.

I agree it will be better to discuss and conclude.

venkatmahalingam · 2021-10-29T22:34:44Z

cfgmgr/intfmgr.cpp

@@ -287,22 +301,149 @@ void IntfMgr::addHostSubIntf(const string&intf, const string &subIntf, const str
    EXEC_WITH_ERROR_THROW(cmd.str(), res);
 }

-void IntfMgr::setHostSubIntfMtu(const string &subIntf, const string &mtu)
+
+std::string IntfMgr::getPortAdminStatus(const string &alias)


s/getPortAdminStatus/getIntfAdminStatus?

venkatmahalingam · 2021-10-29T22:35:39Z

cfgmgr/intfmgr.cpp

+    return admin;
+}
+
+std::string IntfMgr::getPortMtu(const string &alias)


Replace port with intf here as well in the func. name.

prsunny

Please add VS tests to cover the short name as well

prsunny · 2021-10-30T00:10:34Z

cfgmgr/intfmgr.cpp

@@ -22,6 +24,7 @@ using namespace swss;
 #define VRF_MGMT            "mgmt"

 #define LOOPBACK_DEFAULT_MTU_STR "65536"
+#define PORT_MTU_DEFAULT    9100


Please use this as being used in other mgr files - #define DEFAULT_MTU_STR "9100"

Sure. Changed as part of next commit.

prsunny · 2021-10-30T00:11:43Z

cfgmgr/intfmgr.cpp

+            mtu = value;
+        }
+    }
+    if (mtu.empty())


Please use { and at other places added in PR

prsunny · 2021-10-30T00:13:21Z

cfgmgr/intfmgr.cpp

+        if (subIf.parentIntf() == alias)
+        {
+            /*Avoid duplicate parent admin UP event when parent goes down
+             * This avoids intfmgrd carsh due to duplicate port up event when parent is actually going admin down*/


Please rephrase the comments and fix typos. Also, align the comments.

Fixed as part of next commit.

prsunny · 2021-10-30T00:17:33Z

cfgmgr/intfmgr.h

-    void setHostSubIntfMtu(const std::string &subIntf, const std::string &mtu);
-    void setHostSubIntfAdminStatus(const std::string &subIntf, const std::string &admin_status);
+    std::string setHostSubIntfMtu(const std::string &alias, const std::string &configMtu, const std::string &parentMtu);
+    std::string setHostSubIntfAdminStatus(const std::string &alias, const std::string &configAdmin, const std::string &parentAdmin);


Suggest retain the names of existing parameters.

subIntf is a class name provided by subinterface library updating subIntf arg to alias. Having class name as argument creates confusion. Reverting other argument names as part of next commit.

prsunny · 2021-10-30T00:23:50Z

cfgmgr/intfmgr.cpp

@@ -782,7 +960,12 @@ void IntfMgr::doTask(Consumer &consumer)
    while (it != consumer.m_toSync.end())
    {
        KeyOpFieldsValuesTuple t = it->second;
-
+        if ((table_name == STATE_PORT_TABLE_NAME) || (table_name == APP_LAG_TABLE_NAME))


Do not subscribe for events from APP_DB here for a table which is already subscribed by Orchagent. I think what you want here is the config_db entry for both Port and LAG

As per offline discussion, moving to STATE_PORT_TABLE and STATE_LAG_TABLE for parent interface and and mtu change events. Updated same as part of next commit.

prsunny · 2021-10-30T01:07:37Z

orchagent/intfsorch.cpp

@@ -749,6 +749,10 @@ void IntfsOrch::doTask(Consumer &consumer)
            {
                inband_type = value;
            }
+            else if (field == "vlan")
+            {
+                vlan = value;


I don't see vlan defined

Fixed as part of next commit. It was missed in local merge.

prsunny · 2021-10-30T01:08:19Z

cfgmgr/portmgr.cpp

@@ -35,6 +35,7 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu)
    fvs.push_back(fv);
    m_appPortTable.set(alias, fvs);

+    m_statePortTable.set(alias, fvs);


Please remove this

Fixed as part of next commit.

prsunny · 2021-10-30T01:08:28Z

cfgmgr/portmgr.cpp

@@ -67,6 +68,7 @@ bool PortMgr::setPortAdminStatus(const string &alias, const bool up)
    fvs.push_back(fv);
    m_appPortTable.set(alias, fvs);

+    m_statePortTable.set(alias, fvs);


Please remove this

Fixed as part of next commit.

prsunny · 2021-10-30T01:15:54Z

cfgmgr/intfmgr.cpp

@@ -827,6 +1010,7 @@ void IntfMgr::doTask(Consumer &consumer)
        {
            SWSS_LOG_ERROR("Invalid key %s", kfvKey(t).c_str());
        }
+        }


Fix alignment

prsunny · 2021-10-30T01:19:34Z

cfgmgr/intfmgr.cpp

+    Table *portTable;
+    if (!alias.compare(0, strlen("Eth"), "Eth"))
+    {
+        portTable = &m_statePortTable;


Not sure if its a good idea to read this each time from the DBs for parent mtu/admin_status. Do you think its better to cache? @venkatmahalingam , @preetham-singh , any thoughts?

Agree, we can get admin status from cache.

Are we planning to get the admin_status from cache?

Plan to do the caching as another PR

1. use statedb PORT_TABLE and statedb LAG_TABLE to get admin and mtu on front panel and port channel interfaces. Portsyncd and teamsyncd now updates admin and MTU status of corresponding netdev to statedb PORT_TABLE and LAG_TABLE. 2. Subinterface library moved to swss/lib as part of PR sonic-net#2017. Makefile and header inclusion changes updates accordingly.

adyeung · 2021-11-16T00:45:27Z

/azp run

azure-pipelines · 2021-11-16T00:45:32Z

Commenter does not have sufficient privileges for PR 1907 in repo Azure/sonic-swss

preetham-singh · 2021-11-16T07:12:39Z

/azp run

azure-pipelines · 2021-11-16T07:12:44Z

Commenter does not have sufficient privileges for PR 1907 in repo Azure/sonic-swss

prsunny · 2021-11-19T17:47:54Z

/azp run

azure-pipelines · 2021-11-19T17:48:06Z

Azure Pipelines successfully started running 1 pipeline(s).

lgtm-com · 2021-11-22T09:29:20Z

This pull request introduces 9 alerts when merging e38a62c into b91d8ba - view on LGTM.com

new alerts:

6 for Variable defined multiple times
3 for Unused local variable

prsunny · 2021-11-22T21:50:20Z

@venkatmahalingam , could you please review/sign-off?

prsunny · 2021-11-22T22:38:38Z

cfgmgr/intfmgr.cpp

+        return "down";
+    }
+
+    string admin = "down";


please have this as the beginning of the function and return admin variable instead of repeating "down" key word

Fixed as part of next commit

prsunny · 2021-11-22T22:39:04Z

cfgmgr/intfmgr.cpp

+    }
+    vector<FieldValueTuple> temp;
+    portTable->get(alias, temp);
+    string mtu = "0";


same as above.. please assign at the beginning.

Fixed as part of next commit

prsunny · 2021-11-22T22:39:46Z

cfgmgr/intfmgr.cpp

+            /*  Avoid duplicate interface admin UP event. */
+            string curr_admin = m_subIntfList[intf].currAdminStatus;
+            if (curr_admin == "up" && curr_admin == admin)
+                continue;


Please have this in braces

Fixed as part of next commit

prsunny · 2021-11-22T22:40:58Z

cfgmgr/intfmgr.cpp

@@ -459,13 +600,26 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
    string vlanId;
    string parentAlias;
    size_t found = alias.find(VLAN_SUB_INTERFACE_SEPARATOR);
+    subIntf subIf(alias);


seems this comment is not addressed

lgtm-com · 2021-11-23T09:15:34Z

This pull request introduces 9 alerts when merging a851adc into bb0733a - view on LGTM.com

new alerts:

6 for Variable defined multiple times
3 for Unused local variable

prsunny

lgtm. @venkatmahalingam to sign-off before merge

prsunny · 2021-11-23T18:08:37Z

cfgmgr/intfmgr.cpp

+    Table *portTable;
+    if (!alias.compare(0, strlen("Eth"), "Eth"))
+    {
+        portTable = &m_statePortTable;


Plan to do the caching as another PR

691c37b [Route bulk] Fix bugs in case a SET operation follows a DEL operation in the same bulk (sonic-net/sonic-swss#2086) a4c80c3 patch for issue sonic-net/sonic-swss#1971 - enable Rx Drop handling for cisco-8000 (sonic-net/sonic-swss#2041) 71751d1 [macsec] Support setting IPG by gearbox_config.json (sonic-net/sonic-swss#2051) 5d5c169 [bulk mode] Fix bulk conflict when in case there are both remove and set operations (sonic-net/sonic-swss#2071) 8bbdbd2 Fix SRV6 NHOP CRM object type (sonic-net/sonic-swss#2072) ef5b35f [vstest] VS test failure fix after fabric port orch PR merge (sonic-net/sonic-swss#1811) 89ea538 Supply the missing ingress/egress port profile list in document (sonic-net/sonic-swss#2064) 8123437 [pfc_detect] fix RedisReply errors (sonic-net/sonic-swss#2040) b38f527 [swss][CRM][MPLS] MPLS CRM Nexthop - switch back to using SAI OBJECT rather than SWITCH OBJECT ae061e5 create debug_shell_enable config to enable debug shell (sonic-net/sonic-swss#2060) 45e446d [cbf] Fix max FC value (sonic-net/sonic-swss#2049) b1b5b29 Initial p4orch pytest code. (sonic-net/sonic-swss#2054) d352d5a Update default route status to state DB (sonic-net/sonic-swss#2009) 24a64d6 Orchagent: Integrate P4Orch (sonic-net/sonic-swss#2029) 15a3b6c Delete the IPv6 link-local Neighbor when ipv6 link-local mode is disabled (sonic-net/sonic-swss#1897) ed783e1 [orchagent] Add trap flow counter support (sonic-net/sonic-swss#1951) e9b05a3 [vnetorch] ECMP for vnet tunnel routes with endpoint health monitor (sonic-net/sonic-swss#1955) bcb7d61 P4Orch: inital add of source (sonic-net/sonic-swss#1997) f6f6f86 [mclaglink] fix acl out ports (sonic-net/sonic-swss#2026) fd887bf [Reclaim buffer] Reclaim unused buffer for dynamic buffer model (sonic-net/sonic-swss#1910) 9258978 [orchagent, cfgmgr] Add response publisher and state recording (sonic-net/sonic-swss#1992) 3d862a7 Fixing subport vs test script for subport under VNET (sonic-net/sonic-swss#2048) fb0a5fd Don't handle buffer pool watermark during warm reboot reconciling (sonic-net/sonic-swss#1987) 16d4bcd Routed subinterface enhancements (sonic-net/sonic-swss#1907) 9639db7 [vstest/subintf] Add vs test to validate sub interface ingress to a vnet (sonic-net/sonic-swss#1642) Signed-off-by: Stephen Sun stephens@nvidia.com

Add support for long name and short name routed subinterfaces.

0e0c8af

Handling Routed subinterface ADMIN status dependency with parent interface Handling Routed subinterface MTU dependency with parent interface

preetham-singh requested a review from prsunny as a code owner September 15, 2021 08:36

prsunny requested a review from neethajohn September 18, 2021 01:42

preetham-singh mentioned this pull request Oct 1, 2021

Routed Subinterface enhancements sonic-net/SONiC#833

Merged

venkatmahalingam reviewed Oct 29, 2021

View reviewed changes

prsunny reviewed Oct 30, 2021

View reviewed changes

preetham-singh and others added 3 commits November 11, 2021 22:38

Merge branch 'master' into subintf

3653f38

Addressing left over comments

7d84b06

Fixing compilation issue

386a243

rlhui assigned prsunny Nov 17, 2021

UT Fixes and subport Pytest script update

e38a62c

prsunny reviewed Nov 22, 2021

View reviewed changes

Addressing missed review comments

a851adc

prsunny approved these changes Nov 23, 2021

View reviewed changes

venkatmahalingam approved these changes Nov 23, 2021

View reviewed changes

prsunny merged commit 16d4bcd into sonic-net:master Nov 23, 2021

stephenxs mentioned this pull request Nov 29, 2021

[sonic-swss] Submodule update sonic-net/sonic-buildimage#9397

Merged

5 tasks

stephenxs mentioned this pull request Jan 6, 2022

[sonic-swss][202111] Submodule update (#9397) sonic-net/sonic-buildimage#9685

Merged

5 tasks

Routed subinterface enhancements #1907

Routed subinterface enhancements #1907

Conversation

preetham-singh commented Sep 15, 2021 • edited

hasan-brcm commented Sep 23, 2021

adyeung commented Sep 29, 2021

azure-pipelines bot commented Sep 29, 2021

zhangyanzhao commented Sep 29, 2021

azure-pipelines bot commented Sep 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasan-brcm Nov 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetham-singh Nov 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adyeung commented Nov 16, 2021

azure-pipelines bot commented Nov 16, 2021

preetham-singh commented Nov 16, 2021

azure-pipelines bot commented Nov 16, 2021

prsunny commented Nov 19, 2021

azure-pipelines bot commented Nov 19, 2021

lgtm-com bot commented Nov 22, 2021

prsunny commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 23, 2021

prsunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetham-singh commented Sep 15, 2021 •

edited

hasan-brcm Nov 3, 2021 •

edited

preetham-singh Nov 12, 2021 •

edited