From 9dbd1e5e5a41eff88a044b8de992d2f1f14898b3 Mon Sep 17 00:00:00 2001 From: Darragh O'Reilly Date: Sun, 9 Mar 2014 15:14:03 +0000 Subject: [PATCH] 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 | 2 + neutron/agent/linux/dhcp.py | 26 ++--- neutron/tests/unit/test_dhcp_agent.py | 156 +++++++++++--------------- neutron/tests/unit/test_linux_dhcp.py | 37 +++--- 4 files changed, 97 insertions(+), 124 deletions(-) diff --git a/neutron/agent/dhcp_agent.py b/neutron/agent/dhcp_agent.py index 2054d2fe05..80846bc7d7 100644 --- 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 e650c00318..749048da8b 100644 --- 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 58836e8e20..e54f303db5 100644 --- 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 7764bce763..47912f27e8 100644 --- 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)) + fake_net = FakeDualNetwork() + dm = dhcp.Dnsmasq(self.conf, fake_net, 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) + 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),