[aclorch] egress mirroring and action capabilities #2

stepanblyschak · 2019-07-01T08:42:45Z

What I did

Why I did it

How I verified it

Details if related

Signed-off-by: Stepan Blyschak <[email protected]> [test_mirror] add integration test for egress mirror rule creation Signed-off-by: Stepan Blyschak <[email protected]>

Signed-off-by: Stepan Blyschak <[email protected]> [aclorch] add getAclActionTypeFromAttr in AclOrch Signed-off-by: Stepan Blyschak <[email protected]> [aclorch] add isAclActionSupported in AclOrch Signed-off-by: Stepan Blyschak <[email protected]> [aclorch] add generic AclRule::validateAddAction implementation Each derivative of AclRule now has to call AclRule::validateAddAction to validate if action is supported. Signed-off-by: Stepan Blyschak <[email protected]> [aclorch] move action support check in different method Signed-off-by: Stepan Blyschak <[email protected]> [aclorch] fix acl action support check Signed-off-by: Stepan Blyschak <[email protected]>

Signed-off-by: Stepan Blyschak <[email protected]>

Signed-off-by: Stepan Blyschak <[email protected]> [portsyncd] fix undefined behaviour in Linksync ctor Signed-off-by: Stepan Blyschak <[email protected]>

Signed-off-by: Stepan Blyschak <[email protected]>

stepanblyschak · 2019-07-01T11:33:14Z

orchagent/aclorch.h

@@ -164,13 +165,15 @@ struct AclRuleCounters
    }
 };

+class AclTable;


maybe unused

stepanblyschak · 2019-07-01T11:40:14Z

orchagent/aclorch.cpp

    if (attr_name != ACTION_MIRROR_ACTION)
    {
        return false;
    }

-    if (!m_pMirrorOrch->sessionExists(attr_value))
+    split(attr_value, attr_values, ':');
+    if (attr_values.size() > 2)


add a comment // "ingress/egress:mirror_session_name"

stepanblyschak · 2019-07-01T11:41:19Z

orchagent/aclorch.cpp

+
+    if (!m_pMirrorOrch->getSessionOid(m_sessionName, m_mirrorSessionOid))
+    {
+        throw runtime_error("Failed to get mirror session OID");


SWSS_LOG_THROW

stepanblyschak · 2019-07-01T11:58:09Z

orchagent/aclorch.cpp

+            }
+        }
+        fvVector.emplace_back(get_table_field(ACTION_PACKET_ACTION), acl_action_value_stream.str());
+        acl_action_value_stream.str(std::string());


consider to limit stream and delimiter to scope

…gstream in queryAclActionCapabilities Signed-off-by: Stepan Blyschak <[email protected]>

* 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 <[email protected]>

* [cbf] Added initial CBF support (#2) * Added CBF NHG support to NhgOrch * Added NhgMapOrch to handle DSCP_TO_FC and EXP_TO_FC tables * Added UT for the new NhgOrch function and NhgMapOrch Support sonic-net/SONiC#796 Co-authored-by: Alexandru Banu <[email protected]>

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 <[email protected]>

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

Stepan Blyschak added 6 commits June 30, 2019 19:37

[aclorch] support egress mirror action

70c87ef

Signed-off-by: Stepan Blyschak <[email protected]> [test_mirror] add integration test for egress mirror rule creation Signed-off-by: Stepan Blyschak <[email protected]>

[aclorch] put acl actions capabilities in state db

45e993f

Signed-off-by: Stepan Blyschak <[email protected]>

[portsyncd] fix portsyncd restart case

f845742

Signed-off-by: Stepan Blyschak <[email protected]> [portsyncd] fix undefined behaviour in Linksync ctor Signed-off-by: Stepan Blyschak <[email protected]>

[test_acl] add acl rule validation test cases

f691afd

Signed-off-by: Stepan Blyschak <[email protected]>

[doc] update schema for mirror rule

1b5d429

Signed-off-by: Stepan Blyschak <[email protected]>

stepanblyschak force-pushed the acl_cap branch from 5416cba to 1b5d429 Compare July 1, 2019 10:20

stepanblyschak commented Jul 1, 2019

View reviewed changes

orchagent/aclorch.h

@@ -164,13 +165,15 @@ struct AclRuleCounters

}

};

class AclTable;

Copy link

Owner Author

stepanblyschak Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe unused

stepanblyschak commented Jul 1, 2019

View reviewed changes

[aclorch] replace throw with SWSS_LOG_THROW and limit scope for strin…

56023cb

…gstream in queryAclActionCapabilities Signed-off-by: Stepan Blyschak <[email protected]>

stepanblyschak closed this Jul 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[aclorch] egress mirroring and action capabilities #2

[aclorch] egress mirroring and action capabilities #2

stepanblyschak commented Jul 1, 2019

stepanblyschak Jul 1, 2019

stepanblyschak Jul 1, 2019

stepanblyschak Jul 1, 2019

stepanblyschak Jul 1, 2019

[aclorch] egress mirroring and action capabilities #2

[aclorch] egress mirroring and action capabilities #2

Conversation

stepanblyschak commented Jul 1, 2019

stepanblyschak Jul 1, 2019

Choose a reason for hiding this comment

stepanblyschak Jul 1, 2019

Choose a reason for hiding this comment

stepanblyschak Jul 1, 2019

Choose a reason for hiding this comment

stepanblyschak Jul 1, 2019

Choose a reason for hiding this comment