Only update appDB port oper_status when it is empty, alway initialize… (

sonic-net#706)

* Only update appDB port oper_status when it is empty, alway initialize Port SAI_HOSTIF_ATTR_OPER_STATUS based on oper_status value in appDB.

Add vs test for port oper_status handling validation during warm restart restore phase.

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

* Adjustment to make code more readable

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

orchagent/portsorch.cpp

@@ -2378,37 +2378,36 @@ bool PortsOrch::initializePort(Port &p)
 
     /**
      * Create database port oper status as DOWN if attr missing
-     * This status will be updated when receiving port_oper_status_notification.
+     * This status will be updated upon receiving port_oper_status_notification.
      */
-    if (operStatus != "up")
+    if (operStatus == "up")
+    {
+        p.m_oper_status = SAI_PORT_OPER_STATUS_UP;
+    }
+    else if (operStatus.empty())
     {
-        vector<FieldValueTuple> vector;
-        FieldValueTuple tuple("oper_status", "down");
-        vector.push_back(tuple);
-        m_portTable->set(p.m_alias, vector);
         p.m_oper_status = SAI_PORT_OPER_STATUS_DOWN;
+        /* Fill oper_status in db with default value "down" */
+        m_portTable->hset(p.m_alias, "oper_status", "down");
     }
     else
     {
-        p.m_oper_status = SAI_PORT_OPER_STATUS_UP;
+        p.m_oper_status = SAI_PORT_OPER_STATUS_DOWN;
     }
 
     /*
-     * If oper_status is not empty, orchagent is doing warm start, restore hostif oper status.
+     * always initialize Port SAI_HOSTIF_ATTR_OPER_STATUS based on oper_status value in appDB.
      */
-    if (!operStatus.empty())
-    {
-        sai_attribute_t attr;
-        attr.id = SAI_HOSTIF_ATTR_OPER_STATUS;
-        attr.value.booldata = (p.m_oper_status == SAI_PORT_OPER_STATUS_UP);
+    sai_attribute_t attr;
+    attr.id = SAI_HOSTIF_ATTR_OPER_STATUS;
+    attr.value.booldata = (p.m_oper_status == SAI_PORT_OPER_STATUS_UP);
 
-        sai_status_t status = sai_hostif_api->set_hostif_attribute(p.m_hif_id, &attr);
-        if (status != SAI_STATUS_SUCCESS)
-        {
-            SWSS_LOG_WARN("Failed to set operation status %s to host interface %s",
-                          operStatus.c_str(), p.m_alias.c_str());
-            return false;
-        }
+    sai_status_t status = sai_hostif_api->set_hostif_attribute(p.m_hif_id, &attr);
+    if (status != SAI_STATUS_SUCCESS)
+    {
+        SWSS_LOG_WARN("Failed to set operation status %s to host interface %s",
+                      operStatus.c_str(), p.m_alias.c_str());
+        return false;
     }
 
     return true;

tests/conftest.py

@@ -502,6 +502,31 @@ def GetSubscribedAsicDbObjects(self, pubsub, ignore=None, timeout=10):
 
         return (addobjs, delobjs)
 
+    def SubscribeDbObjects(self, dbobjs):
+        # assuming all the db object pairs are in the same db instance
+        r = redis.Redis(unix_socket_path=self.redis_sock)
+        pubsub = r.pubsub()
+        substr = ""
+        for db, obj in dbobjs:
+            pubsub.psubscribe("__keyspace@{}__:{}".format(db, obj))
+        return pubsub
+
+    def GetSubscribedMessages(self, pubsub, timeout=10):
+        messages = []
+        delobjs = []
+        idle = 0
+        prev_key = None
+
+        while True and idle < timeout:
+            message = pubsub.get_message()
+            if message:
+                messages.append(message)
+                idle = 0
+            else:
+                time.sleep(1)
+                idle += 1
+        return (messages)
+
     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")

tests/test_warm_reboot.py

@@ -803,7 +803,11 @@ def test_swss_port_state_syncup(dvs, testlog):
     dvs.servers[1].runcmd("ip link set up dev eth0") == 0
 
     time.sleep(5)
+    dbobjs =[(swsscommon.APPL_DB, swsscommon.APP_PORT_TABLE_NAME + ":*"), \
+        (swsscommon.STATE_DB, swsscommon.STATE_WARM_RESTART_TABLE_NAME + "|orchagent")]
+    pubsubDbs = dvs.SubscribeDbObjects(dbobjs)
     dvs.start_swss()
+    start_restore_neighbors(dvs)
     time.sleep(10)
 
     swss_check_RestoreCount(dvs, state_db, restore_count)
@@ -820,6 +824,33 @@ def test_swss_port_state_syncup(dvs, testlog):
             assert oper_status == "down"
         else:
             assert oper_status == "up"
+
+    # check the pubsub messages.
+    # No appDB port table operation should exist before orchagent state restored flag got set.
+    # appDB port table status sync up happens before WARM_RESTART_TABLE reconciled flag is set
+    # pubsubMessages is an ordered list of pubsub messages.
+    pubsubMessages = dvs.GetSubscribedMessages(pubsubDbs)
+
+    portOperStatusChanged = False
+    # number of times that WARM_RESTART_TABLE|orchagent key was set after the first
+    # appDB port table operation
+    orchStateCount = 0
+    for message in pubsubMessages:
+        print message
+        key = message['channel'].split(':', 1)[1]
+        print key
+        if message['data'] != 'hset' and message['data'] != 'del':
+            continue
+        if key.find(swsscommon.APP_PORT_TABLE_NAME)==0:
+           portOperStatusChanged = True
+        else:
+            # found one orchagent WARM_RESTART_TABLE operation after appDB port table change
+            if portOperStatusChanged == True:
+                orchStateCount += 1;
+
+    # Only WARM_RESTART_TABLE|orchagent state=reconciled operation may exist after port oper status change.
+    assert orchStateCount == 1
+
     #clean up arp
     dvs.runcmd("arp -d 10.0.0.1")
     dvs.runcmd("arp -d 10.0.0.3")