added monitorTX class & add to orchs list by noaOrMlnx · Pull Request #1 · noaOrMlnx/sonic-swss

noaOrMlnx · 2019-12-04T15:13:56Z

What I did: added class in orchagent named MonitorTXOrch and added it to the orchs list in orchdaemon class

Why I did it

How I verified it

Details if related

stepanblyschak · 2019-12-04T17:35:58Z

orchagent/monitorTXorch.h

+{
+
+    public:
+        //MonitorTXOrch(DBConnector* ConfigDB, DBConnector* StateDB);


remove commented code

stepanblyschak · 2019-12-04T17:36:20Z

orchagent/monitorTXorch.h

+    #include "sai.h"
+}
+
+


extra empty line

stepanblyschak · 2019-12-04T17:36:27Z

orchagent/monitorTXorch.h

+
+enum PortState {OK, NOT_OK, UNKNOWN};
+
+


extra empty line

stepanblyschak · 2019-12-04T17:36:53Z

orchagent/monitorTXorch.h

+        Table m_stateTxErrTable;
+
+        // config table
+        // Table m_cfgTxErrTable;


remove commented code

stepanblyschak · 2019-12-04T17:39:03Z

orchagent/monitorTXorch.h

+        MonitorTXOrch(TableConnector configDBTConnector, TableConnector stateDBTConnector);
+        virtual void doTask(SelectableTimer &timer);
+        virtual void doTask(Consumer &consumer);
+


MonitorTXOrch inherits from virtual class, it is more correct to have virtual destructor for MonitorTXOrch

stepanblyschak · 2019-12-04T18:07:45Z

orchagent/monitorTXorch.cpp

+        // SWSS_LOG_NOTICE("port %s state is %s", sai_serialize_object_id(currPortId), stateNames[currState]);
+        SWSS_LOG_NOTICE("port s state is s");
+
+        m_portsState.emplace(currPortId, currState);


In state DB there should be no ID, it should be alias

stepanblyschak · 2019-12-04T18:08:30Z

orchagent/monitorTXorch.cpp

+    SWSS_LOG_ENTER();
+    SWSS_LOG_NOTICE("inside update port state");
+
+    // TODO: check if need to flush here  - clear table before inserting new states. 


your new state will override existing so no need to clear or flush

stepanblyschak · 2019-12-04T18:08:41Z

orchagent/monitorTXorch.cpp

+
+        m_stateTxErrTable.set(oidStr, fieldValuesVector);
+        SWSS_LOG_NOTICE("Set status s to port s");
+


extra newline

stepanblyschak · 2019-12-04T18:09:03Z

orchagent/monitorTXorch.cpp

+    return true; 
+}
+
+// void MonitorTXOrch::insertStateToDB(sai_object_id_t portID, PortState state) {


remove commented code

stepanblyschak · 2019-12-04T18:12:49Z

orchagent/monitorTXorch.cpp

+
+        vector<FieldValueTuple> fieldValuesVector;
+        fieldValuesVector.emplace_back(STATE_DB_TX_STATE, "UNKNOWN");
+        m_stateTxErrTable.set(oidStr, fieldValuesVector);


there are two places where you set the state, can you have a small routine that sets the state so this part of code will be reused.

* Fix the stack-overflow issue in AclOrch::getTableById() In case the table_id and m_mirrorTableId/m_mirrorTableId are empty The function would be called recursively without ending. ``` If we have some rule that does not have table defined, it will trigger this issue. (gdb) bt #0 0x00007f77ac5af3a9 in swss::Logger::write (this=0x7f77ac801ec0 <swss::Logger::getInstance()::m_logger>, prio=swss::Logger::SWSS_DEBUG, fmt=0x7f77ac5f1690 ":> %s: enter") at logger.cpp:209 #1 0x00000000004a5792 in AclOrch::getTableById (this=this@entry=0x1c722a0, table_id="") at aclorch.cpp:2617 #2 0x00000000004a59d1 in AclOrch::getTableById (this=this@entry=0x1c722a0, table_id="") at aclorch.cpp:2634 #3 0x00000000004a59d1 in AclOrch::getTableById (this=this@entry=0x1c722a0, table_id="") at aclorch.cpp:2634 ... #20944 0x00000000004a59d1 in AclOrch::getTableById (this=this@entry=0x1c722a0, table_id="") at aclorch.cpp:2634 #20945 0x00000000004ad3ce in AclOrch::doAclRuleTask (this=this@entry=0x1c722a0, consumer=...) at aclorch.cpp:2437 #20946 0x00000000004b04cd in AclOrch::doTask (this=0x1c722a0, consumer=...) at aclorch.cpp:2141 #20947 0x00000000004231b2 in Orch::doTask (this=0x1c722a0) at orch.cpp:369 #20948 0x000000000041c4e9 in OrchDaemon::start (this=this@entry=0x1c19960) at orchdaemon.cpp:376 #20949 0x0000000000409ffc in main (argc=<optimized out>, argv=0x7ffe2e392d68) at main.cpp:295 (gdb) p table_id $1 = "" (gdb) p m_mirrorTableId $2 = "" (gdb) p m_mirrorV6TableId $3 = "" ``` Signed-off-by: Zhenggen Xu <zxu@linkedin.com>

