From f07c07b16fb0858c45f6cef135a8d8c07a16c505 Mon Sep 17 00:00:00 2001 From: Carl Baldwin Date: Mon, 12 Sep 2016 15:31:08 -0600 Subject: [PATCH] Don't allocate IP on port update when existing subnet specified If a port update specifies only a subnet_id for a fixed_ip then we want to look at existing fixed_ips to see if that subnet_id is already there. This avoids allocating a new IP address on the subnet and deallocating the old one. Without some special care, this breaks the code path for prefix delegation. One could argue that PD needs reworking. However, as a stop-gap measure, we still run the old code path if the address is an EUI-64 address. This allows PD to continue to work as it was originally written and it doesn't do any harm because allocating EUI-64 addresses is repeatable. This commit removes a test case from the DNS integration tests. The test was specifically testing that DNS records we updated in the case where a subnet id was passed to re-allocate a fixed_ip. Since the use case no longer works, the test doesn't make sense. Change-Id: I887cd23cf65a1df9bd1d59ab897a1ecd005a588c Closes-Bug: #1622616 --- neutron/db/ipam_backend_mixin.py | 16 ++++++- .../tests/unit/db/test_ipam_backend_mixin.py | 45 +++++++++++++++++++ .../ml2/extensions/test_dns_integration.py | 31 ------------- 3 files changed, 60 insertions(+), 32 deletions(-) diff --git a/neutron/db/ipam_backend_mixin.py b/neutron/db/ipam_backend_mixin.py index 3974d208408..d829aaddd75 100644 --- a/neutron/db/ipam_backend_mixin.py +++ b/neutron/db/ipam_backend_mixin.py @@ -432,13 +432,27 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon): for ip in itertools.chain(new_ips, original_ips) if 'ip_address' in ip} + original_subnets = {ip['subnet_id']: ip['ip_address'] + for ip in original_ips} new = set() for ip in new_ips: if ip.get('subnet_id') not in delete_subnet_ids: if 'ip_address' in ip: new.add(ip['ip_address']) else: - add_ips.append(ip) + # A subnet_id is specified without an address + orig_ip = original_subnets.pop(ip['subnet_id'], None) + if orig_ip and not ipv6_utils.is_eui64_address(orig_ip): + # Use the original address on this subnet + new.add(orig_ip) + else: + # In the case where there is no IP, we have to include + # it in add_ips directly. In case of EUI64 address, the + # prefix may have changed so we want to make sure IPAM + # gets a chance to re-allocate it. This is safe in + # general because EUI-64 addresses always come out the + # same given the prefix doesn't change. + add_ips.append(ip) # Convert original ip addresses to sets orig = set(ip['ip_address'] for ip in original_ips) diff --git a/neutron/tests/unit/db/test_ipam_backend_mixin.py b/neutron/tests/unit/db/test_ipam_backend_mixin.py index b281a7f6352..164cb6e66ab 100644 --- a/neutron/tests/unit/db/test_ipam_backend_mixin.py +++ b/neutron/tests/unit/db/test_ipam_backend_mixin.py @@ -167,6 +167,51 @@ class TestIpamBackendMixin(base.BaseTestCase): self._mock_slaac_subnet_on() self._test_get_changed_ips_for_port_no_ip_address() + def test__get_changed_ips_for_port_subnet_id_no_ip(self): + # If a subnet is specified without an IP address only allocate a new + # address if one doesn't exist + self._mock_slaac_subnet_off() + new_ips = [{'subnet_id': 'id-3'}] + original_ips = [{'subnet_id': 'id-3', 'ip_address': '4.3.2.1'}] + + expected_change = self.mixin.Changes( + add=[], + original=[{'subnet_id': 'id-3', 'ip_address': '4.3.2.1'}], + remove=[]) + self._test_get_changed_ips_for_port(expected_change, original_ips, + new_ips, self.owner_non_router) + + def test__get_changed_ips_for_port_subnet_id_no_ip_ipv6(self): + # If a subnet is specified without an IP address only allocate a new + # address if one doesn't exist + self._mock_slaac_subnet_off() + new_ips = [{'subnet_id': 'id-3'}] + original_ips = [{'subnet_id': 'id-3', 'ip_address': '2001:db8::8'}] + + expected_change = self.mixin.Changes( + add=[], + original=[{'subnet_id': 'id-3', 'ip_address': '2001:db8::8'}], + remove=[]) + self._test_get_changed_ips_for_port(expected_change, original_ips, + new_ips, self.owner_non_router) + + def test__get_changed_ips_for_port_subnet_id_no_ip_eui64(self): + # If a subnet is specified without an IP address allocate a new address + # if the address is eui-64. This supports changing prefix when prefix + # delegation is in use. + self._mock_slaac_subnet_off() + new_ips = [{'subnet_id': 'id-3'}] + original_ips = [{'subnet_id': 'id-3', + 'ip_address': '2001::eeb1:d7ff:fe2c:9c5f'}] + + expected_change = self.mixin.Changes( + add=[{'subnet_id': 'id-3'}], + original=[], + remove=[{'subnet_id': 'id-3', + 'ip_address': '2001::eeb1:d7ff:fe2c:9c5f'}]) + self._test_get_changed_ips_for_port(expected_change, original_ips, + new_ips, self.owner_non_router) + def test__is_ip_required_by_subnet_for_router_port(self): # Owner -> router: # _get_subnet should not be called, diff --git a/neutron/tests/unit/plugins/ml2/extensions/test_dns_integration.py b/neutron/tests/unit/plugins/ml2/extensions/test_dns_integration.py index e0ac55aa40a..8459502dd42 100644 --- a/neutron/tests/unit/plugins/ml2/extensions/test_dns_integration.py +++ b/neutron/tests/unit/plugins/ml2/extensions/test_dns_integration.py @@ -369,37 +369,6 @@ class DNSIntegrationTestCase(test_plugin.Ml2PluginV2TestCase): previous_dns_name=DNSNAME, original_ips=original_ips) - def test_update_port_fixed_ips_with_subnet_ids(self, *mocks): - net, port, dns_data_db = self._create_port_for_test() - original_ips = [ip['ip_address'] for ip in port['fixed_ips']] - ctx = context.get_admin_context() - kwargs = {'fixed_ips': []} - for ip in port['fixed_ips']: - # Since this tests using an "any" IP allocation to update the port - # IP address, change the allocation pools so that IPAM won't ever - # give us back the IP address we originally had. - subnet = self.plugin.get_subnet(ctx, ip['subnet_id']) - ip_net = netaddr.IPNetwork(subnet['cidr']) - subnet_size = ip_net.last - ip_net.first - subnet_mid_point = int(ip_net.first + subnet_size / 2) - start, end = (netaddr.IPAddress(subnet_mid_point + 1), - netaddr.IPAddress(ip_net.last - 1)) - allocation_pools = [{'start': str(start), 'end': str(end)}] - body = {'allocation_pools': allocation_pools} - req = self.new_update_request('subnets', {'subnet': body}, - ip['subnet_id']) - req.get_response(self.api) - kwargs['fixed_ips'].append( - {'subnet_id': ip['subnet_id']}) - - port, dns_data_db = self._update_port_for_test(port, - new_dns_name=None, - **kwargs) - self._verify_port_dns(net, port, dns_data_db, delete_records=True, - current_dns_name=DNSNAME, - previous_dns_name=DNSNAME, - original_ips=original_ips) - def test_update_port_fixed_ips_with_new_dns_name(self, *mocks): net, port, dns_data_db = self._create_port_for_test() original_ips = [ip['ip_address'] for ip in port['fixed_ips']]