Merge "Refactor remove_router_interface() for DVR"

neutron/db/l3_db.py

@@ -643,11 +643,12 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase):
 
     @staticmethod
     def _make_router_interface_info(
-            router_id, tenant_id, port_id, subnet_id, subnet_ids):
+            router_id, tenant_id, port_id, network_id, subnet_id, subnet_ids):
         return {
             'id': router_id,
             'tenant_id': tenant_id,
             'port_id': port_id,
+            'network_id': network_id,
             'subnet_id': subnet_id,  # deprecated by IPv6 multi-prefix
             'subnet_ids': subnet_ids
         }
@@ -695,8 +696,8 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase):
                         interface_info=interface_info)
 
         return self._make_router_interface_info(
-            router.id, port['tenant_id'], port['id'], subnets[-1]['id'],
+            router.id, port['tenant_id'], port['id'], port['network_id'],
-            [subnet['id'] for subnet in subnets])
+            subnets[-1]['id'], [subnet['id'] for subnet in subnets])
 
     def _confirm_router_interface_not_in_use(self, context, router_id,
                                              subnet_id):
@@ -809,7 +810,8 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase):
                         cidrs=[x['cidr'] for x in subnets],
                         network_id=gw_network_id)
         return self._make_router_interface_info(router_id, port['tenant_id'],
-                                                port['id'], subnets[0]['id'],
+                                                port['id'], port['network_id'],
+                                                subnets[0]['id'],
                                                 [subnet['id'] for subnet in
                                                     subnets])
 

neutron/db/l3_dvr_db.py

@@ -298,8 +298,8 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
                     LOG.debug("CSNAT port updated for IPv6 subnet: "
                               "%s", updated_port)
         router_interface_info = self._make_router_interface_info(
-            router_id, port['tenant_id'], port['id'], subnet['id'],
+            router_id, port['tenant_id'], port['id'], port['network_id'],
-            [subnet['id']])
+            subnet['id'], [subnet['id']])
         self.notify_router_interface_action(
             context, router_interface_info, 'add')
         return router_interface_info
@@ -322,7 +322,7 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
                 return port
 
     def _check_for_multiprefix_csnat_port_and_update(
-        self, context, router, subnet, port):
+        self, context, router, network_id, subnet_id):
         """Checks if the csnat port contains multiple ipv6 prefixes.
 
         If the csnat port contains multiple ipv6 prefixes for the given
@@ -331,13 +331,11 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
         it with the right fixed_ip.
         This function returns true if it is a multiprefix port.
         """
