From 5595239435f799926eb3e0397b3bc9d79b62e473 Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Tue, 14 Dec 2021 12:44:31 +0100 Subject: [PATCH] Allocate IPs in bulk requests in separate transactions In the ML2 plugin in create_port_bulk method, we are iterating over list of the ports to be created and do everything for all ports in single DB transaction (which makes totally sense as this is bulk request). But one of the things which was done during that huge transaction was allocation of the IP addresses for all ports. That action is prone for race conditions and can fail often, especially when there is no many IP addresses available in the subnet(s) for the ports. In case of the error while allocating IP address even for one port from the whole bulk request, whole create_port_bulk method was retried so allocations (and everything else) for all ports was reverted and started from scratch. That takes a lot of time so some requests may be processed very long time, like e.g. 2-3 minutes in my tests. To reproduce that issue I did simple script which created network with /24 subnet and then sent 24 requests to create 10 ports in bulk in each request. That was in totall 240 ports created in that subnet. I measured time of the creation of all those ports in the current master branch (without this patch) and with the patch. Results are like below: +-----+---------------+------------+---------------------------+ | Run | Master branch | This patch | Simulate bulk by creation | | | [mm:ss] | [mm:ss] | of 10 ports one by one | +-----+---------------+------------+---------------------------+ | 1 | 01:37 | 01:02 | 00:57 | | 2 | 02:06 | 00:40 | 01:03 | | 3 | 02:08 | 00:41 | 00:59 | | 4 | 02:14 | 00:45 | 00:55 | | 5 | 01:58 | 00:45 | 00:57 | | 6 | 02:37 | 00:53 | 01:05 | | 7 | 01:59 | 00:42 | 00:58 | | 8 | 02:01 | 00:41 | 00:57 | | 9 | 02:39 | 00:42 | 00:55 | | 10 | 01:59 | 00:41 | 00:56 | +-----+---------------+------------+---------------------------+ | AVG | 00:02:07 | 00:00:45 | 00:58 | +-----+---------------+------------+---------------------------+ Closes-Bug: #1954763 Change-Id: I8877c658446fed155130add6f1c69f2772113c27 (cherry picked from commit 82aabb0aa962a3c5c5ce5ad1067952d8f3d9f992) --- neutron/db/ipam_pluggable_backend.py | 50 ++++++++++---- neutron/plugins/ml2/plugin.py | 69 ++++++++++++------- neutron/tests/unit/plugins/ml2/test_plugin.py | 21 ++++++ 3 files changed, 104 insertions(+), 36 deletions(-) diff --git a/neutron/db/ipam_pluggable_backend.py b/neutron/db/ipam_pluggable_backend.py index ab188465008..a010da33fc2 100644 --- a/neutron/db/ipam_pluggable_backend.py +++ b/neutron/db/ipam_pluggable_backend.py @@ -179,6 +179,24 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): ipam_driver = driver.Pool.get_instance(None, context) return ipam_driver.get_subnet(subnet_id) + @db_api.retry_if_session_inactive() + @db_api.CONTEXT_WRITER + def deallocate_ips_from_port(self, context, port, ips): + """Deallocate set of ips from port. + + Deallocate IP addresses previosly allocated for given port. + Format of the ips: + [{ + "ip_address": IP.ADDRESS, + "subnet_id": subnet_id + }] + """ + if not ips: + return + ipam_driver = driver.Pool.get_instance(None, context) + self._ipam_deallocate_ips( + context, ipam_driver, port['port'], ips) + def allocate_ips_for_port_and_store(self, context, port, port_id): # Make a copy of port dict to prevent changing # incoming dict by adding 'id' to it. @@ -198,28 +216,34 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): port_copy['port']['id'] = port_id network_id = port_copy['port']['network_id'] - ips = [] + ips = self.allocate_ips_for_port(context, port_copy) + self.store_ip_allocation_for_port(context, ips, network_id, port_copy) + return ips + + @db_api.retry_if_session_inactive() + @db_api.CONTEXT_WRITER + def allocate_ips_for_port(self, context, port): + return self._allocate_ips_for_port(context, port) + + def store_ip_allocation_for_port(self, context, ips, network_id, port): try: - ips = self._allocate_ips_for_port(context, port_copy) for ip in ips: ip_address = ip['ip_address'] subnet_id = ip['subnet_id'] IpamPluggableBackend._store_ip_allocation( context, ip_address, network_id, - subnet_id, port_id) - return ips + subnet_id, port['port']['id']) except Exception: with excutils.save_and_reraise_exception(): - if ips: - ipam_driver = driver.Pool.get_instance(None, context) - if not ipam_driver.needs_rollback(): - return + ipam_driver = driver.Pool.get_instance(None, context) + if not ipam_driver.needs_rollback(): + return - LOG.debug("An exception occurred during port creation. " - "Reverting IP allocation") - self._safe_rollback(self._ipam_deallocate_ips, context, - ipam_driver, port_copy['port'], ips, - revert_on_fail=False) + LOG.debug("An exception occurred during port creation. " + "Reverting IP allocation") + self._safe_rollback(self._ipam_deallocate_ips, context, + ipam_driver, port['port'], ips, + revert_on_fail=False) def _allocate_ips_for_port(self, context, port): """Allocate IP addresses for the port. IPAM version. diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index cf8faeaaf86..70c6edec683 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -1431,14 +1431,43 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, return bound_context.current + def allocate_ips_for_ports(self, context, ports): + for port in ports: + port['port']['id'] = ( + port['port'].get('id') or uuidutils.generate_uuid()) + + # Call IPAM to allocate IP addresses + try: + port['ipams'] = self.ipam.allocate_ips_for_port(context, port) + + port['ip_allocation'] = (ipalloc_apidef. + IP_ALLOCATION_IMMEDIATE) + except ipam_exc.DeferIpam: + port['ip_allocation'] = (ipalloc_apidef. + IP_ALLOCATION_DEFERRED) + return ports + @utils.transaction_guard - @db_api.retry_if_session_inactive() def create_port_bulk(self, context, ports): - # TODO(njohnston): Break this up into smaller functions. port_list = ports.get('ports') for port in port_list: self._before_create_port(context, port) + port_list = self.allocate_ips_for_ports(context, port_list) + + try: + return self._create_port_bulk(context, port_list) + except Exception: + with excutils.save_and_reraise_exception(): + # If any issue happened allocated IP addresses needs to be + # deallocated now + for port in port_list: + self.ipam.deallocate_ips_from_port( + context, port, port['ipams']) + + @db_api.retry_if_session_inactive() + def _create_port_bulk(self, context, port_list): + # TODO(njohnston): Break this up into smaller functions. port_data = [] network_cache = dict() macs = self._generate_macs(len(port_list)) @@ -1490,31 +1519,25 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, # Create the Port object db_port_obj = ports_obj.Port(context, mac_address=eui_mac_address, - id=uuidutils.generate_uuid(), + id=port['port']['id'], **bulk_port_data) db_port_obj.create() - # Call IPAM to allocate IP addresses - try: - # TODO(njohnston): IPAM allocation needs to be revamped to - # be bulk-friendly. - ips = self.ipam.allocate_ips_for_port_and_store( - context, port, db_port_obj['id']) - ipam_fixed_ips = [] - for ip in ips: - fixed_ip = ports_obj.IPAllocation( - port_id=db_port_obj['id'], - subnet_id=ip['subnet_id'], - network_id=network_id, - ip_address=ip['ip_address']) - ipam_fixed_ips.append(fixed_ip) + # Call IPAM to store allocated IP addresses + ipams = port.pop("ipams") + self.ipam.store_ip_allocation_for_port( + context, ipams, network_id, port) + ipam_fixed_ips = [] + for ip in ipams: + fixed_ip = ports_obj.IPAllocation( + port_id=db_port_obj['id'], + subnet_id=ip['subnet_id'], + network_id=network_id, + ip_address=ip['ip_address']) + ipam_fixed_ips.append(fixed_ip) - db_port_obj['fixed_ips'] = ipam_fixed_ips - db_port_obj['ip_allocation'] = (ipalloc_apidef. - IP_ALLOCATION_IMMEDIATE) - except ipam_exc.DeferIpam: - db_port_obj['ip_allocation'] = (ipalloc_apidef. - IP_ALLOCATION_DEFERRED) + db_port_obj['fixed_ips'] = ipam_fixed_ips + db_port_obj['ip_allocation'] = port.pop('ip_allocation') fixed_ips = pdata.get('fixed_ips') if validators.is_attr_set(fixed_ips) and not fixed_ips: diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index 8fc450b9903..9834f97418c 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -1416,6 +1416,27 @@ class TestMl2PortsV2(test_plugin.TestPortsV2, Ml2PluginV2TestCase): data, context=ctx) self.assertFalse(m_upd.called) + def test_create_ports_bulk_ip_allocation_reverted_in_case_of_error(self): + ctx = context.get_admin_context() + with self.network() as net: + plugin = directory.get_plugin() + with mock.patch.object( + plugin, '_create_port_bulk', + side_effect=ml2_exc.MechanismDriverError( + method='create_port_bulk')), \ + mock.patch.object( + plugin.ipam, + 'deallocate_ips_from_port') as deallocate_mock: + + res = self._create_port_bulk(self.fmt, 2, net['network']['id'], + 'test', True, context=ctx) + + # We expect a 500 as we injected a fault in the plugin + self._validate_behavior_on_bulk_failure( + res, 'ports', webob.exc.HTTPServerError.code) + + self.assertEqual(2, deallocate_mock.call_count) + def test_delete_port_no_notify_in_disassociate_floatingips(self): ctx = context.get_admin_context() plugin = directory.get_plugin()