Optimize deletion of static routes

Instead of having a separate OVN command for deletion of each
static route, send out the deletion as a single command.

Which significantly improves the performance. Previously
deletion of 1000 routes took >1m30s and >6m30s for 2000 routes,
with this change it takes ~5s and ~8s, respectivily.

Closes-Bug: #2060054
Change-Id: Iaa5204e2e48795c31c502160041bd128189eef5a

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py

@@ -274,21 +274,6 @@ class API(api.API, metaclass=abc.ABCMeta):
         :returns:            :class:`Command` with no result
         """
 
-    @abc.abstractmethod
-    def delete_static_route(self, lrouter, ip_prefix, nexthop, if_exists=True):
-        """Delete static route from logical router.
-
-        :param lrouter:      The unique name of the lrouter
-        :type lrouter:       string
-        :param ip_prefix:    The prefix of the static route
-        :type ip_prefix:     string
-        :param nexthop:      The nexthop of the static route
-        :type nexthop:       string
-        :param if_exists:    Do not fail if router does not exist
-        :type if_exists:     bool
-        :returns:            :class:`Command` with no result
-        """
-
     @abc.abstractmethod
     def get_all_chassis_gateway_bindings(self,
                                          chassis_candidate_list=None):

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py

@@ -675,12 +675,11 @@ class AddStaticRouteCommand(command.BaseCommand):
         _addvalue_to_list(lrouter, 'static_routes', row.uuid)
 
 
-class DelStaticRouteCommand(command.BaseCommand):
+class DelStaticRoutesCommand(command.BaseCommand):
-    def __init__(self, api, lrouter, ip_prefix, nexthop, if_exists):
+    def __init__(self, api, lrouter, routes, if_exists):
-        super(DelStaticRouteCommand, self).__init__(api)
+        super(DelStaticRoutesCommand, self).__init__(api)
         self.lrouter = lrouter
-        self.ip_prefix = ip_prefix
+        self.routes = routes
-        self.nexthop = nexthop
         self.if_exists = if_exists
 
     def run_idl(self, txn):
@@ -693,14 +692,16 @@ class DelStaticRouteCommand(command.BaseCommand):
             msg = _("Logical Router %s does not exist") % self.lrouter
             raise RuntimeError(msg)
 
-        static_routes = getattr(lrouter, 'static_routes', [])
+        routes_to_be_deleted = []
-        for route in static_routes:
+        for route in getattr(lrouter, 'static_routes', []):
-            ip_prefix = getattr(route, 'ip_prefix', '')
+            route_tuple = (getattr(route, 'ip_prefix', ''),
-            nexthop = getattr(route, 'nexthop', '')
+                           getattr(route, 'nexthop', ''))
-            if self.ip_prefix == ip_prefix and self.nexthop == nexthop:
+            if route_tuple in self.routes:
+                routes_to_be_deleted.append(route)
+
+        for route in routes_to_be_deleted:
             _delvalue_from_list(lrouter, 'static_routes', route)
             route.delete()
-                break
 
 
 class UpdateObjectExtIdsCommand(command.BaseCommand):

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py

@@ -474,9 +474,8 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend):
         return cmd.AddStaticRouteCommand(self, lrouter, maintain_bfd,
                                          **columns)
 
-    def delete_static_route(self, lrouter, ip_prefix, nexthop, if_exists=True):
+    def delete_static_routes(self, lrouter, routes, if_exists=True):
-        return cmd.DelStaticRouteCommand(self, lrouter, ip_prefix, nexthop,
+        return cmd.DelStaticRoutesCommand(self, lrouter, routes, if_exists)
-                                         if_exists)
 
     def _get_logical_router_port_gateway_chassis(self, lrp, priorities=None):
         """Get the list of chassis hosting this gateway port.

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py

@@ -972,14 +972,16 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
         for router in self._nb_idl.lr_list().execute(check_error=True):
             if not router.external_ids.get(ovn_const.OVN_REV_NUM_EXT_ID_KEY):
                 continue
-            for route in self._nb_idl.lr_route_list(router.uuid).execute(
+            routes_to_delete = [
-                    check_error=True):
+                (r.ip_prefix, '')
-                if (route.nexthop == '' and
+                for r in self._nb_idl.lr_route_list(router.uuid).execute(
-                        route.ip_prefix in (n_const.IPv4_ANY,
+                    check_error=True)
-                                            n_const.IPv6_ANY)):
+                if r.nexthop == '' and r.ip_prefix in (n_const.IPv4_ANY,
+                                                       n_const.IPv6_ANY)
+            ]
             cmds.append(
-                        self._nb_idl.delete_static_route(
+                self._nb_idl.delete_static_routes(router.name,
-                            router.name, route.ip_prefix, ''))
+                                                  routes_to_delete))
 
         if cmds:
             with self._nb_idl.transaction(check_error=True) as txn:

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py

@@ -1210,15 +1210,17 @@ class OVNClient(object):
         gw_lrouter_name = utils.ovn_name(router_id)
         deleted_ports = []
         for gw_port in self._get_router_gw_ports(context, router_id):
+            routes_to_delete = []
             for gw_info in self._get_gw_info(context, gw_port):
                 if gw_info.ip_version == const.IP_VERSION_4:
                     for network in networks:
                         txn.add(self._nb_idl.delete_nat_rule_in_lrouter(
                             gw_lrouter_name, type='snat', logical_ip=network,
                             external_ip=gw_info.router_ip))
-                txn.add(self._nb_idl.delete_static_route(
+                routes_to_delete.append((gw_info.ip_prefix,
-                    gw_lrouter_name, ip_prefix=gw_info.ip_prefix,
+                                         gw_info.gateway_ip))
-                    nexthop=gw_info.gateway_ip))
+            txn.add(self._nb_idl.delete_static_routes(
+                    gw_lrouter_name, routes_to_delete))
             txn.add(self._nb_idl.delete_lrouter_port(
                 utils.ovn_lrouter_port_name(gw_port['id']),
                 gw_lrouter_name))
@@ -1370,11 +1372,14 @@ class OVNClient(object):
                 self._nb_idl.add_static_route(
                     lrouter_name, ip_prefix=route['destination'],
                     nexthop=route['nexthop']))
-        for route in remove:
+        routes_to_delete = [
+            (r['destination'], r['nexthop'])
+            for r in remove
+        ]
         commands.append(
-                self._nb_idl.delete_static_route(
+            self._nb_idl.delete_static_routes(lrouter_name,
-                    lrouter_name, ip_prefix=route['destination'],
+                                              routes_to_delete)
-                    nexthop=route['nexthop']))
+        )
         self._transaction(commands, txn=txn)
 
     def _get_router_gw_ports(self, context, router_id):

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py

@@ -749,11 +749,12 @@ class OvnNbSynchronizer(OvnDbSynchronizer):
                     if self.mode == SYNC_MODE_REPAIR:
                         LOG.warning("Delete static routes %s from OVN NB DB",
                                     sroute['del'])
