VLAN dependency handling for Dynamic Port Breakout (sonic-net#13)

* VLAN dependency handling for Dynamic Port Breakout:
    Set port::VLAN_DEP bit when port is added to first VLAN
    Clear port::VLAN_DEP bit when port is removed from all VLANs
During port removal check for the dependency bit map.

Also added, pretty printing of dependency list

* Code-review comments addressed

orchagent/port.h

@@ -54,8 +54,17 @@ class Port
             FDB_DEP,
             INTF_DEP,
             LAG_DEP,
-            VLAN_DEP
+            VLAN_DEP,
+            MAX_DEP
     };
+    std::unordered_map<Dependency, std::string> 
+        m_dependency_string = {
+            {ACL_DEP, "ACL"},
+            {FDB_DEP, "FDB"},
+            {INTF_DEP, "INTF"},
+            {LAG_DEP, "LAG"},
+            {VLAN_DEP, "VLAN"}
+        };
 
     Port() {};
     Port(std::string alias, Type type) :
@@ -121,6 +130,22 @@ class Port
     {
         return (m_dependency_bitmap != 0);
     }
+    std::string print_dependency()
+    {
+        std::string deps="";
+        uint32_t dep_bitmap = m_dependency_bitmap;
+
+        for (int dep = ACL_DEP; dep < MAX_DEP && dep_bitmap; ++dep)
+        {
+            uint32_t mask = (1 << dep);
+
+            if (dep_bitmap & mask) {
+                deps += m_dependency_string[static_cast<Dependency>(dep)] + " ";
+                dep_bitmap &= ~mask;
+            }
+        }
+        return deps; 
+    }
 };
 
 }

orchagent/portsorch.cpp

@@ -2306,7 +2306,8 @@ void PortsOrch::doPortTask(Consumer &consumer)
             if (m_portList[alias].has_dependency())
             {
                 // Port has one or more dependencies, cannot remove
-                SWSS_LOG_WARN("Cannot to remove port because of dependency"); 
+                SWSS_LOG_WARN("Please remove port dependenc(y/ies):%s",
+                               m_portList[alias].print_dependency().c_str()); 
                 it++;
                 continue;
             }
@@ -2510,9 +2511,14 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer)
             }
 
             if (addBridgePort(port) && addVlanMember(vlan, port, tagging_mode))
+            {
+                m_portList[port.m_alias].set_dependency(Port::VLAN_DEP);
                 it = consumer.m_toSync.erase(it);
+            }
             else
+            {
                 it++;
+            }
         }
         else if (op == DEL_COMMAND)
         {
@@ -2523,6 +2529,8 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer)
                     if (port.m_vlan_members.empty())
                     {
                         removeBridgePort(port);
+
+                        m_portList[port.m_alias].clear_dependency(Port::VLAN_DEP);
                     }
                     it = consumer.m_toSync.erase(it);
                 }
@@ -2541,6 +2549,7 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer)
             it = consumer.m_toSync.erase(it);
         }
     }
+
 }
 
 void PortsOrch::doLagTask(Consumer &consumer)

tests/conftest.py

@@ -694,6 +694,11 @@ def is_fdb_entry_exists(self, db, table, key_values, attributes):
 
         return exists, extra_info
 
+    def delete_port(self, port):
+        tbl = swsscommon.Table(self.cdb, "PORT")
+        tbl._del(port)
+        time.sleep(1)
+
     def create_vlan(self, vlan):
         tbl = swsscommon.Table(self.cdb, "VLAN")
         fvs = swsscommon.FieldValuePairs([("vlanid", vlan)])

tests/port_dpb.py

@@ -26,6 +26,7 @@ def __init__(self, dvs, name = None):
         self._app_db_ptbl = swsscommon.Table(self._app_db, swsscommon.APP_PORT_TABLE_NAME)
         self._asic_db = swsscommon.DBConnector(swsscommon.ASIC_DB, dvs.redis_sock, 0)
         self._asic_db_ptbl = swsscommon.Table(self._asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_PORT")
+        self._counters_db = redis.Redis(unix_socket_path=self._dvs.redis_sock, db=swsscommon.COUNTERS_DB)
 
     def set_name(self, name):
         self._name = name
@@ -51,6 +52,9 @@ def set_lanes(self, lanes):
     def set_index(self, index):
         self._index = index
 
+    def set_oid(self, oid = None):
+        self._oid = oid 
+
     def get_speed(self):
         return self._speed
 
@@ -129,6 +133,7 @@ def port_split(self, child_ports):
     def delete_from_config_db(self):
         self._cfg_db_ptbl._del(self.get_name())
         self._oid = None
+        time.sleep(2)
 
     def sync_from_config_db(self):
         (status, fvs) = self._cfg_db_ptbl.get(self.get_name())
@@ -149,6 +154,7 @@ def write_to_config_db(self):
                                           ("speed", speed_str),
                                           ("index", index_str)])
         self._cfg_db_ptbl.set(self.get_name(), fvs)
+        time.sleep(1)
 
     def get_fvs_dict(self, fvs):
         fvs_dict = {}
@@ -165,9 +171,7 @@ def exists_in_app_db(self):
         return status
 
     def sync_oid(self):
-        if self._oid is None:
-            counter_redis_conn = redis.Redis(unix_socket_path=self._dvs.redis_sock, db=swsscommon.COUNTERS_DB)
-            self._oid = counter_redis_conn.hget("COUNTERS_PORT_NAME_MAP", self.get_name())
+        self._oid = self._counters_db.hget("COUNTERS_PORT_NAME_MAP", self.get_name())
 
     def exists_in_asic_db(self):
         self.sync_oid()
@@ -242,23 +246,7 @@ def breakin(self, dvs, port_names):
         p.verify_asic_db()
         #print "ASIC DB verification passed!"
 
-    def breakout(self, dvs, port_name, num_child_ports):
-
-        p = Port(dvs, port_name)
-        p.sync_from_config_db()
-
-        # Delete port from config DB and kernel
-        p.delete_from_config_db()
-        # TBD, need vs lib to support hostif removal
-        #dvs.runcmd("ip link delete " + p.get_name())
-        #print "Deleted port:%s from config DB"%port_name
-        time.sleep(6)
-
-        # Verify port is deleted from all DBs
-        assert(p.exists_in_config_db() == False)
-        assert(p.exists_in_app_db() == False)
-        assert(p.exists_in_asic_db() == False)
-
+    def create_child_ports(self, dvs, p, num_child_ports):
         # Create child ports and write to config DB
         child_ports = p.port_split(num_child_ports)
         child_port_names = []
@@ -281,6 +269,26 @@ def breakout(self, dvs, port_name, num_child_ports):
             cp.verify_asic_db()
         #print "ASIC DB verification passed"
 
+    def breakout(self, dvs, port_name, num_child_ports):
+
+        p = Port(dvs, port_name)
+        p.sync_from_config_db()
+
+        # Delete port from config DB and kernel
+        p.delete_from_config_db()
+        # TBD, need vs lib to support hostif removal
+        #dvs.runcmd("ip link delete " + p.get_name())
+        #print "Deleted port:%s from config DB"%port_name
+        time.sleep(6)
+
+        # Verify port is deleted from all DBs
+        assert(p.exists_in_config_db() == False)
+        assert(p.exists_in_app_db() == False)
+        assert(p.exists_in_asic_db() == False)
+
+        self.create_child_ports(dvs, p, num_child_ports)
+
+
     def change_speed_and_verify(self, dvs, port_names, speed = 100000):
         for port_name  in port_names:
             p = Port(dvs, port_name)

tests/test_port_dpb.py

@@ -74,7 +74,9 @@ def test_port_breakout_multiple(self, dvs):
         dpb.breakin(dvs, ["Ethernet64", "Ethernet65", "Ethernet66", "Ethernet67"])
         dpb.breakin(dvs, ["Ethernet112", "Ethernet113", "Ethernet114", "Ethernet115"])
 
+    '''
     @pytest.mark.skip()
+    '''
     def test_port_breakout_all(self, dvs):
         dpb = DPB()
         port_names = []

tests/test_port_dpb_vlan.py

