-
Notifications
You must be signed in to change notification settings - Fork 659
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
[CLI][PFC] Add multi ASIC options for pfcstat and 'show pfc counters' #1057
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
93c688f
Add multi ASIC options for pfcstat and 'show pfc counters'
smaheshm ae7516e
updated help string in pfcstat
smaheshm fff3fcb
make linter happy, fix formatting
smaheshm d214986
if '-c' option is provided, collect stats from all ports
smaheshm f7a997a
Merge remote-tracking branch 'origin/master' into pfc-cli
smaheshm bdfbd11
added unit tests for 'show pfc counters, pfcstat for single and multi…
smaheshm 5fb0e32
remove the updates to sys.path that was added for testing
smaheshm 3cb5591
forgot to add new file
smaheshm 4d11c4d
fixed an issue with unsetting environment value
smaheshm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,10 @@ | |
|
||
##################################################################### | ||
# | ||
# pfcstat is a tool for summarizing Priority-based Flow Control (PFC) statistics. | ||
# pfcstat is a tool for summarizing Priority-based Flow Control (PFC) statistics. | ||
# | ||
##################################################################### | ||
|
||
import swsssdk | ||
import sys | ||
import argparse | ||
import cPickle as pickle | ||
|
@@ -17,6 +16,24 @@ from collections import namedtuple, OrderedDict | |
from natsort import natsorted | ||
from tabulate import tabulate | ||
|
||
from sonic_py_common.multi_asic import get_external_ports | ||
from utilities_common import multi_asic as multi_asic_util | ||
from utilities_common import constants | ||
|
||
# mock the redis for unit test purposes # | ||
try: | ||
if os.environ["UTILITIES_UNIT_TESTING"] == "2": | ||
modules_path = os.path.join(os.path.dirname(__file__), "..") | ||
tests_path = os.path.join(modules_path, "tests") | ||
sys.path.insert(0, modules_path) | ||
sys.path.insert(0, tests_path) | ||
import mock_tables.dbconnector | ||
if os.environ["UTILITIES_UNIT_TESTING_TOPOLOGY"] == "multi_asic": | ||
import mock_tables.mock_multi_asic | ||
mock_tables.dbconnector.load_namespace_config() | ||
|
||
except KeyError: | ||
pass | ||
|
||
PStats = namedtuple("PStats", "pfc0, pfc1, pfc2, pfc3, pfc4, pfc5, pfc6, pfc7") | ||
header_Rx = ['Port Rx', 'PFC0', 'PFC1', 'PFC2', 'PFC3', 'PFC4', 'PFC5', 'PFC6', 'PFC7'] | ||
|
@@ -51,11 +68,14 @@ COUNTER_TABLE_PREFIX = "COUNTERS:" | |
COUNTERS_PORT_NAME_MAP = "COUNTERS_PORT_NAME_MAP" | ||
|
||
class Pfcstat(object): | ||
def __init__(self): | ||
self.db = swsssdk.SonicV2Connector(host='127.0.0.1') | ||
self.db.connect(self.db.COUNTERS_DB) | ||
|
||
def get_cnstat(self, rx): | ||
def __init__(self, namespace, display): | ||
self.multi_asic = multi_asic_util.MultiAsic(display, namespace) | ||
self.db = None | ||
self.config_db = None | ||
self.cnstat_dict = OrderedDict() | ||
|
||
@multi_asic_util.run_on_multi_asic | ||
def collect_cnstat(self, rx): | ||
""" | ||
Get the counters info from database. | ||
""" | ||
|
@@ -70,7 +90,9 @@ class Pfcstat(object): | |
bucket_dict = counter_bucket_tx_dict | ||
for counter_name, pos in bucket_dict.iteritems(): | ||
full_table_id = COUNTER_TABLE_PREFIX + table_id | ||
counter_data = self.db.get(self.db.COUNTERS_DB, full_table_id, counter_name) | ||
counter_data = self.db.get( | ||
self.db.COUNTERS_DB, full_table_id, counter_name | ||
) | ||
if counter_data is None: | ||
fields[pos] = STATUS_NA | ||
else: | ||
|
@@ -79,15 +101,34 @@ class Pfcstat(object): | |
return cntr | ||
|
||
# Get the info from database | ||
counter_port_name_map = self.db.get_all(self.db.COUNTERS_DB, COUNTERS_PORT_NAME_MAP) | ||
counter_port_name_map = self.db.get_all( | ||
self.db.COUNTERS_DB, COUNTERS_PORT_NAME_MAP | ||
) | ||
if counter_port_name_map is None: | ||
return | ||
display_ports_set = set(counter_port_name_map.keys()) | ||
if self.multi_asic.display_option == constants.DISPLAY_EXTERNAL: | ||
display_ports_set = get_external_ports( | ||
display_ports_set, self.multi_asic.current_namespace | ||
) | ||
# Build a dictionary of the stats | ||
cnstat_dict = OrderedDict() | ||
cnstat_dict['time'] = datetime.datetime.now() | ||
if counter_port_name_map is None: | ||
return cnstat_dict | ||
for port in natsorted(counter_port_name_map): | ||
cnstat_dict[port] = get_counters(counter_port_name_map[port]) | ||
return cnstat_dict | ||
if counter_port_name_map is not None: | ||
for port in natsorted(counter_port_name_map): | ||
if port in display_ports_set: | ||
cnstat_dict[port] = get_counters( | ||
counter_port_name_map[port] | ||
) | ||
self.cnstat_dict.update(cnstat_dict) | ||
|
||
def get_cnstat(self, rx): | ||
""" | ||
Get the counters info from database. | ||
""" | ||
self.cnstat_dict.clear() | ||
self.collect_cnstat(rx) | ||
return self.cnstat_dict | ||
|
||
def cnstat_print(self, cnstat_dict, rx): | ||
""" | ||
|
@@ -163,10 +204,22 @@ Examples: | |
pfcstat | ||
pfcstat -c | ||
pfcstat -d | ||
pfcstat -n asic1 | ||
pfcstat -s all -n asic0 | ||
""") | ||
|
||
parser.add_argument('-c', '--clear', action='store_true', help='Clear previous stats and save new ones') | ||
parser.add_argument('-d', '--delete', action='store_true', help='Delete saved stats') | ||
parser.add_argument( '-c', '--clear', action='store_true', | ||
help='Clear previous stats and save new ones' | ||
) | ||
parser.add_argument( | ||
'-d', '--delete', action='store_true', help='Delete saved stats' | ||
) | ||
parser.add_argument('-s', '--show', default=constants.DISPLAY_EXTERNAL, | ||
help='Display all interfaces or only external interfaces' | ||
) | ||
parser.add_argument('-n', '--namespace', default=None, | ||
help='Display interfaces for specific namespace' | ||
) | ||
args = parser.parse_args() | ||
|
||
save_fresh_stats = args.clear | ||
|
@@ -175,15 +228,20 @@ Examples: | |
uid = str(os.getuid()) | ||
cnstat_file = uid | ||
|
||
cnstat_dir = "/tmp/pfcstat-" + uid | ||
cnstat_fqn_file_rx = cnstat_dir + "/" + cnstat_file + "rx" | ||
cnstat_fqn_file_tx = cnstat_dir + "/" + cnstat_file + "tx" | ||
cnstat_dir = os.path.join(os.sep, "tmp", "pfcstat-{}".format(uid)) | ||
cnstat_fqn_file_rx = os.path.join(cnstat_dir, "{}rx".format(cnstat_file)) | ||
cnstat_fqn_file_tx = os.path.join(cnstat_dir, "{}tx".format(cnstat_file)) | ||
|
||
# if '-c' option is provided get stats from all (frontend and backend) ports | ||
if save_fresh_stats: | ||
args.namespace = None | ||
args.show = constants.DISPLAY_ALL | ||
|
||
pfcstat = Pfcstat() | ||
pfcstat = Pfcstat(args.namespace, args.show) | ||
|
||
if delete_all_stats: | ||
for file in os.listdir(cnstat_dir): | ||
os.remove(cnstat_dir + "/" + file) | ||
os.remove(os.path.join(cnstat_dir, file)) | ||
|
||
try: | ||
os.rmdir(cnstat_dir) | ||
|
@@ -235,7 +293,7 @@ Examples: | |
else: | ||
pfcstat.cnstat_print(cnstat_dict_rx, True) | ||
|
||
print() | ||
print("") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just to add a 'line' between tx and rx outputs. |
||
|
||
""" | ||
Print the counters of pfc tx counter | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pfcstat -c should clear stats on all ports and all namespace and save all ports stats in the cached file.
If not, it might lead to confusing output.
For example. if user does the following sequence
Since the internal ports stats are not cleared, they will not be reset to zero,
External ports stats will start from zero while the Internal ports will still have the old value. This might lead to confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's right, will change it. By default non-show commands should apply to all ports and namespaces, and should not have an option for 'namespace' or 'display'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at it further. It doesn't really clear the stats from counters DB. It updates that stats in "/tmp" and prints the diff from previous values. Since the script takes multiple arguments the 'namespace', 'display' also applies to '-c' arg. We can ignore '-c' option when 'namespace' is provided, but this will work as is as all the keys are different. Meaning, if a user runs '-c' with a namespace option and then runs -c with a different namespace option or no namespace, the command will output the 'diff' in counts for the cached ports, and prints as is for un-cached ports or new ports with counts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the prints the "Last cached time was " which might me misleading since some of the ports were not cached.
The change I'm making is to collect stats from all ports when '-c' option is provided. In short the 'namespace' option will be ignored silently when '-c' arg is specified. Comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this. I don't think we will be adding the multi asic options to sonic-clear commands. So the util script will not get the namespace or display, so we should be ok. I follow the similar approach for show interface counters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use '-c' with a namespace and then use the display for another namespace, the problem we have is the cached time that we display will be incorrect because we aren't actually caching any of those ports info.
Either we ignore the namespace for '-c' option and get the stats for all ports and write them to the file or we need to maintain separate files for the namespaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore my comment. saw your change