[DPB portsyncd/portmgrd/portorch] Support dynamic port add/deletion without dependencies (#1112)

Changes were made on portmgrd/portsyncd and orchagent portsorch
so it should be able to remove/add ports in case no configuration
dependencies or runtime depencies (neighbor, mac etc) on them

Also skipped the netlink for port add/delete with master in portsyncd
and cleaned up g_init and g_portSet flag and data strcutures usage.

Added dynamic portbeakout test cases including the conf_test.py changs

Signed-off-by: Zhenggen Xu <zxu@linkedin.com>
Signed-off-by: Vasant Patil <vapatil@linkedin.com>

Author: zhenggen-xu

cfgmgr/portmgr.cpp

@@ -151,6 +151,11 @@ void PortMgr::doTask(Consumer &consumer)
                 SWSS_LOG_NOTICE("Configure %s MAC learn mode to %s", alias.c_str(), learn_mode.c_str());
             }
         }
+        else if (op == DEL_COMMAND)
+        {
+            SWSS_LOG_NOTICE("Delete Port: %s", alias.c_str());
+            m_appPortTable.del(alias);
+        }
 
         it = consumer.m_toSync.erase(it);
     }

orchagent/portsorch.cpp

@@ -24,6 +24,7 @@
 #include "crmorch.h"
 #include "countercheckorch.h"
 #include "notifier.h"
+#include "redisclient.h"
 
 extern sai_switch_api_t *sai_switch_api;
 extern sai_bridge_api_t *sai_bridge_api;
@@ -1427,6 +1428,7 @@ bool PortsOrch::addPort(const set<int> &lane_set, uint32_t speed, int an, string
     }
 
     m_portListLaneMap[lane_set] = port_id;
+    m_portCount++;
 
     SWSS_LOG_NOTICE("Create port %" PRIx64 " with the speed %u", port_id, speed);
 
@@ -1450,7 +1452,10 @@ bool PortsOrch::removePort(sai_object_id_t port_id)
         SWSS_LOG_ERROR("Failed to remove port %" PRIx64 ", rv:%d", port_id, status);
         return false;
     }
+
     removeAclTableGroup(p);
+
+    m_portCount--;
     SWSS_LOG_NOTICE("Remove port %" PRIx64, port_id);
 
     return true;
@@ -1528,6 +1533,24 @@ bool PortsOrch::initPort(const string &alias, const set<int> &lane_set)
     return true;
 }
 
+void PortsOrch::deInitPort(string alias, sai_object_id_t port_id)
+{
+    SWSS_LOG_ENTER();
+
+    Port p(alias, Port::PHY);
+    p.m_port_id = port_id;
+
+    /* remove port from flex_counter_table for updating counters  */
+    port_stat_manager.clearCounterIdList(p.m_port_id);
+
+    /* remove port name map from counter table */
+    RedisClient redisClient(m_counter_db.get());
+    redisClient.hdel(COUNTERS_PORT_NAME_MAP, alias);
+
+    SWSS_LOG_NOTICE("De-Initialized port %s", alias.c_str());
+}
+
+
 bool PortsOrch::bake()
 {
     SWSS_LOG_ENTER();
@@ -1594,6 +1617,35 @@ void PortsOrch::cleanPortTable(const vector<string>& keys)
     }
 }
 
+void PortsOrch::removePortFromLanesMap(string alias)
+{
+
+    for (auto it = m_lanesAliasSpeedMap.begin(); it != m_lanesAliasSpeedMap.end(); it++)
+    {
+        if (get<0>(it->second) == alias)
+        {
+            SWSS_LOG_NOTICE("Removing port %s from lanes map", alias.c_str());
+            it = m_lanesAliasSpeedMap.erase(it);
+            break;
+        }
+    }
+}
+
+void PortsOrch::removePortFromPortListMap(sai_object_id_t port_id)
+{
+
+    for (auto it = m_portListLaneMap.begin(); it != m_portListLaneMap.end(); it++)
+    {
+        if (it->second == port_id)
+        {
+            SWSS_LOG_NOTICE("Removing port-id %lx from port list map", port_id);
+            it = m_portListLaneMap.erase(it);
+            break;
+        }
+    }
+}
+
+
 void PortsOrch::doPortTask(Consumer &consumer)
 {
     SWSS_LOG_ENTER();
@@ -1754,7 +1806,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
              * 2. Create new ports
              * 3. Initialize all ports
              */
-            if (m_portConfigState == PORT_CONFIG_RECEIVED && (m_lanesAliasSpeedMap.size() == m_portCount))
+            if (m_portConfigState == PORT_CONFIG_RECEIVED || m_portConfigState == PORT_CONFIG_DONE)
             {
                 for (auto it = m_portListLaneMap.begin(); it != m_portListLaneMap.end();)
                 {
@@ -1797,7 +1849,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
                         }
                     }
 
-                    it = m_lanesAliasSpeedMap.erase(it);
+                    it++;
                 }
 
                 m_portConfigState = PORT_CONFIG_DONE;
