From 6a080df6bd8817019d204e3142a236b7974f7656 Mon Sep 17 00:00:00 2001 From: Chris Behrens Date: Thu, 7 Mar 2013 21:03:19 +0000 Subject: Fix access_ip_* race The code currently sets access_ip_* in a 2nd DB call after an instance goes ACTIVE, which means there's a slight race when someone is querying the API for their instance. It's possible to see the instance ACTIVE but access_ip_* will be blank. To fix this, we should set access_ip_* at the same time the instance goes ACTIVE. This also removes an unnecessary extra DB call. Fixes bug 1152328 Change-Id: I0fe542d49fbc95f27a24f57f674264f8789bc527 --- nova/compute/manager.py | 79 ++++++++++++++++++++------------------ nova/tests/compute/test_compute.py | 16 +++++++- 2 files changed, 55 insertions(+), 40 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index c4f3e1c6f..e172e6ad8 100755 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -784,9 +784,14 @@ class ComputeManager(manager.SchedulerDependentManager): block_device_info = self._prep_block_device( context, instance, bdms) + set_access_ip = (is_first_time and + not instance['access_ip_v4'] and + not instance['access_ip_v6']) + instance = self._spawn(context, instance, image_meta, network_info, block_device_info, - injected_files, admin_password) + injected_files, admin_password, + set_access_ip=set_access_ip) except exception.InstanceNotFound: # the instance got deleted during the spawn with excutils.save_and_reraise_exception(): @@ -811,11 +816,6 @@ class ComputeManager(manager.SchedulerDependentManager): is_first_time, request_spec, filter_properties) else: # Spawn success: - if (is_first_time and not instance['access_ip_v4'] - and not instance['access_ip_v6']): - instance = self._update_access_ip(context, instance, - network_info) - self._notify_about_instance_usage(context, instance, "create.end", network_info=network_info, extra_usage_info=extra_usage_info) @@ -920,32 +920,6 @@ class ComputeManager(manager.SchedulerDependentManager): LOG.warn(_("Instance build timed out. Set to error state."), instance=instance) - def _update_access_ip(self, context, instance, nw_info): - """Update the access ip values for a given instance. - - If CONF.default_access_ip_network_name is set, this method will - grab the corresponding network and set the access ip values - accordingly. Note that when there are multiple ips to choose from, - an arbitrary one will be chosen. - """ - - network_name = CONF.default_access_ip_network_name - if not network_name: - return instance - - update_info = {} - for vif in nw_info: - if vif['network']['label'] == network_name: - for ip in vif.fixed_ips(): - if ip['version'] == 4: - update_info['access_ip_v4'] = ip['address'] - if ip['version'] == 6: - update_info['access_ip_v6'] = ip['address'] - if update_info: - instance = self._instance_update(context, instance['uuid'], - **update_info) - return instance - def _check_instance_exists(self, context, instance): """Ensure an instance with the same name is not already present.""" if self.driver.instance_exists(instance['name']): @@ -1053,7 +1027,8 @@ class ComputeManager(manager.SchedulerDependentManager): instance=instance) def _spawn(self, context, instance, image_meta, network_info, - block_device_info, injected_files, admin_password): + block_device_info, injected_files, admin_password, + set_access_ip=False): """Spawn an instance with error logging and update its power state.""" instance = self._instance_update(context, instance['uuid'], vm_state=vm_states.BUILDING, @@ -1069,12 +1044,40 @@ class ComputeManager(manager.SchedulerDependentManager): LOG.exception(_('Instance failed to spawn'), instance=instance) current_power_state = self._get_power_state(context, instance) + + update_data = dict(power_state=current_power_state, + vm_state=vm_states.ACTIVE, + task_state=None, + expected_task_state=task_states.SPAWNING, + launched_at=timeutils.utcnow()) + + def _set_access_ip_values(): + """Add access ip values for a given instance. + + If CONF.default_access_ip_network_name is set, this method will + grab the corresponding network and set the access ip values + accordingly. Note that when there are multiple ips to choose + from, an arbitrary one will be chosen. + """ + + network_name = CONF.default_access_ip_network_name + if not network_name: + return + + for vif in network_info: + if vif['network']['label'] == network_name: + for ip in vif.fixed_ips(): + if ip['version'] == 4: + update_data['access_ip_v4'] = ip['address'] + if ip['version'] == 6: + update_data['access_ip_v6'] = ip['address'] + return + + if set_access_ip: + _set_access_ip_values() + return self._instance_update(context, instance['uuid'], - power_state=current_power_state, - vm_state=vm_states.ACTIVE, - task_state=None, - expected_task_state=task_states.SPAWNING, - launched_at=timeutils.utcnow()) + **update_data) def _notify_about_instance_usage(self, context, instance, event_suffix, network_info=None, system_metadata=None, diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py index c86992f3d..e247b41c9 100644 --- a/nova/tests/compute/test_compute.py +++ b/nova/tests/compute/test_compute.py @@ -605,6 +605,18 @@ class ComputeTestCase(BaseTestCase): fake_network.unset_stub_network_methods(self.stubs) instance = jsonutils.to_primitive(self._create_fake_instance()) + orig_update = self.compute._instance_update + + # Make sure the access_ip_* updates happen in the same DB + # update as the set to ACTIVE. + def _instance_update(ctxt, instance_uuid, **kwargs): + if kwargs.get('vm_state', None) == vm_states.ACTIVE: + self.assertEqual(kwargs['access_ip_v4'], '192.168.1.100') + self.assertEqual(kwargs['access_ip_v6'], '2001:db8:0:1::1') + return orig_update(ctxt, instance_uuid, **kwargs) + + self.stubs.Set(self.compute, '_instance_update', _instance_update) + try: self.compute.run_instance(self.context, instance=instance, is_first_time=True) @@ -7355,7 +7367,7 @@ class ComputeRescheduleOrReraiseTestCase(BaseTestCase): actual=task_states.DELETING) self.compute._spawn(mox.IgnoreArg(), self.instance, mox.IgnoreArg(), mox.IgnoreArg(), mox.IgnoreArg(), mox.IgnoreArg(), - mox.IgnoreArg()).AndRaise(exc) + mox.IgnoreArg(), set_access_ip=False).AndRaise(exc) self.mox.ReplayAll() # test succeeds if mocked method '_reschedule_or_reraise' is not @@ -7373,7 +7385,7 @@ class ComputeRescheduleOrReraiseTestCase(BaseTestCase): actual=task_states.SCHEDULING) self.compute._spawn(mox.IgnoreArg(), self.instance, mox.IgnoreArg(), mox.IgnoreArg(), mox.IgnoreArg(), mox.IgnoreArg(), - mox.IgnoreArg()).AndRaise(exc) + mox.IgnoreArg(), set_access_ip=False).AndRaise(exc) self.mox.ReplayAll() self.assertRaises(exception.UnexpectedTaskStateError, -- cgit