Remove RPC to plugin when dhcp sets default route

_set_default_route() was using an RPC to the plugin to get the DHCP
port for the network on the current host, and then used it to form
the tap device name. This happened on every allocation reload too.
This fix removes the RPC and gets the tap device name using local
methods instead. It also removes an unnecessary call to set the
default route in the restart method.

Closes-Bug: 1290068
Related-Bug: 1294254
Change-Id: I639bcf93725c4969d1011d2d20491d461ccfdbed

neutron/agent/dhcp_agent.py

@@ -114,6 +114,8 @@ class DhcpAgent(manager.Manager):
 
     def call_driver(self, action, network, **action_kwargs):
         """Invoke an action on a DHCP driver instance."""
+        LOG.debug(_('Calling driver for network: %(net)s action: %(action)s'),
+                  {'net': network.id, 'action': action})
         try:
             # the Driver expects something that is duck typed similar to
             # the base models.

neutron/agent/linux/dhcp.py

@@ -127,7 +127,6 @@ class DhcpBase(object):
         """Restart the dhcp service for the network."""
         self.disable(retain_port=True)
         self.enable()
-        self.device_manager.update(self.network)
 
     @abc.abstractproperty
     def active(self):
@@ -399,7 +398,7 @@ class Dnsmasq(DhcpLocalProcess):
         else:
             LOG.debug(_('Pid %d is stale, relaunching dnsmasq'), self.pid)
         LOG.debug(_('Reloading allocations for network: %s'), self.network.id)
-        self.device_manager.update(self.network)
+        self.device_manager.update(self.network, self.interface_name)
 
     def _iter_hosts(self):
         """Iterate over hosts.
@@ -702,21 +701,16 @@ class DeviceManager(object):
         host_uuid = uuid.uuid5(uuid.NAMESPACE_DNS, socket.gethostname())
         return 'dhcp%s-%s' % (host_uuid, network.id)
 
-    def _get_device(self, network, port):
+    def _set_default_route(self, network, device_name):
-        """Return DHCP ip_lib device for this host on the network."""
-        interface_name = self.get_interface_name(network, port)
-        return ip_lib.IPDevice(interface_name,
-                               self.root_helper,
-                               network.namespace)
-
-    def _set_default_route(self, network, port):
         """Sets the default gateway for this dhcp namespace.
 
         This method is idempotent and will only adjust the route if adjusting
         it would change it from what it already is.  This makes it safe to call
         and avoids unnecessary perturbation of the system.
         """
-        device = self._get_device(network, port)
+        device = ip_lib.IPDevice(device_name,
+                                 self.root_helper,
+                                 network.namespace)
         gateway = device.route.get_gateway()
         if gateway:
             gateway = gateway['gateway']
@@ -851,18 +845,14 @@ class DeviceManager(object):
             device.route.pullup_route(interface_name)
 
         if self.conf.use_namespaces:
-            self._set_default_route(network, port)
+            self._set_default_route(network, interface_name)
 
         return interface_name
 
-    def update(self, network):
+    def update(self, network, device_name):
         """Update device settings for the network's DHCP on this host."""
         if self.conf.use_namespaces:
-            device_id = self.get_device_id(network)
+            self._set_default_route(network, device_name)
-            port = self.plugin.get_dhcp_port(network.id, device_id)
-            if not port:
-                raise exceptions.NetworkNotFound(net_id=network.id)
-            self._set_default_route(network, port)
 
     def destroy(self, network, device_name):
         """Destroy the device used for the network's DHCP on this host."""

neutron/tests/unit/test_dhcp_agent.py

@@ -1075,6 +1075,7 @@ class FakeV4Network:
     id = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'
     subnets = [FakeV4Subnet()]
     ports = [FakePort1()]
+    namespace = 'qdhcp-aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'
 
 
 class FakeV4NetworkNoSubnet:
@@ -1165,7 +1166,7 @@ class TestDeviceManager(base.BaseTestCase):
                                            namespace=net.namespace))
         self.mock_driver.assert_has_calls(expected)
 
-        dh._set_default_route.assert_called_once_with(net, port)
+        dh._set_default_route.assert_called_once_with(net, 'tap12345678-12')
 
     def test_setup(self):
         cfg.CONF.set_override('enable_metadata_network', False)
@@ -1308,35 +1309,26 @@ class TestDeviceManager(base.BaseTestCase):
                 self.assertEqual(dh.get_device_id(fake_net), expected)
                 uuid5.assert_called_once_with(uuid.NAMESPACE_DNS, 'localhost')
 
-    def _get_device_manager_with_mock_device(self, conf, device):
-            dh = dhcp.DeviceManager(conf, cfg.CONF.root_helper, None)
-            dh._get_device = mock.Mock(return_value=device)
-            return dh
-
     def test_update(self):
         # Try with namespaces and no metadata network
         cfg.CONF.set_override('use_namespaces', True)
         cfg.CONF.set_override('enable_metadata_network', False)
         dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
         dh._set_default_route = mock.Mock()
-
         network = mock.Mock()
-        port = mock.Mock()
-        dh.plugin = mock.Mock()
-        dh.plugin.get_dhcp_port.return_value = port
-        dh.update(network)
 
-        dh._set_default_route.assert_called_once_with(network, port)
+        dh.update(network, 'ns-12345678-12')
+
+        dh._set_default_route.assert_called_once_with(network,
+                                                      'ns-12345678-12')
 
         # No namespaces, shouldn't set default route.
         cfg.CONF.set_override('use_namespaces', False)
         cfg.CONF.set_override('enable_metadata_network', False)
         dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
         dh._set_default_route = mock.Mock()
-        dh.plugin = mock.Mock()
-        dh.plugin.get_dhcp_port.return_value = port
 
-        dh.update(FakeV4Network())
+        dh.update(FakeV4Network(), 'tap12345678-12')
 
         self.assertFalse(dh._set_default_route.called)
 
@@ -1345,10 +1337,8 @@ class TestDeviceManager(base.BaseTestCase):
         cfg.CONF.set_override('enable_metadata_network', True)
         dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
         dh._set_default_route = mock.Mock()
-        dh.plugin = mock.Mock()
-        dh.plugin.get_dhcp_port.return_value = port
 
-        dh.update(FakeV4Network())
+        dh.update(FakeV4Network(), 'ns-12345678-12')
 
         self.assertTrue(dh._set_default_route.called)
 
@@ -1358,123 +1348,109 @@ class TestDeviceManager(base.BaseTestCase):
         dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
         dh._set_default_route = mock.Mock()
 
-        dh.update(FakeV4Network())
+        dh.update(FakeV4Network(), 'ns-12345678-12')
 
         self.assertFalse(dh._set_default_route.called)
 
     def test_set_default_route(self):
-        device = mock.Mock()
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
-        device.route.get_gateway.return_value = None
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
-
+            device = mock.Mock()
-        # Basic one subnet with gateway.
+            mock_IPDevice.return_value = device
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
+            device.route.get_gateway.return_value = None
-        network = FakeV4Network()
+            # Basic one subnet with gateway.
-
+            network = FakeV4Network()
-        dh._set_default_route(network, mock.Mock())
+            dh._set_default_route(network, 'tap-name')
 
         device.route.get_gateway.assert_called_once()
         self.assertFalse(device.route.delete_gateway.called)
         device.route.add_gateway.assert_called_once_with('192.168.0.1')
 
     def test_set_default_route_no_subnet(self):