@@ -2086,6 +2138,31 @@ void PortsOrch::doPortTask(Consumer &consumer)
                 }
             }
         }
+        else if (op == DEL_COMMAND)
+        {
+            SWSS_LOG_NOTICE("Deleting Port %s", alias.c_str());
+            auto port_id = m_portList[alias].m_port_id;
+            auto hif_id = m_portList[alias].m_hif_id;
+
+            deInitPort(alias, port_id);
+
+            SWSS_LOG_NOTICE("Removing hostif %lx for Port %s", hif_id, alias.c_str());
+            sai_status_t status = sai_hostif_api->remove_hostif(hif_id);
+            if (status != SAI_STATUS_SUCCESS)
+            {
+                throw runtime_error("Remove hostif for the port failed");
+            }
+
+            if (!removePort(port_id))
+            {
+                throw runtime_error("Delete port failed");
+            }
+            removePortFromLanesMap(alias);
+            removePortFromPortListMap(port_id);
+
+            /* Delete port from port list */
+            m_portList.erase(alias);
+        }
         else
         {
             SWSS_LOG_ERROR("Unknown operation type %s", op.c_str());

orchagent/portsorch.h

@@ -149,6 +149,8 @@ class PortsOrch : public Orch, public Subject
 
     void doTask(NotificationConsumer &consumer);
 
+    void removePortFromLanesMap(string alias);
+    void removePortFromPortListMap(sai_object_id_t port_id);
     void removeDefaultVlanMembers();
     void removeDefaultBridgePorts();
 
@@ -179,6 +181,7 @@ class PortsOrch : public Orch, public Subject
     bool addPort(const set<int> &lane_set, uint32_t speed, int an=0, string fec="");
     bool removePort(sai_object_id_t port_id);
     bool initPort(const string &alias, const set<int> &lane_set);
+    void deInitPort(string alias, sai_object_id_t port_id);
 
     bool setPortAdminStatus(sai_object_id_t id, bool up);
     bool getPortAdminStatus(sai_object_id_t id, bool& up);

portsyncd/linksync.cpp

@@ -155,6 +155,8 @@ LinkSync::LinkSync(DBConnector *appl_db, DBConnector *state_db) :
 
 void LinkSync::onMsg(int nlmsg_type, struct nl_object *obj)
 {
+    SWSS_LOG_ENTER();
+
     if ((nlmsg_type != RTM_NEWLINK) && (nlmsg_type != RTM_DELLINK))
     {
         return;
@@ -209,6 +211,14 @@ void LinkSync::onMsg(int nlmsg_type, struct nl_object *obj)
         return;
     }
 
+    /* If netlink for this port has master, we ignore that for now
+     * This could be the case where the port was removed from VLAN bridge
+     */
+    if (master)
+    {
+        return;
+    }
+
     /* In the event of swss restart, it is possible to get netlink messages during bridge
      * delete, interface delete etc which are part of cleanup. These netlink messages for
      * the front-panel interface must not be published or it will update the statedb with
@@ -226,27 +236,24 @@ void LinkSync::onMsg(int nlmsg_type, struct nl_object *obj)
     /* Insert or update the ifindex to key map */
     m_ifindexNameMap[ifindex] = key;
 
+    if (nlmsg_type == RTM_DELLINK)
+    {
+        m_statePortTable.del(key);
+        SWSS_LOG_NOTICE("Delete %s(ok) from state db", key.c_str());
+        return;
+    }
+
     /* front panel interfaces: Check if the port is in the PORT_TABLE
      * non-front panel interfaces such as eth0, lo which are not in the
      * PORT_TABLE are ignored. */
     vector<FieldValueTuple> temp;
     if (m_portTable.get(key, temp))
     {
-        /* TODO: When port is removed from the kernel */
-        if (nlmsg_type == RTM_DELLINK)
-        {
-            return;
-        }
-
-        /* Host interface is created */
-        if (!g_init && g_portSet.find(key) != g_portSet.end())
-        {
-            g_portSet.erase(key);
-            FieldValueTuple tuple("state", "ok");
-            vector<FieldValueTuple> vector;
-            vector.push_back(tuple);
-            m_statePortTable.set(key, vector);
-            SWSS_LOG_INFO("Publish %s(ok) to state db", key.c_str());
-        }
+        g_portSet.erase(key);
+        FieldValueTuple tuple("state", "ok");
+        vector<FieldValueTuple> vector;
+        vector.push_back(tuple);
+        m_statePortTable.set(key, vector);
+        SWSS_LOG_NOTICE("Publish %s(ok) to state db", key.c_str());
     }
 }

tests/conftest.py

@@ -77,8 +77,8 @@ def __init__(self, dvs):
 
 class ApplDbValidator(object):
     def __init__(self, dvs):
-        appl_db = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0)
-        self.neighTbl = swsscommon.Table(appl_db, "NEIGH_TABLE")
+        self.appl_db = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0)
+        self.neighTbl = swsscommon.Table(self.appl_db, "NEIGH_TABLE")
 
     def __del__(self):
         # Make sure no neighbors on physical interfaces
