Merge "API does need new image_ref on rebuild immediately."

5 files changed, 49 insertions, 39 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 116bfb9af..32b7d79ce 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1329,6 +1329,7 @@ class API(BaseAPI):
     def rebuild(self, context, instance, image_href, admin_password, **kwargs):
         """Rebuild the given instance with the provided attributes."""
 
+        orig_image_ref = instance['image_ref']
         image = self._get_image(context, image_href)
 
         files_to_inject = kwargs.pop('files_to_inject', [])
@@ -1346,6 +1347,9 @@ class API(BaseAPI):
         self.update(context,
                     instance,
                     vm_state=vm_states.REBUILDING,
+                    # Unfortunately we need to set image_ref early,
+                    # so API users can see it.
+                    image_ref=image_href,
                     task_state=None,
                     progress=0,
                     **kwargs)
@@ -1354,6 +1358,7 @@ class API(BaseAPI):
             "new_pass": admin_password,
             "injected_files": files_to_inject,
             "image_ref": image_href,
+            "orig_image_ref": orig_image_ref,
         }
 
         self._cast_compute_message('rebuild_instance', context, instance,
diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index faccfef50..a00c3aa36 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -793,7 +793,8 @@ class ComputeManager(manager.SchedulerDependentManager):
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
     @checks_instance_lock
     @wrap_instance_fault
-    def rebuild_instance(self, context, instance_uuid, image_ref, **kwargs):
+    def rebuild_instance(self, context, instance_uuid, orig_image_ref,
+            image_ref, **kwargs):
         """Destroy and re-make this instance.
 
         A 'rebuild' effectively purges all existing data from the system and
@@ -801,12 +802,14 @@ class ComputeManager(manager.SchedulerDependentManager):
 
         :param context: `nova.RequestContext` object
         :param instance_uuid: Instance Identifier (UUID)
+        :param orig_image_ref: Original image_ref before rebuild
+        :param image_ref: New image_ref for rebuild
         :param injected_files: Files to inject
         :param new_pass: password to set on rebuilt instance
         """
         try:
-            self._rebuild_instance(context, instance_uuid, image_ref,
-                    kwargs)
+            self._rebuild_instance(context, instance_uuid, orig_image_ref,
+                    image_ref, kwargs)
         except exception.ImageNotFound:
             LOG.error(_('Cannot rebuild instance because the given image does '
                         'not exist.'),
@@ -817,25 +820,26 @@ class ComputeManager(manager.SchedulerDependentManager):
                       context=context, instance_uuid=instance_uuid)
             self._set_instance_error_state(context, instance_uuid)
 
-    def _rebuild_instance(self, context, instance_uuid, image_ref, kwargs):
+    def _rebuild_instance(self, context, instance_uuid, orig_image_ref,
+            image_ref, kwargs):
         context = context.elevated()
 
         LOG.audit(_("Rebuilding instance"), context=context,
                   instance_uuid=instance_uuid)
 
         instance = self.db.instance_get_by_uuid(context, instance_uuid)
-        compute_utils.notify_usage_exists(
-                context, instance, current_period=True)
-
-        # NOTE(comstud): Since we've not updated the DB yet with the new
-        # image_ref, and rebuild.start should have image_ref_url pointing
-        # to the new image ref, we need to override it.
-        # Arguably, image_ref_url should still contain the old image at
-        # this point and we should pass a 'new_image_ref_url', instead.
-        image_ref_url = utils.generate_image_url(image_ref)
-        extra_usage_info = {'image_ref_url': image_ref_url}
+
+        # This instance.exists message should contain the original
+        # image_ref, not the new one.  Since the DB has been updated
+        # to point to the new one... we have to override it.
+        orig_image_ref_url = utils.generate_image_url(orig_image_ref)
+        extra_usage_info = {'image_ref_url': orig_image_ref_url}
+        compute_utils.notify_usage_exists(context, instance,
+                current_period=True, extra_usage_info=extra_usage_info)
+
+        # This message should contain the new image_ref
         self._notify_about_instance_usage(context, instance,
-                "rebuild.start", extra_usage_info=extra_usage_info)
+                "rebuild.start")
 
         current_power_state = self._get_power_state(context, instance)
         self._instance_update(context,
@@ -847,13 +851,10 @@ class ComputeManager(manager.SchedulerDependentManager):
         network_info = self._get_instance_nw_info(context, instance)
         self.driver.destroy(instance, self._legacy_nw_info(network_info))
 
-        # NOTE(comstud): Now that we've shutdown the old instance, we
-        # can update the image_ref in the DB.
         instance = self._instance_update(context,
                               instance_uuid,
                               vm_state=vm_states.REBUILDING,
-                              task_state=task_states.BLOCK_DEVICE_MAPPING,
-                              image_ref=image_ref)
+                              task_state=task_states.BLOCK_DEVICE_MAPPING)
 
         instance.injected_files = kwargs.get('injected_files', [])
         network_info = self.network_api.get_instance_nw_info(context,
@@ -868,7 +869,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         instance.admin_pass = kwargs.get('new_pass',
                 utils.generate_password(FLAGS.password_length))
 
-        image_meta = _get_image_meta(context, instance['image_ref'])
+        image_meta = _get_image_meta(context, image_ref)
 
         self.driver.spawn(context, instance, image_meta,
                           self._legacy_nw_info(network_info), device_info)
diff --git a/nova/compute/utils.py b/nova/compute/utils.py
index 6c2b265dc..016e4aaed 100644
--- a/nova/compute/utils.py
+++ b/nova/compute/utils.py
@@ -34,7 +34,8 @@ LOG = log.getLogger(__name__)
 
 
 def notify_usage_exists(context, instance_ref, current_period=False,
-                        ignore_missing_network_data=True):
+                        ignore_missing_network_data=True,
+                        extra_usage_info=None):
     """Generates 'exists' notification for an instance for usage auditing
     purposes.
 
@@ -85,12 +86,15 @@ def notify_usage_exists(context, instance_ref, current_period=False,
 
         bw[label] = dict(bw_in=b.bw_in, bw_out=b.bw_out)
 
-    extra_usage_info = dict(audit_period_beginning=str(audit_start),
-                            audit_period_ending=str(audit_end),
-                            bandwidth=bw)
+    extra_info = dict(audit_period_beginning=str(audit_start),
+                      audit_period_ending=str(audit_end),
+                      bandwidth=bw)
+
+    if extra_usage_info:
+        extra_info.update(extra_usage_info)
 
     notify_about_instance_usage(
-            context, instance_ref, 'exists', extra_usage_info=extra_usage_info)
+            context, instance_ref, 'exists', extra_usage_info=extra_info)
 
 
 def legacy_network_info(network_model):
diff --git a/nova/tests/api/openstack/compute/test_server_actions.py b/nova/tests/api/openstack/compute/test_server_actions.py
index 43edafb4b..774700361 100644
--- a/nova/tests/api/openstack/compute/test_server_actions.py
+++ b/nova/tests/api/openstack/compute/test_server_actions.py
@@ -457,6 +457,7 @@ class ServerActionsControllerTest(test.TestCase):
         req = fakes.HTTPRequest.blank(self.url)
         context = req.environ['nova.context']
         update(context, mox.IgnoreArg(),
+                image_ref=self._image_href,
                 vm_state=vm_states.REBUILDING,
                 task_state=None, progress=0, **attributes).AndReturn(None)
         self.mox.ReplayAll()
diff --git a/nova/tests/test_compute.py b/nova/tests/test_compute.py
index e1ea40fcd..98bb2d675 100644
--- a/nova/tests/test_compute.py
+++ b/nova/tests/test_compute.py
@@ -487,14 +487,11 @@ class ComputeTestCase(BaseTestCase):
         """Ensure instance can be rebuilt"""
         instance = self._create_fake_instance()
         instance_uuid = instance['uuid']
-
-        new_image_ref = instance['image_ref'] + '-new-image'
+        image_ref = instance['image_ref']
 
         self.compute.run_instance(self.context, instance_uuid)
         self.compute.rebuild_instance(self.context, instance_uuid,
-                new_image_ref)
-        inst_ref = db.instance_get_by_uuid(self.context, instance_uuid)
-        self.assertEqual(inst_ref['image_ref'], new_image_ref)
+                image_ref, image_ref)
         self.compute.terminate_instance(self.context, instance_uuid)
 
     def test_rebuild_launch_time(self):
@@ -504,11 +501,12 @@ class ComputeTestCase(BaseTestCase):
         utils.set_time_override(old_time)
         instance = self._create_fake_instance()
         instance_uuid = instance['uuid']
+        image_ref = instance['image_ref']
 
         self.compute.run_instance(self.context, instance_uuid)
         utils.set_time_override(cur_time)
         self.compute.rebuild_instance(self.context, instance_uuid,
-                instance['image_ref'])
+                image_ref, image_ref)
         instance = db.instance_get_by_uuid(self.context, instance_uuid)
         self.assertEquals(cur_time, instance['launched_at'])
         self.compute.terminate_instance(self.context, instance_uuid)
@@ -1079,10 +1077,13 @@ class ComputeTestCase(BaseTestCase):
 
         image_ref = instance["image_ref"]
         new_image_ref = image_ref + '-new_image_ref'
+        db.instance_update(self.context, instance_uuid,
+                {'image_ref': new_image_ref})
+
         password = "new_password"
 
         self.compute._rebuild_instance(self.context, instance_uuid,
-                new_image_ref, dict(new_pass=password))
+                image_ref, new_image_ref, dict(new_pass=password))
 
         instance = db.instance_get_by_uuid(self.context, instance_uuid)
 
@@ -2217,16 +2218,14 @@ class ComputeAPITestCase(BaseTestCase):
         instance = db.instance_get_by_uuid(self.context, instance_uuid)
         self.assertEqual(instance['task_state'], None)
 
-        # Make sure Compute API doesn't try to update the image_ref.
-        # It should be done on the manager side.  We can't check the DB
-        # at the end of this test method because we've called the
-        # manager by that point..
+        # Make sure Compute API updates the image_ref before casting to
+        # compute manager.
         orig_update = self.compute_api.update
-        info = {'image_ref_updated': False}
+        info = {'image_ref': None}
 
         def update_wrapper(*args, **kwargs):
             if 'image_ref' in kwargs:
-                info['image_ref_updated'] = True
+                info['image_ref'] = kwargs['image_ref']
             return orig_update(*args, **kwargs)
 
         self.stubs.Set(self.compute_api, 'update', update_wrapper)
@@ -2234,7 +2233,7 @@ class ComputeAPITestCase(BaseTestCase):
         image_ref = instance["image_ref"] + '-new_image_ref'
         password = "new_password"
         self.compute_api.rebuild(self.context, instance, image_ref, password)
-        self.assertFalse(info['image_ref_updated'])
+        self.assertEqual(info['image_ref'], image_ref)
 
         instance = db.instance_get_by_uuid(self.context, instance_uuid)
         self.assertEqual(instance['vm_state'], vm_states.REBUILDING)