@@ -0,0 +1,178 @@
+from swsscommon import swsscommon
+import redis
+import time
+import os
+import pytest
+from pytest import *
+import json
+import re
+from port_dpb import Port
+from port_dpb import DPB
+
+@pytest.mark.usefixtures('dpb_setup_fixture')
+class TestPortDPBVlan(object):
+    def check_syslog(self, dvs, marker, log, expected_cnt):
+        (exitcode, num) = dvs.runcmd(['sh', '-c', "awk \'/%s/,ENDFILE {print;}\' /var/log/syslog | grep \"%s\" | wc -l" % (marker, log)])
+        assert num.strip() >= str(expected_cnt)
+
+    '''
+    @pytest.mark.skip()
+    '''
+    def test_dependency(self, dvs):
+        dpb = DPB() 
+        dvs.setup_db()
+        p = Port(dvs, "Ethernet0")
+        p.sync_from_config_db()
+        dvs.create_vlan("100")
+        #print "Created VLAN100"
+        dvs.create_vlan_member("100", p.get_name())
+        #print "Added Ethernet0 to VLAN100"
+        marker = dvs.add_log_marker()
+        p.delete_from_config_db()
+        #Verify that we are looping on dependency
+        time.sleep(2)
+        self.check_syslog(dvs, marker, "doPortTask: Please remove port dependenc(y/ies):VLAN", 1)
+        assert(p.exists_in_asic_db() == True)
+
+        dvs.remove_vlan_member("100", p.get_name())
+        time.sleep(2)
+        # Verify that port is deleted
+        assert(p.exists_in_asic_db() == False)
+
+        #Create the port back and delete the VLAN
+        p.write_to_config_db() 
+        #print "Added port:%s to config DB"%p.get_name()
+        p.verify_config_db()
+        #print "Config DB verification passed!"
+        p.verify_app_db()
+        #print "Application DB verification passed!"
+        p.verify_asic_db()
+        #print "ASIC DB verification passed!"
+
+        dvs.remove_vlan("100") 
+
+    '''
+    @pytest.mark.skip()
+    '''
+    def test_one_port_one_vlan(self, dvs):
+        dpb = DPB()
+        dvs.setup_db()
+
+        # Breakout testing with VLAN dependency
+        dvs.create_vlan("100")
+        #print "Created VLAN100"
+        dvs.create_vlan_member("100", "Ethernet0")
+        #print "Added Ethernet0 to VLAN100"
+
+        p = Port(dvs, "Ethernet0")
+        p.sync_from_config_db()
+        p.delete_from_config_db()
+        assert(p.exists_in_config_db() == False)
+        assert(p.exists_in_app_db() == False)
+        assert(p.exists_in_asic_db() == True)
+        #print "Ethernet0 deleted from config DB and APP DB, waiting to be removed from VLAN"
+
+        dvs.remove_vlan_member("100", "Ethernet0")
+        assert(p.exists_in_asic_db() == False)
+        #print "Ethernet0 removed from VLAN and also from ASIC DB"
+
+        dpb.create_child_ports(dvs, p, 4)
+
+        # Breakin testing with VLAN dependency
+        port_names = ["Ethernet0", "Ethernet1", "Ethernet2", "Ethernet3"]
+        for pname in port_names:
+            dvs.create_vlan_member("100", pname)
+        #print "Add %s to VLAN"%port_names            
+
+        child_ports = []
+        for pname in port_names:
+            cp = Port(dvs, pname)
+            cp.sync_from_config_db()
+            cp.delete_from_config_db()
+            assert(cp.exists_in_config_db() == False)
+            assert(cp.exists_in_app_db() == False)
+            assert(cp.exists_in_asic_db() == True)
+            child_ports.append(cp)
+        #print "Deleted %s from config DB and APP DB"%port_names            
+
+        for cp in child_ports:
+            dvs.remove_vlan_member("100", cp.get_name())
+            time.sleep(1)
+            assert(cp.exists_in_asic_db() == False)
+        #print "Deleted %s from VLAN"%port_names            
+
+        p.write_to_config_db() 
+        #print "Added port:%s to config DB"%p.get_name()
+        p.verify_config_db()
+        #print "Config DB verification passed!"
+        p.verify_app_db()
+        #print "Application DB verification passed!"
+        p.verify_asic_db()
+        #print "ASIC DB verification passed!"
+
+        dvs.remove_vlan("100")
+
+    '''    
+    @pytest.mark.skip()
+    '''
+    def test_one_port_multiple_vlan(self, dvs):
+        dpb = DPB()
+        dvs.setup_db()
+
+        dvs.create_vlan("100")
+        dvs.create_vlan("101")
+        dvs.create_vlan("102")
+        #print "Created VLAN100, VLAN101, and VLAN102"
+        dvs.create_vlan_member("100", "Ethernet0")
+        dvs.create_vlan_member("101", "Ethernet0")
+        dvs.create_vlan_member("102", "Ethernet0")
+        #print "Added Ethernet0 to all three VLANs"
+
+        p = Port(dvs, "Ethernet0")
+        p.sync_from_config_db()
+        p.delete_from_config_db()
+        assert(p.exists_in_config_db() == False)
+        assert(p.exists_in_app_db() == False)
+        assert(p.exists_in_asic_db() == True)
+        #print "Ethernet0 deleted from config DB and APP DB, waiting to be removed from VLANs"
+
+        dvs.remove_vlan_member("100", "Ethernet0")
+        assert(p.exists_in_asic_db() == True)
+        #print "Ethernet0 removed from VLAN100 and its still present in ASIC DB"
+
+        dvs.remove_vlan_member("101", "Ethernet0")
+        assert(p.exists_in_asic_db() == True)
+        #print "Ethernet0 removed from VLAN101 and its still present in ASIC DB"
+
+        dvs.remove_vlan_member("102", "Ethernet0")
+        assert(p.exists_in_asic_db() == False)
+        #print "Ethernet0 removed from VLAN101 and also from ASIC DB"
+
+        dpb.create_child_ports(dvs, p, 4)
+        #print "1X40G ---> 4x10G verified"
+
+        # Breakin
+        port_names = ["Ethernet0", "Ethernet1", "Ethernet2", "Ethernet3"]
+        for pname in port_names:
+            cp = Port(dvs, pname)
+            cp.sync_from_config_db()
+            cp.delete_from_config_db()
+            assert(cp.exists_in_config_db() == False)
+            assert(cp.exists_in_app_db() == False)
+            assert(cp.exists_in_asic_db() == False)
+        #print "Deleted %s and verified all DBs"%port_names            
+
+        #Add back Ethernet0
+        p.write_to_config_db() 
+        p.verify_config_db()
+        p.verify_app_db()
+        p.verify_asic_db()
+        #print "Added port:%s and verified all DBs"%p.get_name()
+
+        # Remove all three VLANs
+        dvs.remove_vlan("100")
+        dvs.remove_vlan("101")
+        dvs.remove_vlan("102")
+        #print "All three VLANs removed"
+
+