[bgpcfgd]: Convert bgpcfgd and bgpmon to python3 (#5746)

* Convert bgpcfgd to python3

Convert bgpmon to python3
Fix some issues in bgpmon

* Add python3-swsscommon as depends

* Install dependencies

* reorder deps

Co-authored-by: Pavel Shirshov <pavel.contrib@gmail.com>

dockers/docker-fpm-frr/frr/bgpd/templates/general/instance.conf.j2

@@ -9,7 +9,7 @@
   neighbor {{ neighbor_addr }} timers {{ bgp_session['keepalive'] | default("60") }} {{ bgp_session['holdtime'] | default("180") }}
 {% endif %}
 !
-{% if bgp_session.has_key('admin_status') and bgp_session['admin_status'] == 'down' or not bgp_session.has_key('admin_status') and CONFIG_DB__DEVICE_METADATA['localhost'].has_key('default_bgp_status') and CONFIG_DB__DEVICE_METADATA['localhost']['default_bgp_status'] == 'down' %}
+{% if 'admin_status' in bgp_session and bgp_session['admin_status'] == 'down' or 'admin_status' not in bgp_session and 'default_bgp_status' in CONFIG_DB__DEVICE_METADATA['localhost'] and CONFIG_DB__DEVICE_METADATA['localhost']['default_bgp_status'] == 'down' %}
   neighbor {{ neighbor_addr }} shutdown
 {% endif %}
 !
@@ -23,11 +23,11 @@
 !
 {% endif %}
 !
-{%   if bgp_session.has_key('rrclient') and bgp_session['rrclient'] | int != 0 %}
+{%   if 'rrclient' in bgp_session and bgp_session['rrclient'] | int != 0 %}
     neighbor {{ neighbor_addr }} route-reflector-client
 {%   endif %}
 !
-{%   if bgp_session.has_key('nhopself') and bgp_session['nhopself'] | int != 0 %}
+{%   if 'nhopself' in bgp_session and bgp_session['nhopself'] | int != 0 %}
     neighbor {{ neighbor_addr }} next-hop-self
 {%   endif %}
 !

rules/docker-config-engine-buster.mk

@@ -3,6 +3,7 @@
 DOCKER_CONFIG_ENGINE_BUSTER = docker-config-engine-buster.gz
 $(DOCKER_CONFIG_ENGINE_BUSTER)_PATH = $(DOCKERS_PATH)/docker-config-engine-buster
 
+$(DOCKER_CONFIG_ENGINE_BUSTER)_DEPENDS += $(LIBSWSSCOMMON) $(PYTHON3_SWSSCOMMON)
 $(DOCKER_CONFIG_ENGINE_BUSTER)_PYTHON_WHEELS += $(SWSSSDK_PY2)
 $(DOCKER_CONFIG_ENGINE_BUSTER)_PYTHON_WHEELS += $(SWSSSDK_PY3)
 $(DOCKER_CONFIG_ENGINE_BUSTER)_PYTHON_WHEELS += $(SONIC_PY_COMMON_PY2)

rules/sonic_bgpcfgd.mk

@@ -1,11 +1,13 @@
 # sonic-bgpcfgd package
 
-SONIC_BGPCFGD = sonic_bgpcfgd-1.0-py2-none-any.whl
+SONIC_BGPCFGD = sonic_bgpcfgd-1.0-py3-none-any.whl
 $(SONIC_BGPCFGD)_SRC_PATH = $(SRC_PATH)/sonic-bgpcfgd
-# These dependencies are only needed becuase they are dependencies
+# These dependencies are only needed because they are dependencies
 # of sonic-config-engine and bgpcfgd explicitly calls sonic-cfggen
 # as part of its unit tests.
 # TODO: Refactor unit tests so that these dependencies are not needed
+
 $(SONIC_BGPCFGD)_DEPENDS += $(SONIC_PY_COMMON_PY2)
-$(SONIC_BGPCFGD)_PYTHON_VERSION = 2
+$(SONIC_BGPCFGD)_DEBS_DEPENDS += $(PYTHON3_SWSSCOMMON)
+$(SONIC_BGPCFGD)_PYTHON_VERSION = 3
 SONIC_PYTHON_WHEELS += $(SONIC_BGPCFGD)

src/sonic-bgpcfgd/.gitignore

@@ -7,3 +7,6 @@ tests/*.pyc
 tests/__pycache__/
 .idea
 .coverage
+bgpcfgd/__pycache__/
+venv
+tests/.coverage*

src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py

@@ -308,7 +308,7 @@ class BGPPeerMgrBase(Manager):
         :return: ipv4 address for Loopback0, None if nothing found
         """
         loopback0_ipv4 = None
-        for loopback in self.directory.get_slot("CONFIG_DB", swsscommon.CFG_LOOPBACK_INTERFACE_TABLE_NAME).iterkeys():
+        for loopback in self.directory.get_slot("CONFIG_DB", swsscommon.CFG_LOOPBACK_INTERFACE_TABLE_NAME).keys():
             if loopback.startswith("Loopback0|"):
                 loopback0_prefix_str = loopback.replace("Loopback0|", "")
                 loopback0_ip_str = loopback0_prefix_str[:loopback0_prefix_str.find('/')]
@@ -333,7 +333,7 @@ class BGPPeerMgrBase(Manager):
         local_address = local_addresses[local_addr]
         interfaces = self.directory.get_slot("LOCAL", "interfaces")
         # Check if the information for the interface of this local address has been set
-        if local_address.has_key("interface") and local_address["interface"] in interfaces:
+        if "interface" in local_address and local_address["interface"] in interfaces:
             return interfaces[local_address["interface"]]
         else:
             return None
@@ -346,7 +346,7 @@ class BGPPeerMgrBase(Manager):
         :return: Return the vnet name of the interface if this interface belongs to a vnet,
                  Otherwise return None
         """
-        if interface.has_key("vnet_name") and interface["vnet_name"]:
+        if "vnet_name" in interface and interface["vnet_name"]:
             return interface["vnet_name"]
         else:
             return None

src/sonic-bgpcfgd/bgpcfgd/utils.py

@@ -16,7 +16,7 @@ def run_command(command, shell=False, hide_errors=False):
     :return: Tuple: integer exit code from the command, stdout as a string, stderr as a string
     """
     log_debug("execute command '%s'." % str(command))
-    p = subprocess.Popen(command, shell=shell, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    p = subprocess.Popen(command, shell=shell, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding='utf-8')
     stdout, stderr = p.communicate()
     if p.returncode != 0:
         if not hide_errors:

src/sonic-bgpcfgd/bgpmon/bgpmon.py

@@ -1,4 +1,4 @@
-#!/usr/bin/env python2
+#!/usr/bin/env python3
 
 """"
 Description: bgpmon.py -- populating bgp related information in stateDB.
@@ -6,7 +6,7 @@ Description: bgpmon.py -- populating bgp related information in stateDB.
 
     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
+    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.
 
@@ -23,7 +23,7 @@ Description: bgpmon.py -- populating bgp related information in stateDB.
     is a need to perform update or the peer is stale to be removed from the
     state DB
 """
-import commands
+import subprocess
 import json
 import os
 import syslog
@@ -32,7 +32,7 @@ import time
 
 PIPE_BATCH_MAX_COUNT = 50
 
-class BgpStateGet():
+class BgpStateGet:
     def __init__(self):
         # list peer_l stores the Neighbor peer Ip address
         # dic peer_state stores the Neighbor peer state entries
@@ -74,13 +74,13 @@ class BgpStateGet():
     # Get a new snapshot of BGP neighbors and store them in the "new" location
     def get_all_neigh_states(self):
         cmd = "vtysh -c 'show bgp summary json'"
-        rc, output = commands.getstatusoutput(cmd)
+        rc, output = subprocess.getstatusoutput(cmd)
         if rc:
             syslog.syslog(syslog.LOG_ERR, "*ERROR* Failed with rc:{} when execute: {}".format(rc, cmd))
             return
 
         peer_info = json.loads(output)
-        # cmd ran successfully, safe to Clean the "new" lists/dic for new sanpshot
+        # cmd ran successfully, safe to Clean the "new" lists/dic for new snapshot
         del self.new_peer_l[:]
         self.new_peer_state.clear()
         for key, value in peer_info.items():
@@ -136,7 +136,7 @@ class BgpStateGet():
                 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
+            # remove this from the stateDB and the current neighbor state entry
             peer = self.peer_l.pop(0)
             del_key = "NEIGH_STATE_TABLE|%s" % peer
             data[del_key] = None
@@ -151,15 +151,15 @@ class BgpStateGet():
 
 def main():
 
-    print "bgpmon service started"
-
+    syslog.syslog(syslog.LOG_INFO, "bgpmon service started")
+    bgp_state_get = None
     try:
         bgp_state_get = BgpStateGet()
     except Exception as e:
-        syslog.syslog(syslog.LOG_ERR, "{}: error exit 1, reason {}".format(THIS_MODULE, str(e)))
+        syslog.syslog(syslog.LOG_ERR, "{}: error exit 1, reason {}".format("THIS_MODULE", str(e)))
         exit(1)
 
-    # periodically obtain the new neighbor infomraton and update if necessary
+    # periodically obtain the new neighbor information and update if necessary
     while True:
         time.sleep(15)
         if bgp_state_get.bgp_activity_detected():

src/sonic-bgpcfgd/tests/swsscommon_test.py

@@ -1,4 +1,4 @@
-from mock import MagicMock
+from unittest.mock import MagicMock
 
 
 swsscommon = MagicMock(CFG_DEVICE_METADATA_TABLE_NAME = "DEVICE_METADATA")

src/sonic-bgpcfgd/tests/test_allow_list.py

@@ -1,7 +1,8 @@
+from unittest.mock import MagicMock, patch
+
 from bgpcfgd.directory import Directory
 from bgpcfgd.template import TemplateFabric
 import bgpcfgd
-from mock import MagicMock, patch
 
 
 swsscommon_module_mock = MagicMock()
@@ -429,7 +430,7 @@ def test___find_peer_group_by_deployment_id():
     }
     mgr = BGPAllowListMgr(common_objs, "CONFIG_DB", "BGP_ALLOWED_PREFIXES")
     values = mgr._BGPAllowListMgr__find_peer_group_by_deployment_id(0)
-    assert values == ['PEER_V4_INT', 'PEER_V6_INT', 'PEER_V6', 'PEER_V4']
+    assert set(values) == set(['PEER_V4_INT', 'PEER_V6_INT', 'PEER_V6', 'PEER_V4'])
 
 @patch.dict("sys.modules", swsscommon=swsscommon_module_mock)
 def test___restart_peers_found_deployment_id():

src/sonic-bgpcfgd/tests/test_bbr.py

@@ -1,10 +1,12 @@
+from unittest.mock import MagicMock, patch
+
 from bgpcfgd.directory import Directory
 from bgpcfgd.template import TemplateFabric
-from mock import MagicMock, patch
 from copy import deepcopy
-import swsscommon_test
+from . import swsscommon_test
 import bgpcfgd
 
+
 with patch.dict("sys.modules", swsscommon=swsscommon_test):
     from bgpcfgd.managers_bbr import BBRMgr
 

src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py

@@ -19,7 +19,7 @@ def parse_instance_conf(filename):
             if TemplateFabric.is_ipv6(neighbor):
                 neighbors[neighbor] = {}
     # Extract peer-groups and route-maps
-    for neighbor, neighbor_data in neighbors.iteritems():
+    for neighbor, neighbor_data in neighbors.items():
         route_map_in_re = re.compile(r'^neighbor\s+%s\s+route-map\s+(\S+) in$' % neighbor)
         peer_group_re = re.compile(r'^neighbor\s+%s\s+peer-group\s+(\S+)$' % neighbor)
         for line in lines:
@@ -30,7 +30,7 @@ def parse_instance_conf(filename):
                 assert "peer-group" not in neighbor_data
                 neighbor_data["peer-group"] = peer_group_re.match(line).group(1)
     # Ensure that every ivp6 neighbor has either route-map or peer-group
-    for neighbor, neighbor_data in neighbors.iteritems():
+    for neighbor, neighbor_data in neighbors.items():
         assert "route-map" in neighbor_data or "peer-group" in neighbor_data,\
             "IPv6 neighbor '%s' must have either route-map in or peer-group %s" % (neighbor, neighbor_data)
     return neighbors

src/sonic-bgpcfgd/tests/test_pfx_filter.py

@@ -42,7 +42,7 @@ def test_pfx_filter_mixed_keys():
         ]
     )
     res = TemplateFabric.pfx_filter(src)
-    assert res == expected
+    assert dict(res) == dict(expected)
 
 
 def test_pfx_filter_pfx_v4_w_mask():
@@ -57,7 +57,7 @@ def test_pfx_filter_pfx_v4_w_mask():
         ]
     )
     res = TemplateFabric.pfx_filter(src)
-    assert res == expected
+    assert dict(res) == dict(expected)
 
 def test_pfx_filter_pfx_v6_w_mask():
     src = {
@@ -85,7 +85,7 @@ def test_pfx_filter_pfx_v4_no_mask():
         ]
     )
     res = TemplateFabric.pfx_filter(src)
-    assert res == expected
+    assert dict(res) == dict(expected)
 
 def test_pfx_filter_pfx_v6_no_mask():
     src = {
@@ -126,7 +126,7 @@ def test_pfx_filter_pfx_comprehensive():
         ]
     )
     res = TemplateFabric.pfx_filter(src)
-    assert res == expected
+    assert dict(res) == dict(expected)
 
 @pytest.fixture
 def test_pfx_filter_wrong_ip(caplog):

src/sonic-bgpcfgd/tests/test_sonic-cfggen.py

@@ -1,9 +1,7 @@
 import os
 import subprocess
 
-
 from bgpcfgd.config import ConfigMgr
-from .test_templates import compress_comments, write_result
 
 
 TEMPLATE_PATH = os.path.abspath('../../dockers/docker-fpm-frr/frr')
@@ -15,11 +13,11 @@ def run_test(name, template_path, json_path, match_path):
     template_path = os.path.join(TEMPLATE_PATH, template_path)
     json_path = os.path.join(DATA_PATH, json_path)
     cfggen = os.path.abspath("../sonic-config-engine/sonic-cfggen")
-    command = [cfggen, "-T", TEMPLATE_PATH, "-t", template_path, "-y", json_path]
-    p = subprocess.Popen(command, shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    command = ['/usr/bin/python2.7', cfggen, "-T", TEMPLATE_PATH, "-t", template_path, "-y", json_path]
+    p = subprocess.Popen(command, shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env={"PYTHONPATH": "."})
     stdout, stderr = p.communicate()
     assert p.returncode == 0, "sonic-cfggen for %s test returned %d code. stderr='%s'" % (name, p.returncode, stderr)
-    raw_generated_result = stdout
+    raw_generated_result = stdout.decode("ascii")
     assert "None" not in raw_generated_result, "Test %s" % name
     canonical_generated_result = ConfigMgr.to_canonical(raw_generated_result)
     match_path = os.path.join(DATA_PATH, match_path)

src/sonic-bgpcfgd/tests/test_templates.py

@@ -6,6 +6,7 @@ from bgpcfgd.template import TemplateFabric
 from bgpcfgd.config import ConfigMgr
 from .util import load_constants
 
+
 TEMPLATE_PATH = os.path.abspath('../../dockers/docker-fpm-frr/frr')