-                        for route in sroute['del']:
+                        routes_to_delete = [
-                            txn.add(self.ovn_api.delete_static_route(
+                            (r['destination'], r['nexthop'])
-                                utils.ovn_name(sroute['id']),
+                            for r in sroute['del']
-                                ip_prefix=route['destination'],
+                        ]
-                                nexthop=route['nexthop']))
+                        txn.add(self.ovn_api.delete_static_routes(
+                            utils.ovn_name(sroute['id']), routes_to_delete))
             for fip in update_fips_list:
                 if fip['del']:
                     LOG.warning("Router %(id)s floating IPs %(fip)s "

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py

@@ -626,6 +626,46 @@ class TestNbApi(BaseOvnIdlTest):
         self.assertIsNone(
             self.nbapi.lookup('HA_Chassis_Group', router_name, default=None))
 
+    def _assert_routes_exist(self, lr_name, expected_count):
+        with self.nbapi.transaction(check_error=True) as txn:
+            lr = txn.add(self.nbapi.lr_get(lr_name))
+        actual_count = len(lr.result.static_routes)
+        self.assertEqual(actual_count, expected_count,
+                         f"Expected {expected_count} routes, "
+                         f"found {actual_count}.")
+
+    def test_del_static_routes(self):
+        lr_name = 'router_with_static_routes_del'
+        routes = [('0.0.0.0/0', '192.0.2.1'), ('10.0.0.0/24', '192.0.3.1')]
+
+        with self.nbapi.transaction(check_error=True) as txn:
+            txn.add(self.nbapi.lr_add(lr_name))
+            for ip_prefix, nexthop in routes:
+                txn.add(self.nbapi.add_static_route(lr_name,
+                                                    ip_prefix=ip_prefix,
+                                                    nexthop=nexthop))
+
+        self._assert_routes_exist(lr_name, 2)
+
+        with self.nbapi.transaction(check_error=True) as txn:
+            txn.add(self.nbapi.delete_static_routes(lr_name, routes))
+
+        self._assert_routes_exist(lr_name, 0)
+
+    def test_del_no_static_routes(self):
+        lr_name = 'router_with_static_routes_del'
+        routes = []
+
+        with self.nbapi.transaction(check_error=True) as txn:
+            txn.add(self.nbapi.lr_add(lr_name))
+
+        self._assert_routes_exist(lr_name, 0)
+
+        with self.nbapi.transaction(check_error=True) as txn:
+            txn.add(self.nbapi.delete_static_routes(lr_name, routes))
+
+        self._assert_routes_exist(lr_name, 0)
+
 
 class TestIgnoreConnectionTimeout(BaseOvnIdlTest):
     @classmethod

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py

@@ -12,6 +12,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from collections import defaultdict
 from collections import namedtuple
 
 import netaddr
@@ -797,10 +798,13 @@ class TestOvnNbSync(base.TestOVNFunctionalBase):
                 txn.add(self.nb_api.add_static_route(lrouter_name,
                                                      ip_prefix=ip_prefix,
                                                      nexthop=nexthop))
-
+            routers = defaultdict(list)
             for lrouter_name, ip_prefix, nexthop in self.delete_lrouter_routes:
-                txn.add(self.nb_api.delete_static_route(lrouter_name,
+                routers[lrouter_name].append((ip_prefix, nexthop))
-                                                        ip_prefix, nexthop,
+
+            for lrouter_name, routes_to_delete in routers.items():
+                txn.add(self.nb_api.delete_static_routes(lrouter_name,
+                                                        routes_to_delete,
                                                         True))
 
             for lrouter_name, nat_dict in self.create_lrouter_nats:

neutron/tests/unit/fake_resources.py

@@ -76,7 +76,7 @@ class FakeOvsdbNbOvnIdl(object):
         self.update_acls = mock.Mock()
         self.idl = mock.Mock()
         self.add_static_route = mock.Mock()
-        self.delete_static_route = mock.Mock()
+        self.delete_static_routes = mock.Mock()
         self.get_all_chassis_gateway_bindings = mock.Mock()
         self.get_chassis_gateways = mock.Mock()
         self.get_gateway_chassis_binding = mock.Mock()

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py

@@ -730,14 +730,14 @@ class TestAddStaticRouteCommand(TestBaseCommand):
                 'static_routes', fake_static_route.uuid)
 
 
-class TestDelStaticRouteCommand(TestBaseCommand):
+class TestDelStaticRoutesCommand(TestBaseCommand):
 
     def _test_lrouter_no_exist(self, if_exists=True):
         with mock.patch.object(idlutils, 'row_by_value',
                                side_effect=idlutils.RowNotFound):
-            cmd = commands.DelStaticRouteCommand(
+            cmd = commands.DelStaticRoutesCommand(
                 self.ovn_api, 'fake-lrouter',
-                '30.0.0.0/24', '40.0.0.100',
+                [('30.0.0.0/24', '40.0.0.100')],
                 if_exists=if_exists)
             if if_exists:
                 cmd.run_idl(self.transaction)
@@ -751,18 +751,21 @@ class TestDelStaticRouteCommand(TestBaseCommand):
         self._test_lrouter_no_exist(if_exists=False)
 
     def test_static_route_del(self):
-        fake_static_route = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+        fake_static_route1 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
             attrs={'ip_prefix': '50.0.0.0/24', 'nexthop': '40.0.0.101'})
+        fake_static_route2 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'ip_prefix': '60.0.0.0/24', 'nexthop': '40.0.0.101'})
         fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row(
-            attrs={'static_routes': [fake_static_route]})
+            attrs={'static_routes': [fake_static_route1, fake_static_route2]})
         with mock.patch.object(idlutils, 'row_by_value',
                                return_value=fake_lrouter):
