[CoPP] Add redesign change - always_enabled field by noaOrMlnx · Pull Request #5 · noaOrMlnx/sonic-swss

noaOrMlnx · 2021-11-07T15:25:14Z

What I did
Add the new design for copp manager - always_enabled field in copp_cfg.json file.

Why I did it
A change was done for traps needs to be installed but doesn't have feature (arp, udld, lacp, ip2me),
in the new implementation, coppmgr will not automatically install traps which has no entry in features table, but will check first if the trap has "always_enabled":"true" field.

How I verified it
run tests for making sure traps are installed and not when expecting them to.
related also to noaOrMlnx/sonic-buildimage#19
Details if related
related to sonic-buildimage change - TBD

dgsudharsan · 2021-11-09T05:43:42Z

cfgmgr/coppmgr.cpp

-            m_coppTrapConfMap[i.first].trap_group = trap_group;
-            m_coppTrapConfMap[i.first].trap_ids = trap_ids;
-            setCoppTrapStateOk(i.first);
+            if (std::find(m_coppDisabledTraps.begin(), m_coppDisabledTraps.end(), i.first) == m_coppDisabledTraps.end())


I don't think this if check is needed here. It may be needed only for setCoppTrapStateOk. The configs need to be captured in the global variables but should not programmed.

dgsudharsan · 2021-11-09T05:50:53Z

cfgmgr/coppmgr.cpp

-        setCoppGroupStateOk(i.first);
-        auto g_cfg = std::find(group_cfg_keys.begin(), group_cfg_keys.end(), i.first);
-        if (g_cfg != group_cfg_keys.end())
+        if (std::find(m_coppDisabledTraps.begin(), m_coppDisabledTraps.end(), i.first) == m_coppDisabledTraps.end())


Isn't i.first trap_group name rather than trap_name? Please remove this check

dgsudharsan · 2021-11-09T05:56:30Z

cfgmgr/coppmgr.cpp

+{
+    removeTrapIdsFromTrapGroup(m_coppTrapConfMap[key].trap_group, m_coppTrapConfMap[key].trap_ids);
+    trap_ids.clear();
+    setCoppTrapStateOk(key);


Shouldn't this be delCoppTrapStateOk?

I copied it from https://github.com/Azure/sonic-swss/blob/master/cfgmgr/coppmgr.cpp#L457 and just put it in one function to reuse the code. do you think it should be del and not set?

Hi Noa. On the flow when it was part of the function set makes sense but when you are moving to a separate function which will be used again it doesn't.
I would suggest move the state DB update outside of the add and remove API

dgsudharsan · 2021-11-09T06:19:24Z

cfgmgr/coppmgr.cpp

+    }
+}
+
+void CoppMgr::addTrap(std::vector<FieldValueTuple> fvs, std::string trap_ids, std::string trap_group, std::string is_always_enabled, std::string trap_group_trap_ids)


Remove unused is_always_enabled.
fvs, trap_group_trap_ids can be a local variables inside this function as callers don't pass any value.

dgsudharsan · 2021-11-09T06:22:06Z