Currently, ASAN sometimes reports the BufferOrch::m_buffer_type_maps and QosOrch::m_qos_maps as leaked. However, their lifetime is the lifetime of a process so they are not really 'leaked'. This also adds a simple way to add more suppressions later if required. Example of ASAN report: Direct leak of 48 byte(s) in 1 object(s) allocated from: #0 0x7f96aa952d30 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xead30) #1 0x55ca1da9f789 in __static_initialization_and_destruction_0 /__w/2/s/orchagent/bufferorch.cpp:39 #2 0x55ca1daa02af in _GLOBAL__sub_I_bufferorch.cpp /__w/2/s/orchagent/bufferorch.cpp:1321 #3 0x55ca1e2a9cd4 (/usr/bin/orchagent+0xe89cd4) Direct leak of 48 byte(s) in 1 object(s) allocated from: #0 0x7f96aa952d30 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xead30) #1 0x55ca1da6d2da in __static_initialization_and_destruction_0 /__w/2/s/orchagent/qosorch.cpp:80 #2 0x55ca1da6ecf2 in _GLOBAL__sub_I_qosorch.cpp /__w/2/s/orchagent/qosorch.cpp:2000 #3 0x55ca1e2a9cd4 (/usr/bin/orchagent+0xe89cd4) - What I did Added an lsan suppression config with static variable leak suppression - Why I did it To suppress ASAN false positives - How I verified it Run a test that produces the static variable leaks report and checked that report has these leaks suppressed. Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>

…onic-net#2446) * Add events publish * Added header file * signature fix * syntax * syntax * syntax * syntax * syntax * syntax * Updated fake code * Remove if and log messages for event_publish * Remove if and log messages for event_publish (#1) * Remove event_handle_t from signature and add globally * Remove extern orchdaemon.cpp * Revert unneeded changes Co-authored-by: zbud-msft <zainbudhwani@microsoft.com> Co-authored-by: Zain Budhwani <99770260+zbud-msft@users.noreply.github.com>

**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) ```

added monitorTX class & add to orchs list

7938e25

stepanblyschak suggested changes Dec 4, 2019

View reviewed changes

Noa Or added 2 commits December 5, 2019 14:50

fixed comments after code review

c1f7d69

add function to set default vals in db

72444db

noaOrMlnx closed this Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added monitorTX class & add to orchs list#1

added monitorTX class & add to orchs list#1
noaOrMlnx wants to merge 3 commits intomasterfrom
noaor_codingAssignment

noaOrMlnx commented Dec 4, 2019

Uh oh!

stepanblyschak Dec 4, 2019

Uh oh!

stepanblyschak Dec 4, 2019

Uh oh!

stepanblyschak Dec 4, 2019

Uh oh!

stepanblyschak Dec 4, 2019

Uh oh!

stepanblyschak Dec 4, 2019

Uh oh!

stepanblyschak Dec 4, 2019

Uh oh!

stepanblyschak Dec 4, 2019

Uh oh!

stepanblyschak Dec 4, 2019

Uh oh!

stepanblyschak Dec 4, 2019

Uh oh!

stepanblyschak Dec 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		m_stateTxErrTable.set(oidStr, fieldValuesVector);
		SWSS_LOG_NOTICE("Set status s to port s");

Conversation

noaOrMlnx commented Dec 4, 2019

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants