Adding logic to update state db table during peer creation/deletion/update

Signed-off-by: Sudarshan Kumar (from Dev Box) <[email protected]>

Author: sudarshankumar4893

…emplates/dynamic/instance_update.conf.j2 …rr/bgpd/templates/dynamic/update.conf.j2dockers/docker-fpm-frr/frr/bgpd/templates/dynamic/instance_update.conf.j2 renamed to dockers/docker-fpm-frr/frr/bgpd/templates/dynamic/update.conf.j2 dockers/docker-fpm-frr/frr/bgpd/templates/dynamic/instance_update.conf.j2 renamed to dockers/docker-fpm-frr/frr/bgpd/templates/dynamic/update.conf.j2

@@ -1,5 +1,5 @@
 !
-! template: bgpd/templates/dynamic/instance_update.conf.j2
+! template: bgpd/templates/dynamic/update.conf.j2
 !
 {% for ip_range in delete_ranges %}
   no bgp listen range {{ ip_range }} peer-group {{ bgp_session['name'] }}
@@ -8,5 +8,5 @@
   bgp listen range {{ ip_range }} peer-group {{ bgp_session['name'] }}
 {% endfor %}
 !
-! end of template: bgpd/templates/dynamic/instance_update.conf.j2
+! end of template: bgpd/templates/dynamic/update.conf.j2
 !

src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py

@@ -101,12 +101,14 @@ def __init__(self, common_objs, db_name, table_name, peer_type, check_neig_meta)
         base_template = "bgpd/templates/" + self.constants["bgp"]["peers"][peer_type]["template_dir"] + "/"
         self.templates = {
             "add":         self.fabric.from_file(base_template + "instance.conf.j2"),
-            "update":      self.fabric.from_file(base_template + "instance_update.conf.j2"),
             "delete":      self.fabric.from_string('no neighbor {{ neighbor_addr }}'),
             "shutdown":    self.fabric.from_string('neighbor {{ neighbor_addr }} shutdown'),
             "no shutdown": self.fabric.from_string('no neighbor {{ neighbor_addr }} shutdown'),
         }
 
