Merge "Refactor ensure_routing_table_for_bridge"

ovn_bgp_agent/constants.py

@@ -109,3 +109,8 @@ ROUTE_DISCARD = 'discard'
 # Family constants
 AF_INET = socket.AF_INET
 AF_INET6 = socket.AF_INET6
+
+# Path to file containing routing tables
+ROUTING_TABLES_FILE = '/etc/iproute2/rt_tables'
+ROUTING_TABLE_MIN = 1
+ROUTING_TABLE_MAX = 252

ovn_bgp_agent/tests/unit/utils/test_linux_net.py

@@ -184,12 +184,6 @@ class TestLinuxNet(test_base.TestCase):
         mock_ndp.assert_called_once_with('fake-bridge')
         mock_arp.assert_called_once_with('fake-bridge')
 
-    def test_ensure_routing_table_for_bridge(self):
-        # TODO(lucasagomes): This method is massive and complex, perhaps
-        #  break it into helper methods for both readibility and maintenance
-        #  of it.
-        pass
-
     @mock.patch.object(linux_net, 'enable_proxy_arp')
     @mock.patch.object(linux_net, 'enable_proxy_ndp')
     @mock.patch(
@@ -780,3 +774,78 @@ class TestLinuxNet(test_base.TestCase):
 
         self.assertEqual({self.dev: []}, routes)
         mock_route_delete.assert_called_once_with(route)
+
+
+class TestEnsureRoutingTableForBridge(test_base.TestCase):
+    def setUp(self):
+        super().setUp()
+        self.ovn_routing_tables = {}
+        self.bridge_name = "br-test"
+        self.vrf_table = 4
+        self.generated_number = 2
+
+        self.testing_multiline_file_content = [
+            "1 foo",
+            "# commented line",
+            "random garbage text",
+            "3	another bridge",
+            "2",
+        ]
+
+        self.m_ensure_rt_routes = mock.patch.object(
+            linux_net, '_ensure_routing_table_routes').start()
+
+        # The 'random' generator will always generate number 2 because the
+        # range is 1-4 while 1 and 3 are used in the file and 4 is the vrf
+        mock.patch.object(constants, 'ROUTING_TABLE_MIN', 1).start()
+        mock.patch.object(constants, 'ROUTING_TABLE_MAX', 4).start()
+
+    def _create_fake_file_content(self):
+        return "\n".join(self.testing_multiline_file_content)
+
+    def test_ensure_routing_table_for_bridge_table_missing(self):
+        self._test_ensure_routing_table_for_bridge_table_missing()
+
+    def _test_ensure_routing_table_for_bridge_table_missing(self):
+        with mock.patch(
+                'builtins.open',
+                mock.mock_open(read_data=self._create_fake_file_content())):
+            linux_net.ensure_routing_table_for_bridge(
+                self.ovn_routing_tables, self.bridge_name, self.vrf_table)
+
+        self.assertDictEqual(
+            {self.bridge_name: self.generated_number}, self.ovn_routing_tables)
+
+    def test_ensure_routing_table_for_bridge_table_present(self):
+        present_bridge_value = 5
+        self.testing_multiline_file_content.insert(
+            2, "%d %s" % (present_bridge_value, self.bridge_name))
+
+        with mock.patch(
+                'builtins.open',
+                mock.mock_open(read_data=self._create_fake_file_content())):
+            linux_net.ensure_routing_table_for_bridge(
+                self.ovn_routing_tables, self.bridge_name, self.vrf_table)
+
+        self.assertDictEqual(
+            {self.bridge_name: present_bridge_value}, self.ovn_routing_tables)
+
+    def test_ensure_routing_table_for_bridge_table_vrf_not_generated(self):
+        self.vrf_table = 2
+        self.generated_number = 4
+        self._test_ensure_routing_table_for_bridge_table_missing()
+
+    def test_ensure_routing_table_for_bridge_tables_depleted(self):
+        present_bridge_value = 2
+        self.testing_multiline_file_content.insert(
+            2, "%d %s" % (present_bridge_value, "foo"))
+
+        with mock.patch(
+                'builtins.open',
+                mock.mock_open(read_data=self._create_fake_file_content())):
+            self.assertRaises(
+                SystemExit,
+                linux_net.ensure_routing_table_for_bridge,
+                self.ovn_routing_tables, self.bridge_name, self.vrf_table)
+
+        self.assertDictEqual({}, self.ovn_routing_tables)

ovn_bgp_agent/utils/linux_net.py

@@ -30,6 +30,8 @@ from ovn_bgp_agent.utils import common as common_utils
 
 LOG = logging.getLogger(__name__)
 
+RE_TABLE_ROW = re.compile(r"^(?P<table>[0-9]+)\s+(?P<bridge>\S+)")
+
 
 def get_ip_version(ip):
     # IP network can consume both an IP address and a network with cidr
@@ -157,38 +159,39 @@ def ensure_arp_ndp_enabled_for_bridge(bridge, offset, vlan_tag=None):
 def ensure_routing_table_for_bridge(ovn_routing_tables, bridge, vrf_table):
     # check a routing table with the bridge name exists on
     # /etc/iproute2/rt_tables
-    regex = r'^[0-9]*[\s]*{}$'.format(bridge)
-    matching_table = [line.replace('\t', ' ')
-                      for line in open('/etc/iproute2/rt_tables')
-                      if re.findall(regex, line)]
-    if matching_table:
-        table_info = matching_table[0].strip().split()
-        ovn_routing_tables[table_info[1]] = int(table_info[0])
-        LOG.debug("Found routing table for %s with: %s", bridge,
-                  table_info)
-    # if not configured, add random number for the table
-    else:
-        LOG.debug("Routing table for bridge %s not configured "
-                  "at /etc/iproute2/rt_tables", bridge)
-        regex = r'^[0-9]+[\s]*'
-        existing_routes = [int(line.replace('\t', ' ').split(' ')[0])
-                           for line in open('/etc/iproute2/rt_tables')
-                           if re.findall(regex, line)]
-        # pick a number between 1 and 252
-        try:
-            table_number = random.choice(list(
-                {x for x in range(1, 253) if x != int(vrf_table)}.difference(
-                    set(existing_routes))))
-        except IndexError:
-            LOG.error("No more routing tables available for bridge %s "
-                      "at /etc/iproute2/rt_tables", bridge)
-            sys.exit()
+    found_tables = {vrf_table}
 
-        ovn_bgp_agent.privileged.linux_net.create_routing_table_for_bridge(
-            table_number, bridge)
-        ovn_routing_tables[bridge] = int(table_number)
-        LOG.debug("Added routing table for %s with number: %s", bridge,
-                  table_number)
+    with open(constants.ROUTING_TABLES_FILE, 'r') as rt_file:
+        for line in rt_file.readlines():
+            match = RE_TABLE_ROW.match(line)
+            if match:
+                if match.group('bridge') == bridge:
+                    # We don't need to catch exception for TypeError because
+                    # the regular expression matches only integers
+                    ovn_routing_tables[match.group('bridge')] = int(
+                        match.group('table'))
+                    LOG.debug("Found routing table for %s with: %s",
+                              bridge, match.group('table'))
+                    break
+                else:
+                    found_tables.add(int(match.group('table')))
+        else:
+            LOG.debug("Routing table for bridge %s not configured at ", bridge)
+            try:
+                routing_table_range = set(
+                    range(constants.ROUTING_TABLE_MIN,
+                          constants.ROUTING_TABLE_MAX + 1))
+                table_number = random.choice(
+                    list(routing_table_range - found_tables))
+            except IndexError:
+                LOG.error("No more routing tables available for bridge %s "
+                          "at %s", constants.ROUTING_TABLES_FILE, bridge)
+                sys.exit(1)
+            ovn_bgp_agent.privileged.linux_net.create_routing_table_for_bridge(
+                table_number, bridge)
+            ovn_routing_tables[bridge] = int(table_number)
+            LOG.debug("Added routing table for %s with number: %s",
+                      bridge, table_number)
 
     return _ensure_routing_table_routes(ovn_routing_tables, bridge)