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

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,