[portsorch,intfsorch] add port, rif rates FC groups by mykolaf · Pull Request #1201 · sonic-net/sonic-swss

mykolaf · 2020-02-21T08:35:19Z

According to HLD

Depends on sonic-swws-common #330

What I did

Add 2 new FC groups, with plugins to calculate port and interface rates and utilization

Why I did it

How I verified it

Details if related

orchagent/Makefile.am

abdosi · 2020-03-29T06:39:50Z

orchagent/interface_rates.lua

+redis.call('SELECT', asic_db)
+local n = table.getn(KEYS)
+for i = 1, n do
+    speeds[i] = 40000 


What does speed of 40000 means for RIF Interface ? How we concluded on this value ?

@abdosi I moved speed calculation to the CLI utility script. Getting speed for interface in lua plugin is tedious and requires connection to multiple DBs. On CLI level we can divide smoothed byterate by speed and get the utilization.

abdosi · 2020-03-29T06:41:56Z

orchagent/port_rates.lua

+redis.call('SELECT', asic_db)
+local n = table.getn(KEYS)
+for i = 1, n do
+    speeds[i] = redis.call('HGET', counters_table_name .. ':' .. KEYS[i], 'SAI_PORT_ATTR_SPEED')


should it not be SAI_PORT_ATTR_OPER_SPEED ?

No, asic db only contains SAI_PORT_ATTR_SPEED

orchagent/interface_rates.lua

orchagent/port_rates.lua

abdosi · 2020-03-29T19:30:26Z

orchagent/port_rates.lua

+
+local asic_db = 1 
+local counters_db = ARGV[1]
+local config_db = 4 


Same comment as above. Can we pass as ARGV the DB Id's

abdosi · 2020-03-29T19:37:12Z

orchagent/interface_rates.lua

+
+-- Get configuration
+redis.call('SELECT', config_db)
+local smooth_interval = redis.call('GET', rates_table_name .. ':' .. 'RIF_SMOOTH_INTERVAL')


do we need smooth_interval here ?

orchagent/interface_rates.lua

daall

See comments.

orchagent/portsorch.cpp

orchagent/intfsorch.cpp

orchagent/portsorch.cpp

orchagent/interface_rates.lua

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

qiluo-msft · 2020-04-21T23:51:55Z

orchagent/intfsorch.h

    bool isPrefixSubnet(const IpPrefix&, const string&);
    string getRouterIntfsAlias(const IpAddress &ip, const string &vrf_name = "");
-
+    string getRifRateFlexCounterTableKey(string key);


getRifRateFlexCounterTableKey [](start = 11, length = 29)

above line uses RouterIntfs, here uses Rif. Are they different? #Closed

The latter refers to RIF_RATE Flex Counter group instead of Router Interface.

qiluo-msft · 2020-04-21T23:52:41Z

orchagent/portsorch.cpp

 #define QUEUE_WATERMARK_FLEX_STAT_COUNTER_POLL_MSECS "10000"
-#define PG_WATERMARK_FLEX_STAT_COUNTER_POLL_MSECS    "10000"
-
+#define PG_WATERMARK_FLEX_STAT_COUNTER_POLL_MSECS "10000"


"10000" [](start = 50, length = 7)

Why change original blanks and break the code alignment? #Closed

qiluo-msft · 2020-04-21T23:53:59Z

orchagent/portsorch.cpp

+    catch (const runtime_error &e)
+    {
+        SWSS_LOG_WARN("Port rate flex counter group plugins was not set successfully: %s", e.what());
+    }


duplicated code #Closed

Combined into one try block

Sorry for misleading. I mean the code change is almost the same as in IntfsOrch::IntfsOrch(). Is it possible to reuse code?

In reply to: 412570352 [](ancestors = 412570352)

I suggest that I revert this particular change,
So we can go forward with the review.
The flex counter plugin registering procedure can be refactored in later PR. There is the FlexCounterManager class, we can extend to manage plugins also, but as separate effort.

qiluo-msft · 2020-04-21T23:54:38Z

orchagent/portsorch.h

    bool addSubPort(Port &port, const string &alias, const bool &adminUp = true, const uint32_t &mtu = 0);
    bool removeSubPort(const string &alias);
 private:
