[intfsorch]: Retry loopback action update failures

orchagent/intfsorch.cpp

@@ -76,6 +76,7 @@ IntfsOrch::IntfsOrch(DBConnector *db, vector<table_name_with_pri_t> tableNames,
         m_vidToRidTable = unique_ptr<Table>(new Table(m_asic_db.get(), "VIDTORID"));
     }
 
+
     auto intervT = timespec { .tv_sec = UPDATE_MAPS_SEC , .tv_nsec = 0 };
     m_updateMapsTimer = new SelectableTimer(intervT);
     auto executorT = new ExecutableTimer(m_updateMapsTimer, this, "UPDATE_MAPS_TIMER");
@@ -436,7 +437,7 @@ bool IntfsOrch::setIntfLoopbackAction(const Port &port, string actionStr)
 
     if (!getSaiLoopbackAction(actionStr, action))
     {
-        return false;
+        return true;
     }
 
     attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION;
@@ -1066,7 +1067,11 @@ void IntfsOrch::doTask(Consumer &consumer)
                     /* Set loopback action */
                     if (!loopbackAction.empty())
                     {
-                        setIntfLoopbackAction(port, loopbackAction);
+                        if (!setIntfLoopbackAction(port, loopbackAction))
+                        {
+                            it++;
+                            continue;
+                        }
                     }
                 }
             }
@@ -1975,4 +1980,3 @@ void IntfsOrch::voqSyncIntfState(string &alias, bool isUp)
     }
 
 }
-

tests/mock_tests/intfsorch_ut.cpp

@@ -16,6 +16,10 @@ namespace intfsorch_test
 
     int create_rif_count = 0;
     int remove_rif_count = 0;
+    bool saw_loopback_action = false;
+    bool fail_next_rif_set = false;
+    int loopback_action_set_count = 0;
+    sai_packet_action_t last_loopback_action = SAI_PACKET_ACTION_FORWARD;
     sai_router_interface_api_t *pold_sai_rif_api;
     sai_router_interface_api_t ut_sai_rif_api;
 
@@ -26,14 +30,35 @@ namespace intfsorch_test
             _In_ const sai_attribute_t *attr_list)
     {
         ++create_rif_count;
-        return SAI_STATUS_SUCCESS;
+        return pold_sai_rif_api->create_router_interface(
+            router_interface_id, switch_id, attr_count, attr_list);
     }
 
     sai_status_t _ut_remove_router_interface(
             _In_ sai_object_id_t router_interface_id)
     {
         ++remove_rif_count;
-        return SAI_STATUS_SUCCESS;
+        return pold_sai_rif_api->remove_router_interface(router_interface_id);
+    }
+
+    sai_status_t _ut_set_router_interface_attribute(
+            _In_ sai_object_id_t router_interface_id,
+            _In_ const sai_attribute_t *attr)
+    {
+        if (attr->id == SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION)
+        {
+            ++loopback_action_set_count;
+            last_loopback_action = static_cast<sai_packet_action_t>(attr->value.s32);
+            if (fail_next_rif_set)
+            {
+                fail_next_rif_set = false;
+                return SAI_STATUS_INSUFFICIENT_RESOURCES;
+            }
+            saw_loopback_action = true;
+            return SAI_STATUS_SUCCESS;
+        }
+        return pold_sai_rif_api->set_router_interface_attribute(
+            router_interface_id, attr);
     }
 
     struct IntfsOrchTest : public ::testing::Test
@@ -61,6 +86,11 @@ namespace intfsorch_test
 
             sai_router_intfs_api->create_router_interface = _ut_create_router_interface;
             sai_router_intfs_api->remove_router_interface = _ut_remove_router_interface;
+            sai_router_intfs_api->set_router_interface_attribute = _ut_set_router_interface_attribute;
+            saw_loopback_action = false;
+            fail_next_rif_set = false;
+            loopback_action_set_count = 0;
+            last_loopback_action = SAI_PACKET_ACTION_FORWARD;
 
             m_app_db = make_shared<swss::DBConnector>("APPL_DB", 0);
             m_config_db = make_shared<swss::DBConnector>("CONFIG_DB", 0);
@@ -489,4 +519,61 @@ namespace intfsorch_test
         ASSERT_EQ(syncd["Loopback6"].vrf_id, gVrfOrch->getVRFid("Vrf-Blue"));
         ASSERT_EQ(gVrfOrch->getVrfRefCount("Vrf-Blue"), base_vrf_ref + 1);
     }
+
+    TEST_F(IntfsOrchTest, IntfsOrchRetriesLoopbackActionSetFailure)
+    {
+        std::deque<KeyOpFieldsValuesTuple> entries{
+            {"Ethernet0", "SET", {{"mtu", "9100"}}}
+        };
+        auto consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        ASSERT_NE(consumer, nullptr);
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+
+        fail_next_rif_set = true;
+        entries = {
+            {"Ethernet0", "SET", {{"loopback_action", "drop"}}}
+        };
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+
+        ASSERT_EQ(consumer->m_toSync.size(), 1u);
+        ASSERT_EQ(loopback_action_set_count, 1);
+        ASSERT_FALSE(saw_loopback_action);
+
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+
+        ASSERT_TRUE(consumer->m_toSync.empty());
+        ASSERT_EQ(loopback_action_set_count, 2);
+        ASSERT_TRUE(saw_loopback_action);
+        ASSERT_EQ(last_loopback_action, SAI_PACKET_ACTION_DROP);
+    }
+
+    TEST_F(IntfsOrchTest, IntfsOrchIgnoresInvalidLoopbackActionField)
+    {
+        std::deque<KeyOpFieldsValuesTuple> entries{
+            {"Ethernet0", "SET", {{"mtu", "9100"}}}
+        };
+        auto consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        ASSERT_NE(consumer, nullptr);
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+
+        entries = {
+            {"Ethernet0", "SET", {
+                {"loopback_action", "invalid"},
+                {"nat_zone", "7"}
+            }}
+        };
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+
+        ASSERT_TRUE(consumer->m_toSync.empty());
+        ASSERT_EQ(loopback_action_set_count, 0);
+        ASSERT_FALSE(saw_loopback_action);
+
+        Port port;
+        ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port));
+        ASSERT_EQ(port.m_nat_zone_id, 7u);
+    }
 }