[orchagent] add & remove port counters dynamically each time port was added or removed #2019

tomer-israel · 2021-11-09T16:45:44Z

What I did
Add support for adding & removing port counters dynamically each time port was added or removed
the counters that were supported are:

pg watermark counters
pg drop counters
queue stat counters
queue watermark counters
debug counters

buffer drop counters and port stat counters are already supported to be added or removed each time port is added/removed

Implemented according to the - HLD document on add/remove ports dynamically feature

Why I did it
In order to support dynamically add or removed ports on sonic

How I verified it
tested manually that the flex counters were added or removed correctly whenever we add or remove ports
added new test cases to the following vs tests:

test_flex_counters.py
test_drop_counters.py
test_pg_drop_counter.py

Details if related

lgtm-com · 2021-11-09T17:49:23Z

This pull request fixes 2 alerts when merging a343a09 into 05c7c05 - view on LGTM.com

fixed alerts:

2 for Unused import

liat-grozovik · 2021-11-11T12:35:52Z

@praveen-li I am unable to assign code review to you. Could you please add yourself as the reviewer?

lgtm-com · 2021-11-11T13:39:41Z

This pull request fixes 2 alerts when merging 550fe05 into 5f8ebfa - view on LGTM.com

fixed alerts:

2 for Unused import

dprital · 2021-11-21T09:08:37Z

@prsunny - Can you please add @praveen-li as a reviewer ?

lgtm-com · 2021-11-22T16:07:46Z

This pull request fixes 2 alerts when merging 3e72fa4 into 4f6cb05 - view on LGTM.com

fixed alerts:

2 for Unused import

tomer-israel · 2021-11-23T07:05:18Z

/azpw run

mssonicbld · 2021-11-23T07:05:20Z

/AzurePipelines run

azure-pipelines · 2021-11-23T07:05:30Z

Azure Pipelines successfully started running 1 pipeline(s).

tomer-israel · 2021-11-24T10:05:37Z

/azpw run

mssonicbld · 2021-11-24T10:05:38Z

/AzurePipelines run

azure-pipelines · 2021-11-24T10:05:48Z

Azure Pipelines successfully started running 1 pipeline(s).

tomer-israel · 2021-11-28T12:44:26Z

/azpw run

mssonicbld · 2021-11-28T12:44:27Z

/AzurePipelines run

azure-pipelines · 2021-11-28T12:44:37Z

Azure Pipelines successfully started running 1 pipeline(s).

tomer-israel · 2021-11-29T10:22:46Z

/azpw run

mssonicbld · 2021-11-29T10:22:48Z

/AzurePipelines run

azure-pipelines · 2021-11-29T10:22:58Z

Azure Pipelines successfully started running 1 pipeline(s).

zhenggen-xu · 2021-11-30T06:41:47Z

orchagent/debugcounterorch.cpp

