[portstat, intfstat] update rates and utilization #817

mykolaf · 2020-02-21T08:48:43Z

According to HLD

- What I did
Update the logic for port rates and utilization
- How I did it

- How to verify it

- Previous command output (if the output of a command-line utility has changed)

admin@sonic:~$ show int count
      IFACE    STATE    RX_OK    RX_BPS    RX_UTIL    RX_ERR    RX_DRP    RX_OVR    TX_OK    TX_BPS    TX_UTIL    TX_ERR    TX_DRP    TX_OVR
-----------  -------  -------  --------  ---------  --------  --------  --------  -------  --------  ---------  --------  --------  --------
  Ethernet0        X        0       N/A        N/A         0         0       N/A        0       N/A        N/A         0         0       N/A
  Ethernet2        U      287       N/A        N/A         0         0       N/A      818       N/A        N/A         0         0       N/A
  Ethernet4        U      380       N/A        N/A         0         0       N/A      858       N/A        N/A         0         0       N/A
  Ethernet6        U      286       N/A        N/A         0         0       N/A      850       N/A        N/A         0         0       N/A

- New command output (if the output of a command-line utility has changed)

admin@sonic:~$ show int count
      IFACE    STATE    RX_OK      RX_BPS    RX_UTIL    RX_ERR    RX_DRP    RX_OVR    TX_OK      TX_BPS    TX_UTIL    TX_ERR    TX_DRP    TX_OVR
-----------  -------  -------  ----------  ---------  --------  --------  --------  -------  ----------  ---------  --------  --------  --------
  Ethernet0        X        0    0.00 B/s      0.00%         0         0       N/A        0    0.00 B/s      0.00%         0         0       N/A
  Ethernet2        U        0    0.00 B/s      0.00%         0         0       N/A        0    0.00 B/s      0.00%         0         0       N/A
  Ethernet4        U        0    0.00 B/s      0.00%         0         0       N/A        0    0.00 B/s      0.00%         0         0       N/A
  Ethernet6        U        0    0.00 B/s      0.00%         0         0       N/A        0    0.00 B/s      0.00%         0         0       N/A

abdosi · 2020-03-29T19:50:08Z

sonic-utilities-tests/intfstat_test.py

