Add Break Before Make mechanism to syncd comparison logic (sonic-net#642)

* [player] Add more log info when fail response

* [syncd] Send notify view response even when exception happen

* [vs] Add resource limiter

* [vs] Start using resource limiter

* [syncd] Fix vendor sai functions names check

* [syncd] Add apply view remove object sanity checks

* [syncd] Lower NPU switch log to notice

* [syncd] Add simlar best match candidate with most same attributes

* [syncd] Add BreakConfig class

* [syncd] Add break config option to command line

* [syncd] Add comparison logic break before make path

* [syncd] Add break config parser

* [syncd] Add break config size method

* [syncd] Fix command line parser help message

* [syncd] Start using break config parser

* Update aspell

* [tests] Add unittests

Author: kcudnik

saiplayer/SaiPlayer.cpp

@@ -321,7 +321,8 @@ void SaiPlayer::match_list_lengths(
 
     if (get_attr_count != attr_count)
     {
-        SWSS_LOG_THROW("list number don't match %u != %u", get_attr_count, attr_count);
+        SWSS_LOG_THROW("list number don't match %u != %u (%s)", get_attr_count, attr_count,
+                sai_serialize_object_type(object_type).c_str());
     }
 
     for (uint32_t i = 0; i < attr_count; ++i)
@@ -447,7 +448,8 @@ void SaiPlayer::match_redis_with_rec(
 
     if (get_attr_count != attr_count)
     {
-        SWSS_LOG_THROW("list number don't match %u != %u", get_attr_count, attr_count);
+        SWSS_LOG_THROW("list number don't match %u != %u (%s)", get_attr_count, attr_count,
+                sai_serialize_object_type(object_type).c_str());
     }
 
     for (uint32_t i = 0; i < attr_count; ++i)

syncd/AsicView.cpp

@@ -754,6 +754,11 @@ void AsicView::asicRemoveObject(
 
     SWSS_LOG_INFO("%s: %s", currentObj->m_str_object_type.c_str(), currentObj->m_str_object_id.c_str());
 
+    if (currentObj->getObjectStatus() != SAI_OBJECT_STATUS_NOT_PROCESSED)
+    {
+        SWSS_LOG_THROW("FATAL: removing object with status: %d, logic error", currentObj->getObjectStatus());
+    }
+
     m_asicOperationId++;
 
     if (currentObj->isOidObject())
@@ -763,6 +768,16 @@ void AsicView::asicRemoveObject(
          * that check here also as sanity check.
          */
 
+        int count = getVidReferenceCount(currentObj->getVid());
+
+        if (count != 0)
+        {
+            SWSS_LOG_THROW("can't remove existing object %s:%s since reference count is %d, FIXME",
+                    currentObj->m_str_object_type.c_str(),
+                    currentObj->m_str_object_id.c_str(),
+                    count);
+        }
+
         m_soOids.erase(currentObj->m_str_object_id);
         m_oOids.erase(currentObj->m_meta_key.objectkey.key.object_id);
 

syncd/BestCandidateFinder.cpp

@@ -2358,6 +2358,117 @@ std::shared_ptr<SaiObj> BestCandidateFinder::findCurrentBestMatch(
     }
 }
 
+std::shared_ptr<SaiObj> BestCandidateFinder::findSimilarBestMatch(
+        _In_ const std::shared_ptr<const SaiObj> &temporaryObj)
+{
+    SWSS_LOG_ENTER();
+
+    // will find object in current view that have most same attributes
+
+    auto notProcessedObjects = m_currentView.getNotProcessedObjectsByObjectType(temporaryObj->getObjectType());
+
+    const auto attrs = temporaryObj->getAllAttributes();
+
+    SWSS_LOG_INFO("not processed objects for %s: %zu, temp attrs: %zu",
+            temporaryObj->m_str_object_type.c_str(),
+            notProcessedObjects.size(),
+            attrs.size());
+
+    std::vector<sai_object_compare_info_t> candidateObjects;
+
+    for (const auto &currentObj: notProcessedObjects)
+    {
+        // log how many attributes current object have
+
+        SWSS_LOG_INFO("current obj %s, attrs: %zu",
+                currentObj->m_str_object_id.c_str(),
+                currentObj->getAllAttributes().size());
+
+        sai_object_compare_info_t soci = { 0, currentObj };
+
+        /*
+         * NOTE: we only iterate by attributes that are present in temporary
+         * view. It may happen that current view has some additional attributes
+         * set that are create only and value can't be updated, we ignore that
+         * since we want to find object with most matching attributes.
+         */
+
+        for (const auto &attr: attrs)
+        {
+            sai_attr_id_t attrId = attr.first;
+
+            // Function hasEqualAttribute check if attribute exists on both objects.
+
+            if (hasEqualAttribute(m_currentView, m_temporaryView, currentObj, temporaryObj, attrId))
+            {
+                soci.equal_attributes++;
+
+                SWSS_LOG_INFO("ob equal %s %s, %s: %s",
+                        temporaryObj->m_str_object_id.c_str(),
+                        currentObj->m_str_object_id.c_str(),
+                        attr.second->getStrAttrId().c_str(),
+                        attr.second->getStrAttrValue().c_str());
+            }
+            else
+            {
+                SWSS_LOG_INFO("ob not equal %s %s, %s: %s",
+                        temporaryObj->m_str_object_id.c_str(),
+                        currentObj->m_str_object_id.c_str(),
+                        attr.second->getStrAttrId().c_str(),
+                        attr.second->getStrAttrValue().c_str());
+            }
+        }
+
+        // NOTE: we could check if current object has some attributes that are
+        // default value on temporary object, and count them in
+
+        candidateObjects.push_back(soci);
+    }
+
+    SWSS_LOG_INFO("number candidate objects for %s is %zu",
+            temporaryObj->m_str_object_id.c_str(),
+            candidateObjects.size());
+
+    if (candidateObjects.size() == 0)
+    {
+        SWSS_LOG_WARN("not processed objects in current view is zero");
+
+        return nullptr;
+    }
+
+    if (candidateObjects.size() == 1)
+    {
+        // We found only one object so return it as candidate
+
+        return candidateObjects.begin()->obj;
+    }
+
+    /*
+     * Sort candidate objects by equal attributes in descending order, we know
+     * here that we have at least 2 candidates.
+     */
+
+    std::sort(candidateObjects.begin(), candidateObjects.end(), compareByEqualAttributes);
+
+    if (candidateObjects.at(0).equal_attributes > candidateObjects.at(1).equal_attributes)
+    {
+        /*
+         * We have only 1 object with the greatest number of equal attributes
+         * lets choose that object as our best match.
+         */
+
+        SWSS_LOG_INFO("eq attributes: %ld vs %ld",
+            candidateObjects.at(0).equal_attributes,
+            candidateObjects.at(1).equal_attributes);
+
+        return candidateObjects.begin()->obj;
+    }
+
+    SWSS_LOG_WARN("same number of attributes equal, selecting first");
+
+    return candidateObjects.begin()->obj;
+}
+
 int BestCandidateFinder::findAllChildsInDependencyTreeCount(
         _In_ const AsicView &view,
         _In_ const std::shared_ptr<const SaiObj> &obj)

syncd/BestCandidateFinder.h

@@ -35,6 +35,9 @@ namespace syncd
             std::shared_ptr<SaiObj> findCurrentBestMatch(
                     _In_ const std::shared_ptr<const SaiObj> &temporaryObj);
 
+            std::shared_ptr<SaiObj> findSimilarBestMatch(
+                    _In_ const std::shared_ptr<const SaiObj> &temporaryObj);
+
         private:
 
             std::shared_ptr<SaiObj> findCurrentBestMatchForGenericObject(

syncd/BreakConfig.cpp

@@ -0,0 +1,49 @@
+#include "BreakConfig.h"
+
+#include "swss/logger.h"
+
+using namespace syncd;
+
+void BreakConfig::insert(
+        _In_ sai_object_type_t objectType)
+{
+    SWSS_LOG_ENTER();
+
+    m_set.insert(objectType);
+}
+
+void BreakConfig::remove(
+        _In_ sai_object_type_t objectType)
+{
+    SWSS_LOG_ENTER();
+
+    auto it = m_set.find(objectType);
+
+    if (it != m_set.end())
+    {
+        m_set.erase(it);
+    }
+}
+
+void BreakConfig::clear()
+{
+    SWSS_LOG_ENTER();
+
+    m_set.clear();
+}
+
+bool BreakConfig::shouldBreakBeforeMake(
+        _In_ sai_object_type_t objectType) const
+{
+    SWSS_LOG_ENTER();
+
+    return m_set.find(objectType) != m_set.end();
+}
+
+size_t BreakConfig::size() const
+{
+    SWSS_LOG_ENTER();
+
+    return m_set.size();
+}
+

syncd/BreakConfig.h

@@ -0,0 +1,39 @@
+#pragma once
+
+extern "C"{
+#include "sai.h"
+}
+
+#include <set>
+
+namespace syncd
+{
+    class BreakConfig
+    {
+        public:
+
+            BreakConfig() = default;
+
+            ~BreakConfig() = default;
+
+        public:
+
+            void insert(
+                    _In_ sai_object_type_t objectType);
+
+            void remove(
+                    _In_ sai_object_type_t objectType);
+
+            void clear();
+
+            bool shouldBreakBeforeMake(
+                    _In_ sai_object_type_t objectType) const;
+
+            size_t size() const;
+
+        private:
+
+            std::set<sai_object_type_t> m_set;
+
+    };
+}

syncd/BreakConfigParser.cpp

@@ -0,0 +1,59 @@
+#include "BreakConfigParser.h"
+
+#include "swss/logger.h"
+
+#include "meta/sai_serialize.h"
+
+using namespace syncd;
+
+std::shared_ptr<BreakConfig> BreakConfigParser::parseBreakConfig(
+        _In_ const std::string& filePath)
+{
+    SWSS_LOG_ENTER();
+
+    auto config = std::make_shared<BreakConfig>();
+
+    if (filePath.size() == 0)
+    {
+        return config; // return empty config
+    }
+
+    std::ifstream file(filePath);
+
+    if (!file.is_open())
+    {
+        SWSS_LOG_ERROR("failed to open break config file: %s: errno: %s, returning empty config", filePath.c_str(), strerror(errno));
+
+        return config;
+    }
+
+    std::string line;
+
+    while (getline(file, line))
+    {
+        if (line.size() > 0 && (line[0] == '#' || line[0] == ';'))
+        {
+            continue;
+        }
+
+        sai_object_type_t objectType;
+
+        try
+        {
+            sai_deserialize_object_type(line, objectType);
+
+            config->insert(objectType);
+
+            SWSS_LOG_INFO("inserting %s to break config", line.c_str());
+        }
+        catch (const std::exception& e)
+        {
+            SWSS_LOG_WARN("failed to parse '%s' as sai_object_type_t", line.c_str());
+        }
+    }
+
+    SWSS_LOG_NOTICE("break config parse success, contains %zu entries", config->size());
+
+
+    return config;
+}

syncd/BreakConfigParser.h

@@ -0,0 +1,23 @@
+#pragma once
+
+#include "BreakConfig.h"
+
+#include <memory>
+
+namespace syncd
+{
+    class BreakConfigParser
+    {
+        private:
+
+            BreakConfigParser() = delete;
+
+            ~BreakConfigParser() = delete;
+
+        public:
+
+            static std::shared_ptr<BreakConfig> parseBreakConfig(
+                    _In_ const std::string& filePath);
+
+    };
+}

syncd/CommandLineOptions.cpp

@@ -27,6 +27,8 @@ CommandLineOptions::CommandLineOptions()
 
     m_contextConfig = "";
 
+    m_breakConfig = "";
+
 #ifdef SAITHRIFT
 
     m_runRPCServer = false;
@@ -53,6 +55,7 @@ std::string CommandLineOptions::getCommandLineString() const
     ss << " ProfileMapFile=" << m_profileMapFile;
     ss << " GlobalContext=" << m_globalContext;
     ss << " ContextConfig=" << m_contextConfig;
+    ss << " BreakConfig=" << m_breakConfig;
 
 #ifdef SAITHRIFT
 

syncd/CommandLineOptions.h

@@ -76,6 +76,8 @@ namespace syncd
 
             std::string m_contextConfig;
 
+            std::string m_breakConfig;
+
 #ifdef SAITHRIFT
             bool m_runRPCServer;
             std::string m_portMapFile;