@@ -155,13 +155,14 @@ def __init__(self, name=None, imgname=None, keeptb=False, fakeplatform=None):
                       'vrfmgrd',
                       'portmgrd']
         self.syncd = ['syncd']
-        self.rtd   = ['fpmsyncd', 'zebra']
+        self.rtd   = ['fpmsyncd', 'zebra', 'staticd']
         self.teamd = ['teamsyncd', 'teammgrd']
         # FIXME: We need to verify that NAT processes are running, once the
         # appropriate changes are merged into sonic-buildimage
         # self.natd = ['natsyncd', 'natmgrd']
         self.alld  = self.basicd + self.swssd + self.syncd + self.rtd + self.teamd # + self.natd
         self.client = docker.from_env()
+        self.appldb = None
 
         if subprocess.check_call(["/sbin/modprobe", "team"]) != 0:
             raise NameError("cannot install kernel team module")
@@ -199,7 +200,7 @@ def __init__(self, name=None, imgname=None, keeptb=False, fakeplatform=None):
             self.mount = "/var/run/redis-vs/{}".format(ctn_sw_name)
 
             self.net_cleanup()
-            self.restart()
+            self.ctn_restart()
         else:
             self.ctn_sw = self.client.containers.run('debian:jessie', privileged=True, detach=True,
                     command="bash", stdin_open=True)
@@ -224,8 +225,20 @@ def __init__(self, name=None, imgname=None, keeptb=False, fakeplatform=None):
                     network_mode="container:%s" % self.ctn_sw.name,
                     volumes={ self.mount: { 'bind': '/var/run/redis', 'mode': 'rw' } })
 
-        self.appldb = None
         self.redis_sock = self.mount + '/' + "redis.sock"
+        self.check_ctn_status_and_db_connect()
+
+    def destroy(self):
+        if self.appldb:
+            del self.appldb
+        if self.cleanup:
+            self.ctn.remove(force=True)
+            self.ctn_sw.remove(force=True)
+            os.system("rm -rf {}".format(self.mount))
+            for s in self.servers:
+                s.destroy()
+
+    def check_ctn_status_and_db_connect(self):
         try:
             # temp fix: remove them once they are moved to vs start.sh
             self.ctn.exec_run("sysctl -w net.ipv6.conf.default.disable_ipv6=0")
@@ -239,15 +252,6 @@ def __init__(self, name=None, imgname=None, keeptb=False, fakeplatform=None):
             self.destroy()
             raise
 
-    def destroy(self):
-        if self.appldb:
-            del self.appldb
-        if self.cleanup:
-            self.ctn.remove(force=True)
-            self.ctn_sw.remove(force=True)
-            os.system("rm -rf {}".format(self.mount))
-            for s in self.servers:
-                s.destroy()
 
     def check_ready(self, timeout=30):
         '''check if all processes in the dvs is ready'''
@@ -314,15 +318,22 @@ def net_cleanup(self):
                     print "remove extra link {}".format(pname)
         return
 
-    def restart(self):
+    def ctn_restart(self):
         self.ctn.restart()
 
+    def restart(self):
+        if self.appldb:
+            del self.appldb
+        self.ctn_restart()
+        self.check_ctn_status_and_db_connect()
+
     # start processes in SWSS
     def start_swss(self):
         cmd = ""
         for pname in self.swssd:
             cmd += "supervisorctl start {}; ".format(pname)
         self.runcmd(['sh', '-c', cmd])
+        time.sleep(5)
 
     # stop processes in SWSS
     def stop_swss(self):
@@ -839,3 +850,25 @@ def testlog(request, dvs):
     dvs.runcmd("logger === start test %s ===" % request.node.name)
     yield testlog
     dvs.runcmd("logger === finish test %s ===" % request.node.name)
+
+##################### DPB fixtures ###########################################
+@pytest.yield_fixture(scope="module")
+def create_dpb_config_file(dvs):
+    cmd = "sonic-cfggen -j /etc/sonic/init_cfg.json -j /tmp/ports.json --print-data > /tmp/dpb_config_db.json"
+    dvs.runcmd(['sh', '-c', cmd])
+    cmd = "mv /etc/sonic/config_db.json /etc/sonic/config_db.json.bak"
+    dvs.runcmd(cmd)
+    cmd = "cp /tmp/dpb_config_db.json /etc/sonic/config_db.json"
+    dvs.runcmd(cmd)
+
+@pytest.yield_fixture(scope="module")
+def remove_dpb_config_file(dvs):
+    cmd = "mv /etc/sonic/config_db.json.bak /etc/sonic/config_db.json"
+    dvs.runcmd(cmd)
+
+@pytest.yield_fixture(scope="module")
+def dpb_setup_fixture(dvs):
+    create_dpb_config_file(dvs)
+    dvs.restart()
+    yield
+    remove_dpb_config_file(dvs)