-                    "PortChannel0003        0  0.00 B/s    0.00/s         0        0  0.00 B/s    0.00/s         0",
-                    "PortChannel0004        0  0.00 B/s    0.00/s         0        0  0.00 B/s    0.00/s         0",
-                    "Vlan1000        0  0.00 B/s    0.00/s         0        0  0.00 B/s    0.00/s         0"]
+        expected = ["Ethernet20   0  0.00 B/s   0.00/s     0.00%         0         0  0.00 B/s    0.00/s     0.00%         0",


what will be command to check RIF Counters ?
Has this been verified if RIF is both Over Port-Channel and VLAN ?

The command to check RIF counters is show interface counters rif .

abdosi · 2020-03-29T19:52:51Z

scripts/portstat

+                                format_prate(rates.rx_pps),
+                                format_util(rates.rx_util),                
+                                ns_diff(cntr.rx_err, old_cntr.rx_err),
+                                ns_diff(cntr.rx_drop, old_cntr.rx_drop),


Is the O/P of show interface counters remains same (backward compatible ) after these changes ? Is there any difference ?

There is no difference in how the output looks,
one difference in behavior is that clear command will not affect the rates&utilization. Still clears the counters.

lgtm-com · 2020-04-21T14:40:30Z

This pull request introduces 2 alerts when merging de7c8588372de20b3f1d042b979fc513ac840b44 into a87173b - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Result of integer division may be truncated

qiluo-msft · 2020-06-08T19:22:08Z

Please fix LGTM new alerts.

In reply to: 640825246

lgtm-com · 2020-06-09T16:23:27Z

This pull request introduces 10 alerts when merging a1806b533ea2c14459e19dcf630faa1c0033a96d into d2c997c - view on LGTM.com

new alerts:

6 for Wrong number of arguments in a call
2 for Result of integer division may be truncated
1 for Unused import
1 for Variable defined multiple times

lgtm-com · 2020-06-09T16:38:31Z

This pull request introduces 3 alerts when merging 45ea09182dc6b310350db47f7c4cf733bd4646c3 into 1ddd3f2 - view on LGTM.com

new alerts:

2 for Result of integer division may be truncated
1 for Unused local variable

lgtm-com · 2020-06-10T12:36:03Z

This pull request introduces 1 alert when merging 42a3a941050c2e99946f9dece87aa78e5cb7e44f into 1ddd3f2 - view on LGTM.com

new alerts:

1 for Result of integer division may be truncated

mykolaf · 2020-06-10T16:10:31Z

retest this please

liat-grozovik · 2020-07-07T13:37:23Z

@abdosi anything else needed to approve this PR?

abdosi

LGTM

mykolaf · 2020-07-20T16:51:05Z

@abdosi can we merge this?

liat-grozovik · 2020-07-30T13:14:57Z

@mykolaf can this be merged or this is pending on another PR?

volodymyrsamotiy · 2020-07-31T17:01:56Z

@abdosi, could you please help to have this PR merged?
It is approved, all checks passed and all other related PRs are already merged, without it we have incomplete functionality in master.
Thanks.

liat-grozovik · 2020-08-10T07:24:32Z

retest this please

liat-grozovik · 2020-08-13T10:03:47Z

retest this please

lguohan

since there is new feature is added, need unit test to cover the new feature.

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

…o rates Conflicts: scripts/portstat

lgtm-com · 2020-10-20T15:40:23Z

This pull request introduces 2 alerts when merging 3b46b03 into 13bd06b - view on LGTM.com

new alerts:

1 for Unused import
1 for Wrong number of arguments in a call

* Fix review comments Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

lguohan · 2020-10-22T16:21:44Z

retest this please

lguohan · 2020-10-22T16:22:45Z

looks like the unit test are failing, can you folks check?

keboliu · 2020-11-04T06:19:06Z

retest this please

liat-grozovik · 2020-11-10T16:28:12Z

retest this please

liat-grozovik · 2020-11-10T16:45:36Z

retest this please

…o rates Conflicts: scripts/intfstat scripts/portstat utilities_common/netstat.py

lgtm-com · 2021-06-10T15:48:07Z

This pull request introduces 1 alert when merging 66cf477 into 25e17de - view on LGTM.com

new alerts:

1 for Wrong number of arguments in a call

* Fix "wrong number of args" issue after merge from upstream Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

liat-grozovik · 2021-07-14T16:24:44Z

/azp run

azure-pipelines · 2021-07-14T16:24:54Z

Azure Pipelines successfully started running 1 pipeline(s).

liat-grozovik · 2021-07-14T16:25:30Z

@lguohan can you please review and approve? we currently have a mismatch between the CLI and the way the rates are calculated by the lua script.

lguohan · 2021-07-15T00:35:20Z

can you check the pr check failure?

Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>

liat-grozovik · 2021-07-29T13:32:20Z

@dgsudharsan can you please also review?

liat-grozovik · 2021-07-29T13:32:55Z

@lguohan can you please review following the fixes provided?

dgsudharsan · 2021-07-31T00:42:02Z

scripts/intfstat

-                            cntr.tx_p_err))
+                old_cntr = NStats._make([0] * len(nstat_fields))
+
+            rates = ratestat_dict.get(key, RateStats._make([STATUS_NA]*len(ratestat_fields)))


I don't think ratestat_dict is defined anywhere else in the file. Is there a missing piece of code?

dgsudharsan · 2021-07-31T01:08:44Z

utilities_common/netstat.py


 STATUS_NA = 'N/A'
 PORT_RATE = 40

+rates_key_list = [ 'RX_BPS', 'RX_PPS', 'RX_UTIL', 'TX_BPS', 'TX_PPS', 'TX_UTIL' ]


I don't think these should be declared in this file. This needs to go inside portstat and rifstat respectively. This file is meant to be like a library for various formatting APIs