+    std::vector<Port> m_portsToAdd;


vector [](start = 9, length = 6)

#include <vector> ``` #Closed

Included in port.h and already used on #L22

qiluo-msft

As comments

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

qiluo-msft

Please also check other reviewers' comments

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

qiluo-msft · 2020-06-06T04:32:25Z

orchagent/port_rates.lua

+
+-- Get configuration
+redis.call('SELECT', counters_db)
+local smooth_interval = redis.call('HGET', rates_table_name .. ':' .. 'PORT', 'PORT_SMOOTH_INTERVAL')


: [](start = 64, length = 1)

The table separator are from database_config.json. Do not hardcode.

It's not trivial to access the database config from the lua script. Should we pass the separator as one of the parameters?
I see the separator hardcoded in a lot of other places, like this. If there is a need to avoid such things in future, I suggest separate PR and refactor it in all the lua scripts.

Some scripts have other dbs/table names hardcoded, and ATM receive the same ARGV. Need to suggest broad solution.

Lua parameter is good enough.
Could you please add a TODO comment or Github issue?

@qiluo-msft Created issue #1319

qiluo-msft

Found new issue

mykolaf · 2020-06-10T14:21:23Z

retest LGTM analysis: C/C++ please

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

qiluo-msft

LGTM. Please also check other reviewers' comments

liat-grozovik · 2020-07-07T13:38:44Z

@abdosi any additional feedback or comments were addressed and we can merge it?

abdosi · 2020-07-07T16:05:48Z

@abdosi any additional feedback or comments were addressed and we can merge it?

@liat-grozovik Reviewing this. Should be done by EOD

abdosi · 2020-07-14T15:21:26Z

LGTM

abdosi · 2020-07-14T15:22:51Z

@daall Can you review this back

daall

LGTM

akokhan · 2020-07-22T12:58:38Z

orchagent/port_rates.lua

+-- Get configuration
+redis.call('SELECT', counters_db)
+local smooth_interval = redis.call('HGET', rates_table_name .. ':' .. 'PORT', 'PORT_SMOOTH_INTERVAL')
+local alpha = redis.call('HGET', rates_table_name .. ':' .. 'PORT', 'PORT_ALPHA')


I think this changes causes the following issue on latest SONiC master

