-
Notifications
You must be signed in to change notification settings - Fork 664
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
[debug dump util] COPP Module Added #1670
Conversation
Signed-off-by: Vivek Reddy Karri <[email protected]>
Signed-off-by: Vivek Reddy Karri <[email protected]>
Signed-off-by: Vivek Reddy Karri <[email protected]>
Signed-off-by: Vivek Reddy Karri <[email protected]>
Signed-off-by: Vivek Reddy Karri <[email protected]>
Signed-off-by: Vivek Reddy Karri <[email protected]>
Signed-off-by: Vivek Reddy Karri <[email protected]>
Signed-off-by: Vivek Reddy Karri <[email protected]>
Signed-off-by: Vivek Reddy Karri <[email protected]>
This pull request introduces 2 alerts when merging 05edcad into a425ca2 - view on LGTM.com new alerts:
|
@dgsudharsan , can you review/sign-off? |
Signed-off-by: Vivek Reddy Karri <[email protected]>
Signed-off-by: Vivek Reddy Karri <[email protected]>
…o dump_util_copp
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Request for 202106 |
Signed-off-by: Vivek Reddy Karri <[email protected]>
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 comments
dump/plugins/copp.py
Outdated
return sai_hostif_vid | ||
|
||
def __get_asic_hostif_obj(self, sai_hostif_vid): | ||
# Not adding tp tables_not_found because of the type of reason specified for policer obj |
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.
indentation is not correct
if not ret["error"] and len(ret["keys"]) > 0: | ||
self.ret_temp["ASIC_DB"]["keys"].append(ret["keys"][0]) | ||
|
||
# When the user writes config to CONFIG_DB, that takes precedence over default config |
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.
indentation is not correct
elif trap_id_key_cf: | ||
self.ret_temp["CONFIG_FILE"]["keys"].append(trap_id_key_cf) | ||
self.copp_trap_cfg_key = trap_id_key_cf.split("|")[-1] | ||
id_in_file, _ = self.__find_trap_id_in_db(self.copp_trap_cfg_key, False) |
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 line 247, the trap_id_key_cf
is retrived only when there are no traps in the db. so in id_in_file
will always be empty string and the key will not save to `ret_temp ?
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.
Sry, i'll rename this to id_in_db.
And no, id_in_db can be trap_id_key_cf or NULL
Eg: Let's say user wanted a dump for trap_id: "ospfv6"
In Config File:
"COPP_TRAP":{
"bgp": {
"trap_ids": "ospfv6,ospf"
}
}
In Config DB:
"COPP_TRAP": {
"bgp": {
"trap_ids": "bgp,bgpv6",
"trap_group": "queue4_group1"
}
}
In this case, after running trap_id_key_db, trap_group_db = self.__find_trap_id_in_db()
, trap_id_key_db will still be empty as there is no direct way to find out the COPP_TRAP|bgp as the config DB entry doesn't have ospfv6,
We then find "trap_id_key_cf" from the file and the use that to find any diff present in the Config DB.
The complexity comes from the fact that there are two sources of truth for COPP config and you can provide a diff of any particular table (COPP_TRAP & COPP_GROUP) in any of the sources.
self.ret_temp["CONFIG_DB"]["keys"].append(trap_id_key_db) | ||
self.copp_trap_cfg_key = trap_id_key_db.split("|")[-1] | ||
id_in_file, _ = self.__find_trap_id_in_conf_file(self.copp_trap_cfg_key, False) | ||
if id_in_file == trap_id_key_db: # If any diff |
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 seems we add the trap_id
to return ret_temp
only when they are same. if they are different they are ignored ?
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.
id_in_file will either be trap_id_key_db or NULL/Empty String.
Eg: Let's say user wanted a dump for trap_id: "ospfv6"
In Config DB:
"COPP_TRAP":{
"bgp": {
"trap_ids": "ospfv6,ospf"
}
}
In Config File:
"COPP_TRAP": {
"bgp": {
"trap_ids": "bgp,bgpv6",
"trap_group": "queue4_group1"
}
}
So, here we found out trap_id_key_db is bgp
given ospfv6 as the trap_id. id_in_file is just a check in config file for any potential diff present in the COPP_TRAP|bgp key and thus id_in_file will either be "bgp" or NULL.
If it's NULL, we won't add it to the "CONFIG_FILE" section in the return template
from dump.helper import handle_error | ||
from .executor import Executor | ||
|
||
TRAP_ID_MAP = { |
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.
Does this handle all the hostif traps defined in the SAI
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.
No, i've copied the table from copporch. LINK. Seems copporch doesn't handle all the hostif traps defined in the SAI
key_tg = ret["keys"][0] | ||
self.ret_temp["CONFIG_FILE"]["keys"].append(key_tg) | ||
return True | ||
elif not_found_report: |
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.
do we need this elif ? seems like the not_found_report
is always 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.
Yeah, but just to keep the implementation consistent with __fill_trap_group_in_conf_db
, i've retained it.
Signed-off-by: Vivek Reddy Karri <[email protected]>
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
@SuvarnaMeenakshi and @arlakshm Can you finish the review and sign-off? |
@arlakshm @SuvarnaMeenakshi Can you please review and sign off? |
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.
lgtm
@SuvarnaMeenakshi Can you please review and sign off? |
@SuvarnaMeenakshi Can you please help here? |
What I did Implemented vlan and vlan_member modules for debug dump utility. How I did it Used infrastructure and followed examples in #1666 #1667 #1668 #1669 #1670 How to verify it On switch: dump state vlan <vlan_name> dump state vlan_member '<vlan_name|<member_name>' Unit test: pytest-3 dump_tests/module_tests/vlan_test.py (same test file covers both vlan and vlan_member)
Signed-off-by: Vivek Reddy Karri [email protected]
What I did
HLD for Dump Utility: HLD.
How I did it
How to verify it
Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)