From 881a93473c32a7c7e23a8e6dcede8394053408c6 Mon Sep 17 00:00:00 2001 From: Vishvananda Ishaya Date: Thu, 20 Dec 2012 20:13:37 -0800 Subject: Eliminate race conditions in floating association This makes associating and disassociating floating ips atomic and idempotent. This means multiple concurrent messages will not leave behind iptables rules and concurrent request will not cause odd failures. Fixes bug 1092762 and bug 1092761. Change-Id: Idbcad6c1d2a3d4881cf7180b848ed3844fac4054 --- nova/db/api.py | 10 ++++- nova/db/sqlalchemy/api.py | 3 ++ nova/network/manager.py | 91 +++++++++++++++++++++++--------------- nova/tests/network/test_manager.py | 2 +- nova/tests/test_db_api.py | 15 +++++++ 5 files changed, 83 insertions(+), 38 deletions(-) diff --git a/nova/db/api.py b/nova/db/api.py index 4acff8a99..3e350fc75 100644 --- a/nova/db/api.py +++ b/nova/db/api.py @@ -290,7 +290,8 @@ def floating_ip_destroy(context, address): def floating_ip_disassociate(context, address): """Disassociate a floating ip from a fixed ip by address. - :returns: the address of the existing fixed ip. + :returns: the address of the previous fixed ip or None + if the ip was not associated to an ip. """ return IMPL.floating_ip_disassociate(context, address) @@ -298,7 +299,12 @@ def floating_ip_disassociate(context, address): def floating_ip_fixed_ip_associate(context, floating_address, fixed_address, host): - """Associate a floating ip to a fixed_ip by address.""" + """Associate a floating ip to a fixed_ip by address. + + :returns: the address of the new fixed ip (fixed_address) or None + if the ip was already associated to the fixed ip. + """ + return IMPL.floating_ip_fixed_ip_associate(context, floating_address, fixed_address, diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index ec85ddcef..a24a5fe2d 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -764,9 +764,12 @@ def floating_ip_fixed_ip_associate(context, floating_address, fixed_ip_ref = fixed_ip_get_by_address(context, fixed_address, session=session) + if floating_ip_ref.fixed_ip_id == fixed_ip_ref["id"]: + return None floating_ip_ref.fixed_ip_id = fixed_ip_ref["id"] floating_ip_ref.host = host floating_ip_ref.save(session=session) + return fixed_address @require_context diff --git a/nova/network/manager.py b/nova/network/manager.py index 1ba1370e0..72ad45980 100644 --- a/nova/network/manager.py +++ b/nova/network/manager.py @@ -575,29 +575,35 @@ class FloatingIP(object): def _associate_floating_ip(self, context, floating_address, fixed_address, interface, instance_uuid): """Performs db and driver calls to associate floating ip & fixed ip""" - # associate floating ip - self.db.floating_ip_fixed_ip_associate(context, - floating_address, - fixed_address, - self.host) - try: - # gogo driver time - self.l3driver.add_floating_ip(floating_address, fixed_address, - interface) - except exception.ProcessExecutionError as e: - fixed_address = self.db.floating_ip_disassociate(context, - floating_address) - if "Cannot find device" in str(e): - LOG.error(_('Interface %(interface)s not found'), locals()) - raise exception.NoFloatingIpInterface(interface=interface) - - payload = dict(project_id=context.project_id, - instance_id=instance_uuid, - floating_ip=floating_address) - notifier.notify(context, - notifier.publisher_id("network"), - 'network.floating_ip.associate', - notifier.INFO, payload=payload) + + @lockutils.synchronized(unicode(floating_address), 'nova-') + def do_associate(): + # associate floating ip + res = self.db.floating_ip_fixed_ip_associate(context, + floating_address, + fixed_address, + self.host) + if not res: + # NOTE(vish): ip was already associated + return + try: + # gogo driver time + self.l3driver.add_floating_ip(floating_address, fixed_address, + interface) + except exception.ProcessExecutionError as e: + self.db.floating_ip_disassociate(context, floating_address) + if "Cannot find device" in str(e): + LOG.error(_('Interface %(interface)s not found'), locals()) + raise exception.NoFloatingIpInterface(interface=interface) + + payload = dict(project_id=context.project_id, + instance_id=instance_uuid, + floating_ip=floating_address) + notifier.notify(context, + notifier.publisher_id("network"), + 'network.floating_ip.associate', + notifier.INFO, payload=payload) + do_associate() @rpc_common.client_exceptions(exception.FloatingIpNotFoundForAddress) @wrap_check_policy @@ -657,18 +663,33 @@ class FloatingIP(object): instance_uuid): """Performs db and driver calls to disassociate floating ip""" # disassociate floating ip - fixed_address = self.db.floating_ip_disassociate(context, address) - - if interface: - # go go driver time - self.l3driver.remove_floating_ip(address, fixed_address, interface) - payload = dict(project_id=context.project_id, - instance_id=instance_uuid, - floating_ip=address) - notifier.notify(context, - notifier.publisher_id("network"), - 'network.floating_ip.disassociate', - notifier.INFO, payload=payload) + + @lockutils.synchronized(unicode(address), 'nova-') + def do_disassociate(): + # NOTE(vish): Note that we are disassociating in the db before we + # actually remove the ip address on the host. We are + # safe from races on this host due to the decorator, + # but another host might grab the ip right away. We + # don't worry about this case because the miniscule + # window where the ip is on both hosts shouldn't cause + # any problems. + fixed_address = self.db.floating_ip_disassociate(context, address) + + if not fixed_address: + # NOTE(vish): ip was already disassociated + return + if interface: + # go go driver time + self.l3driver.remove_floating_ip(address, fixed_address, + interface) + payload = dict(project_id=context.project_id, + instance_id=instance_uuid, + floating_ip=address) + notifier.notify(context, + notifier.publisher_id("network"), + 'network.floating_ip.disassociate', + notifier.INFO, payload=payload) + do_disassociate() @rpc_common.client_exceptions(exception.FloatingIpNotFound) @wrap_check_policy diff --git a/nova/tests/network/test_manager.py b/nova/tests/network/test_manager.py index 0e441eef8..4b266055b 100644 --- a/nova/tests/network/test_manager.py +++ b/nova/tests/network/test_manager.py @@ -669,7 +669,7 @@ class VlanNetworkTestCase(test.TestCase): is_admin=False) def fake1(*args, **kwargs): - pass + return '10.0.0.1' # floating ip that's already associated def fake2(*args, **kwargs): diff --git a/nova/tests/test_db_api.py b/nova/tests/test_db_api.py index 29bce8bf5..4d6471a3f 100644 --- a/nova/tests/test_db_api.py +++ b/nova/tests/test_db_api.py @@ -246,6 +246,21 @@ class DbApiTestCase(test.TestCase): self.assertEqual(0, len(results)) db.instance_update(ctxt, instance['uuid'], {"task_state": None}) + def test_multi_associate_disassociate(self): + ctxt = context.get_admin_context() + values = {'address': 'floating'} + floating = db.floating_ip_create(ctxt, values) + values = {'address': 'fixed'} + fixed = db.fixed_ip_create(ctxt, values) + res = db.floating_ip_fixed_ip_associate(ctxt, floating, fixed, 'foo') + self.assertEqual(res, fixed) + res = db.floating_ip_fixed_ip_associate(ctxt, floating, fixed, 'foo') + self.assertEqual(res, None) + res = db.floating_ip_disassociate(ctxt, floating) + self.assertEqual(res, fixed) + res = db.floating_ip_disassociate(ctxt, floating) + self.assertEqual(res, None) + def test_network_create_safe(self): ctxt = context.get_admin_context() values = {'host': 'localhost', 'project_id': 'project1'} -- cgit