Do not always crash syncd with event process error, provide SAI API f…

lib/inc/sai_redis.h

@@ -45,6 +45,11 @@ void check_notifications_pointers(
 // there is something wrong and we should fail
 #define GET_RESPONSE_TIMEOUT (6*60*1000)
 
+// if we don't receive response for create/set/remove from syncd in 30 seconds
+// there is something wrong and we should fail
+#define CSR_RESPONSE_TIMEOUT (30*1000)
+
+extern sai_status_t redis_get_response(const std::string &key, const std::string &op, sai_object_type_t object_type);
 extern std::string getSelectResultAsString(int result);
 extern void clear_local_state();
 extern void setRecording(bool record);
@@ -65,6 +70,7 @@ extern sai_service_method_table_t                   g_services;
 extern std::shared_ptr<swss::ProducerTable>         g_asicState;
 extern std::shared_ptr<swss::ConsumerTable>         g_redisGetConsumer;
 extern std::shared_ptr<swss::NotificationConsumer>  g_redisNotifications;
+extern std::shared_ptr<swss::NotificationConsumer>  g_redisApiResponseNtf;
 extern std::shared_ptr<swss::RedisClient>           g_redisClient;
 
 extern std::mutex g_apimutex;

lib/inc/sairedis.h

@@ -245,4 +245,17 @@ sai_status_t sai_bulk_remove_fdb_entry(
         _In_ const sai_fdb_entry_t *fdb_entry,
         _In_ sai_bulk_op_error_mode_t mode,
         _Out_ sai_status_t *object_statuses);
+
+#define sai_return_obj_op_status(object_type)  ({   \
+    bool ret;                                       \
+    switch (object_type)                            \
+    {                                               \
+        case SAI_OBJECT_TYPE_ROUTE_ENTRY:           \
+            ret = false; break;                     \
+        default:                                    \
+            ret = true; break;                      \
+    }                                               \
+    ret;                                            \
+})
+
 #endif // __SAIREDIS__

lib/src/sai_redis_generic_create.cpp

@@ -1,6 +1,7 @@
 #include "sai_redis.h"
 #include "meta/sai_serialize.h"
 #include "meta/saiattributelist.h"
+#include "sairedis.h"
 
 bool switch_ids[MAX_SWITCHES] = {};
 
@@ -197,6 +198,93 @@ void redis_free_virtual_object_id(
     }
 }
 