Jul 22 11:17:22.139228 dut ERR syncd[25]: :- runRedisScript: Caught exception while running Redis lua script: ERR Error running script (call to f_fd0ea76fc13f9fcc7910e4b1fd8c9018ea197cf4): @user_script:21: user_script:21: attempt to perform arithmetic on local 'alpha' (a boolean value) : Input/output error Jul 22 11:17:22.786235 dut ERR syncd[25]: :- guard: RedisReply catches system_error: command: *39#015#012$7#015#012EVALSHA#015#012$40#015#01231fc701ca9b1b9f968f501c92b639f50f6346a9c#015#012$2#015#01232#015#012$19#015#012oid:0x100000000004f#015#012$19#015#012oid:0x1000000000067#015#012$19#015#012oid:0x100000000007f#015#012$19#015#012oid:0x1000000000097#015#012$19#015#012oid:0x10000000000af#015#012$19#015#012oid:0x10000000000c7#015#012$19#015#012oid:0x10000000000df#015#012$19#015#012oid:0x10000000000f7#015#012$19#015#012oid:0x100000000010f#015#012$19#015#012oid:0x1000000000127#015#012$19#015#012oid:0x100000000013f#015#012$19#015#012oid:0x1000000000157#015#012$19#015#012oid:0x100000000016f#015#012$19#015#012oid:0x1000000000187#015#012$19#015#012oid:0x100000000019f#015#012$19#015#012oid:0x10000000001b7#015#012$19#015#012oid:0x10000000001cf#015#012$19#015#012oid:0x10000000001e7#015#012$19#015#012oid:0x10000000001ff#015#012$19#015#012oid:0x1000000000217#015#012$19#015#012oid:0x100000000022f#015#012$19#015#012oid:0x1000000000247#015#012$19#015#012oid:0x100000000025f#015#012$19#015#012oid:0x1000000000277#015#012$19#015#012oid:0x100000000028f#015#012$19#015#012oid:0x10000000002a7#015#012$19#015#012oid:0x10000000002bf#015#012$19#015#012oid:0x10000000002d7#015#012$19#015#012oid:0x10000000002ef#015#012$19#015#012oid:0x1000000000307#015#012$19#015#012oid:0x100000000031f#015#012$19#015#012oid:0x1000000000337#015#012$1#015#0122#015#012$8#015#012COUNTERS#015#012$7#015#0121000000#015#012$2#015#012''#015#012, reason: ERR Error running script (call to f_31fc701ca9b1b9f968f501c92b639f50f6346a9c): @user_script:21: user_script:21: attempt to perform arithmetic on local 'alpha' (a boolean value) : Input/output error Jul 22 11:17:22.786235 dut ERR syncd[25]: :- runRedisScript: Caught exception while running Redis lua script: ERR Error running script (call to f_31fc701ca9b1b9f968f501c92b639f50f6346a9c): @user_script:21: user_script:21: attempt to perform arithmetic on local 'alpha' (a boolean value) : Input/output error Jul 22 11:17:23.141016 dut ERR syncd[25]: :- guard: RedisReply catches system_error: command: *12#015#012$7#015#012EVALSHA#015#012$40#015#012fd0ea76fc13f9fcc7910e4b1fd8c9018ea197cf4#015#012$1#015#0125#015#012$19#015#012oid:0x6000000000392#015#012$19#015#012oid:0x6000000000393#015#012$19#015#012oid:0x6000000000394#015#012$19#015#012oid:0x6000000000395#015#012$19#015#012oid:0x6000000000396#015#012$1#015#0122#015#012$8#015#012COUNTERS#015#012$7#015#0121000000#015#012$2#015#012''#015#012, reason: ERR Error running script (call to f_fd0ea76fc13f9fcc7910e4b1fd8c9018ea197cf4): @user_script:21: user_script:21: attempt to perform arithmetic on local 'alpha' (a boolean value) : Input/output error Jul 22 11:17:23.141016 dut ERR syncd[25]: :- runRedisScript: Caught exception while running Redis lua script: ERR Error running script (call to f_fd0ea76fc13f9fcc7910e4b1fd8c9018ea197cf4): @user_script:21: user_script:21: attempt to perform arithmetic on local 'alpha' (a boolean value) : Input/output error Jul 22 11:17:23.337583 dut NOTICE syncd[25]: :- processBulkQuadEvent: bulk SAI_OBJECT_TYPE_NEXT_HOP_GROUP_MEMBER execute with 2 items Jul 22 11:17:23.735421 dut ERR syncd[25]: :- guard: RedisReply catches system_error: command: *39#015#012$7#015#012EVALSHA#015#012$40#015#01231fc701ca9b1b9f968f501c92b639f50f6346a9c#015#012$2#015#01232#015#012$19#015#012oid:0x100000000004f#015#012$19#015#012oid:0x1000000000067#015#012$19#015#012oid:0x100000000007f#015#012$19#015#012oid:0x1000000000097#015#012$19#015#012oid:0x10000000000af#015#012$19#015#012oid:0x10000000000c7#015#012$19#015#012oid:0x10000000000df#015#012$19#015#012oid:0x10000000000f7#015#012$19#015#012oid:0x100000000010f#015#012$19#015#012oid:0x1000000000127#015#012$19#015#012oid:0x100000000013f#015#012$19#015#012oid:0x1000000000157#015#012$19#015#012oid:0x100000000016f#015#012$19#015#012oid:0x1000000000187#015#012$19#015#012oid:0x100000000019f#015#012$19#015#012oid:0x10000000001b7#015#012$19#015#012oid:0x10000000001cf#015#012$19#015#012oid:0x10000000001e7#015#012$19#015#012oid:0x10000000001ff#015#012$19#015#012oid:0x1000000000217#015#012$19#015#012oid:0x100000000022f#015#012$19#015#012oid:0x1000000000247#015#012$19#015#012oid:0x100000000025f#015#012$19#015#012oid:0x1000000000277#015#012$19#015#012oid:0x100000000028f#015#012$19#015#012oid:0x10000000002a7#015#012$19#015#012oid:0x10000000002bf#015#012$19#015#012oid:0x10000000002d7#015#012$19#015#012oid:0x10000000002ef#015#012$19#015#012oid:0x1000000000307#015#012$19#015#012oid:0x100000000031f#015#012$19#015#012oid:0x1000000000337#015#012$1#015#0122#015#012$8#015#012COUNTERS#015#012$7#015#0121000000#015#012$2#015#012''#015#012, reason: ERR Error running script (call to f_31fc701ca9b1b9f968f501c92b639f50f6346a9c): @user_script:21: user_script:21: attempt to perform arithmetic on local 'alpha' (a boolean value) : Input/output error