-        subnet_id = subnet['id']
         if router.gw_port:
             # If router has a gateway port, check if it has IPV6 subnet
             cs_port = (
                 self._find_router_port_by_network_and_device_owner(
-                    router, subnet['network_id'],
+                    router, network_id, l3_const.DEVICE_OWNER_ROUTER_SNAT))
-                    l3_const.DEVICE_OWNER_ROUTER_SNAT))
             if cs_port:
                 fixed_ips = (
                     [fixedip for fixedip in
@@ -351,68 +349,49 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
                     return True
         return False
 
-    def _check_dvr_router_remove_required_and_notify_agent(
-        self, context, router, port, subnet):
-        if router.extra_attributes.distributed:
-            is_multiple_prefix_csport = (
-                self._check_for_multiprefix_csnat_port_and_update(
-                    context, router, subnet, port))
-            if not is_multiple_prefix_csport:
-                # Single prefix port - go ahead and delete the port
-                self.delete_csnat_router_interface_ports(
-                    context.elevated(), router, subnet_id=subnet['id'])
-            plugin = manager.NeutronManager.get_service_plugins().get(
-                        constants.L3_ROUTER_NAT)
-            l3_agents = plugin.get_l3_agents_hosting_routers(context,
-                                                             [router['id']])
-            subnet_ids = plugin.get_subnet_ids_on_router(
-                context, router['id'])
-            if subnet_ids:
-                binding_table = l3_dvrsched_db.CentralizedSnatL3AgentBinding
-                snat_binding = context.session.query(binding_table).filter_by(
-                    router_id=router['id']).first()
-                for l3_agent in l3_agents:
-                    is_this_snat_agent = (
-                        snat_binding and
-                        snat_binding.l3_agent_id == l3_agent['id'])
-                    if (is_this_snat_agent or
-                        plugin.check_dvr_serviceable_ports_on_host(
-                            context, l3_agent['host'], subnet_ids)):
-                        continue
-                    plugin.remove_router_from_l3_agent(
-                        context, l3_agent['id'], router['id'])
-        router_interface_info = self._make_router_interface_info(
-            router['id'], port['tenant_id'], port['id'], subnet['id'],
-            [subnet['id']])
-        self.notify_router_interface_action(
-            context, router_interface_info, 'remove')
-        return router_interface_info
-
     def remove_router_interface(self, context, router_id, interface_info):
-        remove_by_port, remove_by_subnet = (
-            self._validate_interface_info(interface_info, for_removal=True)
-        )
-        port_id = interface_info.get('port_id')
-        subnet_id = interface_info.get('subnet_id')
         router = self._get_router(context, router_id)
-        device_owner = self._get_device_owner(context, router)
+        if not router.extra_attributes.distributed:
+            return super(
+                L3_NAT_with_dvr_db_mixin, self).remove_router_interface(
+                    context, router_id, interface_info)
 
-        if remove_by_port:
+        plugin = manager.NeutronManager.get_service_plugins().get(
-            port, subnets = self._remove_interface_by_port(
+            constants.L3_ROUTER_NAT)
-                    context, router_id, port_id, subnet_id, device_owner)
+        router_hosts_before = plugin._get_dvr_hosts_for_router(
+            context, router_id)
 
-        # remove_by_subnet is not used here, because the validation logic of
+        interface_info = super(
-        # _validate_interface_info ensures that at least one of remote_by_*
+            L3_NAT_with_dvr_db_mixin, self).remove_router_interface(
-        # is True.
+                context, router_id, interface_info)
-        else:
-            port, subnets = self._remove_interface_by_subnet(
-                    context, router_id, subnet_id, device_owner)
 
-        subnet = subnets[0]
+        router_hosts_after = plugin._get_dvr_hosts_for_router(
-        router_interface_info = (
+            context, router_id)
-            self._check_dvr_router_remove_required_and_notify_agent(
+        removed_hosts = set(router_hosts_before) - set(router_hosts_after)
-                context, router, port, subnet))
+        if removed_hosts:
-        return router_interface_info
+            agents = plugin.get_l3_agents(context,
+                                          filters={'host': removed_hosts})
+            binding_table = l3_dvrsched_db.CentralizedSnatL3AgentBinding
+            snat_binding = context.session.query(binding_table).filter_by(
+                router_id=router_id).first()
+            for agent in agents:
+                is_this_snat_agent = (
+                    snat_binding and snat_binding.l3_agent_id == agent['id'])
+                if not is_this_snat_agent:
+                    plugin.remove_router_from_l3_agent(
+                        context, agent['id'], router_id)
+
+        is_multiple_prefix_csport = (
+            self._check_for_multiprefix_csnat_port_and_update(
+                context, router, interface_info['network_id'],
+                interface_info['subnet_id']))
+        if not is_multiple_prefix_csport:
+            # Single prefix port - go ahead and delete the port
+            self.delete_csnat_router_interface_ports(
+                context.elevated(), router,
+                subnet_id=interface_info['subnet_id'])
+
+        return interface_info
 
     def _get_snat_sync_interfaces(self, context, router_ids):
         """Query router interfaces that relate to list of router_ids."""

neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py

@@ -727,3 +727,40 @@ class L3DvrTestCase(ml2_test_base.ML2TestFramework):
                 self.assertEqual(self.l3_agent['id'],
                                  agents['agents'][0]['id'])
                 self.assertFalse(remove_mock.called)
+
+    def test_remove_router_interface(self):
+        HOST1 = 'host1'
+        dvr_agent = helpers.register_l3_agent(
+            host=HOST1, agent_mode=constants.L3_AGENT_MODE_DVR)
+        router = self._create_router()
+        arg_list = (portbindings.HOST_ID,)
+        with self.subnet() as subnet,\
+                self.port(subnet=subnet,
+                          device_owner=DEVICE_OWNER_COMPUTE,
+                          arg_list=arg_list,
+                          **{portbindings.HOST_ID: HOST1}):
+            l3_notifier = mock.Mock()
+            self.l3_plugin.l3_rpc_notifier = l3_notifier
+            self.l3_plugin.agent_notifiers[
+                    constants.AGENT_TYPE_L3] = l3_notifier
+
+            self.l3_plugin.add_router_interface(
+                self.context, router['id'],
+                {'subnet_id': subnet['subnet']['id']})
+            self.l3_plugin.schedule_router(self.context, router['id'])
+
+            # router should be scheduled to the agent on HOST1
+            agents = self.l3_plugin.list_l3_agents_hosting_router(
+                self.context, router['id'])['agents']
+            self.assertEqual(1, len(agents))
+            self.assertEqual(dvr_agent['id'], agents[0]['id'])
+
+            self.l3_plugin.remove_router_interface(
+                self.context, router['id'],
+                {'subnet_id': subnet['subnet']['id']})
+
+            agents = self.l3_plugin.list_l3_agents_hosting_router(
+                self.context, router['id'])['agents']
+            self.assertEqual(0, len(agents))
+            l3_notifier.router_removed_from_agent.assert_called_once_with(
+                self.context, router['id'], HOST1)

neutron/tests/unit/db/test_l3_dvr_db.py

@@ -471,7 +471,6 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
                        'distributed': True}
         router = self._create_router(router_dict)
         plugin = mock.MagicMock()
-        plugin.get_subnet_ids_on_router = mock.Mock()
         with self.network() as net_ext,\
                 self.subnet() as subnet1,\
                 self.subnet(cidr='20.0.0.0/24') as subnet2:
@@ -517,7 +516,6 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
             dvr_ports = self.core_plugin.get_ports(
                 self.ctx, filters=dvr_filters)
             self.assertEqual(1, len(dvr_ports))
-            self.assertEqual(1, plugin.get_subnet_ids_on_router.call_count)
 
     def test__validate_router_migration_notify_advanced_services(self):
         router = {'name': 'foo_router', 'admin_state_up': False}