dgsudharsan · 2021-07-31T01:24:23Z

scripts/intfstat

+        """
+            Get the Intf speed
+        """
+        full_table_id = PORT_STATUS_TABLE_PREFIX + port_name


PORT_STATUS_TABLE_PREFIX doesn't seem to be defined anywhere. Missing code?

dgsudharsan · 2021-07-31T01:24:55Z

scripts/intfstat

+        """
+        full_table_id = PORT_STATUS_TABLE_PREFIX + port_name
+        admin_state = self.db.get(self.db.APPL_DB, full_table_id, PORT_ADMIN_STATUS_FIELD)
+        oper_state = self.db.get(self.db.APPL_DB, full_table_id, PORT_OPER_STATUS_FIELD)


PORT_OPER_STATUS_FIELD doesn't seem to be defined anywhere. Missing code?

dgsudharsan · 2021-07-31T01:25:39Z

scripts/intfstat

+        if admin_state is None or oper_state is None:
+             return STATUS_NA
+        elif admin_state.upper() == PORT_STATUS_VALUE_DOWN:
+            return PORT_STATE_DISABLED
+        elif admin_state.upper() == PORT_STATUS_VALUE_UP and oper_state.upper() == PORT_STATUS_VALUE_UP:
+            return PORT_STATE_UP
+        elif admin_state.upper() == PORT_STATUS_VALUE_UP and oper_state.upper() == PORT_STATUS_VALUE_DOWN:
+            return PORT_STATE_DOWN
+        else:


Please check all the variables used there. They don't seem to be defined.

dgsudharsan · 2021-07-31T01:26:16Z

scripts/intfstat

@@ -127,6 +127,24 @@ class Intfstat(object):
        else:
            return STATUS_NA

+    def get_intf_speed(self, port_name):


This API is not called anywhere. Why is this API needed

Please also check why get_intf_state is needed. I don't see it called anywhere

dgsudharsan · 2021-07-31T01:31:03Z

tests/intfstat_test.py

@@ -110,17 +110,9 @@ def test_clear(self):
        assert result.exit_code == 0
        result = runner.invoke(show.cli.commands["interfaces"].commands["counters"].commands["rif"], [])
        print(result.stdout)
-        expected = ["Ethernet20        0  0.00 B/s    0.00/s         0        0  0.00 B/s    0.00/s         0",


Can you please include the new output in the test rather than just removing the expected here? I believe it's required to validate the output format

dgsudharsan · 2021-07-31T01:41:09Z

utilities_common/netstat.py

+    if brate == STATUS_NA:
+        return STATUS_NA
+    else:
+        util = brate/(float(port_rate)*1024*1024*1024/8.0)*100


This assumes the port speed to be Gbps. Though most of the devices have lowest speed as 1 Gbps. I don't think it's a good design here. If SONiC is planned to be used for management and other low capacity devices in the future this will become a bug for ports less than 1Gbps. I would rather prefer logic using the port speed to determine the factor to divide rather than blindly assuming Gbps

Could you please describe the suggested approach in more detail?

I believe port speed can be used to figure out if it is in Gbps or Mbps. By default I believe speed is in Mbps (eg 40G is 40000)

volodymyrsamotiy · 2021-08-10T10:56:59Z

Closing since there is new PR #1750 with the same changes + some fixes and rebased code.

mykolaf force-pushed the rates branch from 5e401e3 to dd9360c Compare February 25, 2020 14:28

rlhui assigned abdosi Mar 4, 2020

abdosi requested changes Mar 29, 2020

View reviewed changes

mykolaf force-pushed the rates branch from dd9360c to 10c75dc Compare April 9, 2020 07:24

mykolaf force-pushed the rates branch from 10c75dc to de7c858 Compare April 21, 2020 14:34

abdosi previously approved these changes May 18, 2020

View reviewed changes

mykolaf dismissed abdosi’s stale review via 8c619a0 June 9, 2020 15:50

abdosi previously approved these changes Jul 7, 2020

View reviewed changes

liat-grozovik previously approved these changes Aug 3, 2020

View reviewed changes

mykolaf dismissed stale reviews from liat-grozovik and abdosi via 309d2b6 August 4, 2020 12:58

mykolaf force-pushed the rates branch from 42a3a94 to 309d2b6 Compare August 4, 2020 12:58

This comment has been minimized.

Sign in to view

lguohan requested changes Aug 13, 2020

View reviewed changes

Mykola Faryma added 3 commits August 18, 2020 19:02

[portstat, intfstat] update rates and utilization

dec5cd8

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

fix get_rates

47e355c

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

[UT] add rates_test.py

93e2cd0

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

mykolaf force-pushed the rates branch from 4779e75 to 93e2cd0 Compare August 18, 2020 16:04

liat-grozovik mentioned this pull request Oct 18, 2020

Issues in portstat/show interface counters sonic-net/sonic-buildimage#5655

Closed

Merge branch 'master' of https://github.com/Azure/sonic-utilities int…

3b46b03

…o rates Conflicts: scripts/portstat

lguohan mentioned this pull request Oct 20, 2020

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

Merged

3 tasks

[portstat, intfstat] update rates and utilization

0f98cc9

* Fix review comments Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

liat-grozovik previously approved these changes Oct 20, 2020

View reviewed changes

lguohan previously approved these changes Oct 22, 2020

View reviewed changes

Merge branch 'master' of https://github.com/Azure/sonic-utilities int…

66cf477

…o rates Conflicts: scripts/intfstat scripts/portstat utilities_common/netstat.py

volodymyrsamotiy dismissed stale reviews from lguohan and liat-grozovik via 66cf477 June 10, 2021 15:37

[portstat, intfstat] update rates and utilization

3344cbe

* Fix "wrong number of args" issue after merge from upstream Signed-off-by: Volodymyr Samotiy <volodymyrs@nvidia.com>

liat-grozovik added the Request for 202012 Branch label Jul 14, 2021

Reworked portstat script and UT's according to the new code base

c791d35

Signed-off-by: Vadym Hlushko <vadymh@nvidia.com>

dgsudharsan suggested changes Jul 31, 2021

View reviewed changes

vadymhlushko-mlnx mentioned this pull request Aug 10, 2021

[portstat, intfstat] added rates and utilization #1750

Merged

volodymyrsamotiy closed this Aug 10, 2021

liat-grozovik removed the Request for 202012 Branch label Aug 10, 2021

[portstat, intfstat] update rates and utilization #817

[portstat, intfstat] update rates and utilization #817

Conversation

mykolaf commented Feb 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

lgtm-com bot commented Apr 21, 2020

qiluo-msft commented Jun 8, 2020 • edited Loading

lgtm-com bot commented Jun 9, 2020

lgtm-com bot commented Jun 9, 2020

lgtm-com bot commented Jun 10, 2020

mykolaf commented Jun 10, 2020

liat-grozovik commented Jul 7, 2020

abdosi left a comment

Choose a reason for hiding this comment

mykolaf commented Jul 20, 2020

liat-grozovik commented Jul 30, 2020

volodymyrsamotiy commented Jul 31, 2020

This comment has been minimized.

liat-grozovik commented Aug 10, 2020

liat-grozovik commented Aug 13, 2020

lguohan left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Oct 20, 2020

lguohan commented Oct 22, 2020

lguohan commented Oct 22, 2020

keboliu commented Nov 4, 2020

liat-grozovik commented Nov 10, 2020

liat-grozovik commented Nov 10, 2020

lgtm-com bot commented Jun 10, 2021

liat-grozovik commented Jul 14, 2021

azure-pipelines bot commented Jul 14, 2021

liat-grozovik commented Jul 14, 2021

lguohan commented Jul 15, 2021

liat-grozovik commented Jul 29, 2021

liat-grozovik commented Jul 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

volodymyrsamotiy commented Aug 10, 2021

mykolaf commented Feb 21, 2020 •

edited

Loading

qiluo-msft commented Jun 8, 2020 •

edited

Loading