-        device = mock.Mock()
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
-        device.route.get_gateway.return_value = None
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
-
+            device = mock.Mock()
-        # Try a namespace but no subnet.
+            mock_IPDevice.return_value = device
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
+            device.route.get_gateway.return_value = None
-        network = FakeV4NetworkNoSubnet()
+            network = FakeV4NetworkNoSubnet()
-
+            network.namespace = 'qdhcp-1234'
-        dh._set_default_route(network, mock.Mock())
+            dh._set_default_route(network, 'tap-name')
 
         device.route.get_gateway.assert_called_once()
         self.assertFalse(device.route.delete_gateway.called)
         self.assertFalse(device.route.add_gateway.called)
 
     def test_set_default_route_no_subnet_delete_gateway(self):
-        device = mock.Mock()
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
-        device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
-
+            device = mock.Mock()
-        # Try a namespace but no subnet where a gateway needs to be deleted.
+            mock_IPDevice.return_value = device
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
+            device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
-        network = FakeV4NetworkNoSubnet()
+            network = FakeV4NetworkNoSubnet()
-
+            network.namespace = 'qdhcp-1234'
-        dh._set_default_route(network, mock.Mock())
+            dh._set_default_route(network, 'tap-name')
 
         device.route.get_gateway.assert_called_once()
         device.route.delete_gateway.assert_called_once_with('192.168.0.1')
         self.assertFalse(device.route.add_gateway.called)
 
     def test_set_default_route_no_gateway(self):
-        device = mock.Mock()
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
-        device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
-
+            device = mock.Mock()
-        # Try a subnet with no gateway
+            mock_IPDevice.return_value = device
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
+            device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
-        network = FakeV4NetworkNoGateway()
+            network = FakeV4NetworkNoGateway()
-
+            network.namespace = 'qdhcp-1234'
-        dh._set_default_route(network, mock.Mock())
+            dh._set_default_route(network, 'tap-name')
 
         device.route.get_gateway.assert_called_once()
         device.route.delete_gateway.assert_called_once_with('192.168.0.1')
         self.assertFalse(device.route.add_gateway.called)
 
     def test_set_default_route_do_nothing(self):
-        device = mock.Mock()
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
-        device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
-
+            device = mock.Mock()
-        # Try a subnet where the gateway doesn't change.  Should do nothing.
+            mock_IPDevice.return_value = device
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
+            device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
-        network = FakeV4Network()
+            network = FakeV4Network()
-
+            dh._set_default_route(network, 'tap-name')
-        dh._set_default_route(network, mock.Mock())
 
         device.route.get_gateway.assert_called_once()
         self.assertFalse(device.route.delete_gateway.called)
         self.assertFalse(device.route.add_gateway.called)
 
     def test_set_default_route_change_gateway(self):
-        device = mock.Mock()
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
-        device.route.get_gateway.return_value = dict(gateway='192.168.0.2')
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
-
+            device = mock.Mock()
-        # Try a subnet with a gateway this is different than the current.
+            mock_IPDevice.return_value = device
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
+            device.route.get_gateway.return_value = dict(gateway='192.168.0.2')
-        network = FakeV4Network()
+            network = FakeV4Network()
-
+            dh._set_default_route(network, 'tap-name')
-        dh._set_default_route(network, mock.Mock())
 
         device.route.get_gateway.assert_called_once()
         self.assertFalse(device.route.delete_gateway.called)
         device.route.add_gateway.assert_called_once_with('192.168.0.1')
 
     def test_set_default_route_two_subnets(self):
-        device = mock.Mock()
+        # Try two subnets. Should set gateway from the first.
-        device.route.get_gateway.return_value = None
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
-
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
-        # Try two subnets.  Should set gateway from the first.
+            device = mock.Mock()
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
+            mock_IPDevice.return_value = device
-        network = FakeV4Network()
+            device.route.get_gateway.return_value = None
-        subnet2 = FakeV4Subnet()
+            network = FakeV4Network()
-        subnet2.gateway_ip = '192.168.1.1'
+            subnet2 = FakeV4Subnet()
-        network.subnets = [subnet2, FakeV4Subnet()]
+            subnet2.gateway_ip = '192.168.1.1'
-
+            network.subnets = [subnet2, FakeV4Subnet()]
-        dh._set_default_route(network, mock.Mock())
+            dh._set_default_route(network, 'tap-name')
 
         device.route.get_gateway.assert_called_once()
         self.assertFalse(device.route.delete_gateway.called)
         device.route.add_gateway.assert_called_once_with('192.168.1.1')
 
