First time config push for AN and FEC #1

dgsudharsan · 2021-04-05T15:42:58Z

What I did
Enabling first time config push for AN/FEC
Why I did it
To force set CLI for the first time. Due to the duplicate checks present and initializations of default values, the first time configuration is not pushed.

How I verified it
Adding CLI and checking if it is pushed to hardware

Details if related

liat-grozovik

Missing unit test.
PR title is not aligned with the type of configuration to be pushed on first configuration.

BTW how does it work with upgrades?

liat-grozovik · 2021-04-05T15:49:38Z

orchagent/port.h

@@ -125,6 +125,8 @@ class Port

    std::unordered_set<sai_object_id_t> m_ingress_acl_tables_uset;
    std::unordered_set<sai_object_id_t> m_egress_acl_tables_uset;
+    bool             m_fec_cfg = false;


is the extra spaces is required?

Removed the space

liat-grozovik · 2021-04-05T16:05:26Z

@dgsudharsan when you have it verified and upstream please remember to mention if this is required for 201911 and 202012 and if so if it can be cherry picked without any issue or need to push a PR directly to them.

dgsudharsan · 2021-04-05T16:15:41Z

Missing unit test.
PR title is not aligned with the type of configuration to be pushed on first configuration.

BTW how does it work with upgrades?

Updated the PR with comments.
Only if external configuration is present it will be pushed. With systems without existing explicit configuration no issues. For systems with explicit configuration, during all 3 boot scenarios the configuration will be pushed down to SAI. While I expect no impact i would still need to test warm boot and fast boot scenarios which i will perform on integration testing.

liat-grozovik · 2021-04-05T16:16:48Z

tests/test_port.py

+        fvs = swsscommon.FieldValuePairs([("fec","none")])
+        ptbl.set("Ethernet0", fvs)
+
+        time.sleep(1)


i think on another PR stepan mentioned that tests moved to wait instead of sleep. can you please check?

should we test it on different type of ports? breakout port and non breakout port?
should you test different fec options? only only none?
how this test cover the new push flow?

liat-grozovik · 2021-04-05T16:19:16Z

tests/test_port_an.py

+        tbl = swsscommon.ProducerStateTable(db, "PORT_TABLE")
+
+        fvs = swsscommon.FieldValuePairs([("autoneg","0"), ("speed", "1000")])
+


remove extra line

liat-grozovik · 2021-04-05T16:19:44Z

tests/test_port_an.py

+    def test_PortAutoNegForce(self, dvs, testlog):
+
+        db = swsscommon.DBConnector(0, dvs.redis_sock, 0)
+


should you prepare the adb now as you did on the prev test for fec? any reason here or there it is not the same flow?

It is initialized in the flow lines below.

I know
I was asking if you should have the 2 DBs unit just after each other like you did in the prev function just to have the same flow

Got you. Will move the initialization to one place.

liat-grozovik · 2021-04-05T16:19:58Z

tests/test_port_an.py

+
+        tbl.set("Ethernet0", fvs)
+
+        time.sleep(1)


same comment as above: wait instead of sleep

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

First time config push

4a640d2

liat-grozovik suggested changes Apr 5, 2021

View reviewed changes

dgsudharsan changed the title ~~First time config push~~ First time config push for AN and FEC Apr 5, 2021

Addressing comments

c6e2acb

liat-grozovik reviewed Apr 5, 2021

View reviewed changes

Modifying Unit tests

a171a53

liat-grozovik approved these changes Apr 6, 2021

View reviewed changes

dgsudharsan merged commit df61bc5 into 202012 Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First time config push for AN and FEC #1

First time config push for AN and FEC #1

dgsudharsan commented Apr 5, 2021 •

edited

Loading

liat-grozovik left a comment

liat-grozovik Apr 5, 2021

dgsudharsan Apr 5, 2021

liat-grozovik commented Apr 5, 2021

dgsudharsan commented Apr 5, 2021

liat-grozovik Apr 5, 2021

liat-grozovik Apr 5, 2021

liat-grozovik Apr 5, 2021

dgsudharsan Apr 5, 2021

liat-grozovik Apr 5, 2021

dgsudharsan Apr 5, 2021

liat-grozovik Apr 5, 2021

		tbl = swsscommon.ProducerStateTable(db, "PORT_TABLE")

		fvs = swsscommon.FieldValuePairs([("autoneg","0"), ("speed", "1000")])

		def test_PortAutoNegForce(self, dvs, testlog):

		db = swsscommon.DBConnector(0, dvs.redis_sock, 0)

First time config push for AN and FEC #1

First time config push for AN and FEC #1

Conversation

dgsudharsan commented Apr 5, 2021 • edited Loading

liat-grozovik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liat-grozovik commented Apr 5, 2021

dgsudharsan commented Apr 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgsudharsan commented Apr 5, 2021 •

edited

Loading