[lldp] add lldpRemManAddrTable & lldpLocManAddrTable #72
Conversation
| lldp_loc_sys_cap_enabled = 6 | ||
|
|
||
|
|
||
| class ManAddrConst(Enum): |
There was a problem hiding this comment.
Enum [](start = 19, length = 4)
Why oid is a Enum member? #Closed
There was a problem hiding this comment.
This OID is one of the return values, used by lldpLocManAddrTable and lldpRemManAddrTable. The whole ManAddrConst enum contains hardcoded return values. It's purpose is to avoid hardcoding it in 2 places and increase readability. #Closed
There was a problem hiding this comment.
You could remove 'Enum' and make code cleaner. Mixing different types of constants are not like an enum.
In reply to: 205701552 [](ancestors = 205701552)
| msg = pubsub.get_message() | ||
|
|
||
| if not msg: | ||
| return None, None, None |
There was a problem hiding this comment.
None, None, None [](start = 15, length = 16)
ret = None, None, None
so you can reuse. #Closed
| return None, None, None | ||
|
|
||
| lldp_entry = msg["channel"].split(b":")[-1].decode() | ||
| data = msg['data'] |
There was a problem hiding this comment.
Handle many exception cases? #Closed
|
|
||
| def table_lookup_integer(self, table_name): | ||
| subtype_str = self.table_lookup(table_name) | ||
| return int(subtype_str) if subtype_str is not None else None |
There was a problem hiding this comment.
None [](start = 64, length = 4)
None is not an int. Is there a safe fallback value? #Closed
There was a problem hiding this comment.
The function is used to get ChassisIdSubtype, which is expected to be 4(mac address). I could hardcode it here, but i don't see the point.
Returning None here will result in END_OF_MIB, which is not that bad. If something is missing from LOC_CHASSIS table, table_lookup() has a log for that.
What is the point of having fallback value for ChassisIdSubtype, if probably will also be missing the ChassisId itself? They are provided by the same flow. Looking up ChassisId will return the same None if misssing.
There was a problem hiding this comment.
@qiluo-msft do you insist on more changes or agree with my previous comment? #Resolved
| @@ -109,34 +196,54 @@ def update_data(self): | |||
| redis_client = self.db_conn.get_redis_client(self.db_conn.APPL_DB) | |||
| db = self.db_conn.db_map[self.db_conn.APPL_DB]["db"] | |||
| self.pubsub = redis_client.pubsub() | |||
| self.pubsub.psubscribe("__keyspace@{}__:{}".format(db, "LLDP_ENTRY_TABLE:*")) | |||
| self.pubsub.psubscribe("__keyspace@{}__:{}".format(db, mibs.lldp_entry_table(b'Ethernet*'))) | |||
There was a problem hiding this comment.
Ethernet [](start = 91, length = 8)
You add assumption of port name. I think original one is more accurate. #Closed
There was a problem hiding this comment.
I added it to avoid spam of log message about 'eth0' not being a valid interface name. In the end, i just changed dbsyncd to not submit LLDP_ENTRY:eth0 to DB, as it is never used anyway.
There was a problem hiding this comment.
| # get interface from interface name | ||
| if_index = port_util.get_index_from_str(interface) | ||
| def local_port_num(self, sub_id): | ||
| if len(sub_id) <= 0: |
There was a problem hiding this comment.
<= [](start = 23, length = 2)
== #Closed
| .format(lldp_entry)) | ||
| continue | ||
| def local_port_id(self, sub_id): | ||
| if len(sub_id) <= 0: |
There was a problem hiding this comment.
<= [](start = 23, length = 2)
== #Closed
| if b"set" in data: | ||
| self.update_interface_data(interface.encode('utf-8')) | ||
| def port_table_lookup(self, sub_id, table_name): | ||
| if len(sub_id) <= 0: |
There was a problem hiding this comment.
<= [](start = 23, length = 2)
== #Closed
What I did:
Also requires sonic-net/sonic-dbsyncd#8
lldpRemManAddrTable
lldpLocManAddrTable