-            cmd = commands.DelStaticRouteCommand(
+            cmd = commands.DelStaticRoutesCommand(
                 self.ovn_api, fake_lrouter.name,
-                fake_static_route.ip_prefix, fake_static_route.nexthop,
+                [(fake_static_route1.ip_prefix, fake_static_route1.nexthop),
+                 (fake_static_route2.ip_prefix, fake_static_route2.nexthop)],
                 if_exists=True)
             cmd.run_idl(self.transaction)
-            fake_lrouter.delvalue.assert_called_once_with(
+            fake_lrouter.delvalue.assert_called_with(
                 'static_routes', mock.ANY)
 
     def test_static_route_del_not_found(self):
@@ -774,9 +777,9 @@ class TestDelStaticRouteCommand(TestBaseCommand):
             attrs={'static_routes': [fake_static_route2]})
         with mock.patch.object(idlutils, 'row_by_value',
                                return_value=fake_lrouter):
-            cmd = commands.DelStaticRouteCommand(
+            cmd = commands.DelStaticRoutesCommand(
                 self.ovn_api, fake_lrouter.name,
-                fake_static_route1.ip_prefix, fake_static_route1.nexthop,
+                [(fake_static_route1.ip_prefix, fake_static_route1.nexthop)],
                 if_exists=True)
             cmd.run_idl(self.transaction)
             fake_lrouter.delvalue.assert_not_called()

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py

@@ -1034,13 +1034,13 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight,
         self.assertRaises(
             periodics.NeverAgain,
             self.periodic.check_router_default_route_empty_dst_ip)
-        nb_idl.delete_static_route.assert_has_calls([
+        nb_idl.delete_static_routes.assert_has_calls([
-            mock.call(router1.name, route1.ip_prefix, route1.nexthop),
+            mock.call(router1.name, [(route1.ip_prefix, route1.nexthop),
-            mock.call(router1.name, route3.ip_prefix, route3.nexthop),
+                                     (route3.ip_prefix, route3.nexthop)]),
         ])
         self.assertEqual(
-            2,
+            1,
-            nb_idl.delete_static_route.call_count)
+            nb_idl.delete_static_routes.call_count)
 
     @mock.patch.object(ports_obj.PortBinding, 'get_port_binding_by_vnic_type')
     def test_add_vnic_type_and_pb_capabilities_to_lsp(self, mock_get_pb):

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py

@@ -483,7 +483,7 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase):
         ovn_api.delete_lrouter = mock.Mock()
         ovn_api.delete_lrouter_port = mock.Mock()
         ovn_api.add_static_route = mock.Mock()
-        ovn_api.delete_static_route = mock.Mock()
+        ovn_api.delete_static_routes = mock.Mock()
         ovn_api.get_all_dhcp_options.return_value = {
             'subnets': {'n1-s1': {'cidr': '10.0.0.0/24',
                                   'options':
@@ -641,13 +641,14 @@ class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase):
                                                   any_order=True)
         self.assertEqual(len(add_static_route_list),
                          ovn_api.add_static_route.call_count)
-        del_route_calls = [mock.call(mock.ANY, ip_prefix=route['destination'],
+        routes_to_delete = [(route['destination'], route['nexthop'])
-                                     nexthop=route['nexthop'])
                            for route in del_static_route_list]
-        ovn_api.delete_static_route.assert_has_calls(del_route_calls,
+        del_route_call = [mock.call(mock.ANY, routes_to_delete)] \
-                                                     any_order=True)
+            if routes_to_delete else []
-        self.assertEqual(len(del_static_route_list),
+
-                         ovn_api.delete_static_route.call_count)
+        ovn_api.delete_static_routes.assert_has_calls(del_route_call)
+        self.assertEqual(1 if len(del_static_route_list) else 0,
+                         ovn_api.delete_static_routes.call_count)
 
         add_nat_calls = [mock.call(mock.ANY, **nat) for nat in add_snat_list]
         ovn_api.add_nat_rule_in_lrouter.assert_has_calls(add_nat_calls,

neutron/tests/unit/services/ovn_l3/test_plugin.py

@@ -562,7 +562,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
                                               'nexthop': '10.0.0.2'}]}}
         self.l3_inst.update_router(self.context, router_id, update_data)
         self.assertFalse(self.l3_inst._nb_ovn.add_static_route.called)
-        self.assertFalse(self.l3_inst._nb_ovn.delete_static_route.called)
+        self.assertFalse(self.l3_inst._nb_ovn.delete_static_routes.called)
 
     @mock.patch.object(utils, 'get_lrouter_non_gw_routes')
     @mock.patch('neutron.db.extraroute_db.ExtraRoute_dbonly_mixin.'
@@ -592,9 +592,8 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
         self.l3_inst._nb_ovn.add_static_route.assert_called_once_with(
             'neutron-router-id',
             ip_prefix='2.2.2.0/24', nexthop='10.0.0.3')
-        self.l3_inst._nb_ovn.delete_static_route.assert_called_once_with(
+        self.l3_inst._nb_ovn.delete_static_routes.assert_called_once_with(
-            'neutron-router-id',
+            'neutron-router-id', [('1.1.1.0/24', '10.0.0.2')])
-            ip_prefix='1.1.1.0/24', nexthop='10.0.0.2')
 
     @mock.patch.object(utils, 'get_lrouter_non_gw_routes')
     @mock.patch('neutron.db.extraroute_db.ExtraRoute_dbonly_mixin.'
@@ -621,9 +620,8 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
                                             events.AFTER_UPDATE,
                                             self, payload)
         self.l3_inst._nb_ovn.add_static_route.assert_not_called()
-        self.l3_inst._nb_ovn.delete_static_route.assert_called_once_with(
+        self.l3_inst._nb_ovn.delete_static_routes.assert_called_once_with(
-            'neutron-router-id',
+            'neutron-router-id', [('1.1.1.0/24', '10.0.0.2')])
-            ip_prefix='1.1.1.0/24', nexthop='10.0.0.2')
 
     @mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.'
                 'ovn_client.OVNClient._get_v4_network_of_all_router_ports')
@@ -984,7 +982,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
         self.l3_inst.update_router(self.context, 'router-id', router)
 
         nb_ovn.lrp_del.assert_not_called()
-        nb_ovn.delete_static_route.assert_not_called()
+        nb_ovn.delete_static_routes.assert_not_called()
         nb_ovn.delete_nat_rule_in_lrouter.assert_not_called()
         nb_ovn.add_lrouter_port.assert_not_called()
         nb_ovn.set_lrouter_port_in_lswitch_port.assert_not_called()
@@ -1046,7 +1044,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
                                             events.AFTER_UPDATE,
                                             self, payload)
 
-        self.l3_inst._nb_ovn.delete_static_route.assert_not_called()
+        self.l3_inst._nb_ovn.delete_static_routes.assert_not_called()
         self.l3_inst._nb_ovn.delete_nat_rule_in_lrouter.assert_not_called()
         self.l3_inst._nb_ovn.add_static_route.assert_not_called()
         self.l3_inst._nb_ovn.add_nat_rule_in_lrouter.assert_called_once_with(
@@ -1084,7 +1082,7 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase):
                                             self, payload)
 
         nb_ovn = self.l3_inst._nb_ovn
-        nb_ovn.delete_static_route.assert_not_called()
+        nb_ovn.delete_static_routes.assert_not_called()
         nb_ovn.delete_nat_rule_in_lrouter.assert_called_once_with(
             'neutron-router-id', type='snat', logical_ip='10.0.0.0/24',
             external_ip='192.168.1.1')
@@ -2165,8 +2163,8 @@ class OVNL3ExtrarouteTests(test_l3_gw.ExtGwModeIntTestCase,
             test_update_subnet_gateway_for_external_net()
         self.l3_inst._nb_ovn.add_static_route.assert_called_once_with(
             'neutron-fake_device', ip_prefix='0.0.0.0/0', nexthop='120.0.0.2')
-        self.l3_inst._nb_ovn.delete_static_route.assert_called_once_with(
+        self.l3_inst._nb_ovn.delete_static_routes.assert_called_once_with(
-            'neutron-fake_device', ip_prefix='0.0.0.0/0', nexthop='120.0.0.1')
+            'neutron-fake_device', [('0.0.0.0/0', '120.0.0.1')])
 
     def test_router_update_gateway_upon_subnet_create_max_ips_ipv6(self):
         super(OVNL3ExtrarouteTests, self). \