Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed porstat rate and util issues #1140

Merged
merged 1 commit into from
Nov 20, 2020
Merged

Fixed porstat rate and util issues #1140

merged 1 commit into from
Nov 20, 2020

Conversation

rajkumar38
Copy link
Contributor

- What I did
Fixed "portstat" rate and utilization issue.
Changed size factor from 1024 to 1000 for rate throughput calculation.
- How I did it
Pls check the diff.
- How to verify it
"portstat", "portstat -a"
- Previous command output (if the output of a command-line utility has changed)
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

Ethernet41 U 353,628,250 98.67 MB/s 1.93% 0 0 0 353,660,334 98.67 MB/s 1.93% 0 0 0
- New command output (if the output of a command-line utility has changed)
FACE 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

Ethernet2 U 5,065,263,485 124.98 MB/s 99.99% 0 0 0 113 7.63 B/s 0.00% 0 0 0

@lgtm-com
Copy link

lgtm-com bot commented Sep 29, 2020

This pull request introduces 1 alert when merging 3cfef9595ec0ce8bdbfbc01340f7a2da0fd399e5 into 07a4e26 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@lguohan
Copy link
Contributor

lguohan commented Oct 7, 2020

can you describe the issue? Not sure I understand the issue here.

@rajkumar38
Copy link
Contributor Author

can you describe the issue? Not sure I understand the issue here.

Two issues:

  1. port speed was not passed to "ns_util()", which was always taking default value "PORT_RATE" for command "portstat".
  2. "RX_BPS", "RX_UTIL" calculation was not showing correct values, when traffic is passed at line rate. Functions "ns_brate()" and
    "ns_util" was using value 1024 instead of 1000 for throughput calculations.
    Example, for 1G traffic on port,

util = rate/(port_rate102410241024/8.0)100 --> 93% (wrong)
util = rate/(port_rate
1000
1000*1000/8.0)*100 --> 100% (correct)

rate = "{:.2f}".format(rate/1024/1024)+' MB' --> 119MB/s (Wrong)
rate = "{:.2f}".format(rate/1000/1000)+' MB' --> 125MB/s (Correct)

scripts/portstat Outdated
for ns in self.multi_asic.get_ns_list_based_on_options():
self.db = multi_asic.connect_to_all_dbs_for_ns(ns)
speed = self.db.get(self.db.APPL_DB, full_table_id, PORT_SPEED_FIELD)
if speed is not None:
return int(speed)//1000
return speed
return PORT_RATE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not understand the fix here. why does this fix problem?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like a personal preference, you do not need to change the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix is nothing to do with issue reported.
I was getting below Jenkins error, hence the code change here to fix error.
Variable "port_rate" was updated from "get_port_speed()"

18:22:17 tests/portstat_test.py:227: AssertionError
18:22:17 ----------------------------- Captured stdout call -----------------------------
18:22:17 1
18:22:17 The rates are calculated within 3 seconds period
18:22:17
18:22:17 ----------------------------- Captured stderr call -----------------------------
18:22:17 Traceback (most recent call last):
18:22:17 File "/sonic/sonic-utilities/scripts/portstat", line 464, in
18:22:17 main()
18:22:17 File "/sonic/sonic-utilities/scripts/portstat", line 461, in main
18:22:17 portstat.cnstat_diff_print(cnstat_new_dict, cnstat_dict, intf_list, use_json, print_all, errors_only, rates_only)
18:22:17 File "/sonic/sonic-utilities/scripts/portstat", line 297, in cnstat_diff_print
18:22:17 ns_util(cntr.rx_byt, old_cntr.rx_byt, time_gap, port_speed),
18:22:17 File "/sonic/sonic-utilities/utilities_common/netstat.py", line 52, in ns_util
18:22:17 util = rate/(port_rate10001000*1000/8.0)*100
18:22:17 TypeError: unsupported operand type(s) for *: 'NoneType' and 'int'
18:22:17 ____________________ TestPortStat.test_clear_intf_counters _____________________
18:22:17
18:22:17 self = <tests.portstat_test.TestPortStat object at 0x7f84cca63c50>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do not understand how the error is related to the fix here. previous it is returning either PORT_RATE or int(speed)//1000. after your fix, it is the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here "speed" is updated from below line inside for loop:
speed = self.db.get(self.db.APPL_DB, full_table_id, PORT_SPEED_FIELD)

Previously, the function was returning "int(speed)//1000" (if speed is not None ) or "None" (if "self.db.get" failed)
With fix, now it returns "PORT_RATE", when "self.db.get" fails.

In latest code, this is fix is already pushed as part of commit "13bd06be696fc14663ddfe5bb6c19abb9b647759". I will rebase and update the PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the explanation

@lguohan lguohan merged commit be834be into sonic-net:master Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants