From 70907b3e5513828c330425d7629dae71d632ac72 Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Mon, 19 Sep 2016 16:02:45 -0700 Subject: [PATCH] Always cleanup stale devices on DHCP setup failure If the DHCP port setup process fails in the DHCP agent device manager, it will throw a conflict exception, which will bubble all of the way up to the main DHCP agent. The issue is that, during a 'restart' call, the config files are wiped out while maintaining the VIF before calling setup. This means that, if setup fails, there is no reference to the interface name anymore so a subsequent destroy will not first unplug the VIF before destroying the namespace. This leaves a bunch of orphaned tap ports behind in the OVS case that don't have an accessible namespace. This patch addresses the issue by cleaning up all ports inside of a namespace on a 'setup' failure before reraising the exception. This ensures that the namespace is clear if destroy is called in the future without another successful setup. Closes-Bug: #1625325 Change-Id: I0211422de51ce6acc6eb593eb890b606101cb9f0 --- neutron/agent/linux/dhcp.py | 29 ++++++++++++++------- neutron/tests/unit/agent/dhcp/test_agent.py | 14 ++++++++++ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index c2fdbb70d04..7f914af4826 100644 --- a/neutron/agent/linux/dhcp.py +++ b/neutron/agent/linux/dhcp.py @@ -1291,14 +1291,22 @@ class DeviceManager(object): network.ports.append(port) def _cleanup_stale_devices(self, network, dhcp_port): + """Unplug any devices found in the namespace except for dhcp_port.""" LOG.debug("Cleaning stale devices for network %s", network.id) - dev_name = self.driver.get_device_name(dhcp_port) + skip_dev_name = (self.driver.get_device_name(dhcp_port) + if dhcp_port else None) ns_ip = ip_lib.IPWrapper(namespace=network.namespace) + if not ns_ip.netns.exists(network.namespace): + return for d in ns_ip.get_devices(exclude_loopback=True): # delete all devices except current active DHCP port device - if d.name != dev_name: + if d.name != skip_dev_name: LOG.debug("Found stale device %s, deleting", d.name) - self.unplug(d.name, network) + try: + self.unplug(d.name, network) + except Exception: + LOG.exception(_LE("Exception during stale " + "dhcp device cleanup")) def plug(self, network, port, interface_name): """Plug device settings for the network's DHCP on this host.""" @@ -1311,7 +1319,13 @@ class DeviceManager(object): def setup(self, network): """Create and initialize a device for network's DHCP on this host.""" - port = self.setup_dhcp_port(network) + try: + port = self.setup_dhcp_port(network) + except Exception: + with excutils.save_and_reraise_exception(): + # clear everything out so we don't leave dangling interfaces + # if setup never succeeds in the future. + self._cleanup_stale_devices(network, dhcp_port=None) self._update_dhcp_port(network, port) interface_name = self.get_interface_name(network, port) @@ -1355,12 +1369,7 @@ class DeviceManager(object): namespace=network.namespace) self._set_default_route(network, interface_name) - try: - self._cleanup_stale_devices(network, port) - except Exception: - # catch everything as we don't want to fail because of - # cleanup step - LOG.error(_LE("Exception during stale dhcp device cleanup")) + self._cleanup_stale_devices(network, port) return interface_name diff --git a/neutron/tests/unit/agent/dhcp/test_agent.py b/neutron/tests/unit/agent/dhcp/test_agent.py index 657f7ec03da..b3b2a038287 100644 --- a/neutron/tests/unit/agent/dhcp/test_agent.py +++ b/neutron/tests/unit/agent/dhcp/test_agent.py @@ -1441,6 +1441,20 @@ class TestDeviceManager(base.BaseTestCase): expected = [mock.call.add_rule('POSTROUTING', rule)] self.mangle_inst.assert_has_calls(expected) + def test_setup_dhcp_port_doesnt_orphan_devices(self): + with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice: + plugin = mock.Mock() + device = mock.Mock() + mock_IPDevice.return_value = device + device.route.get_gateway.return_value = None + net = copy.deepcopy(fake_network) + plugin.create_dhcp_port.side_effect = exceptions.Conflict() + dh = dhcp.DeviceManager(cfg.CONF, plugin) + clean = mock.patch.object(dh, '_cleanup_stale_devices').start() + with testtools.ExpectedException(exceptions.Conflict): + dh.setup(net) + clean.assert_called_once_with(net, dhcp_port=None) + def test_setup_create_dhcp_port(self): with mock.patch.object(dhcp.ip_lib, 'IPDevice') as mock_IPDevice: plugin = mock.Mock()