Retry the creation of decap tunnel if dependant QoS map is not ready (s…

…onic-net#2252)

*Retry logic in decaporch, if depending QoS map is not ready
Signed-off-by: bingwang <wang.bing@microsoft.com>

orchagent/qosorch.cpp

@@ -1995,3 +1995,21 @@ sai_object_id_t QosOrch::resolveTunnelQosMap(std::string referencing_table_name,
         return SAI_NULL_OBJECT_ID;
     }
 }
+
+/**
+ * Function Description:
+ *    @brief Remove the reference from tunnel object. Called after tunnel is removed
+ *
+ * Arguments:
+ *    @param[in] referencing_table_name - The name of table that is referencing the QoS map
+ *    @param[in] tunnle_name - The name of tunnel
+ *
+ * Return Values:
+ *    @return no return
+ */
+void QosOrch::removeTunnelReference(std::string referencing_table_name, std::string tunnel_name)
+{
+    removeObject(m_qos_maps, referencing_table_name, tunnel_name);
+    SWSS_LOG_INFO("Freed QoS objects referenced by %s:%s", referencing_table_name.c_str(), tunnel_name.c_str());
+}
+

orchagent/qosorch.h

@@ -169,6 +169,7 @@ class QosOrch : public Orch
     static type_map m_qos_maps;
 
     sai_object_id_t resolveTunnelQosMap(std::string referencing_table_name, std::string tunnel_name, std::string map_type_name, KeyOpFieldsValuesTuple& tuple);
+    void removeTunnelReference(std::string referencing_table_name, std::string tunnel_name);
 private:
     void doTask() override;
     virtual void doTask(Consumer& consumer);

orchagent/tunneldecaporch.cpp

@@ -50,13 +50,14 @@ void TunnelDecapOrch::doTask(Consumer& consumer)
         string ecn_mode;
         string encap_ecn_mode;
         string ttl_mode;
-        sai_object_id_t dscp_to_dc_map_id = SAI_NULL_OBJECT_ID;
+        sai_object_id_t dscp_to_tc_map_id = SAI_NULL_OBJECT_ID;
         sai_object_id_t tc_to_pg_map_id = SAI_NULL_OBJECT_ID;
         // The tc_to_dscp_map_id and tc_to_queue_map_id are parsed here for muxorch to retrieve
         sai_object_id_t tc_to_dscp_map_id = SAI_NULL_OBJECT_ID;
         sai_object_id_t tc_to_queue_map_id = SAI_NULL_OBJECT_ID;
 
         bool valid = true;
+        task_process_status task_status = task_process_status::task_success;
 
         sai_object_id_t tunnel_id = SAI_NULL_OBJECT_ID;
 