+// wait for response
+sai_status_t redis_get_response(
+    _In_ const std::string &key,
+    _In_ const std::string &op,
+    _In_ sai_object_type_t object_type)
+{
+    SWSS_LOG_ENTER();
+
+    swss::Select s;
+
+    sai_status_t status;
+    std::string csr;
+
+    // For certain object types, no response is expected
+    if (!sai_return_obj_op_status(object_type))
+    {
+        return SAI_STATUS_SUCCESS;
+    }
+
+    if (op == "create")
+    {
+        csr = "C";
+    }
+    else if (op == "set")
+    {
+        csr = "S";
+    }
+    else if (op == "remove")
+    {
+        csr = "R";
+    }
+    else
+    {
+        //getting repsonse for other operations not supported.
+        return SAI_STATUS_SUCCESS;
+    }
+
+    s.addSelectable(g_redisApiResponseNtf.get());
+
+    while (true)
+    {
+        SWSS_LOG_DEBUG("wait for response");
+
+        swss::Selectable *sel;
+
+        int result = s.select(&sel, CSR_RESPONSE_TIMEOUT);
+
+        if (result == swss::Select::OBJECT)
+        {
+            std::string retKey;
+            std::string str_sai_status;
+            std::vector<swss::FieldValueTuple> values;
+
+            g_redisApiResponseNtf->pop(retKey, str_sai_status, values);
+
+            // op in response should match the original request
+            if (key != retKey)
+            {
+                SWSS_LOG_ERROR("response key = %s, not matching request key %s", retKey.c_str(), key.c_str());
+                return SAI_STATUS_FAILURE;
+            }
+            SWSS_LOG_DEBUG("response: key = %s, op = %s, status = %s", key.c_str(), op.c_str(), str_sai_status.c_str());
+
+            sai_deserialize_status(str_sai_status, status);
+
+            if (g_record)
+            {
+                // first serialized is status
+                recordLine(csr + "|" + str_sai_status + "|" + key);
+            }
+
+            return status;
+        }
+
+        SWSS_LOG_ERROR("generic %s on %s failed due to SELECT operation result: %s",
+                op.c_str(), key.c_str(), getSelectResultAsString(result).c_str());
+        break;
+    }
+
+    if (g_record)
+    {
+        recordLine(csr + "|SAI_STATUS_FAILURE|" + key);
+    }
+
+    return SAI_STATUS_FAILURE;
+}
+
 sai_status_t internal_redis_generic_create(
         _In_ sai_object_type_t object_type,
         _In_ const std::string &serialized_object_id,
@@ -233,9 +321,7 @@ sai_status_t internal_redis_generic_create(
 
     g_asicState->set(key, entry, "create");
 
-    // we assume create will always succeed which may not be true
-    // we should make this synchronous call
-    return SAI_STATUS_SUCCESS;
+    return redis_get_response(key, "create", object_type);
 }
 
 sai_status_t redis_generic_create(

lib/src/sai_redis_generic_remove.cpp

@@ -21,7 +21,7 @@ sai_status_t internal_redis_generic_remove(
 
     g_asicState->del(key, "remove");
 
-    return SAI_STATUS_SUCCESS;
+    return redis_get_response(key, "remove", object_type);
 }
 
 sai_status_t redis_generic_remove(

lib/src/sai_redis_generic_set.cpp

@@ -28,7 +28,7 @@ sai_status_t internal_redis_generic_set(
 
     g_asicState->set(key, entry, "set");
 
-    return SAI_STATUS_SUCCESS;
+    return redis_get_response(key, "set", object_type);
 }
 
 sai_status_t internal_redis_bulk_generic_set(

lib/src/sai_redis_interfacequery.cpp

@@ -19,6 +19,7 @@ std::shared_ptr<swss::DBConnector>          g_dbNtf;
 std::shared_ptr<swss::ProducerTable>        g_asicState;
 std::shared_ptr<swss::ConsumerTable>        g_redisGetConsumer;
 std::shared_ptr<swss::NotificationConsumer> g_redisNotifications;
+std::shared_ptr<swss::NotificationConsumer> g_redisApiResponseNtf;
 std::shared_ptr<swss::RedisClient>          g_redisClient;
 
 void clear_local_state()
@@ -120,6 +121,7 @@ sai_status_t sai_api_initialize(
     g_asicState          = std::make_shared<swss::ProducerTable>(g_db.get(), ASIC_STATE_TABLE);
     g_redisGetConsumer   = std::make_shared<swss::ConsumerTable>(g_db.get(), "GETRESPONSE");
     g_redisNotifications = std::make_shared<swss::NotificationConsumer>(g_dbNtf.get(), "NOTIFICATIONS");
+    g_redisApiResponseNtf = std::make_shared<swss::NotificationConsumer>(g_dbNtf.get(), "APIRESPONSENTF");
     g_redisClient        = std::make_shared<swss::RedisClient>(g_db.get());
 
     clear_local_state();

syncd/syncd.cpp

@@ -29,6 +29,7 @@ std::mutex g_mutex;
 std::shared_ptr<swss::RedisClient>          g_redisClient;
 std::shared_ptr<swss::ProducerTable>        getResponse;
 std::shared_ptr<swss::NotificationProducer> notifications;
+std::shared_ptr<swss::NotificationProducer> apiResponseNotifications;
 
 /*
  * TODO: Those are hard coded values for mlnx integration for v1.0.1 they need
@@ -74,6 +75,32 @@ volatile bool g_asicInitViewMode = false;
  */
 sai_object_id_t gSwitchId;
 
+void internal_syncd_status_send(
+        _In_ const std::string &key,
+        _In_ sai_status_t status,
+        _In_ sai_object_type_t object_type,
+        _In_ const std::string &op)
+{
+    SWSS_LOG_ENTER();
+    if (!sai_return_obj_op_status(object_type))
+    {
+        return;
+    }
+
+    // Supporting SAI api call status return for these operations.
+    if (op != "create" && op != "set" && op != "remove" && op != "none")
+    {
+        return;
+    }
+
+    std::string str_status = sai_serialize_status(status);
+
+    SWSS_LOG_DEBUG("sending response for %s on %s with status: %s", op.c_str(), key.c_str(), str_status.c_str());
+
+    std::vector<swss::FieldValueTuple> entry;
+    apiResponseNotifications->send(key, str_status, entry);
+}
+
 struct cmdOptions
 {
     bool diagShell;
@@ -1160,7 +1187,8 @@ sai_status_t handle_generic(
 
     if (info->isnonobjectid)
     {
-        SWSS_LOG_THROW("passing non object id %s as generic object", info->objecttypename);
+        SWSS_LOG_ERROR("passing non object id %s as generic object", info->objecttypename);
+        return SAI_STATUS_INVALID_OBJECT_TYPE;
     }
 
     switch (api)
@@ -1176,7 +1204,8 @@ sai_status_t handle_generic(
 
                 if (switch_id == SAI_NULL_OBJECT_ID)
                 {
-                    SWSS_LOG_THROW("invalid switch_id translated from VID 0x%lx", object_id);
+                    SWSS_LOG_ERROR("invalid switch_id translated from VID 0x%lx", object_id);
+                    return SAI_STATUS_INVALID_OBJECT_ID;
                 }
 
                 if (object_type != SAI_OBJECT_TYPE_SWITCH)
@@ -1198,8 +1227,8 @@ sai_status_t handle_generic(
                          * NOTE: to support multiple switches we need support
                          * here for create.
                          */
-
-                        SWSS_LOG_THROW("creating multiple switches is not supported yet, FIXME");
+                        SWSS_LOG_ERROR("creating multiple switches is not supported yet, FIXME");
+                        return SAI_STATUS_NOT_SUPPORTED;
                     }
                 }
 
@@ -1338,8 +1367,8 @@ sai_status_t handle_generic(
             }
 
         default:
-
-            SWSS_LOG_THROW("common api (%s) is not implemented", sai_serialize_common_api(api).c_str());
+            SWSS_LOG_ERROR("common api (%s) is not implemented", sai_serialize_common_api(api).c_str());
+            return SAI_STATUS_NOT_SUPPORTED;
     }
 }
 
@@ -1393,7 +1422,8 @@ sai_status_t handle_non_object_id(
             return info->get(&meta_key, attr_count, attr_list);
 
         default:
-            SWSS_LOG_THROW("other apis not implemented");
+            SWSS_LOG_ERROR("other apis not implemented");
+            return SAI_STATUS_NOT_SUPPORTED;
     }
 }
 
@@ -2335,6 +2365,7 @@ sai_status_t processEvent(
 
     const std::string &str_object_type = key.substr(0, key.find(":"));
     const std::string &str_object_id = key.substr(key.find(":") + 1);
+    sai_status_t status;
 
     SWSS_LOG_INFO("key: %s op: %s", key.c_str(), op.c_str());
 
@@ -2378,7 +2409,10 @@ sai_status_t processEvent(
     }
     else
     {
-        SWSS_LOG_THROW("api '%s' is not implemented", op.c_str());
+        status = SAI_STATUS_NOT_IMPLEMENTED;
+        SWSS_LOG_ERROR("api '%s' is not implemented", op.c_str());
+        internal_syncd_status_send(key, status, SAI_OBJECT_TYPE_MAX, "none");
+        return status;
     }
 
     sai_object_type_t object_type;
@@ -2390,7 +2424,10 @@ sai_status_t processEvent(
 
     if (object_type == SAI_OBJECT_TYPE_NULL || object_type >= SAI_OBJECT_TYPE_MAX)
     {
-        SWSS_LOG_THROW("undefined object type %s", sai_serialize_object_type(object_type).c_str());
+        status = SAI_STATUS_INVALID_OBJECT_TYPE;
+        SWSS_LOG_ERROR("undefined object type %s", sai_serialize_object_type(object_type).c_str());
+        internal_syncd_status_send(key, status, object_type, op);
+        return status;
     }
 
     const std::vector<swss::FieldValueTuple> &values = kfvFieldsValues(kco);
@@ -2430,7 +2467,9 @@ sai_status_t processEvent(
 
     if (isInitViewMode())
     {
-        return processEventInInitViewMode(object_type, str_object_id, api, attr_count, attr_list);
+        status = processEventInInitViewMode(object_type, str_object_id, api, attr_count, attr_list);
+        internal_syncd_status_send(key, status, object_type, op);
+        return status;
     }
 
     if (api != SAI_COMMON_API_GET)
@@ -2449,8 +2488,6 @@ sai_status_t processEvent(
     // TODO use metadata utils
     auto info = sai_metadata_get_object_type_info(object_type);
 
-    sai_status_t status;
-
     /*
      * TODO use sai meta key deserialize
      */
@@ -2461,6 +2498,8 @@ sai_status_t processEvent(
 
         meta_key.objecttype = object_type;
 
+        status = SAI_STATUS_SUCCESS;
+
         switch (object_type)
         {
             case SAI_OBJECT_TYPE_FDB_ENTRY:
@@ -2476,11 +2515,13 @@ sai_status_t processEvent(
                 break;
 
             default:
-
-                SWSS_LOG_THROW("non object id %s is not supported yet, FIXME", info->objecttypename);
+                status = SAI_STATUS_NOT_SUPPORTED;
+                SWSS_LOG_ERROR("non object id %s is not supported yet, FIXME", info->objecttypename);
+        }
+        if (status == SAI_STATUS_SUCCESS)
+        {
+            status = handle_non_object_id(meta_key, api, attr_count, attr_list);
         }
-
-        status = handle_non_object_id(meta_key, api, attr_count, attr_list);
     }
     else
     {
@@ -2505,6 +2546,7 @@ sai_status_t processEvent(
         sai_object_id_t switch_vid = extractSwitchVid(object_type, str_object_id);
 
         internal_syncd_get_send(object_type, str_object_id, switch_vid, status, attr_count, attr_list);
+        return status;
     }
     else if (status != SAI_STATUS_SUCCESS)
     {
@@ -2525,12 +2567,13 @@ sai_status_t processEvent(
             SWSS_LOG_ERROR("attr: %s: %s", fvField(v).c_str(), fvValue(v).c_str());
         }
 
-        SWSS_LOG_THROW("failed to execute api: %s, key: %s, status: %s",
+        SWSS_LOG_ERROR("failed to execute api: %s, key: %s, status: %s",
                 op.c_str(),
                 key.c_str(),
                 sai_serialize_status(status).c_str());
     }
 
+    internal_syncd_status_send(key, status, object_type, op);
     return status;
 }
 
@@ -3269,6 +3312,7 @@ int syncd_main(int argc, char **argv)
 
     getResponse  = std::make_shared<swss::ProducerTable>(dbAsic.get(), "GETRESPONSE");
     notifications = std::make_shared<swss::NotificationProducer>(dbNtf.get(), "NOTIFICATIONS");
+    apiResponseNotifications = std::make_shared<swss::NotificationProducer>(dbNtf.get(), "APIRESPONSENTF");
 
     g_veryFirstRun = isVeryFirstRun();