-
Notifications
You must be signed in to change notification settings - Fork 666
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
[crm]: Add utility for CRM configuration #187
Conversation
Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>
0789cec
to
23af16e
Compare
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.
As mentioned in section <2.7.3 Show CLI command>, should we also incorporate this to show utility?
crm/main.py
Outdated
for res in ["ipv4_route", "ipv6_route", "ipv4_nexthop", "ipv6_nexthop", "ipv4_neighbor", "ipv6_neighbor", | ||
"nexthop_group_member", "nexthop_group_object", "acl_table", "acl_group", "acl_entry", | ||
"acl_counter", "fdb_entry"]: | ||
data.append([res, crm_info[res + "_threshold_type"], crm_info[res + "_low_threshold"], crm_info[res + "_high_threshold"]]) |
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.
is crm_info[] from config_db prefixed with "crm" as mentioned in the design spec (CRM_IPV4_ROUTE_THRESHOLD_TYPE)?. Can you place the config_db output for understanding?
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.
Currently "CONFIG_DB" output looks as following:
127.0.0.1:6379[4]> HGETALL CRM|Config
1) "ipv4_route_threshold_type"
2) "percentage"
3) "ipv4_route_low_threshold"
4) "70"
5) "ipv4_route_high_threshold"
6) "85"
...
crm/main.py
Outdated
print tabulate(data, headers=header, tablefmt="simple", missingval="") | ||
print '\n' | ||
|
||
def show_acl_table_resources(self, table_id): |
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.
Show table resource requires user to specify table_id? Just thinking if we can also display the complete table. What do you say?
Was this intended in design spec, [resources|thresholds] acl group [entry|counter]
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.
Currently it requires table_id, I agree that it is better to display complete table. Will check what we can do.
@click.argument('value', type=click.Choice(['percentage', 'used', 'free'])) | ||
@click.pass_context | ||
def type(ctx, value): | ||
"""CRM threshod type configuration""" |
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.
Typo - threshold.. Please correct the ones below as well
Can you show one sample output for any one of the threshold and resource? |
Yes, I will incorporate this script to the "show" and "config" utilities. |
Please find "threshold" sample output below.
|
Please find "resource" sample output below.
|
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.
Approving. Few pending changes are not critical and can be addressed in future commits
can you change the name to
|
can give the output for acl_entry/acl_counter for a table? |
* Change "show acl table" command in order to show complete table without passing table ID * Rename "nexthop_group_object" resource to "nexthop_group" * Change CRM polling interval represantation from minutes to seconds Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>
|
Below is the output for the ACL per table resources: entry and counter.
|
crm/main.py
Outdated
data = [] | ||
|
||
crm_stats = countersdb.get_all(countersdb.COUNTERS_DB, 'CRM:ACL_TABLE_STATS:{0}'.format(table_id)) | ||
# Retrieve all ACL table keys from CRM:ACL_TABLE_STATS | ||
output = os.popen("docker exec -i database redis-cli --raw -n 2 KEYS *CRM:ACL_TABLE_STATS*").readlines() |
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.
In general, subprocess is being used instead of os.popen
crm/main.py
Outdated
""" | ||
CRM Handler to display ACL table information. | ||
""" | ||
countersdb = swsssdk.SonicV2Connector(host='127.0.0.1') | ||
countersdb.connect(countersdb.COUNTERS_DB) | ||
|
||
header = ("Resource Name", "Used Count", "Available Count") | ||
header = ("Table ID", "Resource Name", "Used Count", "Available Count") |
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.
Just checking, is it possible to display Table name as well, if it's available in the DB?
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.
Looks like no, currently we cannot display ACL table name here.
* Use subprocess instead of os.popen * Fix incorrect output of ACL table info Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>
RB=1288101 G=lnos-reviewers R=ntrianta,pmao,rmolina,sfardeen,zxu * github: [acl_loader] Support Service ACL binding to multiple services (sonic-net#236) [show] Rename 'show session' to 'show mirror session' (sonic-net#235) [pfcstat]: create python cli tool to show pfc counters (sonic-net#233) [queuestat] add python CLI tool to show queue counters [acl-loader] Not to crash upon invalid rule (sonic-net#232) Show FDB type in fdbshow/show mac (sonic-net#231) [show] add 'show runningconfiguration all' subcommand (sonic-net#230) [reboot scripts] remove -t option in docker exec commands (sonic-net#228) [reboot] reduce stop service to only stop syncd (sonic-net#223) [crm]: Fix failures in CLI show commands (sonic-net#221) [Fast-reboot]: Gracefully shutdown syncd in fast-reboot (sonic-net#212) add fast-reboot support for nephos platform by stop kernel modules (sonic-net#220) [config bgp] Convert user input ipv6 addr to lower case before comparing (sonic-net#218) [PFCWD]: set default configuration when enabled by default (sonic-net#213) Add fast-reboot support for Aboot based images (sonic-net#214) sonic-utilities: Format show vlan config output (sonic-net#210) [AAA] Support login(ascii) authentication type (sonic-net#217) [sfputil] Adapt new way of getting PLATFORM(sonic-net#216) [Fast-Reboot]: Adapt fast-reboot-dump script for SAIv1.2 (sonic-net#211) Refactor fast-reboot script. Generate fast-reboot-dumps into configurable directory (sonic-net#208) Find correct opennsl module name before stopping it (sonic-net#207) [crm]: Add utility for CRM configuration (sonic-net#187) [reboot] update reboot script to retrieve platform with new format (sonic-net#206) Adapt to config engine change to load platform info properly (sonic-net#205) [config] Add qos clear and qos reload support (sonic-net#204) Dump default routes from APPL_DB table before fast-reboot (sonic-net#203) [acl_loader] Fix a crash issue when appdb is not consistent with cfgdb (sonic-net#202) [pfcwd]: add command to set pfcwd polling interval (sonic-net#192) [acl-loader] Prevent from hanging if run by non-root user (sonic-net#199) [config] Store ConfigDB init indicator boolean value as 1/0 in Redis to be language-agnostic (sonic-net#197) Get Vlan Id from SAI Vlan Object if bvid present (sonic-net#196) [TACACS+]: Fix aaa show error without configuration (sonic-net#191) 'config bgp [shutdown|startup] neighbor <hostname>' now affects all sessions for neighbor (sonic-net#195) [sonic-clear] add a clear fdb command (sonic-net#186)
header = ("Table ID", "Resource Name", "Used Count", "Available Count") | ||
|
||
# Retrieve all ACL table keys from CRM:ACL_TABLE_STATS | ||
proc = Popen("docker exec -i database redis-cli --raw -n 2 KEYS *CRM:ACL_TABLE_STATS*", stdout=PIPE, stderr=PIPE, shell=True) |
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.
It exec another script to make DB operation. Could you please use redis-py or existing XxxxDBConnector/etc to do DB changes?
…common classes (sonic-net#187) #### Description Existing test will mix true swsscommon package with the mocked one in sonic_py_common #### Motivation and Context This is blocking sonic-net/sonic-buildimage#7655 #### How Has This Been Tested? Unit test
…net#187) Add a stop function to thermal manager. Allow thermalctld to call the stop function to stop run_policy function fast
Signed-off-by: Volodymyr Samotiy volodymyrs@mellanox.com
Initial CRM UI utility implementation for review. Please do not merge yet.