From b937f2c2aa16c4b70842e006d655bab7e2bdcb72 Mon Sep 17 00:00:00 2001 From: Carl Baldwin Date: Fri, 20 May 2016 22:08:37 +0000 Subject: [PATCH] Make IPAM segment aware on port update With this change, Neutron will perform late port binding in the case where no IPs were assigned on port create (because no host binding was provided then). It will filter subnets based on the binding host provider on port update. Change-Id: I90046136e495f3612bbe213483aba2605c66f787 Partially-Implements: blueprint routed-networks --- neutron/db/db_base_plugin_v2.py | 19 +- neutron/db/ipam_backend_mixin.py | 40 ++++ neutron/db/ipam_non_pluggable_backend.py | 8 +- neutron/db/ipam_pluggable_backend.py | 7 +- .../unit/db/test_ipam_pluggable_backend.py | 3 +- neutron/tests/unit/extensions/test_segment.py | 211 +++++++++++++++++- 6 files changed, 270 insertions(+), 18 deletions(-) diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index 6622776f5b7..e35e8990875 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -1297,8 +1297,23 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, new_port['dns_name'] = '' new_mac = new_port.get('mac_address') self._validate_port_for_update(context, db_port, new_port, new_mac) - changes = self.ipam.update_port_with_ips(context, db_port, - new_port, new_mac) + # Note: _make_port_dict is called here to load extension data + # (specifically host binding). The IPAM plugin is separate from + # the core plugin, so extensions are not loaded. + # + # The IPAM code could cheat and get it directly from db_port but it + # would have to know about the implementation (remember ml2 has its + # own port binding schema that differs from the generic one) + # + # This code could extract just the port binding host here and pass + # that in. The problem is that db_base_plugin_common shouldn't + # know anything about port binding. This compromise sends IPAM a + # port_dict with all of the extension data loaded. + changes = self.ipam.update_port( + context, + old_port_db=db_port, + old_port=self._make_port_dict(db_port), + new_port=new_port) if 'dns-integration' in self.supported_extension_aliases: dns_assignment = self._get_dns_names_for_updated_port( context, original_ips, original_dns_name, diff --git a/neutron/db/ipam_backend_mixin.py b/neutron/db/ipam_backend_mixin.py index 00254e8fdb7..3d7ac16033b 100644 --- a/neutron/db/ipam_backend_mixin.py +++ b/neutron/db/ipam_backend_mixin.py @@ -14,6 +14,7 @@ # under the License. import collections +import copy import itertools import netaddr @@ -34,6 +35,7 @@ from neutron.common import utils as common_utils from neutron.db import db_base_plugin_common from neutron.db import models_v2 from neutron.db import segments_db +from neutron.extensions import portbindings from neutron.extensions import segment from neutron.ipam import utils as ipam_utils from neutron.services.segments import db as segment_svc_db @@ -594,3 +596,41 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon): if validators.is_attr_set(subnet.get(segment.SEGMENT_ID)): args['segment_id'] = subnet[segment.SEGMENT_ID] return args + + def update_port(self, context, old_port_db, old_port, new_port): + """Update the port IPs + + Updates the port's IPs based on any new fixed_ips passed in or if + deferred IP allocation is in effect because allocation requires host + binding information that wasn't provided until port update. + + :param old_port_db: The port database record + :param old_port: A port dict created by calling _make_port_dict. This + must be called before calling this method in order to + load data from extensions, specifically host binding. + :param new_port: The new port data passed through the API. + """ + old_host = old_port.get(portbindings.HOST_ID) + new_host = new_port.get(portbindings.HOST_ID) + host = new_host if validators.is_attr_set(new_host) else old_host + + changes = self.update_port_with_ips(context, + host, + old_port_db, + new_port, + new_port.get('mac_address')) + + fixed_ips_requested = validators.is_attr_set(new_port.get('fixed_ips')) + deferred_ip_allocation = (host and not old_host + and not old_port.get('fixed_ips') + and not fixed_ips_requested) + if not deferred_ip_allocation: + return changes + + # Allocate as if this were the port create. + port_copy = copy.deepcopy(old_port) + port_copy['fixed_ips'] = const.ATTR_NOT_SPECIFIED + port_copy.update(new_port) + ips = self.allocate_ips_for_port_and_store( + context, {'port': port_copy}, port_copy['id']) + return self.Changes(add=ips, original=[], remove=[]) diff --git a/neutron/db/ipam_non_pluggable_backend.py b/neutron/db/ipam_non_pluggable_backend.py index 3d8fbaac472..6ad064e3b49 100644 --- a/neutron/db/ipam_non_pluggable_backend.py +++ b/neutron/db/ipam_non_pluggable_backend.py @@ -211,14 +211,14 @@ class IpamNonPluggableBackend(ipam_backend_mixin.IpamBackendMixin): subnet_id, port_id) return ips - def update_port_with_ips(self, context, db_port, new_port, new_mac): + def update_port_with_ips(self, context, host, db_port, new_port, new_mac): changes = self.Changes(add=[], original=[], remove=[]) # Check if the IPs need to be updated network_id = db_port['network_id'] if 'fixed_ips' in new_port: original = self._make_port_dict(db_port, process_extensions=False) changes = self._update_ips_for_port( - context, network_id, + context, network_id, host, original["fixed_ips"], new_port['fixed_ips'], original['mac_address'], db_port['device_owner']) @@ -313,14 +313,14 @@ class IpamNonPluggableBackend(ipam_backend_mixin.IpamBackendMixin): 'subnet_id': result['subnet_id']}) return ips - def _update_ips_for_port(self, context, network_id, original_ips, + def _update_ips_for_port(self, context, network_id, host, original_ips, new_ips, mac_address, device_owner): """Add or remove IPs from the port.""" added = [] changes = self._get_changed_ips_for_port(context, original_ips, new_ips, device_owner) subnets = self._ipam_get_subnets( - context, network_id=network_id, host=None) + context, network_id=network_id, host=host) # Check if the IP's to add are OK to_add = self._test_fixed_ips_for_port(context, network_id, changes.add, device_owner, diff --git a/neutron/db/ipam_pluggable_backend.py b/neutron/db/ipam_pluggable_backend.py index 8b9581b0560..df3df176b63 100644 --- a/neutron/db/ipam_pluggable_backend.py +++ b/neutron/db/ipam_pluggable_backend.py @@ -263,7 +263,7 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): self._validate_max_ips_per_port(fixed_ip_list, device_owner) return fixed_ip_list - def _update_ips_for_port(self, context, port, + def _update_ips_for_port(self, context, port, host, original_ips, new_ips, mac): """Add or remove IPs from the port. IPAM version""" added = [] @@ -271,7 +271,7 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): changes = self._get_changed_ips_for_port( context, original_ips, new_ips, port['device_owner']) subnets = self._ipam_get_subnets( - context, network_id=port['network_id'], host=None) + context, network_id=port['network_id'], host=host) # Check if the IP's to add are OK to_add = self._test_fixed_ips_for_port( context, port['network_id'], changes.add, @@ -301,7 +301,7 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): last_ip=last_ip) context.session.add(ip_pool) - def update_port_with_ips(self, context, db_port, new_port, new_mac): + def update_port_with_ips(self, context, host, db_port, new_port, new_mac): changes = self.Changes(add=[], original=[], remove=[]) if 'fixed_ips' in new_port: @@ -309,6 +309,7 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): process_extensions=False) changes = self._update_ips_for_port(context, db_port, + host, original["fixed_ips"], new_port['fixed_ips'], new_mac) diff --git a/neutron/tests/unit/db/test_ipam_pluggable_backend.py b/neutron/tests/unit/db/test_ipam_pluggable_backend.py index 04c942b8457..57a02e69f2f 100644 --- a/neutron/tests/unit/db/test_ipam_pluggable_backend.py +++ b/neutron/tests/unit/db/test_ipam_pluggable_backend.py @@ -617,7 +617,7 @@ class TestDbBasePluginIpam(test_db_base.NeutronDbPluginV2TestCase): port_dict = {'device_owner': uuidutils.generate_uuid(), 'network_id': uuidutils.generate_uuid()} - mocks['ipam']._update_ips_for_port(context, port_dict, + mocks['ipam']._update_ips_for_port(context, port_dict, None, original_ips, new_ips, mac) mocks['driver'].get_address_request_factory.assert_called_once_with() mocks['ipam']._ipam_get_subnets.assert_called_once_with( @@ -752,6 +752,7 @@ class TestDbBasePluginIpam(test_db_base.NeutronDbPluginV2TestCase): self.assertRaises(db_exc.DBDeadlock, mocks['ipam'].update_port_with_ips, context, + None, db_port, new_port, mock.Mock()) diff --git a/neutron/tests/unit/extensions/test_segment.py b/neutron/tests/unit/extensions/test_segment.py index 9f989c549c1..0d2be909ea5 100644 --- a/neutron/tests/unit/extensions/test_segment.py +++ b/neutron/tests/unit/extensions/test_segment.py @@ -22,6 +22,7 @@ from neutron.api.v2 import attributes from neutron import context from neutron.db import agents_db from neutron.db import db_base_plugin_v2 +from neutron.db import portbindings_db from neutron.db import segments_db from neutron.extensions import portbindings from neutron.extensions import segment as ext_segment @@ -100,6 +101,7 @@ class SegmentTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): class SegmentTestPlugin(db_base_plugin_v2.NeutronDbPluginV2, + portbindings_db.PortBindingMixin, db.SegmentDbMixin): __native_pagination_support = True __native_sorting_support = True @@ -112,6 +114,12 @@ class SegmentTestPlugin(db_base_plugin_v2.NeutronDbPluginV2, def get_plugin_type(self): return "segments" + def create_port(self, context, port): + port_dict = super(SegmentTestPlugin, self).create_port(context, port) + self._process_portbindings_create_and_update( + context, port['port'], port_dict) + return port_dict + class TestSegment(SegmentTestCase): @@ -534,6 +542,13 @@ class TestSegmentAwareIpam(SegmentTestCase): # Don't allocate IPs in this case because we didn't give binding info self.assertEqual(0, len(res['port']['fixed_ips'])) + def _assert_one_ip_in_subnet(self, response, cidr): + res = self.deserialize(self.fmt, response) + self.assertEqual(1, len(res['port']['fixed_ips'])) + ip = res['port']['fixed_ips'][0]['ip_address'] + ip_net = netaddr.IPNetwork(cidr) + self.assertIn(ip, ip_net) + def test_port_create_with_binding_information(self): """Binding information is provided, subnets are on segments""" network, segments, subnets = self._create_test_segments_with_subnets(3) @@ -549,12 +564,9 @@ class TestSegmentAwareIpam(SegmentTestCase): tenant_id=network['network']['tenant_id'], arg_list=(portbindings.HOST_ID,), **{portbindings.HOST_ID: 'fakehost'}) - res = self.deserialize(self.fmt, response) # Since host mapped to middle segment, IP must come from middle subnet - ip = res['port']['fixed_ips'][0]['ip_address'] - ip_net = netaddr.IPNetwork(subnets[1]['subnet']['cidr']) - self.assertIn(ip, ip_net) + self._assert_one_ip_in_subnet(response, subnets[1]['subnet']['cidr']) def test_port_create_with_binding_and_no_subnets(self): """Binding information is provided, no subnets.""" @@ -594,12 +606,9 @@ class TestSegmentAwareIpam(SegmentTestCase): tenant_id=network['network']['tenant_id'], arg_list=(portbindings.HOST_ID,), **{portbindings.HOST_ID: 'fakehost'}) - res = self.deserialize(self.fmt, response) # Since the subnet is not on a segment, fall back to it - ip = res['port']['fixed_ips'][0]['ip_address'] - ip_net = netaddr.IPNetwork(subnet['subnet']['cidr']) - self.assertIn(ip, ip_net) + self._assert_one_ip_in_subnet(response, subnet['subnet']['cidr']) def test_port_create_on_unconnected_host(self): """Binding information provided, host not connected to any segment""" @@ -646,3 +655,189 @@ class TestSegmentAwareIpam(SegmentTestCase): self.assertEqual(webob.exc.HTTPConflict.code, response.status_int) self.assertEqual(segment_exc.HostConnectedToMultipleSegments.__name__, res['NeutronError']['type']) + + def test_port_update_excludes_hosts_on_segments(self): + """No binding information is provided, subnets on segments""" + with self.network() as network: + segment = self._test_create_segment( + network_id=network['network']['id'], + physical_network='physnet') + + # Create a port with no IP address (since there is no subnet) + port = self._create_deferred_ip_port(network) + + # Create the subnet and try to update the port to get an IP + with self.subnet(network=network, + segment_id=segment['segment']['id']) as subnet: + # Try requesting an IP (but the only subnet is on a segment) + data = {'port': { + 'fixed_ips': [{'subnet_id': subnet['subnet']['id']}]}} + port_id = port['port']['id'] + port_req = self.new_update_request('ports', data, port_id) + response = port_req.get_response(self.api) + + # Gets bad request because there are no eligible subnets. + self.assertEqual(webob.exc.HTTPBadRequest.code, response.status_int) + + def test_port_update_is_host_aware(self): + """Binding information is provided, subnets on segments""" + with self.network() as network: + segment = self._test_create_segment( + network_id=network['network']['id'], + physical_network='physnet') + + # Map the host to the segment + self._setup_host_mappings([(segment['segment']['id'], 'fakehost')]) + + # Create a bound port with no IP address (since there is no subnet) + response = self._create_port(self.fmt, + net_id=network['network']['id'], + tenant_id=network['network']['tenant_id'], + arg_list=(portbindings.HOST_ID,), + **{portbindings.HOST_ID: 'fakehost'}) + port = self.deserialize(self.fmt, response) + + # Create the subnet and try to update the port to get an IP + with self.subnet(network=network, + segment_id=segment['segment']['id']) as subnet: + # Try requesting an IP (but the only subnet is on a segment) + data = {'port': { + 'fixed_ips': [{'subnet_id': subnet['subnet']['id']}]}} + port_id = port['port']['id'] + port_req = self.new_update_request('ports', data, port_id) + response = port_req.get_response(self.api) + + # Since port is bound and there is a mapping to segment, it succeeds. + self.assertEqual(webob.exc.HTTPOk.code, response.status_int) + self._assert_one_ip_in_subnet(response, subnet['subnet']['cidr']) + + def _create_deferred_ip_port(self, network): + response = self._create_port(self.fmt, + net_id=network['network']['id'], + tenant_id=network['network']['tenant_id']) + port = self.deserialize(self.fmt, response) + ips = port['port']['fixed_ips'] + self.assertEqual(0, len(ips)) + return port + + def test_port_update_deferred_allocation(self): + """Binding information is provided on update, subnets on segments""" + network, segment, subnet = self._create_test_segment_with_subnet() + + # Map the host to the segment + self._setup_host_mappings([(segment['segment']['id'], 'fakehost')]) + + port = self._create_deferred_ip_port(network) + + # Try requesting an IP (but the only subnet is on a segment) + data = {'port': {portbindings.HOST_ID: 'fakehost'}} + port_id = port['port']['id'] + port_req = self.new_update_request('ports', data, port_id) + response = port_req.get_response(self.api) + + # Port update succeeds and allocates a new IP address. + self.assertEqual(webob.exc.HTTPOk.code, response.status_int) + self._assert_one_ip_in_subnet(response, subnet['subnet']['cidr']) + + def test_port_update_deferred_allocation_no_segments(self): + """Binding information is provided, subnet created after port""" + with self.network() as network: + pass + + port = self._create_deferred_ip_port(network) + + # Create the subnet and try to update the port to get an IP + with self.subnet(network=network) as subnet: + data = {'port': {portbindings.HOST_ID: 'fakehost'}} + port_id = port['port']['id'] + port_req = self.new_update_request('ports', data, port_id) + response = port_req.get_response(self.api) + + self.assertEqual(webob.exc.HTTPOk.code, response.status_int) + self._assert_one_ip_in_subnet(response, subnet['subnet']['cidr']) + + def test_port_update_deferred_allocation_no_segments_manual_alloc(self): + """Binding information is provided, subnet created after port""" + with self.network() as network: + pass + + port = self._create_deferred_ip_port(network) + + # Create the subnet and try to update the port to get an IP + with self.subnet(network=network) as subnet: + data = {'port': { + portbindings.HOST_ID: 'fakehost', + 'fixed_ips': [{'subnet_id': subnet['subnet']['id']}]}} + port_id = port['port']['id'] + port_req = self.new_update_request('ports', data, port_id) + response = port_req.get_response(self.api) + + self.assertEqual(webob.exc.HTTPOk.code, response.status_int) + self._assert_one_ip_in_subnet(response, subnet['subnet']['cidr']) + + # Do a show to be sure that only one IP is recorded + port_req = self.new_show_request('ports', port_id) + response = port_req.get_response(self.api) + self.assertEqual(webob.exc.HTTPOk.code, response.status_int) + self._assert_one_ip_in_subnet(response, subnet['subnet']['cidr']) + + def test_port_update_deferred_allocation_no_segments_empty_alloc(self): + """Binding information is provided, subnet created after port""" + with self.network() as network: + pass + + port = self._create_deferred_ip_port(network) + + # Create the subnet and update the port but specify no IPs + with self.subnet(network=network): + data = {'port': { + portbindings.HOST_ID: 'fakehost', + 'fixed_ips': []}} + port_id = port['port']['id'] + port_req = self.new_update_request('ports', data, port_id) + response = port_req.get_response(self.api) + + self.assertEqual(webob.exc.HTTPOk.code, response.status_int) + res = self.deserialize(self.fmt, response) + # Since I specifically requested no IP addresses, I shouldn't get one. + self.assertEqual(0, len(res['port']['fixed_ips'])) + + def test_port_update_deferred_allocation_no_host_mapping(self): + """Binding information is provided on update, subnets on segments""" + network, segment, subnet = self._create_test_segment_with_subnet() + + port = self._create_deferred_ip_port(network) + + # Try requesting an IP (but the only subnet is on a segment) + data = {'port': {portbindings.HOST_ID: 'fakehost'}} + port_id = port['port']['id'] + port_req = self.new_update_request('ports', data, port_id) + response = port_req.get_response(self.api) + res = self.deserialize(self.fmt, response) + + # Gets conflict because it can't map the host to a segment + self.assertEqual(webob.exc.HTTPConflict.code, response.status_int) + self.assertEqual(segment_exc.HostNotConnectedToAnySegment.__name__, + res['NeutronError']['type']) + + def test_port_update_deferred_allocation_multiple_host_mapping(self): + """Binding information is provided on update, subnets on segments""" + network, segments, _s = self._create_test_segments_with_subnets(2) + + port = self._create_deferred_ip_port(network) + + # This host is bound to multiple segments + self._setup_host_mappings([(segments[0]['segment']['id'], 'fakehost'), + (segments[1]['segment']['id'], 'fakehost')]) + + # Try requesting an IP (but the only subnet is on a segment) + data = {'port': {portbindings.HOST_ID: 'fakehost'}} + port_id = port['port']['id'] + port_req = self.new_update_request('ports', data, port_id) + response = port_req.get_response(self.api) + res = self.deserialize(self.fmt, response) + + # Gets conflict because it can't map the host to a segment + self.assertEqual(webob.exc.HTTPConflict.code, response.status_int) + self.assertEqual(segment_exc.HostConnectedToMultipleSegments.__name__, + res['NeutronError']['type'])