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

diff --git a/neutron/agent/dhcp_agent.py b/neutron/agent/dhcp_agent.py
index 2054d2fe05abef3c45627f11cd6d1e84de13e37b..80846bc7d7663da3f2fd8f2cf19b2793d83280d4 100644 (file)
--- a/neutron/agent/dhcp_agent.py
+++ b/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.

diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py
index e650c00318d049f79320af5bcb97f43c59b6340e..749048da8bcc64cd67a14cef5b4caba33111a48f 100644 (file)
--- a/neutron/agent/linux/dhcp.py
+++ b/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):
-        """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):
+    def _set_default_route(self, network, device_name):
         """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)
-            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)
+            self._set_default_route(network, device_name)
 
     def destroy(self, network, device_name):
         """Destroy the device used for the network's DHCP on this host."""

diff --git a/neutron/tests/unit/test_dhcp_agent.py b/neutron/tests/unit/test_dhcp_agent.py
index 58836e8e206d5693ba1064b7a47bcd7bab24f53f..e54f303db5f68ee0b79d62d3d75d903f1e190e70 100644 (file)
--- a/neutron/tests/unit/test_dhcp_agent.py
+++ b/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()
-        device.route.get_gateway.return_value = None
-
-        # Basic one subnet with gateway.
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
-        network = FakeV4Network()
-
-        dh._set_default_route(network, mock.Mock())
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
+            device = mock.Mock()
+            mock_IPDevice.return_value = device
+            device.route.get_gateway.return_value = None
+            # Basic one subnet with gateway.
+            network = FakeV4Network()
+            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()
-        device.route.get_gateway.return_value = None
-
-        # Try a namespace but no subnet.
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
-        network = FakeV4NetworkNoSubnet()
-
-        dh._set_default_route(network, mock.Mock())
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
+            device = mock.Mock()
+            mock_IPDevice.return_value = device
+            device.route.get_gateway.return_value = None
+            network = FakeV4NetworkNoSubnet()
+            network.namespace = 'qdhcp-1234'
+            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()
-        device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
-
-        # Try a namespace but no subnet where a gateway needs to be deleted.
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
-        network = FakeV4NetworkNoSubnet()
-
-        dh._set_default_route(network, mock.Mock())
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
+            device = mock.Mock()
+            mock_IPDevice.return_value = device
+            device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
+            network = FakeV4NetworkNoSubnet()
+            network.namespace = 'qdhcp-1234'
+            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()
-        device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
-
-        # Try a subnet with no gateway
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
-        network = FakeV4NetworkNoGateway()
-
-        dh._set_default_route(network, mock.Mock())
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
+            device = mock.Mock()
+            mock_IPDevice.return_value = device
+            device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
+            network = FakeV4NetworkNoGateway()
+            network.namespace = 'qdhcp-1234'
+            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()
-        device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
-
-        # Try a subnet where the gateway doesn't change.  Should do nothing.
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
-        network = FakeV4Network()
-
-        dh._set_default_route(network, mock.Mock())
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
+            device = mock.Mock()
+            mock_IPDevice.return_value = device
+            device.route.get_gateway.return_value = dict(gateway='192.168.0.1')
+            network = FakeV4Network()
+            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_change_gateway(self):
-        device = mock.Mock()
-        device.route.get_gateway.return_value = dict(gateway='192.168.0.2')
-
-        # Try a subnet with a gateway this is different than the current.
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
-        network = FakeV4Network()
-
-        dh._set_default_route(network, mock.Mock())
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
+            device = mock.Mock()
+            mock_IPDevice.return_value = device
+            device.route.get_gateway.return_value = dict(gateway='192.168.0.2')
+            network = FakeV4Network()
+            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_two_subnets(self):
-        device = mock.Mock()
-        device.route.get_gateway.return_value = None
-
-        # Try two subnets.  Should set gateway from the first.
-        dh = self._get_device_manager_with_mock_device(cfg.CONF, device)
-        network = FakeV4Network()
-        subnet2 = FakeV4Subnet()
-        subnet2.gateway_ip = '192.168.1.1'
-        network.subnets = [subnet2, FakeV4Subnet()]
-
-        dh._set_default_route(network, mock.Mock())
+        # Try two subnets. Should set gateway from the first.
+        dh = dhcp.DeviceManager(cfg.CONF, cfg.CONF.root_helper, None)
+        with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice:
+            device = mock.Mock()
+            mock_IPDevice.return_value = device
+            device.route.get_gateway.return_value = None
+            network = FakeV4Network()
+            subnet2 = FakeV4Subnet()
+            subnet2.gateway_ip = '192.168.1.1'
+            network.subnets = [subnet2, FakeV4Subnet()]
+            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):

diff --git a/neutron/tests/unit/test_linux_dhcp.py b/neutron/tests/unit/test_linux_dhcp.py
index 7764bce763999b07406c6f7bcb005c6d005555f1..47912f27e88d3bc44542251fc4f8815e494c24cb 100644 (file)
--- a/neutron/tests/unit/test_linux_dhcp.py
+++ b/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:
-            isdir.return_value = True
-            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 mock.patch.object(dhcp.Dnsmasq,
-                                           method_name) as ip_map:
-                        ip_map.return_value = {}
-                        dm.reload_allocations()
-                        self.assertTrue(ip_map.called)
-
+        fake_net = FakeDualNetwork()
+        dm = dhcp.Dnsmasq(self.conf, fake_net, version=float(2.59))
+
+        with contextlib.nested(
+            mock.patch('os.path.isdir', return_value=True),
+            mock.patch.object(dhcp.Dnsmasq, 'active'),
+            mock.patch.object(dhcp.Dnsmasq, 'pid'),
+            mock.patch.object(dhcp.Dnsmasq, 'interface_name'),
+            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()
-                        self.assertTrue(ipmap.called)
+                        with mock.patch.object(dhcp.Dnsmasq, 'interface_name'):
+                            dm.reload_allocations()
+                            self.assertTrue(ipmap.called)
 
             self.safe.assert_has_calls([
                 mock.call(exp_host_name, exp_host_data),