From d1fb4238306f854ab610204a50c5cdc20ea5c770 Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Thu, 22 Sep 2016 23:38:33 -0700 Subject: [PATCH] Fixes for missing IPs on subnets in DHCP agent There is a race condition server-side where a port request containing a subnet_id is processed at the same time the subnet is being deleted, the port operation may be successful without having a fixed IP on the requested subnet. This patch makes the DHCP agent resillient to this bug by checking the port response and raising a SubnetMismatchForPort to trigger a resync if it doesn't have all of the requested subnet IDs. Additionally, it avoids skipping assignment of IPv6 addresses to the interface if they are stateless. The original logic to skip assignment was only meant to be for SLAAC addresses. Both of these issues were resulting in the KeyError observed in the bug report. Related-Bug: #1627480 Closes-Bug: #1624079 Change-Id: I85ef1f4d60efd0309d6a0706e29fdbcc16f0b59d --- neutron/agent/dhcp/agent.py | 4 ++++ neutron/agent/linux/dhcp.py | 15 ++++++++++++++- neutron/common/ipv6_utils.py | 6 ++++++ neutron/tests/unit/agent/dhcp/test_agent.py | 17 ++++++++++++++++- neutron/tests/unit/agent/linux/test_dhcp.py | 16 +++++++++------- 5 files changed, 49 insertions(+), 9 deletions(-) diff --git a/neutron/agent/dhcp/agent.py b/neutron/agent/dhcp/agent.py index 6f363e0d90e..9a9680a9fa0 100644 --- a/neutron/agent/dhcp/agent.py +++ b/neutron/agent/dhcp/agent.py @@ -120,6 +120,10 @@ class DhcpAgent(manager.Manager): 'is a conflict with its current state; please ' 'check that the network and/or its subnet(s) ' 'still exist.', {'net_id': network.id, 'action': action}) + except exceptions.SubnetMismatchForPort as e: + # FIXME(kevinbenton): get rid of this once bug/1627480 is fixed + LOG.debug("Error configuring DHCP port, scheduling resync: %s", e) + self.schedule_resync(e, network.id) except Exception as e: if getattr(e, 'exc_type', '') != 'IpAddressGenerationFailure': # Don't resync if port could not be created because of an IP diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index 94221bc3bd8..d30daf3903c 100644 --- a/neutron/agent/linux/dhcp.py +++ b/neutron/agent/linux/dhcp.py @@ -1280,6 +1280,19 @@ class DeviceManager(object): else: raise exceptions.Conflict() + # FIXME(kevinbenton): ensure we have the IPs we actually need. + # can be removed once bug/1627480 is fixed + if not self.driver.use_gateway_ips: + expected = set(dhcp_subnets) + actual = {fip.subnet_id for fip in dhcp_port.fixed_ips} + missing = expected - actual + if missing: + LOG.debug("Requested DHCP port with IPs on subnets " + "%(expected)s but only got IPs on subnets " + "%(actual)s.", {'expected': expected, + 'actual': actual}) + raise exceptions.SubnetMismatchForPort( + port_id=dhcp_port.id, subnet_id=list(missing)[0]) # Convert subnet_id to subnet dict fixed_ips = [dict(subnet_id=fixed_ip.subnet_id, ip_address=fixed_ip.ip_address, @@ -1359,7 +1372,7 @@ class DeviceManager(object): ip_cidrs = [] for fixed_ip in port.fixed_ips: subnet = fixed_ip.subnet - if not ipv6_utils.is_auto_address_subnet(subnet): + if not ipv6_utils.is_slaac_subnet(subnet): net = netaddr.IPNetwork(subnet.cidr) ip_cidr = '%s/%s' % (fixed_ip.ip_address, net.prefixlen) ip_cidrs.append(ip_cidr) diff --git a/neutron/common/ipv6_utils.py b/neutron/common/ipv6_utils.py index f54ef4c759b..aee35db13ad 100644 --- a/neutron/common/ipv6_utils.py +++ b/neutron/common/ipv6_utils.py @@ -76,6 +76,12 @@ def is_auto_address_subnet(subnet): or subnet['ipv6_ra_mode'] in modes) +def is_slaac_subnet(subnet): + """Check if subnet is a slaac subnet.""" + return (subnet['ipv6_address_mode'] == const.IPV6_SLAAC + or subnet['ipv6_ra_mode'] == const.IPV6_SLAAC) + + def is_eui64_address(ip_address): """Check if ip address is EUI64.""" ip = netaddr.IPAddress(ip_address) diff --git a/neutron/tests/unit/agent/dhcp/test_agent.py b/neutron/tests/unit/agent/dhcp/test_agent.py index 12ea6b82c47..7b7dee163a6 100644 --- a/neutron/tests/unit/agent/dhcp/test_agent.py +++ b/neutron/tests/unit/agent/dhcp/test_agent.py @@ -85,6 +85,8 @@ fake_meta_subnet = dhcp.DictModel(dict(id='bbbbbbbb-1111-2222-bbbbbbbbbbbb', fake_fixed_ip1 = dhcp.DictModel(dict(id='', subnet_id=fake_subnet1.id, ip_address='172.9.9.9')) +fake_fixed_ip_subnet2 = dhcp.DictModel(dict(id='', subnet_id=fake_subnet2.id, + ip_address='172.9.8.9')) fake_fixed_ip2 = dhcp.DictModel(dict(id='', subnet_id=fake_subnet1.id, ip_address='172.9.9.10')) fake_fixed_ipv6 = dhcp.DictModel(dict(id='', subnet_id=fake_ipv6_subnet.id, @@ -1531,13 +1533,26 @@ class TestDeviceManager(base.BaseTestCase): [{'subnet_id': fake_fixed_ip1.subnet_id}], 'device_id': mock.ANY}})]) - def test_create_dhcp_port_update_add_subnet(self): + def test_create_dhcp_port_update_add_subnet_bug_1627480(self): + # this can go away once bug/1627480 is fixed plugin = mock.Mock() dh = dhcp.DeviceManager(cfg.CONF, plugin) fake_network_copy = copy.deepcopy(fake_network) fake_network_copy.ports[0].device_id = dh.get_device_id(fake_network) fake_network_copy.subnets[1].enable_dhcp = True plugin.update_dhcp_port.return_value = fake_network.ports[0] + with testtools.ExpectedException(exceptions.SubnetMismatchForPort): + dh.setup_dhcp_port(fake_network_copy) + + def test_create_dhcp_port_update_add_subnet(self): + plugin = mock.Mock() + dh = dhcp.DeviceManager(cfg.CONF, plugin) + fake_network_copy = copy.deepcopy(fake_network) + fake_network_copy.ports[0].device_id = dh.get_device_id(fake_network) + fake_network_copy.subnets[1].enable_dhcp = True + updated_port = copy.deepcopy(fake_network_copy.ports[0]) + updated_port.fixed_ips.append(fake_fixed_ip_subnet2) + plugin.update_dhcp_port.return_value = updated_port dh.setup_dhcp_port(fake_network_copy) port_body = {'port': { 'network_id': fake_network.id, diff --git a/neutron/tests/unit/agent/linux/test_dhcp.py b/neutron/tests/unit/agent/linux/test_dhcp.py index f6ca3b100c5..636377fd004 100644 --- a/neutron/tests/unit/agent/linux/test_dhcp.py +++ b/neutron/tests/unit/agent/linux/test_dhcp.py @@ -86,7 +86,9 @@ class FakeReservedPort(object): self.device_owner = constants.DEVICE_OWNER_DHCP self.fixed_ips = [ FakeIPAllocation('192.168.0.6', - 'dddddddd-dddd-dddd-dddd-dddddddddddd')] + 'dddddddd-dddd-dddd-dddd-dddddddddddd'), + FakeIPAllocation('fdca:3ba5:a17a:4ba3::2', + 'ffffffff-ffff-ffff-ffff-ffffffffffff')] self.mac_address = '00:00:80:aa:bb:ee' self.device_id = n_const.DEVICE_ID_RESERVED_DHCP_PORT self.extra_dhcp_opts = [] @@ -2259,11 +2261,11 @@ class TestDeviceManager(TestConfBase): plugin.update_dhcp_port.assert_called_with(reserved_port.id, mock.ANY) - except_ips = ['192.168.0.6/24'] + expect_ips = ['192.168.0.6/24', 'fdca:3ba5:a17a:4ba3::2/64'] if enable_isolated_metadata or force_metadata: - except_ips.append(dhcp.METADATA_DEFAULT_CIDR) + expect_ips.append(dhcp.METADATA_DEFAULT_CIDR) mgr.driver.init_l3.assert_called_with('ns-XXX', - except_ips, + expect_ips, namespace='qdhcp-ns') def test_setup_reserved_and_disable_metadata(self): @@ -2331,9 +2333,9 @@ class TestDeviceManager(TestConfBase): plugin.update_dhcp_port.assert_called_with(reserved_port_2.id, mock.ANY) - mgr.driver.init_l3.assert_called_with('ns-XXX', - ['192.168.0.6/24'], - namespace='qdhcp-ns') + mgr.driver.init_l3.assert_called_with( + 'ns-XXX', ['192.168.0.6/24', 'fdca:3ba5:a17a:4ba3::2/64'], + namespace='qdhcp-ns') class TestDictModel(base.BaseTestCase):