From 869b3bc415e963ea4839e97596e1d226ae70358f Mon Sep 17 00:00:00 2001 From: bingwang-ms <66248323+bingwang-ms@users.noreply.github.com> Date: Wed, 27 Jan 2021 15:01:42 +0800 Subject: [PATCH] [bgpmon]: Fix exception in bgpmon caused by duplicate bgp neighbor ID (#6546) * Fix exception in bgpmon caused by duplicate keys It is possible that BGP neighbors in IPv4 and IPv6 address families share the same name (such as bgp monitor). However, such case is not handled in bgpmon, and an Exception will be raised. This commit will address the issue by Using set instead of list to avoid duplicate keys. --- src/sonic-bgpcfgd/bgpmon/bgpmon.py | 45 +++++++++++++++--------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpmon/bgpmon.py b/src/sonic-bgpcfgd/bgpmon/bgpmon.py index 74a1a2b4d0..07b37b8af6 100644 --- a/src/sonic-bgpcfgd/bgpmon/bgpmon.py +++ b/src/sonic-bgpcfgd/bgpmon/bgpmon.py @@ -3,19 +3,19 @@ """" Description: bgpmon.py -- populating bgp related information in stateDB. script is started by supervisord in bgp docker when the docker is started. - Initial creation of this daemon is to assist SNMP agent in obtaining the + Initial creation of this daemon is to assist SNMP agent in obtaining the BGP related information for its MIB support. The MIB that this daemon is - assiting is for the CiscoBgp4MIB (Neighbor state only). If there are other - BGP related items that needs to be updated in a periodic manner in the + assisting is for the CiscoBgp4MIB (Neighbor state only). If there are other + BGP related items that needs to be updated in a periodic manner in the future, then more can be added into this process. The script check if there are any bgp activities by monitoring the bgp frr.log file timestamp. If activity is detected, then it will request bgp - neighbor state via vtysh cli interface. This bgp activity monitoring is + neighbor state via vtysh cli interface. This bgp activity monitoring is done periodically (every 15 second). When triggered, it looks specifically for the neighbor state in the json output of show ip bgp neighbors json and update the state DB for each neighbor accordingly. In order to not disturb and hold on to the State DB access too long and - removal of the stale neighbors (neighbors that was there previously on + removal of the stale neighbors (neighbors that was there previously on previous get request but no longer there in the current get request), a "previous" neighbor dictionary will be kept and used to determine if there is a need to perform update or the peer is stale to be removed from the @@ -32,13 +32,13 @@ PIPE_BATCH_MAX_COUNT = 50 class BgpStateGet(): def __init__(self): - # list peer_l stores the Neighbor peer Ip address + # set peer_l stores the Neighbor peer Ip address # dic peer_state stores the Neighbor peer state entries - # list new_peer_l stores the new snapshot of Neighbor peer ip address + # set new_peer_l stores the new snapshot of Neighbor peer ip address # dic new_peer_state stores the new snapshot of Neighbor peer states - self.peer_l = [] + self.peer_l = set() self.peer_state = {} - self.new_peer_l = [] + self.new_peer_l = set() self.new_peer_state = {} self.cached_timestamp = 0 self.db = swsssdk.SonicV2Connector() @@ -65,9 +65,9 @@ class BgpStateGet(): def update_new_peer_states(self, peer_dict): peer_l = peer_dict["peers"].keys() - self.new_peer_l.extend(peer_l) - for i in range (0, len(peer_l)): - self.new_peer_state[peer_l[i]] = peer_dict["peers"][peer_l[i]]["state"] + self.new_peer_l.update(peer_l) + for peer in peer_l: + self.new_peer_state[peer] = peer_dict["peers"][peer]["state"] # Get a new snapshot of BGP neighbors and store them in the "new" location def get_all_neigh_states(self): @@ -78,8 +78,8 @@ class BgpStateGet(): return peer_info = json.loads(output) - # cmd ran successfully, safe to Clean the "new" lists/dic for new sanpshot - del self.new_peer_l[:] + # cmd ran successfully, safe to Clean the "new" set/dict for new snapshot + self.new_peer_l.clear() self.new_peer_state.clear() for key, value in peer_info.items(): if key == "ipv4Unicast" or key == "ipv6Unicast": @@ -113,8 +113,7 @@ class BgpStateGet(): def update_neigh_states(self): data = {} - for i in range (0, len(self.new_peer_l)): - peer = self.new_peer_l[i] + for peer in self.new_peer_l: key = "NEIGH_STATE_TABLE|%s" % peer if peer in self.peer_l: # only update the entry if state changed @@ -123,7 +122,7 @@ class BgpStateGet(): state = self.new_peer_state[peer] data[key] = {'state':state} self.peer_state[peer] = state - # remove this neighbor from old list since it is accounted for + # remove this neighbor from old set since it is accounted for self.peer_l.remove(peer) else: # New neighbor found case. Add to dictionary and state DB @@ -133,19 +132,19 @@ class BgpStateGet(): if len(data) > PIPE_BATCH_MAX_COUNT: self.flush_pipe(data) # Check for stale state entries to be cleaned up - while len(self.peer_l) > 0: - # remove this from the stateDB and the current nighbor state entry - peer = self.peer_l.pop(0) + for peer in self.peer_l: + # remove this from the stateDB and the current neighbor state entry del_key = "NEIGH_STATE_TABLE|%s" % peer data[del_key] = None - del self.peer_state[peer] + if peer in self.peer_state: + del self.peer_state[peer] if len(data) > PIPE_BATCH_MAX_COUNT: self.flush_pipe(data) # If anything in the pipeline not yet flushed, flush them now if len(data) > 0: self.flush_pipe(data) - # Save the new List - self.peer_l = self.new_peer_l[:] + # Save the new set + self.peer_l = self.new_peer_l.copy() def main():