[lldp_syncd] add new OIDs - lldpRemTable & lldpLocPortTable #5
Conversation
|
Hi @qiluo-msft, |
|
@pavel-shirshov , can you also review it. |
|
Could you help resolve the conflict first? Be aware there is changes on test data, which we found in prod environment. #Closed |
src/lldp_syncd/daemon.py
Outdated
| @@ -56,7 +56,7 @@ class LldpSyncDaemon(SonicSyncDaemon): | |||
| within the same Redis instance on a switch | |||
| """ | |||
| LLDP_ENTRY_TABLE = 'LLDP_ENTRY_TABLE' | |||
|
|
|||
| LLDP_LOC_CHASSIS_TABLE = 'LLDP_LOC_CHASSIS' | |||
| @unique | |||
There was a problem hiding this comment.
Could you check the code format with some python lint tool? At least there are some new errors reported by http://pep8online.com/
#Closed
There was a problem hiding this comment.
Checked with flake8, with max line length=100.
As far as i know we don't have a set of rules for code style yet. #Closed
There was a problem hiding this comment.
src/lldp_syncd/daemon.py
Outdated
| # [{'enabled': ..., 'type': 'capability1'}, {'enabled': ..., 'type': 'capability2'}] | ||
| capability_list = if_attributes['chassis'].values()[0]['capability'] | ||
| except KeyError: | ||
| logger.error("Failed to get system capabilities") |
There was a problem hiding this comment.
logger [](start = 12, length = 6)
After logging, it continue with below logic, which seems not correct for this path. #Closed
There was a problem hiding this comment.
Line 124-125 covers the case when we have only 1 capability in the list, it should be above the except case. Fixed this.
src/lldp_syncd/daemon.py
Outdated
| return None | ||
| else: | ||
| def scrap_output(cmd): | ||
| try: |
There was a problem hiding this comment.
try [](start = 12, length = 3)
Original 2 try-except blocks are better. #Closed
src/lldp_syncd/daemon.py
Outdated
| # [{'enabled': ..., 'type': 'capability1'}, {'enabled': ..., 'type': 'capability2'}] | ||
| capability_list = if_attributes['chassis'].values()[0]['capability'] | ||
| # {'enabled': ..., 'type': 'capability'} | ||
| if not isinstance(capability_list, list): |
There was a problem hiding this comment.
capability_list [](start = 30, length = 15)
Is this case ever possible? If not, just return None.
I am worry about the blindness to error will mess parse_sys_capabilities() #Closed
There was a problem hiding this comment.
I think it's possible for a device to have only 1 supported or enabled capability. And in the same way in tests/subproc_outputs/lldpctl_mgmt_only.json we have a dictionary of 1 interface under ['lldp]['interfaces'], where for many interfaces a list would be present. It is treated in a similar fashion in parse_update() L#169. #Closed
There was a problem hiding this comment.
Could you provide proof for your thought? Either LLDPD source code or observed data. I don't see the case inside test data.
Even it is possible, I suggest test capability_list type positively instead of 'not list'.
In reply to: 203343589 [](ancestors = 203343589)
There was a problem hiding this comment.
Checked lldpd source code, looks like capabilities are formatted https://github.com/vincentbernat/lldpd/blob/f9da7943df174f048033aeca242014855f836e3b/src/client/display.c#L265
the same way interfaces are here https://github.com/vincentbernat/lldpd/blob/f9da7943df174f048033aeca242014855f836e3b/src/client/display.c#L710 . Note that both display_cap() and display_interface() use tag_start("capability')/tag_start('interface') and tag_end() for each element. As we can see from test data for interfaces we get list for multiple https://github.com/Azure/sonic-dbsyncd/blob/master/tests/subproc_outputs/lldpctl.json#L3 , and dict for one https://github.com/Azure/sonic-dbsyncd/blob/master/tests/subproc_outputs/lldpctl_mgmt_only.json#L3 .
Checking for positive type is a good thing nevertheless.
src/lldp_syncd/daemon.py
Outdated
| except subprocess.CalledProcessError: | ||
| logger.exception("lldpctl exited with non-zero status") | ||
| return None | ||
| def scrap_output(cmd): |
There was a problem hiding this comment.
scrap_output [](start = 12, length = 12)
Could you not use embedded function? suggest a name _scrap_output to indicate it is for internal use. #Closed
src/lldp_syncd/daemon.py
Outdated
| capability_list = [capability_list] | ||
| except KeyError: | ||
| logger.error("Failed to get system capabilities") | ||
| return None |
There was a problem hiding this comment.
I suggest to return here an empty list.
It is better because
- :return: list of capabilities <- None value is not a list value
- later it would be easier to work with empty list, then always check you have a list or None value
src/lldp_syncd/daemon.py
Outdated
| # if chassis is incomplete, missing capabilities | ||
| # TODO check if it does not break snmpagent | ||
| if not capability_list: | ||
| return None |
There was a problem hiding this comment.
let's return an empty string here. Below you use value of this function as it is without checking it None.
| capability_list = self.get_sys_capability_list(if_attributes) | ||
| # lldpSysCapSupported | ||
| parsed_interfaces[if_name].update({'lldp_rem_sys_cap_supported': | ||
| self.parse_sys_capabilities(capability_list)}) |
There was a problem hiding this comment.
what if self.parse_sys_capabilities returns None
There was a problem hiding this comment.
It will put None to redis db, which interprets it as "None" string. Now with your comments we return "" empty string, which is much better.
| capability_list = self.get_sys_capability_list(if_attributes) | ||
| # lldpSysCapSupported | ||
| parsed_interfaces[if_name].update({'lldp_rem_sys_cap_supported': | ||
| self.parse_sys_capabilities(capability_list)}) |
There was a problem hiding this comment.
what if self.parse_sys_capabilities returns None
| capability_list = self.get_sys_capability_list(if_attributes) | ||
| # lldpSysCapSupported | ||
| parsed_interfaces[if_name].update({'lldp_rem_sys_cap_supported': | ||
| self.parse_sys_capabilities(capability_list)}) |
There was a problem hiding this comment.
what if self.parse_sys_capabilities returns None
| capability_list = self.get_sys_capability_list(if_attributes) | ||
| # lldpSysCapSupported | ||
| parsed_interfaces[if_name].update({'lldp_rem_sys_cap_supported': | ||
| self.parse_sys_capabilities(capability_list)}) |
There was a problem hiding this comment.
what if self.parse_sys_capabilities returns None
| capability_list = self.get_sys_capability_list(if_attributes) | ||
| # lldpSysCapSupported | ||
| parsed_interfaces[if_name].update({'lldp_rem_sys_cap_supported': | ||
| self.parse_sys_capabilities(capability_list)}) |
There was a problem hiding this comment.
what if self.parse_sys_capabilities returns None
| capability_list = self.get_sys_capability_list(if_attributes) | ||
| # lldpSysCapSupported | ||
| parsed_interfaces[if_name].update({'lldp_rem_sys_cap_supported': | ||
| self.parse_sys_capabilities(capability_list)}) |
There was a problem hiding this comment.
what if self.parse_sys_capabilities returns None
| self.parse_sys_capabilities(capability_list)}) | ||
| # lldpSysCapEnabled | ||
| parsed_interfaces[if_name].update({'lldp_rem_sys_cap_enabled': | ||
| self.parse_sys_capabilities( |
There was a problem hiding this comment.
self.parse_sys_capabilities could return None here
src/lldp_syncd/daemon.py
Outdated
| :return: new, changed, deleted keys tuple | ||
| """ | ||
| new_keys = [key for key in update.keys() if key not in cache.keys()] | ||
| changed_keys = list(set([key for key in update.keys() + cache.keys() |
There was a problem hiding this comment.
You can write it without square brackets
changed_keys = list(set(key for key in update.keys() + cache.keys() if update[key] != cache.get(key)))
…sing system capabilities
src/lldp_syncd/daemon.py
Outdated
| @@ -20,6 +20,22 @@ | |||
| LLDPD_UPTIME_RE_SPLIT_PATTERN = r' days?, ' | |||
| MANAGEMENT_PORT_NAME = 'eth0' | |||
|
|
|||
| def _scrap_output(cmd): | |||
There was a problem hiding this comment.
_scrap_output [](start = 4, length = 13)
You may define this function inside LldpSyncDaemon class. #Closed
What I did:
Add support for lldpLocPortTable and lldpRemTable OIDs:
Also requires merge of sonic-net/sonic-snmpagent#70