Disable FDB hardware learning on all bridge ports before planned warm restart (sonic-net#628)

* Disable FDB learning on all bridge ports before freezing orchagent
* Refine create_bridge_port attr
* Flush before freeze
* Add vs test
* Fix error message
* Generalize setBridgePortLearningFDB()
* Tune test

Author: qiluo-msft

orchagent/orchdaemon.cpp

@@ -363,6 +363,14 @@ void OrchDaemon::start()
                 // Should sleep here or continue handling timers and etc.??
                 if (!gSwitchOrch->checkRestartNoFreeze())
                 {
+                    // Disable FDB learning on all bridge ports
+                    for (auto& pair: gPortsOrch->getAllPorts())
+                    {
+                        auto& port = pair.second;
+                        gPortsOrch->setBridgePortLearningFDB(port, SAI_BRIDGE_PORT_FDB_LEARNING_MODE_DISABLE);
+                    }
+                    flush();
+
                     SWSS_LOG_WARN("Orchagent is frozen for warm restart!");
                     sleep(UINT_MAX);
                 }

orchagent/portsorch.cpp

@@ -2312,6 +2312,27 @@ bool PortsOrch::addHostIntfs(Port &port, string alias, sai_object_id_t &host_int
     return true;
 }
 
+bool PortsOrch::setBridgePortLearningFDB(Port &port, sai_bridge_port_fdb_learning_mode_t mode)
+{
+    // TODO: how to support 1D bridge?
+    if (port.m_type != Port::PHY) return false;
+
+    auto bridge_port_id = port.m_bridge_port_id;
+    if (bridge_port_id == SAI_NULL_OBJECT_ID) return false;
+
+    sai_attribute_t bport_attr;
+    bport_attr.id = SAI_BRIDGE_PORT_ATTR_FDB_LEARNING_MODE;
+    bport_attr.value.s32 = mode;
+    auto status = sai_bridge_api->set_bridge_port_attribute(bridge_port_id, &bport_attr);
+    if (status != SAI_STATUS_SUCCESS)
+    {
+        SWSS_LOG_ERROR("Failed to set bridge port %lx learning_mode attribute: %d", bridge_port_id, status);
+        return false;
+    }
+    SWSS_LOG_NOTICE("Disable FDB learning on bridge port %s(%lx)", port.m_alias.c_str(), bridge_port_id);
+    return true;
+}
+
 bool PortsOrch::addBridgePort(Port &port)
 {
     SWSS_LOG_ENTER();
@@ -2350,6 +2371,11 @@ bool PortsOrch::addBridgePort(Port &port)
     attr.value.booldata = true;
     attrs.push_back(attr);
 
+    /* And with hardware FDB learning mode set to HW (explicit default value) */
+    attr.id = SAI_BRIDGE_PORT_ATTR_FDB_LEARNING_MODE;
+    attr.value.s32 = SAI_BRIDGE_PORT_FDB_LEARNING_MODE_HW;
+    attrs.push_back(attr);
+
     sai_status_t status = sai_bridge_api->create_bridge_port(&port.m_bridge_port_id, gSwitchId, (uint32_t)attrs.size(), attrs.data());
     if (status != SAI_STATUS_SUCCESS)
     {

orchagent/portsorch.h

@@ -59,6 +59,7 @@ class PortsOrch : public Orch, public Subject
     bool bake() override;
     void cleanPortTable(const vector<string>& keys);
     bool getBridgePort(sai_object_id_t id, Port &port);
+    bool setBridgePortLearningFDB(Port &port, sai_bridge_port_fdb_learning_mode_t mode);
     bool getPort(string alias, Port &port);
     bool getPort(sai_object_id_t id, Port &port);
     bool getPortByBridgePortId(sai_object_id_t bridge_port_id, Port &port);

tests/conftest.py

@@ -269,15 +269,28 @@ def copy_file(self, path, filename):
         self.ctn.put_archive(path, tarstr.getvalue())
         tarstr.close()
 
+    def get_map_iface_bridge_port_id(self, asic_db):
+        port_id_2_iface = self.asicdb.portoidmap
+        tbl = swsscommon.Table(asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT")
+        iface_2_bridge_port_id = {}
+        for key in tbl.getKeys():
+            status, data = tbl.get(key)
+            assert status
+            values = dict(data)
+            iface_id = values["SAI_BRIDGE_PORT_ATTR_PORT_ID"]
+            iface_name = port_id_2_iface[iface_id]
+            iface_2_bridge_port_id[iface_name] = key
+
+        return iface_2_bridge_port_id
+
     def is_table_entry_exists(self, db, table, keyregex, attributes):
         tbl = swsscommon.Table(db, table)
         keys = tbl.getKeys()
 
-        exists = False
         extra_info = []
-        key_found = False
         for key in keys:
-            key_found = re.match(keyregex, key)
+            if re.match(keyregex, key) is None:
+                continue
 
             status, fvs = tbl.get(key)
             assert status, "Error reading from table %s" % table
@@ -288,17 +301,13 @@ def is_table_entry_exists(self, db, table, keyregex, attributes):
                     del d_attributes[k]
 
             if len(d_attributes) != 0:
-                exists = False
                 extra_info.append("Desired attributes %s was not found for key %s" % (str(d_attributes), key))
             else:
-                exists = True
-                break
-
-        if not key_found:
-            exists = False
-            extra_info.append("Desired key with parameters %s was not found" % str(key_values))
-
-        return exists, extra_info
+                return True, extra_info
+        else:
+            if not extra_info:
+                extra_info.append("Desired key regex %s was not found" % str(keyregex))
+            return False, extra_info
 
     def is_fdb_entry_exists(self, db, table, key_values, attributes):
         tbl =  swsscommon.Table(db, table)

tests/test_fdb_cold.py

@@ -20,21 +20,6 @@ def create_entry_pst(db, table, key, pairs):
     tbl = swsscommon.ProducerStateTable(db, table)
     create_entry(tbl, key, pairs)
 
-
-def get_map_iface_bridge_port_id(asic_db, dvs):
-    port_id_2_iface = dvs.asicdb.portoidmap
-    tbl = swsscommon.Table(asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT")
-    iface_2_bridge_port_id = {}
-    for key in tbl.getKeys():
-        status, data = tbl.get(key)
-        assert status
-        values = dict(data)
-        iface_id = values["SAI_BRIDGE_PORT_ATTR_PORT_ID"]
-        iface_name = port_id_2_iface[iface_id]
-        iface_2_bridge_port_id[iface_name] = key
-
-    return iface_2_bridge_port_id
-
 def how_many_entries_exist(db, table):
     tbl =  swsscommon.Table(db, table)
     return len(tbl.getKeys())
@@ -90,7 +75,7 @@ def test_FDBAddedAfterMemberCreated(dvs):
     assert vm_after - vm_before == 1, "The vlan member wasn't added"
 
     # Get mapping between interface name and its bridge port_id
-    iface_2_bridge_port_id = get_map_iface_bridge_port_id(dvs.adb, dvs)
+    iface_2_bridge_port_id = dvs.get_map_iface_bridge_port_id(dvs.adb)
 
     # check that the FDB entry was inserted into ASIC DB
     assert how_many_entries_exist(dvs.adb, "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY") == 1, "The fdb entry wasn't inserted to ASIC"

tests/test_fdb_warm.py

@@ -20,29 +20,14 @@ def create_entry_pst(db, table, key, pairs):
     tbl = swsscommon.ProducerStateTable(db, table)
     create_entry(tbl, key, pairs)
 
-
-def get_map_iface_bridge_port_id(asic_db, dvs):
-    port_id_2_iface = dvs.asicdb.portoidmap
-    tbl = swsscommon.Table(asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT")
-    iface_2_bridge_port_id = {}
-    for key in tbl.getKeys():
-        status, data = tbl.get(key)
-        assert status
-        values = dict(data)
-        iface_id = values["SAI_BRIDGE_PORT_ATTR_PORT_ID"]
-        iface_name = port_id_2_iface[iface_id]
-        iface_2_bridge_port_id[iface_name] = key
-
-    return iface_2_bridge_port_id
-
 def how_many_entries_exist(db, table):
     tbl =  swsscommon.Table(db, table)
     return len(tbl.getKeys())
 
 def test_fdb_notifications(dvs):
     dvs.setup_db()
 
-    #dvs.runcmd("sonic-clear fdb all")
+    dvs.runcmd("sonic-clear fdb all")
 
     dvs.runcmd("crm config polling interval 1")
     dvs.setReadOnlyAttr('SAI_OBJECT_TYPE_SWITCH', 'SAI_SWITCH_ATTR_AVAILABLE_FDB_ENTRY', '1000')
@@ -55,6 +40,20 @@ def test_fdb_notifications(dvs):
     dvs.create_vlan_member("6", "Ethernet64")
     dvs.create_vlan_member("6", "Ethernet68")
 
+    # Get mapping between interface name and its bridge port_id
+    time.sleep(2)
+    iface_2_bridge_port_id = dvs.get_map_iface_bridge_port_id(dvs.adb)
+
+    # check FDB learning mode
+    ok, extra = dvs.is_table_entry_exists(dvs.adb, 'ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT',
+        iface_2_bridge_port_id["Ethernet64"],
+        [("SAI_BRIDGE_PORT_ATTR_FDB_LEARNING_MODE", "SAI_BRIDGE_PORT_FDB_LEARNING_MODE_HW")])
+    assert ok, str(extra)
+    ok, extra = dvs.is_table_entry_exists(dvs.adb, 'ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT',
+        iface_2_bridge_port_id["Ethernet68"],
+        [("SAI_BRIDGE_PORT_ATTR_FDB_LEARNING_MODE", "SAI_BRIDGE_PORT_FDB_LEARNING_MODE_HW")])
+    assert ok, str(extra)
+
     # bring up vlan and member
     dvs.set_interface_status("Vlan6", "up")
     dvs.add_ip_address("Vlan6", "6.6.6.1/24")
@@ -72,9 +71,6 @@ def test_fdb_notifications(dvs):
     rc = dvs.servers[17].runcmd("ping -c 1 6.6.6.6")
     assert rc == 0
 
-    # Get mapping between interface name and its bridge port_id
-    time.sleep(2)
-    iface_2_bridge_port_id = get_map_iface_bridge_port_id(dvs.adb, dvs)
 
     # check that the FDB entries were inserted into ASIC DB
     ok, extra = dvs.is_fdb_entry_exists(dvs.adb, "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY",
@@ -113,24 +109,34 @@ def test_fdb_notifications(dvs):
     assert ok, str(extra)
 
     # enable warm restart
-    # TODO: use cfg command to config it
-    create_entry_tbl(
-        dvs.cdb,
-        swsscommon.CFG_WARM_RESTART_TABLE_NAME, "swss",
-        [
-            ("enable", "true"),
-        ]
-    )
+    (exitcode, result) = dvs.runcmd("config warm_restart enable swss")
+    assert exitcode == 0
+
+    # freeze orchagent for warm restart
+    (exitcode, result) = dvs.runcmd("/usr/bin/orchagent_restart_check")
+    assert result == "RESTARTCHECK succeeded\n"
+    time.sleep(2)
 
     try:
         # restart orchagent
         dvs.stop_swss()
+
+        # check FDB learning mode
+        ok, extra = dvs.is_table_entry_exists(dvs.adb, 'ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT',
+            iface_2_bridge_port_id["Ethernet64"],
+            [("SAI_BRIDGE_PORT_ATTR_FDB_LEARNING_MODE", "SAI_BRIDGE_PORT_FDB_LEARNING_MODE_DISABLE")])
+        assert ok, str(extra)
+        ok, extra = dvs.is_table_entry_exists(dvs.adb, 'ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT',
+            iface_2_bridge_port_id["Ethernet68"],
+            [("SAI_BRIDGE_PORT_ATTR_FDB_LEARNING_MODE", "SAI_BRIDGE_PORT_FDB_LEARNING_MODE_DISABLE")])
+        assert ok, str(extra)
+
         dvs.start_swss()
         time.sleep(2)
 
         # Get mapping between interface name and its bridge port_id
         # Note: they are changed
-        iface_2_bridge_port_id = get_map_iface_bridge_port_id(dvs.adb, dvs)
+        iface_2_bridge_port_id = dvs.get_map_iface_bridge_port_id(dvs.adb)
 
         # check that the FDB entries were inserted into ASIC DB
         ok, extra = dvs.is_fdb_entry_exists(dvs.adb, "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY",
@@ -148,17 +154,22 @@ def test_fdb_notifications(dvs):
         )
         assert ok, str(extra)
 
+        # check FDB learning mode
+        ok, extra = dvs.is_table_entry_exists(dvs.adb, 'ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT',
+            iface_2_bridge_port_id["Ethernet64"],
+            [("SAI_BRIDGE_PORT_ATTR_FDB_LEARNING_MODE", "SAI_BRIDGE_PORT_FDB_LEARNING_MODE_HW")])
+        assert ok, str(extra)
+        ok, extra = dvs.is_table_entry_exists(dvs.adb, 'ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT',
+            iface_2_bridge_port_id["Ethernet68"],
+            [("SAI_BRIDGE_PORT_ATTR_FDB_LEARNING_MODE", "SAI_BRIDGE_PORT_FDB_LEARNING_MODE_HW")])
+        assert ok, str(extra)
+
         time.sleep(2)
         counter_restarted = dvs.getCrmCounterValue('STATS', 'crm_stats_fdb_entry_used')
         assert counter_inserted == counter_restarted
 
     finally:
-        # disable warm restart
-        # TODO: use cfg command to config it
-        create_entry_tbl(
-            dvs.cdb,
-            swsscommon.CFG_WARM_RESTART_TABLE_NAME, "swss",
-            [
-                ("enable", "false"),
-            ]
-        )
+        # enable warm restart
+        dvs.runcmd("config warm_restart enable swss")
+        # slow down crm polling
+        dvs.runcmd("crm config polling interval 10000")