cfgmgr/coppmgr.cpp

            {
-                it = consumer.m_toSync.erase(it);
-                continue;
+                std::vector<std::string> feature_keys;


Please don't add code inside duplicate check. The duplicate check is meant to return without processing when config is duplicate.
Let's assume if we have ability to change two fields in future(e.g. trap_group and always_enabled). This logic will not work.

Revisit the duplicate check and modify it according to new field added.

dgsudharsan · 2021-11-09T06:26:27Z

cfgmgr/coppmgr.cpp

+
+                    if (std::find(feature_keys.begin(), feature_keys.end(), key) != feature_keys.end())
+                    {
+                        m_cfgFeatureTable.get(key, feature_fvs);


Can we cache the featureTable in a local datastructure in coppmgr class? Since we are listening to feature table notifications and we are reading feature table during init, it should be easier to build a cache.

dgsudharsan · 2021-11-13T01:19:18Z

cfgmgr/coppmgr.cpp

    {
        std::vector<FieldValueTuple> feature_fvs;
        m_cfgFeatureTable.get(i, feature_fvs);
+        m_featuresTableCache.emplace(i, feature_fvs);


Can we rename it to something like m_featureCfg or similar? We don't use Cache in variable names.

dgsudharsan · 2021-11-13T01:34:00Z

cfgmgr/coppmgr.cpp

    }
 }

+void CoppMgr::removeTrap(std::string key, std::string trap_ids, std::vector<FieldValueTuple> fvs)


Why do you need to pass trap_ids and fvs as parameter here? Can't they be declared as local variables?

dgsudharsan · 2021-11-13T01:35:00Z

cfgmgr/coppmgr.cpp

+    }
+}
+
+void CoppMgr::addTrap(std::vector<FieldValueTuple> fvs, std::string trap_ids, std::string trap_group)


Please use fvs as local variable

dgsudharsan · 2021-11-13T01:48:34Z

cfgmgr/coppmgr.cpp

            }