-    def test_get_device_dont_call_get_dhcp_port(self):
-        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
-        dh.get_interface_name = mock.Mock()
-        dh.plugin = mock.Mock()
-        network = mock.Mock()
-        port = mock.Mock()
-
-        dh._get_device(network, port)
-
-        self.assertFalse(dh.plugin.get_dhcp_port.called)
-        dh.get_interface_name.assert_called_once_with(network, port)
-
 
 class TestDictModel(base.BaseTestCase):
     def test_basic_dict(self):

neutron/tests/unit/test_linux_dhcp.py

@@ -15,6 +15,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import contextlib
 import os
 
 import mock
@@ -1016,26 +1017,29 @@ tag:tag1,249,%s,%s""".lstrip() % (fake_v6,
 
         exp_args = ['kill', '-HUP', 5]
 
-        with mock.patch('os.path.isdir') as isdir:
+        fake_net = FakeDualNetwork()
-            isdir.return_value = True
+        dm = dhcp.Dnsmasq(self.conf, fake_net, version=float(2.59))
-            with mock.patch.object(dhcp.Dnsmasq, 'active') as active:
-                active.__get__ = mock.Mock(return_value=True)
-                with mock.patch.object(dhcp.Dnsmasq, 'pid') as pid:
-                    pid.__get__ = mock.Mock(return_value=5)
-                    dm = dhcp.Dnsmasq(self.conf, FakeDualNetwork(),
-                                      version=float(2.59))
 
-                    method_name = '_make_subnet_interface_ip_map'
+        with contextlib.nested(
-                    with mock.patch.object(dhcp.Dnsmasq,
+            mock.patch('os.path.isdir', return_value=True),
-                                           method_name) as ip_map:
+            mock.patch.object(dhcp.Dnsmasq, 'active'),
-                        ip_map.return_value = {}
+            mock.patch.object(dhcp.Dnsmasq, 'pid'),
-                        dm.reload_allocations()
+            mock.patch.object(dhcp.Dnsmasq, 'interface_name'),
-                        self.assertTrue(ip_map.called)
+            mock.patch.object(dhcp.Dnsmasq, '_make_subnet_interface_ip_map'),
+            mock.patch.object(dm, 'device_manager')
+        ) as (isdir, active, pid, interface_name, ip_map, device_manager):
+            active.__get__ = mock.Mock(return_value=True)
+            pid.__get__ = mock.Mock(return_value=5)
+            interface_name.__get__ = mock.Mock(return_value='tap12345678-12')
+            ip_map.return_value = {}
+            dm.reload_allocations()
 
+        self.assertTrue(ip_map.called)
         self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data),
                                     mock.call(exp_addn_name, exp_addn_data),
                                     mock.call(exp_opt_name, exp_opt_data)])
         self.execute.assert_called_once_with(exp_args, 'sudo')
+        device_manager.update.assert_called_with(fake_net, 'tap12345678-12')
 
     def test_reload_allocations_stale_pid(self):
         (exp_host_name, exp_host_data,
@@ -1057,8 +1061,9 @@ tag:tag1,249,%s,%s""".lstrip() % (fake_v6,
                     method_name = '_make_subnet_interface_ip_map'
                     with mock.patch.object(dhcp.Dnsmasq, method_name) as ipmap:
                         ipmap.return_value = {}
-                        dm.reload_allocations()
+                        with mock.patch.object(dhcp.Dnsmasq, 'interface_name'):
-                        self.assertTrue(ipmap.called)
+                            dm.reload_allocations()
+                            self.assertTrue(ipmap.called)
 
             self.safe.assert_has_calls([
                 mock.call(exp_host_name, exp_host_data),