Please double check.

) Added checks to ignore files/directories that are not present while generating the dump. Signed-off-by: Sabareesh Kumar Anandan <sanandan@marvell.com>

* [portsorch,intfsorch] add port, rif rates FC groups * fix comments * remove speed calc from lua scripts * trigger lgtm

rlhui requested review from abdosi, daall, qiluo-msft and zhenggen-xu March 4, 2020 00:33

abdosi requested changes Mar 29, 2020

View reviewed changes

abdosi reviewed Mar 29, 2020

View reviewed changes

orchagent/interface_rates.lua Show resolved Hide resolved

daall reviewed Mar 30, 2020

View reviewed changes

orchagent/portsorch.cpp Outdated Show resolved Hide resolved

orchagent/intfsorch.cpp Outdated Show resolved Hide resolved

orchagent/portsorch.cpp Outdated Show resolved Hide resolved

orchagent/interface_rates.lua Show resolved Hide resolved

Mykola Faryma added 3 commits April 6, 2020 18:40

[portsorch,intfsorch] add port, rif rates FC groups

4b1e0c7

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

fix comments

a2e95d2

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

fix comments

7290ee2

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

mykolaf force-pushed the rates branch from 1890936 to 7290ee2 Compare April 9, 2020 07:25

fix comments

6ea64bd

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

mykolaf marked this pull request as ready for review April 21, 2020 16:26

qiluo-msft reviewed Apr 21, 2020

View reviewed changes

fix comments

0c22b13

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

qiluo-msft previously approved these changes May 13, 2020

View reviewed changes

remove speed calc from lua scripts

57613ce

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

mykolaf dismissed qiluo-msft’s stale review via 57613ce June 5, 2020 12:15

qiluo-msft reviewed Jun 6, 2020

View reviewed changes

qiluo-msft requested changes Jun 6, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

trigger lgtm

c7fdbac

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

qiluo-msft approved these changes Jun 10, 2020

View reviewed changes

mykolaf requested a review from abdosi June 16, 2020 13:15

abdosi approved these changes Jul 14, 2020

View reviewed changes

daall approved these changes Jul 14, 2020

View reviewed changes

qiluo-msft merged commit 310e0aa into sonic-net:master Jul 14, 2020

akokhan reviewed Jul 22, 2020

View reviewed changes

liat-grozovik mentioned this pull request Nov 18, 2020

[enable counters] provide initial rates parameters sonic-net/sonic-buildimage#5048

Merged

3 tasks

Janetxxx pushed a commit to Janetxxx/sonic-swss that referenced this pull request Nov 10, 2025

[portsorch,intfsorch] add port, rif rates FC groups (sonic-net#1201)

8a6ffa0

* [portsorch,intfsorch] add port, rif rates FC groups * fix comments * remove speed calc from lua scripts * trigger lgtm

Conversation

mykolaf commented Feb 21, 2020

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qiluo-msft Apr 21, 2020 • 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!

qiluo-msft Apr 21, 2020 • 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!

qiluo-msft Apr 21, 2020 • 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!

qiluo-msft Apr 21, 2020 • 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!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

qiluo-msft left a comment

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!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

mykolaf commented Jun 10, 2020

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

liat-grozovik commented Jul 7, 2020

Uh oh!

qiluo-msft Apr 21, 2020 •

edited

Loading

qiluo-msft Apr 21, 2020 •

edited

Loading

qiluo-msft Apr 21, 2020 •

edited

Loading

qiluo-msft Apr 21, 2020 •

edited

Loading