+
+            /* if always_enabled field has been changed */
+            if (conf_present &&


Pkease move the code below the if code
/* Incomplete configuration. Do not process until both trap group
* and trap_ids are available
*/
if (trap_group.empty() || trap_ids.empty())
{
it = consumer.m_toSync.erase(it);
continue;
}

Else if there is incomplete configuration there might be a crash

dgsudharsan · 2021-11-13T01:52:06Z

cfgmgr/coppmgr.cpp

+                    if the trap is not installed, install it.
+                    otherwise, do nothing. */
+
+                    m_coppAlwaysEnabledTraps.insert(key);


Since you are also storing is_always_enabled in copp_cfg, is there a need for this map m_coppAlwaysEnabledTraps?

Just like the featureCache - to be able to check easily if the feature is always enabled.

dgsudharsan · 2021-11-13T02:10:19Z

cfgmgr/coppmgr.cpp

+                    if no, remove the trap. is yes, do nothing. */
+                    m_coppAlwaysEnabledTraps.erase(key);
+
+                    auto cacheIt = m_featuresTableCache.find(key);


Please avoid variable with name cache.

dgsudharsan · 2021-11-13T02:13:30Z

cfgmgr/coppmgr.cpp

+                            if (fvField(i) == "state" && fvValue(i) == "enabled")
+                            {
+                                it = consumer.m_toSync.erase(it);
+                                continue;


'continue' will be applicable for the immediately preceding for loop. So this logic will remove trap even if enabled. Can you double check?

… into noaor_copp_redesign

**What I did** Fix the Mem Leak by moving the raw pointers in type_maps to use smart pointers **Why I did it** ``` Indirect leak of 83776 byte(s) in 476 object(s) allocated from: #0 0x7f0a2a414647 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99 #1 0x5555590cc923 in __gnu_cxx::new_allocator, std::allocator > const, referenced_object> > >::allocate(unsigned long, void const*) /usr/include/c++/10/ext/new_allocator.h:115 #2 0x5555590cc923 in std::allocator_traits, std::allocator > const, referenced_object> > > >::allocate(std::allocator, std::allocator > const, referenced_object> > >&, unsigned long) /usr/include/c++/10/bits/alloc_traits.h:460 #3 0x5555590cc923 in std::_Rb_tree, std::allocator >, std::pair, std::allocator > const, referenced_object>, std::_Select1st, std::allocator > const, referenced_object> >, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >::_M_get_node() /usr/include/c++/10/bits/stl_tree.h:584 #4 0x5555590cc923 in std::_Rb_tree_node, std::allocator > const, referenced_object> >* std::_Rb_tree, std::allocator >, std::pair, std::allocator > const, referenced_object>, std::_Select1st, std::allocator > const, referenced_object> >, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >::_M_create_node, std::allocator > const&>, std::tuple<> >(std::piecewise_construct_t const&, std::tuple, std::allocator > const&>&&, std::tuple<>&&) /usr/include/c++/10/bits/stl_tree.h:634 #5 0x5555590cc923 in std::_Rb_tree_iterator, std::allocator > const, referenced_object> > std::_Rb_tree, std::allocator >, std::pair, std::allocator > const, referenced_object>, std::_Select1st, std::allocator > const, referenced_object> >, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >::_M_emplace_hint_unique, std::allocator > const&>, std::tuple<> >(std::_Rb_tree_const_iterator, std::allocator > const, referenced_object> >, std::piecewise_construct_t const&, std::tuple, std::allocator > const&>&&, std::tuple<>&&) /usr/include/c++/10/bits/stl_tree.h:2461 #6 0x5555590e8757 in std::map, std::allocator >, referenced_object, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >::operator[](std::__cxx11::basic_string, std::allocator > const&) /usr/include/c++/10/bits/stl_map.h:501 #7 0x5555590d48b0 in Orch::setObjectReference(std::map, std::allocator >, std::map, std::allocator >, referenced_object, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >*, std::less, std::allocator > >, std::allocator, std::allocator > const, std::map, std::allocator >, referenced_object, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >*> > >&, std::__cxx11::basic_string, std::allocator > const&, std::__cxx11::basic_string, std::allocator > const&, std::__cxx11::basic_string, std::allocator > const&, std::__cxx11::basic_string, std::allocator > const&) orchagent/orch.cpp:450 #8 0x5555594ff66b in QosOrch::handleQueueTable(Consumer&, std::tuple, std::allocator >, std::__cxx11::basic_string, std::allocator >, std::vector, std::allocator >, std::__cxx11::basic_string, std::allocator > >, std::allocator, std::allocator >, std::__cxx11::basic_string, std::allocator > > > > >&) orchagent/qosorch.cpp:1763 #9 0x5555594edbd6 in QosOrch::doTask(Consumer&) orchagent/qosorch.cpp:2179 #10 0x5555590c8743 in Consumer::drain() orchagent/orch.cpp:241 #11 0x5555590c8743 in Consumer::drain() orchagent/orch.cpp:238 #12 0x5555590c8743 in Consumer::execute() orchagent/orch.cpp:235 #13 0x555559090dad in OrchDaemon::start() orchagent/orchdaemon.cpp:755 #14 0x555558e9be25 in main orchagent/main.cpp:766 #15 0x7f0a299b6d09 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x23d09) ```

noaOrMlnx mentioned this pull request Nov 8, 2021

[CoPP] Add always_enabled field to copp_cfg.j2 and yang model noaOrMlnx/sonic-buildimage#19

Closed

5 tasks

dgsudharsan suggested changes Nov 9, 2021

View reviewed changes

noaOrMlnx added 3 commits November 11, 2021 14:08

[CoPP] Add always_enabled field to coppmgr logic

f22615d

[CoPP] Add always_enabled field to coppmgr logic

d31852d

Fix CR comments

37df59f

noaOrMlnx force-pushed the noaor_copp_redesign branch from c674355 to 37df59f Compare November 11, 2021 14:08

dgsudharsan suggested changes Nov 13, 2021

View reviewed changes

noaOrMlnx added 2 commits November 14, 2021 15:22

Fix CR comments

1def31f

Merge branch 'noaor_copp_redesign' of github.com:noaOrMlnx/sonic-swss…

3182ebf

… into noaor_copp_redesign

noaOrMlnx force-pushed the noaor_copp_redesign branch from 6b8df19 to 3182ebf Compare November 15, 2021 22:42

dgsudharsan approved these changes Nov 15, 2021

View reviewed changes

noaOrMlnx closed this Nov 18, 2021

Conversation

noaOrMlnx commented Nov 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noaOrMlnx Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

noaOrMlnx commented Nov 7, 2021 •

edited

Loading

noaOrMlnx Nov 14, 2021 •

edited

Loading