Check for routers correctness

This patch is updating networking-ovn to check for correctness when
creating, updating or deleting routers.

Partial-Bug: #1605089
Change-Id: Ida7e6100071a1e484927ca9638702e438918937a

networking_ovn/common/constants.py

@@ -98,3 +98,4 @@ INITIAL_REV_NUM = -1
 TYPE_NETWORKS = 'networks'
 TYPE_PORTS = 'ports'
 TYPE_SECURITY_GROUP_RULES = 'security_group_rules'
+TYPE_ROUTERS = 'routers'

networking_ovn/common/maintenance.py

@@ -180,6 +180,39 @@ class DBInconsistenciesPeriodics(object):
             LOG.error("SG rule %s found with a revision number while this "
                       "resource doesn't support updates.", row.resource_uuid)
 
+    def _fix_create_update_routers(self, row):
+        # Get the latest version of the resource in Neutron DB
+        admin_context = n_context.get_admin_context()
+        r_db_obj = self._ovn_client._l3_plugin.get_router(
+            admin_context, row.resource_uuid)
+        ovn_router = self._nb_idl.get_lrouter(
+            utils.ovn_name(row.resource_uuid))
+
+        if not ovn_router:
+            # If the resource doesn't exist in the OVN DB, create it.
+            self._ovn_client.create_router(r_db_obj)
+        else:
+            ext_ids = getattr(ovn_router, 'external_ids', {})
+            ovn_revision = int(ext_ids.get(
+                ovn_const.OVN_REV_NUM_EXT_ID_KEY, -1))
+            # If the resource exist in the OVN DB but the revision
+            # number is different from Neutron DB, updated it.
+            if ovn_revision != r_db_obj['revision_number']:
+                self._ovn_client.update_router(r_db_obj)
+            else:
+                # If the resource exist and the revision number
+                # is equal on both databases just bump the revision on
+                # the cache table.
+                db_rev.bump_revision(r_db_obj, ovn_const.TYPE_ROUTERS)
+
+    def _fix_delete_router(self, row):
+        ovn_router = self._nb_idl.get_lrouter(
+            utils.ovn_name(row.resource_uuid))
+        if not ovn_router:
+            db_rev.delete_revision(row.resource_uuid)
+        else:
+            self._ovn_client.delete_router(row.resource_uuid)
+
     @periodics.periodic(spacing=DB_CONSISTENCY_CHECK_INTERVAL,
                         run_immediately=True)
     def check_for_inconsistencies(self):
@@ -203,6 +236,8 @@ class DBInconsistenciesPeriodics(object):
                     self._fix_create_update_port(row)
                 elif row.resource_type == ovn_const.TYPE_SECURITY_GROUP_RULES:
                     self._fix_create_sg_rule(row)
+                elif row.resource_type == ovn_const.TYPE_ROUTERS:
+                    self._fix_create_update_routers(row)
             except Exception:
                 LOG.exception('Failed to fix resource %(res_uuid)s '
                               '(type: %(res_type)s)',
@@ -218,6 +253,8 @@ class DBInconsistenciesPeriodics(object):
                     self._fix_delete_port(row)
                 elif row.resource_type == ovn_const.TYPE_SECURITY_GROUP_RULES:
                     self._fix_delete_sg_rule(row)
+                elif row.resource_type == ovn_const.TYPE_ROUTERS:
+                    self._fix_delete_router(row)
             except Exception:
                 LOG.exception('Failed to fix deleted resource %(res_uuid)s '
                               '(type: %(res_type)s)',

networking_ovn/common/ovn_client.py

@@ -839,7 +839,9 @@ class OVNClient(object):
             ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY:
                 router.get('name', 'no_router_name'),
             ovn_const.OVN_GW_PORT_EXT_ID_KEY:
-                router.get('gw_port_id') or ''}
+                router.get('gw_port_id') or '',
+            ovn_const.OVN_REV_NUM_EXT_ID_KEY: str(utils.get_revision_number(
+                router, ovn_const.TYPE_ROUTERS))}
 
     def create_router(self, router, add_external_gateway=True):
         """Create a logical router."""
@@ -860,6 +862,7 @@ class OVNClient(object):
                     context, router['id'])
                 if router.get(l3.EXTERNAL_GW_INFO) and networks is not None:
                     self._add_router_ext_gw(context, router, networks, txn)
+        db_rev.bump_revision(router, ovn_const.TYPE_ROUTERS)
 
     # TODO(lucasagomes): The ``router_object`` parameter was added to
     # keep things backward compatible with old routers created prior to
@@ -877,7 +880,10 @@ class OVNClient(object):
         ovn_snats = utils.get_lrouter_snats(ovn_router)
         networks = self._get_v4_network_of_all_router_ports(context, router_id)
         try:
+            check_rev_cmd = self._nb_idl.check_revision_number(
+                router_id, new_router, ovn_const.TYPE_ROUTERS)
             with self._nb_idl.transaction(check_error=True) as txn:
+                txn.add(check_rev_cmd)
                 if gateway_new and not gateway_old:
                     # Route gateway is set
                     self._add_router_ext_gw(
@@ -921,6 +927,10 @@ class OVNClient(object):
                         old_routes, routes)
                     self.update_router_routes(
                         context, router_id, added, removed, txn=txn)
+
+            if check_rev_cmd.result == ovn_const.TXN_COMMITTED:
+                db_rev.bump_revision(new_router, ovn_const.TYPE_ROUTERS)
+
         except Exception as e:
             with excutils.save_and_reraise_exception():
                 LOG.error('Unable to update router %(router)s. '
@@ -932,6 +942,7 @@ class OVNClient(object):
         lrouter_name = utils.ovn_name(router_id)
         with self._nb_idl.transaction(check_error=True) as txn:
             txn.add(self._nb_idl.delete_lrouter(lrouter_name))
+        db_rev.delete_revision(router_id)
 
     def get_candidates_for_scheduling(self, extnet):
         if extnet.get(pnet.NETWORK_TYPE) in [const.TYPE_FLAT,

networking_ovn/common/utils.py

@@ -205,7 +205,8 @@ def get_revision_number(resource, resource_type):
     """Get the resource's revision number based on its type."""
     if resource_type in (constants.TYPE_NETWORKS,
                          constants.TYPE_PORTS,
-                         constants.TYPE_SECURITY_GROUP_RULES):
+                         constants.TYPE_SECURITY_GROUP_RULES,
+                         constants.TYPE_ROUTERS):
         return resource['revision_number']
     else:
         raise ovn_exc.UnknownResourceType(resource_type=resource_type)

networking_ovn/l3/l3_ovn.py

@@ -34,9 +34,11 @@ from neutron.db import l3_gwmode_db
 from neutron.db.models import l3 as l3_models
 from neutron.quota import resource_registry
 
+from networking_ovn.common import constants as ovn_const
 from networking_ovn.common import extensions
 from networking_ovn.common import ovn_client
 from networking_ovn.common import utils
+from networking_ovn.db import revision as db_rev
 from networking_ovn.l3 import l3_ovn_scheduler
 from networking_ovn.ovsdb import impl_idl_ovn
 
@@ -69,6 +71,12 @@ class OVNL3RouterPlugin(service_base.ServicePluginBase,
         self._plugin_property = None
         self._ovn_client_inst = None
         self.scheduler = l3_ovn_scheduler.get_scheduler()
+        self._register_precommit_callbacks()
+
+    def _register_precommit_callbacks(self):
+        registry.subscribe(
+            self.create_router_precommit, resources.ROUTER,
+            events.PRECOMMIT_CREATE)
 
     @property
     def _ovn_client(self):
@@ -107,6 +115,11 @@ class OVNL3RouterPlugin(service_base.ServicePluginBase,
         return ("L3 Router Service Plugin for basic L3 forwarding"
                 " using OVN")
 
+    def create_router_precommit(self, resource, event, trigger, context,
+                                router, router_id, router_db):
+        db_rev.create_initial_revision(
+            router_id, ovn_const.TYPE_ROUTERS, context.session)
+
     def create_router(self, context, router):
         router = super(OVNL3RouterPlugin, self).create_router(context, router)
         try:

networking_ovn/ovsdb/commands.py

@@ -21,6 +21,7 @@ from networking_ovn.common import utils
 RESOURCE_TYPE_MAP = {
     ovn_const.TYPE_NETWORKS: 'Logical_Switch',
     ovn_const.TYPE_PORTS: 'Logical_Switch_Port',
+    ovn_const.TYPE_ROUTERS: 'Logical_Router',
 }
 
 

networking_ovn/tests/functional/test_revision_numbers.py

@@ -57,6 +57,24 @@ class TestRevisionNumbers(base.TestOVNFunctionalBase):
                     ovn_const.OVN_PORT_NAME_EXT_ID_KEY) == name):
                 return row
 
+    def _create_router(self, name):
+        data = {'router': {'name': name, 'tenant_id': self._tenant_id}}
+        req = self.new_create_request('routers', data, self.fmt)
+        res = req.get_response(self.api)
+        return self.deserialize(self.fmt, res)['router']
+
+    def _update_router_name(self, net_id, new_name):
+        data = {'router': {'name': new_name}}
+        req = self.new_update_request('routers', data, net_id, self.fmt)
+        res = req.get_response(self.api)
+        return self.deserialize(self.fmt, res)['router']
+
+    def _find_router_row_by_name(self, name):
+        for row in self.nb_api._tables['Logical_Router'].rows.values():
+            if (row.external_ids.get(
+                    ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY) == name):
+                return row
+
     def test_create_network(self):
         name = 'net1'
         neutron_net = self._create_network(name)
@@ -100,3 +118,27 @@ class TestRevisionNumbers(base.TestOVNFunctionalBase):
         self.assertEqual(str(3), ovn_revision)
         # Assert it also matches with the newest returned by neutron API
         self.assertEqual(str(updated_port['revision_number']), ovn_revision)
+
+    def test_create_router(self):
+        name = 'router1'
+        neutron_router = self._create_router(name)
+        ovn_router = self._find_router_row_by_name(name)
+
+        ovn_revision = ovn_router.external_ids[
+            ovn_const.OVN_REV_NUM_EXT_ID_KEY]
+        self.assertEqual(str(0), ovn_revision)
+        # Assert it also matches with the newest returned by neutron API
+        self.assertEqual(str(neutron_router['revision_number']), ovn_revision)
+
+    def test_update_router(self):
+        new_name = 'newrouter'
+        neutron_router = self._create_router('router1')
+        updated_router = self._update_router_name(neutron_router['id'],
+                                                  new_name)
+        ovn_router = self._find_router_row_by_name(new_name)
+
+        ovn_revision = ovn_router.external_ids[
+            ovn_const.OVN_REV_NUM_EXT_ID_KEY]
+        self.assertEqual(str(1), ovn_revision)
+        # Assert it also matches with the newest returned by neutron API
+        self.assertEqual(str(updated_router['revision_number']), ovn_revision)

networking_ovn/tests/unit/fakes.py

@@ -168,6 +168,8 @@ class FakeResource(dict):
         self._add_details(info)
         self._add_methods(methods)
         self._loaded = loaded
+        # Add a revision number by default
+        setattr(self, 'revision_number', 1)
 
     @property
     def db_obj(self):

networking_ovn/tests/unit/l3/test_l3_ovn.py

@@ -15,6 +15,7 @@
 import copy
 
 import mock
+from neutron.services.revisions import revision_plugin
 from neutron.tests.unit.api import test_extensions
 from neutron.tests.unit.extensions import test_extraroute
 from neutron.tests.unit.extensions import test_l3
@@ -44,6 +45,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
 
     def setUp(self):
         super(OVNL3RouterPlugin, self).setUp()
+        revision_plugin.RevisionPlugin()
         network_attrs = {'router:external': True}
         self.fake_network = \
             fakes.FakeNetwork.create_one_network(attrs=network_attrs).info()
@@ -266,6 +268,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
         self.l3_inst._ovn.update_lrouter.assert_called_once_with(
             'neutron-router-id', enabled=True, external_ids={
                 ovn_const.OVN_GW_PORT_EXT_ID_KEY: '',
+                ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
                 ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router'})
 
     @mock.patch('neutron.db.extraroute_db.ExtraRoute_dbonly_mixin.'
@@ -285,6 +288,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
         self.l3_inst._ovn.update_lrouter.assert_called_once_with(
             'neutron-router-id', enabled=False,
             external_ids={ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'test',
+                          ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
                           ovn_const.OVN_GW_PORT_EXT_ID_KEY: ''})
 
     @mock.patch.object(utils, 'get_lrouter_non_gw_routes')
@@ -340,11 +344,13 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
             'neutron-router-id',
             ip_prefix='1.1.1.0/24', nexthop='10.0.0.2')
 
+    @mock.patch('networking_ovn.db.revision.bump_revision')
     @mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.get_port')
     @mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.get_subnet')
     @mock.patch('networking_ovn.common.ovn_client.OVNClient.'
                 '_get_v4_network_of_all_router_ports')
-    def test_create_router_with_ext_gw(self, get_rps, get_subnet, get_port):
+    def test_create_router_with_ext_gw(self, get_rps, get_subnet, get_port,
+                                       mock_bump):
         self.l3_inst._ovn.is_col_present.return_value = True
         router = {'router': {'name': 'router'}}
         get_subnet.return_value = self.fake_ext_subnet
@@ -354,6 +360,7 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
         self.l3_inst.create_router(self.context, router)
 
         external_ids = {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router',
+                        ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1',
                         ovn_const.OVN_GW_PORT_EXT_ID_KEY: 'gw-port-id'}
         self.l3_inst._ovn.create_lrouter.assert_called_once_with(
             'neutron-router-id', external_ids=external_ids,
@@ -370,6 +377,8 @@ class OVNL3RouterPlugin(test_mech_driver.OVNMechanismDriverTestCase):
             assert_called_once_with('gw-port-id', 'lrp-gw-port-id',
                                     is_gw_port=True)
         self.l3_inst._ovn.add_static_route.assert_has_calls(expected_calls)
+        mock_bump.assert_called_once_with(self.fake_router_with_ext_gw,
+                                          ovn_const.TYPE_ROUTERS)
 
     @mock.patch('networking_ovn.common.ovn_client.OVNClient._get_router_ports')
     @mock.patch('neutron.db.l3_db.L3_NAT_dbonly_mixin.get_router')
@@ -972,6 +981,7 @@ class OVNL3ExtrarouteTests(test_l3_gw.ExtGwModeIntTestCase,
         super(test_l3.L3BaseForIntTests, self).setUp(
             plugin=plugin, ext_mgr=ext_mgr,
             service_plugins=service_plugins)
+        revision_plugin.RevisionPlugin()
         l3_gw_mgr = test_l3_gw.TestExtensionManager()
         test_extensions.setup_extensions_middleware(l3_gw_mgr)
         self.l3_inst = directory.get_plugin(plugin_constants.L3)