+        if (os.path.exists(self.fabric.env.loader.searchpath[0] + "/" + base_template + "update.conf.j2")):
+            self.templates["update"] = self.fabric.from_file(base_template + "update.conf.j2")
+
         deps = [
             ("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),
             ("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/type"),
@@ -229,10 +231,46 @@ def add_peer(self, vrf, nbr, data):
             self.apply_op(cmd, vrf)
             key = (vrf, nbr)
             self.peers.add(key)
+            self.update_state_db(vrf, nbr, data, "SET")
             log_info("Peer '(%s|%s)' has been scheduled to be added with attributes '%s'" % print_data)
 
         return True
 
+    def update_state_db(self, vrf, nbr, data, op):
+        """
+        Update the database with the new data
+        :param vrf: vrf name. Name is equal "default" for the global vrf
+        :param nbr: neighbor ip address (name for dynamic peer type)
+        :param data: associated data (will be empty for deletion case)
+        :param op: operation type. It can be "SET" or "DEL"
+        :return: True if this adding was successful, False otherwise
+        """
+        if (vrf == "default"):
+            key = nbr
+        else:
+            key = vrf + "|" + nbr
+        # Update the peer in the STATE_DB table
+        try:
+            state_db = swsscommon.DBConnector("STATE_DB", 0)
+            state_peer_table = swsscommon.Table(state_db, swsscommon.STATE_BGP_PEER_CONFIGURED_TABLE_NAME)
+            if (op == "SET"):
+                state_peer_table.set(key, list(sorted(data.items())))
+                log_info("Peer '(%s)' has been added to BGP_PEER_CONFIGURED_TABLE with attributes '%s'" % (key, data))
+            elif (op == "DEL"):
+                (status, fvs) = state_peer_table.get(key)
+                if status == True:
+                    state_peer_table.delete(key)
+                    log_info("Peer '(%s)' has been deleted from BGP_PEER_CONFIGURED_TABLE" % (key))
+                else:
+                    log_warn("Peer '(%s)' not found in BGP_PEER_CONFIGURED_TABLE" % (key))
+            else:
+                log_err("Update state DB called for Peer '(%s)' with unkown operation '%s'" % (key, op))
+                return False
+            return True
+        except Exception as e:
+            log_err("Update of state db failed for peer '(%s)' with error: %s" % (key, str(e)))
+            return False
+
     def update_peer(self, vrf, nbr, data):
         """
         Update a peer. This is used when the peer is already in the FRR
@@ -244,11 +282,8 @@ def update_peer(self, vrf, nbr, data):
         """
         if "admin_status" in data:
             self.change_admin_status(vrf, nbr, data)
-        elif "ip_range" in data and self.peer_type == 'dynamic':
-            if (os.path.exists(self.templates["update"])):
-                self.change_ip_range(vrf, nbr, data)
-            else:
-                log_err("Peer '(%s|%s)': Can't update the peer. Template 'update' doesn't exist" % (vrf, nbr))
+        elif "update" in self.templates and "ip_range" in data and self.peer_type == 'dynamic':
+            self.change_ip_range(vrf, nbr, data)
         else:
             log_err("Peer '(%s|%s)': Can't update the peer. Only 'admin_status' attribute is supported" % (vrf, nbr))
 
@@ -263,14 +298,14 @@ def change_admin_status(self, vrf, nbr, data):
         :return: True if this adding was successful, False otherwise
         """
         if data['admin_status'] == 'up':
-            self.apply_admin_status(vrf, nbr, "no shutdown", "up")
+            self.apply_admin_status(vrf, nbr, "no shutdown", "up", data)
         elif data['admin_status'] == 'down':
-            self.apply_admin_status(vrf, nbr, "shutdown", "down")
+            self.apply_admin_status(vrf, nbr, "shutdown", "down", data)
         else:
             print_data = vrf, nbr, data['admin_status']
             log_err("Peer '%s|%s': Can't update the peer. It has wrong attribute value attr['admin_status'] = '%s'" % print_data)
 
-    def apply_admin_status(self, vrf, nbr, template_name, admin_state):
+    def apply_admin_status(self, vrf, nbr, template_name, admin_state, data):
         """
         Render admin state template and apply the command to the FRR
         :param vrf: vrf name. Name is equal "default" for the global vrf
@@ -282,6 +317,7 @@ def apply_admin_status(self, vrf, nbr, template_name, admin_state):
         print_data = vrf, nbr, admin_state
         ret_code = self.apply_op(self.templates[template_name].render(neighbor_addr=nbr), vrf)
         if ret_code:
+            self.update_state_db(vrf, nbr, data, "SET")
             log_info("Peer '%s|%s' admin state is set to '%s'" % print_data)
         else:
             log_err("Can't set peer '%s|%s' admin state to '%s'." % print_data)
@@ -321,7 +357,7 @@ def get_existing_ip_ranges(self, vrf, nbr):
         Get existing ip range of a peer
         :param vrf: vrf name. Name is equal "default" for the global vrf
         :param nbr: neighbor ip address (name for dynamic peer type)
-        :return: existing ip range of a peer
+        :return: existing ipv4 and ipv6 ranges of a peer if they exist.
         """
         ipv4_ranges = []
         ipv6_ranges = []
@@ -334,10 +370,10 @@ def get_existing_ip_ranges(self, vrf, nbr):
             if ret_code == 0:
                 js_bgp = json.loads(out)
                 if nbr in js_bgp and 'dynamicRanges' in js_bgp[nbr] and 'IPv4' in js_bgp[nbr]['dynamicRanges'] and 'ranges' in js_bgp[nbr]['dynamicRanges']['IPv4']:
-                    ipv4_ranges= js_bgp[nbr]['dynamicRanges']['IPv4']['ranges']
+                    ipv4_ranges = js_bgp[nbr]['dynamicRanges']['IPv4']['ranges']
                     log_info("Peer '(%s|%s)' already has ipV4 range: %s" % (vrf, nbr, ipv4_ranges))
                 if nbr in js_bgp and 'dynamicRanges' in js_bgp[nbr] and 'IPv6' in js_bgp[nbr]['dynamicRanges'] and 'ranges' in js_bgp[nbr]['dynamicRanges']['IPv6']:
-                    ipv6_ranges= js_bgp[nbr]['dynamicRanges']['IPv6']['ranges']
+                    ipv6_ranges = js_bgp[nbr]['dynamicRanges']['IPv6']['ranges']
                     log_info("Peer '(%s|%s)' already has ipV6 range: %s" % (vrf, nbr, ipv6_ranges))
                 return ipv4_ranges, ipv6_ranges
             else:
@@ -357,22 +393,22 @@ def apply_range_changes(self, vrf, nbr, new_ip_range, ip_ranges_to_del, data):
         """
         print_data = vrf, nbr, data
         kwargs = {
-            'operation': 'update',
+            'CONFIG_DB__DEVICE_METADATA': self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME),
             'vrf': vrf,
-            'neighbor_addr': nbr,
             'bgp_session': data,
             'delete_ranges': ip_ranges_to_del,
             'add_ranges': new_ip_range
         }
         try:
-            cmd = self.templates["add"].render(**kwargs)
+            cmd = self.templates["update"].render(**kwargs)
         except jinja2.TemplateError as e:
             msg = "Peer '(%s|%s)'. Error in rendering the template for 'SET' command '%s'" % print_data
             log_err("%s: %s" % (msg, str(e)))
             return True
         if cmd is not None:
             self.apply_op(cmd, vrf)
-            log_info("Peer '(%s|%s)' ip range has been scheduled to be updated with new range '%s'" % (vrf, nbr, new_ip_range))
+            self.update_state_db(vrf, nbr, data, "SET")
+            log_info("Peer '(%s|%s)' ip range has been scheduled to be updated with range '%s'" % (vrf, nbr, data['ip_range']))
 
     def del_handler(self, key):
         """
@@ -387,6 +423,7 @@ def del_handler(self, key):
         cmd = self.templates["delete"].render(neighbor_addr=nbr)
         ret_code = self.apply_op(cmd, vrf)
         if ret_code:
+            self.update_state_db(vrf, nbr, {}, "DEL")
             log_info("Peer '(%s|%s)' has been removed" % (vrf, nbr))
             self.peers.remove(peer_key)
         else:

src/sonic-bgpcfgd/tests/test_bgp.py

@@ -33,7 +33,9 @@ def constructor(constants_path, bgp_router_id="", peer_type="general", with_lo0_
 
     return_value_map = {
         "['vtysh', '-H', '/dev/null', '-c', 'show bgp vrfs json']": (0, "{\"vrfs\": {\"default\": {}}}", ""),
-        "['vtysh', '-c', 'show bgp vrf default neighbors json']": (0, "{\"10.10.10.1\": {}, \"20.20.20.1\": {}, \"fc00:10::1\": {}}", "")
+        "['vtysh', '-c', 'show bgp vrf default neighbors json']": (0, "{\"10.10.10.1\": {}, \"20.20.20.1\": {}, \"fc00:10::1\": {}, \"DynNbr1\": {}, \"DynNbr2\": {}}", ""),
+        "['vtysh', '-c', 'show bgp peer-group DynNbr1 json']": (0, "{\"DynNbr1\":{\"dynamicRanges\":{\"IPv4\":{\"count\":1,\"ranges\":[\"10.255.0.0/24\"]}}}}", ""),
+        "['vtysh', '-c', 'show bgp peer-group DynNbr2 json']": (0, "{\"DynNbr2\":{\"dynamicRanges\":{\"IPv4\":{\"count\":1,\"ranges\":[\"192.168.0.0/24\",\"192.168.1.0/24\"]}}}}", "")
     }
 
     bgpcfgd.managers_bgp.run_command = lambda cmd: return_value_map[str(cmd)]
@@ -162,29 +164,66 @@ def test_add_peer_ipv6_in_vnet():
     for constant in load_constant_files():
         m = constructor(constant)
         res = m.set_handler("Vnet-10|fc00:20::1", {'asn': '65200', 'holdtime': '180', 'keepalive': '60', 'local_addr': 'fc00:20::20', 'name': 'TOR', 'nhopself': '0', 'rrclient': '0'})
-def test_add_dynamic_peer():
+
+@patch('bgpcfgd.managers_bgp.log_info')
+def test_add_dynamic_peer(mocked_log_info):
     for constant in load_constant_files():
         m = constructor(constant, peer_type="dynamic")
-        res = m.set_handler("BGPSLBPassive", {"peer_asn": "65200", "ip_range": "10.250.0.0/27"})
+        m.check_neig_meta = False
+        res = m.set_handler("BGPSLBPassive", {"peer_asn": "65200", "ip_range": "10.250.0.0/27", "name": "BGPSLBPassive", "src_address": "10.250.0.1"})
+        mocked_log_info.assert_called_with("Peer '(default|BGPSLBPassive)' has been scheduled to be added with attributes '{'peer_asn': '65200', 'ip_range': '10.250.0.0/27', 'name': 'BGPSLBPassive', 'src_address': '10.250.0.1'}'")
         assert res, "Expect True return value"
 
-def test_add_dynamic_peer_ipv6():
+@patch('bgpcfgd.managers_bgp.log_info')
+def test_add_dynamic_peer_ipv6(mocked_log_info):
     for constant in load_constant_files():
         m = constructor(constant, peer_type="dynamic")
-        res = m.set_handler("BGPSLBPassive", {"peer_asn": "65200", "ip_range": "fc00:20::/64"})
+        m.check_neig_meta = False
+        res = m.set_handler("BGPSLBPassive", {"peer_asn": "65200", "ip_range": "fc00:20::/64", "name": "BGPSLBPassive", "src_address": "fc00:20::1"})
+        mocked_log_info.assert_called_with("Peer '(default|BGPSLBPassive)' has been scheduled to be added with attributes '{'peer_asn': '65200', 'ip_range': 'fc00:20::/64', 'name': 'BGPSLBPassive', 'src_address': 'fc00:20::1'}'")
         assert res, "Expect True return value"
 
-def test_modify_dynamic_peer_range():
+@patch('bgpcfgd.managers_bgp.log_info')
+@patch('bgpcfgd.managers_bgp.swsscommon.Table')
+@patch('bgpcfgd.managers_bgp.swsscommon.DBConnector')
+def modify_dynamic_peer_common(mock_db_conn, mock_table, mocked_log_info, peer, data, expected_cmds, expected_log):
     for constant in load_constant_files():
         m = constructor(constant, peer_type="dynamic")
-        res = m.set_handler("BGPSLBPassive", {"peer_asn": "65200", "ip_range": "10.250.0.0/27"})
-        assert res, "Expect True return value"
-        res = m.set_handler("BGPSLBPassive", {"peer_asn": "65200", "ip_range": "10.250.0.0/27,10.251.0.0/27"})
-        assert res, "Expect True return value"
-        res = m.set_handler("BGPSLBPassive", {"peer_asn": "65200", "ip_range": "10.250.0.0/27,10.251.0.0/26"})
-        assert res, "Expect True return value"
-        res = m.set_handler("BGPSLBPassive", {"peer_asn": "65200", "ip_range": "10.251.0.0/27"})
+        m.cfg_mgr.push = MagicMock(return_value = None)
+        m.check_neig_meta = False
+        swsscommon.STATE_BGP_PEER_CONFIGURED_TABLE_NAME = "BGP_PEER_CONFIGURED_TABLE"
+        mock_state_db_table = MagicMock()
+        mock_table.return_value = mock_state_db_table
+        res = m.set_handler(peer, data)
         assert res, "Expect True return value"
+        mock_state_db_table.set.assert_called_once_with(peer, list(sorted(data.items())))
+        mocked_log_info.assert_called_with(expected_log)
+        m.cfg_mgr.push.assert_called_once_with(expected_cmds)
+
+def test_add_dynamic_peer_range():
+    expected_cmds = 'router bgp 65100\n bgp suppress-fib-pending\n!\n! template: bgpd/templates/dynamic/update.conf.j2\n!\n\n\n' \
+        '  bgp listen range 10.255.1.0/24 peer-group DynNbr1\n\n!\n! end of template: bgpd/templates/dynamic/update.conf.j2\n!\nexit'
+    data = {"peer_asn": "65200", "ip_range": "10.255.0.0/24,10.255.1.0/24", "name": "DynNbr1"}
+    peer = "DynNbr1"
+    expected_log = "Peer '(default|DynNbr1)' ip range has been scheduled to be updated with range '10.255.0.0/24,10.255.1.0/24'"
+    modify_dynamic_peer_common(peer=peer, data=data, expected_cmds=expected_cmds, expected_log=expected_log)
+
+def test_modify_dynamic_peer_range():
+    expected_cmds = 'router bgp 65100\n bgp suppress-fib-pending\n!\n! template: bgpd/templates/dynamic/update.conf.j2\n!\n\n' \
+        '  no bgp listen range 10.255.0.0/24 peer-group DynNbr1\n\n\n  bgp listen range 10.255.0.0/26 peer-group DynNbr1\n\n!\n!' \
+        ' end of template: bgpd/templates/dynamic/update.conf.j2\n!\nexit'
+    data = {"peer_asn": "65200", "ip_range": "10.255.0.0/26", "name": "DynNbr1"}
+    peer = "DynNbr1"
+    expected_log = "Peer '(default|DynNbr1)' ip range has been scheduled to be updated with range '10.255.0.0/26'"
+    modify_dynamic_peer_common(peer=peer, data=data, expected_cmds=expected_cmds, expected_log=expected_log)
+
+def test_delete_dynamic_peer_range():
+    expected_cmds = 'router bgp 65100\n bgp suppress-fib-pending\n!\n! template: bgpd/templates/dynamic/update.conf.j2\n!\n\n' \
+        '  no bgp listen range 192.168.1.0/24 peer-group DynNbr2\n\n\n!\n! end of template: bgpd/templates/dynamic/update.conf.j2\n!\nexit'
+    data = {"peer_asn": "65200", "ip_range": "192.168.0.0/24", "name": "DynNbr2"}
+    peer = "DynNbr2"
+    expected_log = "Peer '(default|DynNbr2)' ip range has been scheduled to be updated with range '192.168.0.0/24'"
+    modify_dynamic_peer_common(peer=peer, data=data, expected_cmds=expected_cmds, expected_log=expected_log)
 
 @patch('bgpcfgd.managers_bgp.log_warn')
 def test_add_peer_no_local_addr(mocked_log_warn):