Fix flex counters logic of converting poll interval to seconds from MS #878

noaOrMlnx · 2021-08-03T07:13:42Z

According to https://github.com/Azure/SONiC/blob/ec6d35dd2c28491bfade19cfee990fe612f1e5e9/doc/rates-and-utilization/Rates_and_utilization_HLD.md, counterpoll command gives polling interval in milliseconds.
So when converting them to seconds to be supplied to lua script this should be divide by 1000. However, syncd multiplies it by 1000.

Changed the multiplication to none, and did the converting in lua script - sonic-net/sonic-swss#1855
Fixed issue - sonic-net/sonic-buildimage#8392

kcudnik · 2021-08-03T09:28:22Z

syncd/FlexCounter.cpp

@@ -1597,7 +1597,7 @@ void FlexCounter::runPlugins(
    {
        std::to_string(counters_db.getDbId()),
        COUNTERS_TABLE,
-        std::to_string(m_pollInterval * 1000)
+        std::to_string(m_pollInterval / 1000)


m_pollInterval is set via orchagent with values in milliseconds (MS), currently this value was extended to microseconds, and now you changing this to seconds, so if m_pollInterval will be equal to let say 60ms or 100ms or 900ms, then your value in db will be set to zero (0) because of integer division, is that what you want ?

Done - removed the multiplication and did the required multiplication in the lua script - sonic-net/sonic-swss#1855

liat-grozovik · 2021-08-16T10:02:22Z

@kcudnik could you please help review?

kcudnik · 2021-08-16T10:26:40Z

@kcudnik could you please help review?

i already posted my concerns in comment above, and still this is not addressed nor responded how this will be handled

noaOrMlnx · 2021-08-16T10:31:27Z

@kcudnik Hi Kamil,
the request was addressed and the code has been changed.
appreciate if you'll take a look.
Thanks

kcudnik · 2021-08-16T12:31:43Z

i still don't see consistency here, what value is set in OA, what pooliterval value is stored in db and what value valie of pollinterval i passed to lua script

dgsudharsan · 2021-08-17T05:12:07Z

i still don't see consistency here, what value is set in OA, what pooliterval value is stored in db and what value valie of pollinterval i passed to lua script

Hi Kamil. The value set in CLI is in milliseconds which is again set as milliseconds in orchagent. This value is again passed to lua script in milliseconds. So end to end we are using milliseconds. The lua scripts are updated to treat the input arg in milliseconds which is here sonic-net/sonic-swss#1855
Your initial concern was correct and we avoided converting milliseconds to seconds (since lua script expected seconds in arg). Now we have changed end to end and we do not convert anywhere and lua script rate calculation logic is updated considering poll interval as milliseconds.

kcudnik · 2021-08-17T07:21:30Z

thanks for deep answer on that, now i get the flow

liat-grozovik · 2021-08-17T07:39:36Z

@noaOrMlnx could you please review the failures?

… to get real results

noaOrMlnx · 2021-08-29T11:50:09Z

@kcudnik could you please re-approve?

qiluo-msft · 2021-08-30T06:18:31Z

The PR check "license/cla" seems have some issue. Could you commit an empty commit, to retrigger the check.
I checked https://cla.opensource.microsoft.com/Azure/sonic-sairedis?pullRequest=878, and it is not signed.

… into flexcounter_fix

noaOrMlnx · 2021-08-30T06:21:36Z

@qiluo-msft Done

qiluo-msft · 2021-08-30T06:25:15Z

@kcudnik Could you review again, and merge after all checkers pass?

#878) According to https://github.com/Azure/SONiC/blob/ec6d35dd2c28491bfade19cfee990fe612f1e5e9/doc/rates-and-utilization/Rates_and_utilization_HLD.md, counterpoll command gives polling interval in milliseconds. So when converting them to seconds to be supplied to lua script this should be divide by 1000. However, syncd multiplies it by 1000. Changed the multiplication to none, and did the converting in lua script - sonic-net/sonic-swss#1855 Fixed issue - sonic-net/sonic-buildimage#8392

… MS (#1855) **What I did** Update the rif_rates/lua script to multiply by 1000 instead of FlexCounter class. related also to this PR - sonic-net/sonic-sairedis#878 Fix issue - sonic-net/sonic-buildimage#8392 **Why I did it** times were not calculated properly. **How I verified it** check that the output of cli and redis is close enough:

PFCWD storm detection and restoration LUA scripts require values in microseconds. Due to recent changes polling interval is now passed in milliseconds by "FlexCounter". sonic-net/sonic-sairedis#878 Signed-off-by: Volodymyr Samotiy <[email protected]>

PFCWD storm detection and restoration LUA scripts require values in microseconds. Due to recent changes polling interval is now passed in milliseconds by "FlexCounter". * sonic-net/sonic-sairedis#878 Signed-off-by: Volodymyr Samotiy <[email protected]>

… MS (#1855) **What I did** Update the rif_rates/lua script to multiply by 1000 instead of FlexCounter class. related also to this PR - sonic-net/sonic-sairedis#878 Fix issue - sonic-net/sonic-buildimage#8392 **Why I did it** times were not calculated properly. **How I verified it** check that the output of cli and redis is close enough:

PFCWD storm detection and restoration LUA scripts require values in microseconds. Due to recent changes polling interval is now passed in milliseconds by "FlexCounter". * sonic-net/sonic-sairedis#878 Signed-off-by: Volodymyr Samotiy <[email protected]>

…#1909) **What I did** Converted polling interval from milliseconds to microseconds in PFCWD LUA scripts. Same as PR #1908 but created directly against ```202012``` release branch **Why I did it** PFCWD storm detection and restoration LUA scripts require values in microseconds. Due to recent changes polling interval is now passed in milliseconds by "FlexCounter". * sonic-net/sonic-sairedis#878 So need to convert values to microseconds (as it was before) in order to make PFCWD working, **How I verified it** Ran PFCWD tests from sonic-mgmt.

**What I did** Converted polling interval from milliseconds to microseconds in PFCWD LUA scripts. **Why I did it** PFCWD storm detection and restoration LUA scripts require values in microseconds. Due to recent changes polling interval is now passed in milliseconds by "FlexCounter". * sonic-net/sonic-sairedis#878 So need to convert values to microseconds (as it was before) in order to make PFCWD working, **How I verified it** Ran PFCWD tests from sonic-mgmt.

Fixes sonic-net/sonic-buildimage#8773 Pfcwd regressions were observed due to the changes in the PR sonic-net/sonic-sairedis#878. Hence aligning the polling interval in lua script to use microseconds **How I verified it** Ran pfcwd tests that were failing prior to this change and they passed

#878) According to https://github.com/Azure/SONiC/blob/ec6d35dd2c28491bfade19cfee990fe612f1e5e9/doc/rates-and-utilization/Rates_and_utilization_HLD.md, counterpoll command gives polling interval in milliseconds. So when converting them to seconds to be supplied to lua script this should be divide by 1000. However, syncd multiplies it by 1000. Changed the multiplication to none, and did the converting in lua script - sonic-net/sonic-swss#1855 Fixed issue - sonic-net/sonic-buildimage#8392

… MS (sonic-net#1855) **What I did** Update the rif_rates/lua script to multiply by 1000 instead of FlexCounter class. related also to this PR - sonic-net/sonic-sairedis#878 Fix issue - sonic-net/sonic-buildimage#8392 **Why I did it** times were not calculated properly. **How I verified it** check that the output of cli and redis is close enough:

…net#1908) **What I did** Converted polling interval from milliseconds to microseconds in PFCWD LUA scripts. **Why I did it** PFCWD storm detection and restoration LUA scripts require values in microseconds. Due to recent changes polling interval is now passed in milliseconds by "FlexCounter". * sonic-net/sonic-sairedis#878 So need to convert values to microseconds (as it was before) in order to make PFCWD working, **How I verified it** Ran PFCWD tests from sonic-mgmt.

Fixes sonic-net/sonic-buildimage#8773 Pfcwd regressions were observed due to the changes in the PR sonic-net/sonic-sairedis#878. Hence aligning the polling interval in lua script to use microseconds **How I verified it** Ran pfcwd tests that were failing prior to this change and they passed

**What I did** Converted polling interval from milliseconds to microseconds in PFCWD LUA scripts. **Why I did it** PFCWD storm detection and restoration LUA scripts require values in microseconds. Due to recent changes polling interval is now passed in milliseconds by "FlexCounter". * sonic-net/sonic-sairedis#878 So need to convert values to microseconds (as it was before) in order to make PFCWD working, **How I verified it** Ran PFCWD tests from sonic-mgmt.

PFCWD storm detection and restoration LUA scripts require values in microseconds. Due to recent changes polling interval is now passed in milliseconds by "FlexCounter". * sonic-net/sonic-sairedis#878 Signed-off-by: Volodymyr Samotiy <[email protected]>

sonic-net#878) According to https://github.com/Azure/SONiC/blob/ec6d35dd2c28491bfade19cfee990fe612f1e5e9/doc/rates-and-utilization/Rates_and_utilization_HLD.md, counterpoll command gives polling interval in milliseconds. So when converting them to seconds to be supplied to lua script this should be divide by 1000. However, syncd multiplies it by 1000. Changed the multiplication to none, and did the converting in lua script - sonic-net/sonic-swss#1855 Fixed issue - sonic-net/sonic-buildimage#8392

kcudnik requested changes Aug 3, 2021

View reviewed changes

noaOrMlnx force-pushed the flexcounter_fix branch from bb95461 to eb3aae7 Compare August 4, 2021 13:23

noaOrMlnx mentioned this pull request Aug 4, 2021

Update lua script to convert poll_interval to MS noaOrMlnx/sonic-swss#3

Closed

noaOrMlnx marked this pull request as draft August 4, 2021 14:11

noaOrMlnx mentioned this pull request Aug 5, 2021

Update lua script to convert poll_interval to MS sonic-net/sonic-swss#1855

Merged

noaOrMlnx marked this pull request as ready for review August 9, 2021 07:21

noaOrMlnx force-pushed the flexcounter_fix branch from eb3aae7 to e9db3dd Compare August 9, 2021 08:15

kcudnik previously approved these changes Aug 17, 2021

View reviewed changes

Change flex counters logic to multiply by 1000 in lua script in order…

d178409

… to get real results

noaOrMlnx dismissed kcudnik’s stale review via d178409 August 17, 2021 13:14

noaOrMlnx force-pushed the flexcounter_fix branch from e9db3dd to d178409 Compare August 17, 2021 13:14

Change flex counters logic to multiply by 1000 in lua script in order…

fbccb19

… to get real results

kcudnik previously approved these changes Aug 29, 2021

View reviewed changes

Merge branch 'flexcounter_fix' of github.com:noaOrMlnx/sonic-sairedis…

32556a3

… into flexcounter_fix

noaOrMlnx dismissed kcudnik’s stale review via 32556a3 August 30, 2021 06:21

noaOrMlnx force-pushed the flexcounter_fix branch from a83386e to 32556a3 Compare August 30, 2021 06:21

qiluo-msft approved these changes Aug 30, 2021

View reviewed changes

kcudnik approved these changes Aug 30, 2021

View reviewed changes

liat-grozovik added the Request for 202012 Branch label Sep 1, 2021

liat-grozovik added the Request for 202106 Branch label Sep 1, 2021

qiluo-msft merged commit 536af40 into sonic-net:master Sep 2, 2021

qiluo-msft added the Included in 202012 Branch label Sep 2, 2021

volodymyrsamotiy mentioned this pull request Sep 14, 2021

[pfcwd] Convert polling interval from ms to us in LUA scripts volodymyrsamotiy/sonic-swss#3

Closed

volodymyrsamotiy mentioned this pull request Sep 15, 2021

[pfcwd] Convert polling interval from ms to us in LUA scripts sonic-net/sonic-swss#1908

Merged

volodymyrsamotiy mentioned this pull request Sep 15, 2021

[202012][pfcwd] Convert polling interval from ms to us in LUA scripts sonic-net/sonic-swss#1909

Merged

neethajohn mentioned this pull request Sep 15, 2021

Investigate/fix pfcwd failures sonic-net/sonic-buildimage#8773

Closed

neethajohn mentioned this pull request Sep 16, 2021

[pfcwd] Fix the polling interval time granularity sonic-net/sonic-swss#1912

Merged

judyjoseph added the Included in 202106 Branch label Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flex counters logic of converting poll interval to seconds from MS #878

Fix flex counters logic of converting poll interval to seconds from MS #878

noaOrMlnx commented Aug 3, 2021 •

edited

Loading

kcudnik Aug 3, 2021

noaOrMlnx Aug 16, 2021

liat-grozovik commented Aug 16, 2021

kcudnik commented Aug 16, 2021

noaOrMlnx commented Aug 16, 2021

kcudnik commented Aug 16, 2021

dgsudharsan commented Aug 17, 2021

kcudnik commented Aug 17, 2021 •

edited

Loading

liat-grozovik commented Aug 17, 2021

noaOrMlnx commented Aug 29, 2021

qiluo-msft commented Aug 30, 2021

noaOrMlnx commented Aug 30, 2021

qiluo-msft commented Aug 30, 2021

Fix flex counters logic of converting poll interval to seconds from MS #878

Fix flex counters logic of converting poll interval to seconds from MS #878

Conversation

noaOrMlnx commented Aug 3, 2021 • edited Loading

kcudnik Aug 3, 2021

Choose a reason for hiding this comment

noaOrMlnx Aug 16, 2021

Choose a reason for hiding this comment

liat-grozovik commented Aug 16, 2021

kcudnik commented Aug 16, 2021

noaOrMlnx commented Aug 16, 2021

kcudnik commented Aug 16, 2021

dgsudharsan commented Aug 17, 2021

kcudnik commented Aug 17, 2021 • edited Loading

liat-grozovik commented Aug 17, 2021

noaOrMlnx commented Aug 29, 2021

qiluo-msft commented Aug 30, 2021

noaOrMlnx commented Aug 30, 2021

qiluo-msft commented Aug 30, 2021

noaOrMlnx commented Aug 3, 2021 •

edited

Loading

kcudnik commented Aug 17, 2021 •

edited

Loading