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

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

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

orchagent/intfsorch.h

@@ -35,7 +36,7 @@ class IntfsOrch : public Orch
    sai_object_id_t getRouterIntfsId(const string&);
    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

@@ -47,8 +47,8 @@ extern BufferOrch *gBufferOrch;
 #define PORT_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS     1000
 #define QUEUE_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS   10000
 #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

@@ -93,6 +94,8 @@ class PortsOrch : public Orch, public Subject
    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 <[email protected]>

qiluo-msft

Please also check other reviewers' comments

Signed-off-by: Mykola Faryma <[email protected]>

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

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

rlhui requested review from abdosi, zhenggen-xu, daall and qiluo-msft 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 Outdated 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 Outdated 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 <[email protected]>

fix comments

a2e95d2

Signed-off-by: Mykola Faryma <[email protected]>

fix comments

7290ee2

Signed-off-by: Mykola Faryma <[email protected]>

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

fix comments

6ea64bd

Signed-off-by: Mykola Faryma <[email protected]>

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

qiluo-msft previously approved these changes May 13, 2020

View reviewed changes

remove speed calc from lua scripts

57613ce

Signed-off-by: Mykola Faryma <[email protected]>

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mykolaf commented Feb 21, 2020

abdosi Mar 29, 2020

mykolaf Jun 5, 2020

abdosi Mar 29, 2020

mykolaf Apr 21, 2020

abdosi Mar 29, 2020

abdosi Mar 29, 2020

daall left a comment

qiluo-msft Apr 21, 2020 •

edited

Loading

mykolaf Apr 23, 2020

qiluo-msft Apr 21, 2020 •

edited

Loading

mykolaf Apr 23, 2020

qiluo-msft Apr 21, 2020 •

edited

Loading

mykolaf Apr 23, 2020

qiluo-msft Apr 23, 2020

mykolaf May 13, 2020

qiluo-msft Apr 21, 2020 •

edited

Loading

mykolaf Apr 23, 2020

qiluo-msft left a comment

qiluo-msft left a comment

qiluo-msft Jun 6, 2020

mykolaf Jun 9, 2020

qiluo-msft Jun 9, 2020

mykolaf Jun 10, 2020

qiluo-msft left a comment

This comment has been minimized.

mykolaf commented Jun 10, 2020

qiluo-msft left a comment

liat-grozovik commented Jul 7, 2020

abdosi commented Jul 7, 2020

abdosi commented Jul 14, 2020

abdosi commented Jul 14, 2020

daall left a comment

akokhan Jul 22, 2020

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

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

Conversation

mykolaf commented Feb 21, 2020

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

daall left a comment

Choose a reason for hiding this comment

qiluo-msft Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

qiluo-msft Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

qiluo-msft left a 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

qiluo-msft left a comment

Choose a reason for hiding this comment

This comment has been minimized.

mykolaf commented Jun 10, 2020

qiluo-msft left a comment

Choose a reason for hiding this comment

liat-grozovik commented Jul 7, 2020

abdosi commented Jul 7, 2020

abdosi commented Jul 14, 2020

abdosi commented Jul 14, 2020

daall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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