Restructure code

orchagent/Makefile.am

@@ -3,7 +3,8 @@ INCLUDES = -I $(top_srcdir)/lib \
            -I $(top_srcdir)/warmrestart \
            -I flex_counter \
            -I debug_counter \
-           -I pbh
+           -I pbh \
+           -I nhg
 
 CFLAGS_SAI = -I /usr/include/sai
 
@@ -39,9 +40,11 @@ orchagent_SOURCES = \
             orchdaemon.cpp \
             orch.cpp \
             notifications.cpp \
-            nhgbase.cpp \
-            nhghandler.cpp \
-            cbfnhghandler.cpp \
+            nhgorch.cpp \
+            nhg/nhgbase.cpp \
+            nhg/nhghandler.cpp \
+            nhg/cbfnhghandler.cpp  \
+            nhgmaporch.cpp \
             routeorch.cpp \
             mplsrouteorch.cpp \
             neighorch.cpp \
@@ -82,8 +85,7 @@ orchagent_SOURCES = \
             isolationgrouporch.cpp \
             muxorch.cpp \
             macsecorch.cpp \
-            lagid.cpp  \
-            nhgmaporch.cpp
+            lagid.cpp
 
 orchagent_SOURCES += flex_counter/flex_counter_manager.cpp flex_counter/flex_counter_stat_manager.cpp
 orchagent_SOURCES += debug_counter/debug_counter.cpp debug_counter/drop_counter.cpp

orchagent/cbfnhghandler.cpp → orchagent/nhg/cbfnhghandler.cpp

@@ -43,7 +43,7 @@ void CbfNhgHandler::doTask(Consumer& consumer)
         string op = kfvOp(t);
 
         bool success;
-        const auto &cbf_nhg_it = m_syncedNextHopGroups.find(index);
+        const auto &cbf_nhg_it = m_syncdNextHopGroups.find(index);
 
         if (op == SET_COMMAND)
         {
@@ -81,7 +81,7 @@ void CbfNhgHandler::doTask(Consumer& consumer)
             /*
              * If the CBF group does not exist, create it.
              */
-            if (cbf_nhg_it == m_syncedNextHopGroups.end())
+            if (cbf_nhg_it == m_syncdNextHopGroups.end())
             {
                 /*
                  * If we reached the NHG limit, postpone the creation.
@@ -93,21 +93,21 @@ void CbfNhgHandler::doTask(Consumer& consumer)
                 }
                 else
                 {
-                    auto cbf_nhg = CbfNhg(index, p.second, selection_map);
-                    success = cbf_nhg.sync();
+                    auto cbf_nhg = std::make_unique<CbfNhg>(index, p.second, selection_map);
+                    success = cbf_nhg->sync();
 
                     if (success)
                     {
                         /*
                          * If the CBF NHG contains temporary NHGs as members,
                          * we have to keep checking for updates.
                          */
-                        if (cbf_nhg.hasTemps())
+                        if (cbf_nhg->hasTemps())
                         {
                             success = false;
                         }
 
-                        m_syncedNextHopGroups.emplace(index, NhgEntry<CbfNhg>(move(cbf_nhg)));
+                        m_syncdNextHopGroups.emplace(index, NhgEntry<CbfNhg>(move(cbf_nhg)));
                     }
                 }
             }
@@ -116,13 +116,13 @@ void CbfNhgHandler::doTask(Consumer& consumer)
              */
             else
             {
-                success = cbf_nhg_it->second.nhg.update(p.second, selection_map);
+                success = cbf_nhg_it->second.nhg->update(p.second, selection_map);
 
                 /*
                  * If the CBF NHG has temporary NHGs synced, we need to keep
                  * checking this group in case they are promoted.
                  */
-                if (cbf_nhg_it->second.nhg.hasTemps())
+                if (cbf_nhg_it->second.nhg->hasTemps())
                 {
                     success = false;
                 }
@@ -143,7 +143,7 @@ void CbfNhgHandler::doTask(Consumer& consumer)
                 success = true;
             }
             /* If the group doesn't exist, do nothing. */
-            else if (cbf_nhg_it == m_syncedNextHopGroups.end())
+            else if (cbf_nhg_it == m_syncdNextHopGroups.end())
             {
                 SWSS_LOG_WARN("Deleting inexistent CBF NHG %s", index.c_str());
                 /*
@@ -161,11 +161,11 @@ void CbfNhgHandler::doTask(Consumer& consumer)
             /* Otherwise, delete it. */
             else
             {
-                success = cbf_nhg_it->second.nhg.remove();
+                success = cbf_nhg_it->second.nhg->remove();
 
                 if (success)
                 {
-                    m_syncedNextHopGroups.erase(cbf_nhg_it);
+                    m_syncdNextHopGroups.erase(cbf_nhg_it);
                 }
             }
         }

orchagent/nhgbase.cpp → orchagent/nhg/nhgbase.cpp

@@ -4,7 +4,7 @@
 
 extern sai_object_id_t gSwitchId;
 
-unsigned NhgBase::m_syncedCount = 0;
+unsigned NhgBase::m_syncdCount = 0;
 
 /*
  * Purpose: Destructor.
@@ -17,11 +17,10 @@ NhgBase::~NhgBase()
 {
     SWSS_LOG_ENTER();
 
-    if (isSynced())
-    {
-        SWSS_LOG_ERROR("Destroying next hop group with SAI ID %lu which is still synced.", m_id);
-        assert(false);
-    }
+    /*
+     * The group member should be removed from its group before destroying it.
+     */
+    assert(!isSynced());
 }
 
 /*
@@ -35,11 +34,11 @@ void NhgBase::decSyncedCount()
 {
     SWSS_LOG_ENTER();
 
-    if (m_syncedCount == 0)
+    if (m_syncdCount == 0)
     {
         SWSS_LOG_ERROR("Decreasing next hop groups count while already 0");
         throw logic_error("Decreasing next hop groups count while already 0");
     }
 
-    --m_syncedCount;
+    --m_syncdCount;
 }

orchagent/nhgbase.h → orchagent/nhg/nhgbase.h

@@ -49,7 +49,7 @@ class NhgBase
      */
     inline sai_object_id_t getId() const { SWSS_LOG_ENTER(); return m_id; }
     static inline unsigned getSyncedCount()
-                                    { SWSS_LOG_ENTER(); return m_syncedCount; }
+                                    { SWSS_LOG_ENTER(); return m_syncdCount; }
 
     /*
      * Check if the next hop group is synced or not.
@@ -67,7 +67,7 @@ class NhgBase
     virtual NextHopGroupKey getNhgKey() const = 0;
 
     /* Increment the number of existing groups. */
-    static inline void incSyncedCount() { SWSS_LOG_ENTER(); ++m_syncedCount; }
+    static inline void incSyncedCount() { SWSS_LOG_ENTER(); ++m_syncdCount; }
 
     /* Decrement the number of existing groups. */
     static void decSyncedCount();
@@ -83,7 +83,7 @@ class NhgBase
      * decremented when an object is removed.  This will also account for the
      * groups created by RouteOrch.
      */
-    static unsigned m_syncedCount;
+    static unsigned m_syncdCount;
 };
 
 /*
@@ -116,34 +116,15 @@ class NhgMember
     NhgMember(const NhgMember&) = delete;
     void operator=(const NhgMember&) = delete;
 
-    virtual ~NhgMember()
-    {
-        SWSS_LOG_ENTER();
-
-        if (isSynced())
-        {
-            SWSS_LOG_ERROR("Deleting next hop group member which is still synced");
-            assert(false);
-        }
-    }
-
     /*
      * Sync the NHG member, setting its SAI ID.
      */
     virtual void sync(sai_object_id_t gm_id)
     {
         SWSS_LOG_ENTER();
 
-        /*
-            * The SAI ID should be updated from invalid to something valid.
-            */
-        if ((m_id != SAI_NULL_OBJECT_ID) || (gm_id == SAI_NULL_OBJECT_ID))
-        {
-            SWSS_LOG_ERROR("Setting invalid SAI ID %lu to next hop group "
-                            "membeer %s, with current SAI ID %lu",
-                            gm_id, to_string().c_str(), m_id);
-            throw logic_error("Invalid SAI ID assigned to next hop group member");
-        }
+        /* The SAI ID should be updated from invalid to something valid. */
+        assert((m_gm_id == SAI_NULL_OBJECT_ID) && (gm_id != SAI_NULL_OBJECT_ID));
 
         m_id = gm_id;
         gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_NEXTHOP_GROUP_MEMBER);