@@ -176,23 +177,41 @@ void TunnelDecapOrch::doTask(Consumer& consumer)
                 }
                 else if (fvField(i) == decap_dscp_to_tc_field_name)
                 {
-                    dscp_to_dc_map_id = gQosOrch->resolveTunnelQosMap(table_name, key, decap_dscp_to_tc_field_name, t); 
-                    if (exists && dscp_to_dc_map_id != SAI_NULL_OBJECT_ID)
+                    dscp_to_tc_map_id = gQosOrch->resolveTunnelQosMap(table_name, key, decap_dscp_to_tc_field_name, t);
+                    if (dscp_to_tc_map_id == SAI_NULL_OBJECT_ID)
                     {
-                        setTunnelAttribute(fvField(i), dscp_to_dc_map_id, tunnel_id);
+                        SWSS_LOG_NOTICE("QoS map %s is not ready yet", decap_dscp_to_tc_field_name.c_str());
+                        task_status = task_process_status::task_need_retry;
+                        break;
+                    }
+                    if (exists)
+                    {
+                        setTunnelAttribute(fvField(i), dscp_to_tc_map_id, tunnel_id);
                     }
                 }
                 else if (fvField(i) == decap_tc_to_pg_field_name)
                 {
-                    tc_to_pg_map_id = gQosOrch->resolveTunnelQosMap(table_name, key, decap_tc_to_pg_field_name, t); 
-                    if (exists && tc_to_pg_map_id != SAI_NULL_OBJECT_ID)
+                    tc_to_pg_map_id = gQosOrch->resolveTunnelQosMap(table_name, key, decap_tc_to_pg_field_name, t);
+                    if (tc_to_pg_map_id == SAI_NULL_OBJECT_ID)
+                    {
+                        SWSS_LOG_NOTICE("QoS map %s is not ready yet", decap_tc_to_pg_field_name.c_str());
+                        task_status = task_process_status::task_need_retry;
+                        break;
+                    }
+                    if (exists)
                     {
                         setTunnelAttribute(fvField(i), tc_to_pg_map_id, tunnel_id);
                     }
                 }
                 else if (fvField(i) == encap_tc_to_dscp_field_name)
                 {
                     tc_to_dscp_map_id = gQosOrch->resolveTunnelQosMap(table_name, key, encap_tc_to_dscp_field_name, t);
+                    if (tc_to_dscp_map_id == SAI_NULL_OBJECT_ID)
+                    {
+                        SWSS_LOG_NOTICE("QoS map %s is not ready yet", encap_tc_to_dscp_field_name.c_str());
+                        task_status = task_process_status::task_need_retry;
+                        break;
+                    }
                     if (exists)
                     {
                         // Record only
@@ -202,6 +221,12 @@ void TunnelDecapOrch::doTask(Consumer& consumer)
                 else if (fvField(i) == encap_tc_to_queue_field_name)
                 {
                     tc_to_queue_map_id = gQosOrch->resolveTunnelQosMap(table_name, key, encap_tc_to_queue_field_name, t);
+                    if (tc_to_queue_map_id == SAI_NULL_OBJECT_ID)
+                    {
+                        SWSS_LOG_NOTICE("QoS map %s is not ready yet", encap_tc_to_queue_field_name.c_str());
+                        task_status = task_process_status::task_need_retry;
+                        break;
+                    }
                     if (exists)
                     {
                         // Record only
@@ -210,12 +235,18 @@ void TunnelDecapOrch::doTask(Consumer& consumer)
                 }
             }
 
+            if (task_status == task_process_status::task_need_retry)
+            {
+                ++it;
+                continue;
+            }
+
             //create new tunnel if it doesn't exists already
             if (valid && !exists)
             {
 
                 if (addDecapTunnel(key, tunnel_type, ip_addresses, p_src_ip, dscp_mode, ecn_mode, encap_ecn_mode, ttl_mode,
-                                    dscp_to_dc_map_id, tc_to_pg_map_id))
+                                    dscp_to_tc_map_id, tc_to_pg_map_id))
                 {
                     // Record only
                     tunnelTable[key].encap_tc_to_dscp_map_id = tc_to_dscp_map_id;
@@ -233,7 +264,7 @@ void TunnelDecapOrch::doTask(Consumer& consumer)
         {
             if (exists)
             {
-                removeDecapTunnel(key);
+                removeDecapTunnel(table_name, key);
             }
             else
             {
@@ -717,12 +748,13 @@ bool TunnelDecapOrch::setIpAttribute(string key, IpAddresses new_ip_addresses, s
  *    @brief remove decap tunnel
  *
  * Arguments:
+ *    @param[in] table_name - name of the table in APP_DB
  *    @param[in] key - key of the tunnel from APP_DB
  *
  * Return Values:
  *    @return true on success and false if there's an error
  */
-bool TunnelDecapOrch::removeDecapTunnel(string key)
+bool TunnelDecapOrch::removeDecapTunnel(string table_name, string key)
 {
     sai_status_t status;
     TunnelEntry *tunnel_info = &tunnelTable.find(key)->second;
@@ -764,6 +796,7 @@ bool TunnelDecapOrch::removeDecapTunnel(string key)
     }
 
     tunnelTable.erase(key);
+    gQosOrch->removeTunnelReference(table_name, key);
     return true;
 }
 
@@ -996,4 +1029,4 @@ bool TunnelDecapOrch::getQosMapId(const std::string &tunnelKey, const std::strin
         return false;
     }
     return true;
-}
+}

orchagent/tunneldecaporch.h

@@ -78,7 +78,7 @@ class TunnelDecapOrch : public Orch
     bool addDecapTunnel(std::string key, std::string type, swss::IpAddresses dst_ip, swss::IpAddress* p_src_ip,
                         std::string dscp, std::string ecn, std::string encap_ecn, std::string ttl,
                         sai_object_id_t dscp_to_tc_map_id, sai_object_id_t tc_to_pg_map_id);
-    bool removeDecapTunnel(std::string key);
+    bool removeDecapTunnel(std::string table_name, std::string key);
 
     bool addDecapTunnelTermEntries(std::string tunnelKey, swss::IpAddress src_ip, swss::IpAddresses dst_ip, sai_object_id_t tunnel_id, TunnelTermType type);
     bool removeDecapTunnelTermEntry(sai_object_id_t tunnel_term_id, std::string ip);

tests/test_tunnel.py

@@ -83,18 +83,22 @@ def create_and_test_tunnel(self, db, asicdb, tunnel_name, **kwargs):
 
         decap_dscp_to_tc_map_oid = None
         decap_tc_to_pg_map_oid = None
+        skip_tunnel_creation = False
 
         if "decap_dscp_to_tc_map_oid" in kwargs:
             decap_dscp_to_tc_map_oid = kwargs.pop("decap_dscp_to_tc_map_oid")
 
         if "decap_tc_to_pg_map_oid" in kwargs:
             decap_tc_to_pg_map_oid = kwargs.pop("decap_tc_to_pg_map_oid")
 
-        fvs = create_fvs(**kwargs)
-
-        # create tunnel entry in DB
-        ps = swsscommon.ProducerStateTable(db, self.APP_TUNNEL_DECAP_TABLE_NAME)
-        ps.set(tunnel_name, fvs)
+        if "skip_tunnel_creation" in kwargs:
+            skip_tunnel_creation = kwargs.pop("skip_tunnel_creation")
+
+        if not skip_tunnel_creation:
+            fvs = create_fvs(**kwargs)
+            # create tunnel entry in DB
+            ps = swsscommon.ProducerStateTable(db, self.APP_TUNNEL_DECAP_TABLE_NAME)
+            ps.set(tunnel_name, fvs)
 
         # wait till config will be applied
         time.sleep(1)
@@ -191,10 +195,10 @@ def add_qos_map(self, configdb, asicdb, qos_map_type_name, qos_map_name, qos_map
         oid = diff.pop()
         return oid
 
-    def remove_qos_map(self, configdb, qos_map_type_name, qos_map_oid):
+    def remove_qos_map(self, configdb, qos_map_type_name, qos_map_name):
         """ Remove the testing qos map"""
         table = swsscommon.Table(configdb, qos_map_type_name)
-        table._del(qos_map_oid)
+        table._del(qos_map_name)
 
     def cleanup_left_over(self, db, asicdb):
         """ Cleanup APP and ASIC tables """
@@ -207,10 +211,9 @@ def cleanup_left_over(self, db, asicdb):
         for key in tunnel_term_table.getKeys():
             tunnel_term_table._del(key)
 
-        tunnel_app_table = swsscommon.Table(asicdb, self.APP_TUNNEL_DECAP_TABLE_NAME)
+        tunnel_app_table = swsscommon.Table(db, self.APP_TUNNEL_DECAP_TABLE_NAME)
         for key in tunnel_app_table.getKeys():
-            tunnel_table._del(key)
-
+            tunnel_app_table._del(key)
 
 class TestDecapTunnel(TestTunnelBase):
     """ Tests for decap tunnel creation and removal """
@@ -268,11 +271,59 @@ def test_TunnelDecap_MuxTunnel(self, dvs, testlog):
             "decap_tc_to_pg_map_oid": tc_to_pg_map_oid
         }
         self.create_and_test_tunnel(db, asicdb, tunnel_name="MuxTunnel0", **params)
+
+        # Remove Tunnel first
+        self.remove_and_test_tunnel(db, asicdb,"MuxTunnel0")
+
+        self.remove_qos_map(configdb, swsscommon.CFG_DSCP_TO_TC_MAP_TABLE_NAME, self.TUNNEL_QOS_MAP_NAME)
+        self.remove_qos_map(configdb, swsscommon.CFG_TC_TO_PRIORITY_GROUP_MAP_TABLE_NAME, self.TUNNEL_QOS_MAP_NAME)
+
+    def test_TunnelDecap_MuxTunnel_with_retry(self, dvs, testlog):
+        """ Test MuxTunnel creation. """
+        db = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0)
+        asicdb = swsscommon.DBConnector(swsscommon.ASIC_DB, dvs.redis_sock, 0)
+        configdb = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0)
+
+        self.cleanup_left_over(db, asicdb)
 