+{
+    SWSS_LOG_ENTER();
+
+    SWSS_LOG_NOTICE("add debug counter for port 0x%" PRIu64 , port_id);


I will change it to INFO

zhenggen-xu · 2021-11-30T06:42:54Z

orchagent/debugcounterorch.cpp

+
+    SWSS_LOG_NOTICE("add debug counter for port 0x%" PRIu64 , port_id);
+
+    for (const auto& debug_counter: debug_counters) {


put { in the second line as consistency as other code.

will be changed

zhenggen-xu · 2021-11-30T06:43:55Z

orchagent/debugcounterorch.cpp

+        auto counter_stat = counter->getDebugCounterSAIStat();
+        auto flex_counter_type = getFlexCounterType(counter_type);
+
+        if (flex_counter_type == CounterType::PORT_DEBUG){


put { in the second line

will be changed

zhenggen-xu · 2021-11-30T06:44:04Z

orchagent/debugcounterorch.cpp

+{
+    SWSS_LOG_ENTER();
+
+    SWSS_LOG_NOTICE("remove debug counter for port 0x%" PRIu64 , port_id);


will be change to INFO

zhenggen-xu · 2021-11-30T06:44:11Z

orchagent/debugcounterorch.cpp

+
+    SWSS_LOG_NOTICE("remove debug counter for port 0x%" PRIu64 , port_id);
+
+    for (const auto& debug_counter: debug_counters) {


put { in the second line

will be change

zhenggen-xu · 2021-11-30T06:44:20Z

orchagent/debugcounterorch.cpp

+        auto counter_stat = counter->getDebugCounterSAIStat();
+        auto flex_counter_type = getFlexCounterType(counter_type);
+
+        if (flex_counter_type == CounterType::PORT_DEBUG){


put { in the second line

will be change

zhenggen-xu · 2021-11-30T06:56:45Z

orchagent/debugcounterorch.cpp

+    }
+}
+
+void DebugCounterOrch::removePortDebugCounter(sai_object_id_t port_id)


re-use existing function like uninstallDebugFlexCounters()?

uninstallDebugFlexCounters is removing the debug counters from all the ports, in order to fit the function uninstallDebugFlexCounters to remove only one port debug counter we need to add extra argument for indicating which ports to remove (specific port or all the ports)

do you think it's better to change uninstallDebugFlexCounters for that?

like this example:

void DebugCounterOrch::uninstallDebugFlexCounters(const string& counter_type, const string& counter_stat, sai_object_id_t port_id = SAI_NULL_OBJECT_ID) { SWSS_LOG_ENTER(); CounterType flex_counter_type = getFlexCounterType(counter_type); if (flex_counter_type == CounterType::SWITCH_DEBUG) { flex_counter_manager.removeFlexCounterStat(gSwitchId, flex_counter_type, counter_stat); } else if (flex_counter_type == CounterType::PORT_DEBUG) { if (port_id != SAI_NULL_OBJECT_ID) { flex_counter_manager.removeFlexCounterStat( curr.second.m_port_id, flex_counter_type, counter_stat); } else { for (auto const &curr : gPortsOrch->getAllPorts()) { if (curr.second.m_type != Port::Type::PHY) { continue; } flex_counter_manager.removeFlexCounterStat( curr.second.m_port_id, flex_counter_type, counter_stat); } } } }

or this option:

void DebugCounterOrch::uninstallDebugFlexCounters(const string& counter_type, const string& counter_stat, sai_object_id_t port_id = SAI_NULL_OBJECT_ID) { SWSS_LOG_ENTER(); CounterType flex_counter_type = getFlexCounterType(counter_type); if (flex_counter_type == CounterType::SWITCH_DEBUG) { flex_counter_manager.removeFlexCounterStat(gSwitchId, flex_counter_type, counter_stat); } else if (flex_counter_type == CounterType::PORT_DEBUG) { for (auto const &curr : gPortsOrch->getAllPorts()) { if (port_id != SAI_NULL_OBJECT_ID) { if (curr.second.m_port_id != port_id) { continue; } } if (curr.second.m_type != Port::Type::PHY) { continue; } flex_counter_manager.removeFlexCounterStat( curr.second.m_port_id, flex_counter_type, counter_stat); } } }

The point was to not introduce more functions that has similar needs. One of the option should be fine but be consistent with the Add counter function case.

was changed to the second option for add and remove

zhenggen-xu · 2021-11-30T06:57:15Z

orchagent/debugcounterorch.cpp

@@ -616,3 +640,50 @@ bool DebugCounterOrch::isDropReasonValid(const string& drop_reason) const

    return true;
 }
+
+void DebugCounterOrch::addPortDebugCounter(sai_object_id_t port_id)


re-use installDebugFlexCounters()?

same as above

zhenggen-xu · 2021-11-30T07:06:16Z

orchagent/portsorch.cpp

+    if (!getPort(port_id, p))
+    {
+        SWSS_LOG_ERROR("Failed to get port object for port id 0x%" PRIx64, port_id);
+        p.m_port_id = port_id;


why assign p.m_port_id only when failed?

I used the way they did it in previous version but, i think it's not needed, if get_port is failing so we just need to add error message and return from this function.
I will change it

lgtm-com · 2021-11-30T11:06:21Z

This pull request fixes 2 alerts when merging d868d8c into f6f6f86 - view on LGTM.com

fixed alerts:

2 for Unused import

tomer-israel · 2021-11-30T15:12:56Z

/azpw run

mssonicbld · 2021-11-30T15:13:01Z

/AzurePipelines run

azure-pipelines · 2021-11-30T15:13:14Z

Azure Pipelines successfully started running 1 pipeline(s).

tomer-israel · 2021-12-01T11:41:05Z

/azpw run

mssonicbld · 2021-12-01T11:41:06Z

/AzurePipelines run

azure-pipelines · 2021-12-01T11:41:17Z

Azure Pipelines successfully started running 1 pipeline(s).

lgtm-com · 2021-12-01T12:56:44Z

This pull request fixes 2 alerts when merging 904b583 into 15a3b6c - view on LGTM.com

fixed alerts:

2 for Unused import

lgtm-com · 2021-12-02T09:06:15Z

This pull request fixes 2 alerts when merging 3bc9fb5 into d352d5a - view on LGTM.com

fixed alerts:

2 for Unused import

changes according to coding style

remove unused function declerations

lgtm-com · 2022-03-27T18:38:57Z

This pull request fixes 2 alerts when merging e92133c into 7350d49 - view on LGTM.com

fixed alerts:

2 for Unused import

liat-grozovik · 2022-03-28T08:34:08Z

/azp run Azure.sonic-swss

azure-pipelines · 2022-03-28T08:34:17Z

Azure Pipelines successfully started running 1 pipeline(s).

zhenggen-xu · 2022-03-28T19:58:47Z

orchagent/portsorch.cpp

@@ -2378,6 +2378,16 @@ bool PortsOrch::initPort(const string &alias, const string &role, const int inde
                    port_buffer_drop_stat_manager.setCounterIdList(p.m_port_id, CounterType::PORT, port_buffer_drop_stats);
                }

+                /* when a port is added and priority group map counter is enabled --> we need to add pg counter for it */
+                if (m_isPriorityGroupMapGenerated) {


"{" to separate line to be consistent.

Can you go through the whole PR to make sure we follow the same coding style?

zhenggen-xu · 2022-03-28T19:58:51Z

orchagent/portsorch.cpp

+                }
+
+                /* when a port is added and queue map counter is enabled --> we need to add queue map counter for it */
+                if (m_isQueueMapGenerated) {


zhenggen-xu · 2022-03-28T19:59:21Z

orchagent/portsorch.cpp

@@ -2425,9 +2440,18 @@ void PortsOrch::deInitPort(string alias, sai_object_id_t port_id)
        port_buffer_drop_stat_manager.clearCounterIdList(p.m_port_id);
    }

+    /* remove pg port counters */
+    if (m_isPriorityGroupMapGenerated) {


{ to separate line

zhenggen-xu · 2022-03-28T19:59:25Z

orchagent/portsorch.cpp

+    }
+
+    /* remove queue port counters */
+    if (m_isQueueMapGenerated) {


{ to separate line

lgtm-com · 2022-03-28T21:46:28Z

This pull request fixes 2 alerts when merging ca47331 into 8941cc0 - view on LGTM.com

fixed alerts:

2 for Unused import

dprital · 2022-03-28T23:24:19Z

@prsunny - Can you please approve this PR ?

liat-grozovik · 2022-03-29T07:20:59Z

/azp run Azure.sonic-swss

azure-pipelines · 2022-03-29T07:21:09Z

Azure Pipelines successfully started running 1 pipeline(s).

dprital · 2022-03-30T17:41:45Z

@prsunny - can you please review ?

dprital · 2022-04-05T16:42:42Z

@prsunny - Can you please review ?

… added or removed (sonic-net#2019) - What I did Add support for adding & removing port counters dynamically each time port was added or removed the counters that were supported are: 1. pg watermark counters 2. pg drop counters 3. queue stat counters 4. queue watermark counters 5. debug counters 6. buffer drop counters and port stat counters are already supported to be added or removed each time port is added/removed Implemented according to the - 'HLD document on add/remove ports dynamically feature' sonic-net/SONiC#900 - Why I did it In order to support dynamically add or removed ports on sonic - How I verified it tested manually that the flex counters were added or removed correctly whenever we add or remove ports added new test cases to the following vs tests: test_flex_counters.py test_drop_counters.py test_pg_drop_counter.py Co-authored-by: dprital <[email protected]>

tomer-israel requested a review from prsunny as a code owner November 9, 2021 16:45

tomer-israel mentioned this pull request Nov 11, 2021

HLD document on add/remove ports dynamically feature sonic-net/SONiC#900

Merged

prsunny requested a review from zhenggen-xu November 22, 2021 18:34

zhenggen-xu reviewed Nov 30, 2021

View reviewed changes

tomer-israel and others added 13 commits March 27, 2022 11:19

countrs support for dynamic port add/remove

c0fda4b

countrs support for dynamic port add/remove

42de362

countrs support for dynamic port add/remove

eb82018

countrs support for dynamic port add/remove - code review fixes

b3c6197

Update test_flex_counters.py

fd79762

Update test_flex_counters.py

2c23a2d

Update debugcounterorch.cpp

2728ba0

changes according to coding style

Update debugcounterorch.h

cf77ec1

remove unused function declerations

Fix PR review comment

7087ed4

replace code

b16ee2a

fix pg counter code

a781471

Fix the way we delete the counters upon port delete

8d0c345

Fix merge issues

e92133c

tomer-israel force-pushed the dynamic_port_counters_support branch from 3578197 to e92133c Compare March 27, 2022 17:06

zhenggen-xu reviewed Mar 28, 2022

View reviewed changes

Align coding style

ca47331

zhenggen-xu approved these changes Mar 28, 2022

View reviewed changes

liat-grozovik approved these changes Mar 31, 2022

View reviewed changes

liat-grozovik merged commit 53620f3 into sonic-net:master Apr 7, 2022


		SWSS_LOG_NOTICE("add debug counter for port 0x%" PRIu64 , port_id);

		for (const auto& debug_counter: debug_counters) {


		SWSS_LOG_NOTICE("remove debug counter for port 0x%" PRIu64 , port_id);

		for (const auto& debug_counter: debug_counters) {

[orchagent] add & remove port counters dynamically each time port was added or removed #2019

[orchagent] add & remove port counters dynamically each time port was added or removed #2019

Conversation

tomer-israel commented Nov 9, 2021 • edited by liat-grozovik Loading

lgtm-com bot commented Nov 9, 2021

liat-grozovik commented Nov 11, 2021

lgtm-com bot commented Nov 11, 2021

dprital commented Nov 21, 2021

lgtm-com bot commented Nov 22, 2021

tomer-israel commented Nov 23, 2021

mssonicbld commented Nov 23, 2021

azure-pipelines bot commented Nov 23, 2021

tomer-israel commented Nov 24, 2021

mssonicbld commented Nov 24, 2021

azure-pipelines bot commented Nov 24, 2021

tomer-israel commented Nov 28, 2021

mssonicbld commented Nov 28, 2021

azure-pipelines bot commented Nov 28, 2021

tomer-israel commented Nov 29, 2021

mssonicbld commented Nov 29, 2021

azure-pipelines bot commented Nov 29, 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

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

tomer-israel Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomer-israel Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomer-israel Dec 15, 2021 • edited Loading

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

lgtm-com bot commented Nov 30, 2021

tomer-israel commented Nov 30, 2021

mssonicbld commented Nov 30, 2021

azure-pipelines bot commented Nov 30, 2021

tomer-israel commented Dec 1, 2021

mssonicbld commented Dec 1, 2021

azure-pipelines bot commented Dec 1, 2021

lgtm-com bot commented Dec 1, 2021

lgtm-com bot commented Dec 2, 2021

lgtm-com bot commented Mar 27, 2022

liat-grozovik commented Mar 28, 2022

azure-pipelines bot commented Mar 28, 2022

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

Choose a reason for hiding this comment

lgtm-com bot commented Mar 28, 2022

dprital commented Mar 28, 2022

liat-grozovik commented Mar 29, 2022

azure-pipelines bot commented Mar 29, 2022

dprital commented Mar 30, 2022

dprital commented Apr 5, 2022

tomer-israel commented Nov 9, 2021 •

edited by liat-grozovik

Loading

tomer-israel Nov 30, 2021 •

edited

Loading

tomer-israel Nov 30, 2021 •

edited

Loading

tomer-israel Dec 15, 2021 •

edited

Loading