@@ -388,22 +369,24 @@ class NhgCommon : public NhgBase
 };
 
 /*
- * Structure describing a next hop group which NhgHandler owns.  Beside having
- * a next hop group, we also want to keep a ref count so we don't delete
- * objects that are still referenced.
+ * Structure describing a next hop group which NhgOrch owns.  Beside having a
+ * unique pointer to that next hop group, we also want to keep a ref count so
+ * NhgOrch knows how many other objects reference the next hop group in order
+ * not to remove them while still being referenced.
  */
 template <typename NhgClass>
 struct NhgEntry
 {
-    /* The next hop group object in this entry. */
-    NhgClass nhg;
+    /* Pointer to the next hop group.  NhgOrch is the sole owner of it. */
+    std::unique_ptr<NhgClass> nhg;
 
     /* Number of external objects referencing this next hop group. */
     unsigned ref_count;
 
     NhgEntry() = default;
-    explicit NhgEntry(NhgClass&& _nhg, unsigned int _ref_count = 0) :
-                nhg(move(_nhg)), ref_count(_ref_count) { SWSS_LOG_ENTER(); }
+    explicit NhgEntry(std::unique_ptr<NhgClass>&& _nhg,
+                      unsigned int _ref_count = 0) :
+        nhg(std::move(_nhg)), ref_count(_ref_count) {}
 };
 
 /*
@@ -419,22 +402,22 @@ class NhgHandlerCommon
     inline bool hasNhg(const string &index) const
     {
         SWSS_LOG_ENTER();
-        return m_syncedNextHopGroups.find(index) != m_syncedNextHopGroups.end();
+        return m_syncdNextHopGroups.find(index) != m_syncdNextHopGroups.end();
     }
 
     /*
      * Get the next hop group with the given index.  If the index does not
      * exist in the map, a out_of_range eexception will be thrown.
      */
     inline const NhgClass& getNhg(const string &index) const
-                    { SWSS_LOG_ENTER(); return m_syncedNextHopGroups.at(index).nhg; }
+                                { return *m_syncdNextHopGroups.at(index).nhg; }
 
     /* Increase the ref count for a NHG given by it's index. */
     void incNhgRefCount(const string& index)
     {
         SWSS_LOG_ENTER();
 
-        auto& nhg_entry = m_syncedNextHopGroups.at(index);
+        auto& nhg_entry = m_syncdNextHopGroups.at(index);
         ++nhg_entry.ref_count;
     }
 
@@ -443,23 +426,16 @@ class NhgHandlerCommon
     {
         SWSS_LOG_ENTER();
 
-        auto& nhg_entry = m_syncedNextHopGroups.at(index);
+        auto& nhg_entry = m_syncdNextHopGroups.at(index);
 
         /* Sanity check so we don't overflow. */
-        if (nhg_entry.ref_count == 0)
-        {
-            SWSS_LOG_ERROR("Trying to decrement next hop group %s reference "
-                            "count while none are left.",
-                            nhg_entry.nhg.to_string().c_str());
-            throw logic_error("Decreasing ref count which is already 0");
-        }
-
+        assert(nhg_entry.ref_count > 0);
         --nhg_entry.ref_count;
     }
 
 protected:
     /*
      * Map of synced next hop groups.
      */
-    unordered_map<string, NhgEntry<NhgClass>> m_syncedNextHopGroups;
+    unordered_map<string, NhgEntry<NhgClass>> m_syncdNextHopGroups;
 };