+        # Create MuxTunnel0 with QoS remapping attributes
+        params = {
+            "tunnel_type": "IPINIP",
+            "src_ip": "1.1.1.1",
+            "dst_ip": "1.1.1.2",
+            "dscp_mode": "pipe",
+            "ecn_mode": "copy_from_outer",
+            "ttl_mode": "uniform",
+            "decap_dscp_to_tc_map": "AZURE_TUNNEL",
+            "decap_tc_to_pg_map": "AZURE_TUNNEL", 
+        }
+        # Verify tunnel is not created when decap_dscp_to_tc_map/decap_tc_to_pg_map is specified while oid is not ready in qosorch
+        fvs = create_fvs(**params)
+        # create tunnel entry in DB
+        ps = swsscommon.ProducerStateTable(db, self.APP_TUNNEL_DECAP_TABLE_NAME)
+        ps.set("MuxTunnel0", fvs)
+
+        time.sleep(1)
+        # check asic db table
+        tunnel_table = swsscommon.Table(asicdb, self.ASIC_TUNNEL_TABLE)
+        tunnels = tunnel_table.getKeys()
+        assert len(tunnels) == 0
+
+        #Verify tunneldecaporch creates tunnel when qos map is available
+        dscp_to_tc_map_oid = self.add_qos_map(configdb, asicdb, swsscommon.CFG_DSCP_TO_TC_MAP_TABLE_NAME, self.TUNNEL_QOS_MAP_NAME, self.DSCP_TO_TC_MAP)
+        tc_to_pg_map_oid = self.add_qos_map(configdb, asicdb, swsscommon.CFG_TC_TO_PRIORITY_GROUP_MAP_TABLE_NAME, self.TUNNEL_QOS_MAP_NAME, self.TC_TO_PRIORITY_GROUP_MAP)
+        params.update({
+                "decap_dscp_to_tc_map_oid": dscp_to_tc_map_oid,
+                "decap_tc_to_pg_map_oid": tc_to_pg_map_oid,
+                "skip_tunnel_creation": True
+            })
+        self.create_and_test_tunnel(db, asicdb, tunnel_name="MuxTunnel0", **params)
+
+        # Cleanup
+        self.remove_and_test_tunnel(db, asicdb,"MuxTunnel0")
         self.remove_qos_map(configdb, swsscommon.CFG_DSCP_TO_TC_MAP_TABLE_NAME, dscp_to_tc_map_oid)
         self.remove_qos_map(configdb, swsscommon.CFG_TC_TO_PRIORITY_GROUP_MAP_TABLE_NAME, tc_to_pg_map_oid)
 
-
 class TestSymmetricTunnel(TestTunnelBase):
     """ Tests for symmetric tunnel creation and removal """