Try to reuse existing IPs when a port update specifies subnet
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. This commit also preserves the ability to add an additional IP from a subnet for which the port already has IPs. Change-Id: Iba5d54efa7f99ed82275ffc8e5be975b373c29d3 Related-Bug: #1622616 Closes-Bug: #1625334
This commit is contained in:
Carl Baldwin
parent
7c6071bbbc
commit
14fee9cfcc
|
|
@ -426,44 +426,69 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon):
|
|||
# the new_ips contain all of the fixed_ips that are to be updated
|
||||
self._validate_max_ips_per_port(ips, device_owner)
|
||||
|
||||
add_ips = []
|
||||
remove_ips = []
|
||||
add_ips, prev_ips, remove_candidates = [], [], []
|
||||
|
||||
ips_map = {ip['ip_address']: ip
|
||||
for ip in itertools.chain(new_ips, original_ips)
|
||||
if 'ip_address' in ip}
|
||||
# Consider fixed_ips that specify a specific address first to see if
|
||||
# they already existed in original_ips or are completely new.
|
||||
orig_by_ip = {ip['ip_address']: ip for ip in original_ips}
|
||||
for ip in ips:
|
||||
if 'ip_address' not in ip:
|
||||
continue
|
||||
|
||||
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)
|
||||
original = orig_by_ip.pop(ip['ip_address'], None)
|
||||
if original:
|
||||
prev_ips.append(original)
|
||||
else:
|
||||
add_ips.append(ip)
|
||||
|
||||
# Convert original ip addresses to sets
|
||||
orig = set(ip['ip_address'] for ip in original_ips)
|
||||
# Consider fixed_ips that don't specify ip_address. Try to match them
|
||||
# up with originals to see if they can be reused. Create a new map of
|
||||
# the remaining, unmatched originals for this step.
|
||||
orig_by_subnet = collections.defaultdict(list)
|
||||
for ip in orig_by_ip.values():
|
||||
orig_by_subnet[ip['subnet_id']].append(ip)
|
||||
|
||||
add = new - orig
|
||||
unchanged = new & orig
|
||||
remove = orig - new
|
||||
for ip in ips:
|
||||
if 'ip_address' in ip:
|
||||
continue
|
||||
|
||||
# Convert results back to list of dicts
|
||||
add_ips += [ips_map[ip] for ip in add]
|
||||
prev_ips = [ips_map[ip] for ip in unchanged]
|
||||
orig = orig_by_subnet.get(ip['subnet_id'])
|
||||
if not orig:
|
||||
add_ips.append(ip)
|
||||
continue
|
||||
|
||||
# Try to match this new request up with an existing IP
|
||||
orig_ip = orig.pop()
|
||||
if ipv6_utils.is_eui64_address(orig_ip['ip_address']):
|
||||
# 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)
|
||||
remove_candidates.append(orig_ip)
|
||||
else:
|
||||
# Reuse the existing address on this subnet.
|
||||
prev_ips.append(orig_ip)
|
||||
|
||||
# Iterate through any unclaimed original ips (orig_by_subnet) *and* the
|
||||
# remove_candidates with this compound chain.
|
||||
maybe_remove = itertools.chain(
|
||||
itertools.chain.from_iterable(orig_by_subnet.values()),
|
||||
remove_candidates)
|
||||
|
||||
# Mark ip for removing if it is not found in new_ips
|
||||
# and subnet requires ip to be set manually.
|
||||
# For auto addressed subnet leave ip unchanged
|
||||
# unless it is explicitly marked for delete.
|
||||
for ip in remove:
|
||||
subnet_id = ips_map[ip]['subnet_id']
|
||||
remove_ips = []
|
||||
for ip in maybe_remove:
|
||||
subnet_id = ip['subnet_id']
|
||||
ip_required = self._is_ip_required_by_subnet(context, subnet_id,
|
||||
device_owner)
|
||||
if ip_required or subnet_id in delete_subnet_ids:
|
||||
remove_ips.append(ips_map[ip])
|
||||
remove_ips.append(ip)
|
||||
else:
|
||||
prev_ips.append(ips_map[ip])
|
||||
prev_ips.append(ip)
|
||||
|
||||
return self.Changes(add=add_ips,
|
||||
original=prev_ips,
|
||||
|
|
|
|||
|
|
@ -71,13 +71,24 @@ class TestIpamBackendMixin(base.BaseTestCase):
|
|||
|
||||
self.mixin._get_subnet = mock.Mock(side_effect=_get_subnet)
|
||||
|
||||
def _test_get_changed_ips_for_port(self, expected_change, original_ips,
|
||||
def _test_get_changed_ips_for_port(self, expected, original_ips,
|
||||
new_ips, owner):
|
||||
change = self.mixin._get_changed_ips_for_port(self.ctx,
|
||||
original_ips,
|
||||
new_ips,
|
||||
owner)
|
||||
self.assertEqual(expected_change, change)
|
||||
|
||||
def assertUnorderedListOfDictEqual(a, b):
|
||||
# Dicts are unorderable in py34. Return something orderable.
|
||||
def key(d):
|
||||
return sorted(d.items())
|
||||
|
||||
# Compare the sorted lists since the order isn't deterministic
|
||||
self.assertEqual(sorted(a, key=key), sorted(b, key=key))
|
||||
|
||||
assertUnorderedListOfDictEqual(expected.add, change.add)
|
||||
assertUnorderedListOfDictEqual(expected.original, change.original)
|
||||
assertUnorderedListOfDictEqual(expected.remove, change.remove)
|
||||
|
||||
def test__get_changed_ips_for_port(self):
|
||||
new_ips = self._prepare_ips(self.default_new_ips)
|
||||
|
|
@ -167,6 +178,86 @@ 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_multiple_ips_one_subnet_add_third(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', 'ip_address': '4.3.2.1'},
|
||||
{'subnet_id': 'id-3'},
|
||||
{'subnet_id': 'id-3', 'ip_address': '4.3.2.10'}]
|
||||
original_ips = [{'subnet_id': 'id-3', 'ip_address': '4.3.2.1'},
|
||||
{'subnet_id': 'id-3', 'ip_address': '4.3.2.10'}]
|
||||
|
||||
expected_change = self.mixin.Changes(
|
||||
add=[{'subnet_id': 'id-3'}],
|
||||
original=[{'subnet_id': 'id-3', 'ip_address': '4.3.2.1'},
|
||||
{'subnet_id': 'id-3', 'ip_address': '4.3.2.10'}],
|
||||
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_multiple_ips_one_subnet_noip(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'},
|
||||
{'subnet_id': 'id-3'}]
|
||||
original_ips = [{'subnet_id': 'id-3', 'ip_address': '4.3.2.1'},
|
||||
{'subnet_id': 'id-3', 'ip_address': '4.3.2.10'}]
|
||||
|
||||
expected_change = self.mixin.Changes(
|
||||
add=[],
|
||||
original=[{'subnet_id': 'id-3', 'ip_address': '4.3.2.1'},
|
||||
{'subnet_id': 'id-3', 'ip_address': '4.3.2.10'}],
|
||||
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,
|
||||
|
|
|
|||
|
|
@ -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']]
|
||||
|
|
|
|||
Loading…
Reference in New Issue