Defer image_ref update to manager on rebuild

We shouldn't update the DB and use it for passing the new image_ref for
a rebuild.  This causes a usage exists notification for the new image
instead of the old... and technically the new image is not accurate
until we shutdown the old instance and try building the new one..

Fixes bug 997245

Change-Id: Ia4bd4b8af90080a11875d9ee56661286bc3f5593

6 files changed, 67 insertions, 23 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index b54aa1b57..116bfb9af 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1345,7 +1345,6 @@ class API(BaseAPI):
 
         self.update(context,
                     instance,
-                    image_ref=image_href,
                     vm_state=vm_states.REBUILDING,
                     task_state=None,
                     progress=0,
@@ -1354,6 +1353,7 @@ class API(BaseAPI):
         rebuild_params = {
             "new_pass": admin_password,
             "injected_files": files_to_inject,
+            "image_ref": image_href,
         }
 
         self._cast_compute_message('rebuild_instance', context, instance,
diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index aa94448d8..faccfef50 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -793,7 +793,7 @@ 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, **kwargs):
+    def rebuild_instance(self, context, instance_uuid, image_ref, **kwargs):
         """Destroy and re-make this instance.
 
         A 'rebuild' effectively purges all existing data from the system and
@@ -805,7 +805,8 @@ class ComputeManager(manager.SchedulerDependentManager):
         :param new_pass: password to set on rebuilt instance
         """
         try:
-            self._rebuild_instance(context, instance_uuid, kwargs)
+            self._rebuild_instance(context, instance_uuid, image_ref,
+                    kwargs)
         except exception.ImageNotFound:
             LOG.error(_('Cannot rebuild instance because the given image does '
                         'not exist.'),
@@ -816,7 +817,7 @@ 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, kwargs):
+    def _rebuild_instance(self, context, instance_uuid, image_ref, kwargs):
         context = context.elevated()
 
         LOG.audit(_("Rebuilding instance"), context=context,
@@ -825,7 +826,17 @@ class ComputeManager(manager.SchedulerDependentManager):
         instance = self.db.instance_get_by_uuid(context, instance_uuid)
         compute_utils.notify_usage_exists(
                 context, instance, current_period=True)
-        self._notify_about_instance_usage(context, instance, "rebuild.start")
+
+        # 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}
+        self._notify_about_instance_usage(context, instance,
+                "rebuild.start", extra_usage_info=extra_usage_info)
+
         current_power_state = self._get_power_state(context, instance)
         self._instance_update(context,
                               instance_uuid,
@@ -836,17 +847,20 @@ class ComputeManager(manager.SchedulerDependentManager):
         network_info = self._get_instance_nw_info(context, instance)
         self.driver.destroy(instance, self._legacy_nw_info(network_info))
 
-        self._instance_update(context,
+        # 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)
+                              task_state=task_states.BLOCK_DEVICE_MAPPING,
+                              image_ref=image_ref)
 
         instance.injected_files = kwargs.get('injected_files', [])
         network_info = self.network_api.get_instance_nw_info(context,
                                                              instance)
         device_info = self._setup_block_device_mapping(context, instance)
 
-        self._instance_update(context,
+        instance = self._instance_update(context,
                               instance_uuid,
                               vm_state=vm_states.REBUILDING,
                               task_state=task_states.SPAWNING)
diff --git a/nova/compute/utils.py b/nova/compute/utils.py
index 5da501c8a..6c2b265dc 100644
--- a/nova/compute/utils.py
+++ b/nova/compute/utils.py
@@ -212,8 +212,7 @@ def _usage_from_instance(context, instance_ref, network_info=None, **kw):
     def null_safe_str(s):
         return str(s) if s else ''
 
-    image_ref_url = "%s/images/%s" % (utils.generate_glance_url(),
-                                      instance_ref['image_ref'])
+    image_ref_url = utils.generate_image_url(instance_ref['image_ref'])
 
     usage_info = dict(
           tenant_id=instance_ref['project_id'],
diff --git a/nova/tests/api/openstack/compute/test_server_actions.py b/nova/tests/api/openstack/compute/test_server_actions.py
index 774700361..43edafb4b 100644
--- a/nova/tests/api/openstack/compute/test_server_actions.py
+++ b/nova/tests/api/openstack/compute/test_server_actions.py
@@ -457,7 +457,6 @@ 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 fad6ba5e1..e1ea40fcd 100644
--- a/nova/tests/test_compute.py
+++ b/nova/tests/test_compute.py
@@ -60,6 +60,7 @@ flags.DECLARE('live_migration_retry_count', 'nova.compute.manager')
 flags.DECLARE('additional_compute_capabilities', 'nova.compute.manager')
 
 
+FAKE_IMAGE_REF = 'fake-image-ref'
 orig_rpc_call = rpc.call
 orig_rpc_cast = rpc.cast
 
@@ -134,7 +135,7 @@ class BaseTestCase(test.TestCase):
 
         inst = {}
         inst['vm_state'] = vm_states.ACTIVE
-        inst['image_ref'] = 1
+        inst['image_ref'] = FAKE_IMAGE_REF
         inst['reservation_id'] = 'r-fakeres'
         inst['launch_time'] = '10'
         inst['user_id'] = self.user_id
@@ -487,8 +488,13 @@ class ComputeTestCase(BaseTestCase):
         instance = self._create_fake_instance()
         instance_uuid = instance['uuid']
 
+        new_image_ref = instance['image_ref'] + '-new-image'
+
         self.compute.run_instance(self.context, instance_uuid)
-        self.compute.rebuild_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)
         self.compute.terminate_instance(self.context, instance_uuid)
 
     def test_rebuild_launch_time(self):
@@ -501,7 +507,8 @@ class ComputeTestCase(BaseTestCase):
 
         self.compute.run_instance(self.context, instance_uuid)
         utils.set_time_override(cur_time)
-        self.compute.rebuild_instance(self.context, instance_uuid)
+        self.compute.rebuild_instance(self.context, instance_uuid,
+                instance['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)
@@ -867,7 +874,7 @@ class ComputeTestCase(BaseTestCase):
         self.assertTrue('created_at' in payload)
         self.assertTrue('launched_at' in payload)
         self.assertTrue(payload['launched_at'])
-        image_ref_url = "%s/images/1" % utils.generate_glance_url()
+        image_ref_url = utils.generate_image_url(FAKE_IMAGE_REF)
         self.assertEquals(payload['image_ref_url'], image_ref_url)
         self.compute.terminate_instance(self.context, instance_uuid)
 
@@ -907,7 +914,7 @@ class ComputeTestCase(BaseTestCase):
         self.assertTrue('launched_at' in payload)
         self.assertTrue('deleted_at' in payload)
         self.assertEqual(payload['deleted_at'], str(cur_time))
-        image_ref_url = "%s/images/1" % utils.generate_glance_url()
+        image_ref_url = utils.generate_image_url(FAKE_IMAGE_REF)
         self.assertEquals(payload['image_ref_url'], image_ref_url)
 
     def test_run_instance_existing(self):
@@ -1071,20 +1078,26 @@ class ComputeTestCase(BaseTestCase):
         instance = db.instance_get_by_uuid(self.context, instance_uuid)
 
         image_ref = instance["image_ref"]
+        new_image_ref = image_ref + '-new_image_ref'
         password = "new_password"
+
         self.compute._rebuild_instance(self.context, instance_uuid,
-                                        dict(image_ref=image_ref,
-                                             new_pass=password))
+                new_image_ref, dict(new_pass=password))
 
         instance = db.instance_get_by_uuid(self.context, instance_uuid)
 
+        image_ref_url = utils.generate_image_url(image_ref)
+        new_image_ref_url = utils.generate_image_url(new_image_ref)
+
         self.assertEquals(len(test_notifier.NOTIFICATIONS), 3)
         msg = test_notifier.NOTIFICATIONS[0]
         self.assertEquals(msg['event_type'],
                           'compute.instance.exists')
+        self.assertEquals(msg['payload']['image_ref_url'], image_ref_url)
         msg = test_notifier.NOTIFICATIONS[1]
         self.assertEquals(msg['event_type'],
                           'compute.instance.rebuild.start')
+        self.assertEquals(msg['payload']['image_ref_url'], new_image_ref_url)
         msg = test_notifier.NOTIFICATIONS[2]
         self.assertEquals(msg['event_type'],
                           'compute.instance.rebuild.end')
@@ -1100,8 +1113,7 @@ class ComputeTestCase(BaseTestCase):
         self.assertTrue('created_at' in payload)
         self.assertTrue('launched_at' in payload)
         self.assertEqual(payload['launched_at'], str(cur_time))
-        image_ref_url = "%s/images/1" % utils.generate_glance_url()
-        self.assertEquals(payload['image_ref_url'], image_ref_url)
+        self.assertEquals(payload['image_ref_url'], new_image_ref_url)
         self.compute.terminate_instance(self.context, instance_uuid)
 
     def test_finish_resize_instance_notification(self):
@@ -1150,7 +1162,7 @@ class ComputeTestCase(BaseTestCase):
         self.assertTrue('created_at' in payload)
         self.assertTrue('launched_at' in payload)
         self.assertEqual(payload['launched_at'], str(cur_time))
-        image_ref_url = "%s/images/1" % utils.generate_glance_url()
+        image_ref_url = utils.generate_image_url(FAKE_IMAGE_REF)
         self.assertEquals(payload['image_ref_url'], image_ref_url)
         self.compute.terminate_instance(context, instance_uuid)
 
@@ -1195,7 +1207,7 @@ class ComputeTestCase(BaseTestCase):
         self.assertTrue('display_name' in payload)
         self.assertTrue('created_at' in payload)
         self.assertTrue('launched_at' in payload)
-        image_ref_url = "%s/images/1" % utils.generate_glance_url()
+        image_ref_url = utils.generate_image_url(FAKE_IMAGE_REF)
         self.assertEquals(payload['image_ref_url'], image_ref_url)
         self.compute.terminate_instance(context, instance_uuid)
 
@@ -2205,9 +2217,24 @@ class ComputeAPITestCase(BaseTestCase):
         instance = db.instance_get_by_uuid(self.context, instance_uuid)
         self.assertEqual(instance['task_state'], None)
 
-        image_ref = instance["image_ref"]
+        # 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..
+        orig_update = self.compute_api.update
+        info = {'image_ref_updated': False}
+
+        def update_wrapper(*args, **kwargs):
+            if 'image_ref' in kwargs:
+                info['image_ref_updated'] = True
+            return orig_update(*args, **kwargs)
+
+        self.stubs.Set(self.compute_api, 'update', update_wrapper)
+
+        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'])
 
         instance = db.instance_get_by_uuid(self.context, instance_uuid)
         self.assertEqual(instance['vm_state'], vm_states.REBUILDING)
diff --git a/nova/utils.py b/nova/utils.py
index 0c20dd34d..69011fb61 100644
--- a/nova/utils.py
+++ b/nova/utils.py
@@ -1259,6 +1259,11 @@ def generate_glance_url():
     return "http://%s:%d" % (FLAGS.glance_host, FLAGS.glance_port)
 
 
+def generate_image_url(image_ref):
+    """Generate a image URL from an image_ref."""
+    return "%s/images/%s" % (generate_glance_url(), image_ref)
+
+
 @contextlib.contextmanager
 def logging_error(message):
     """Catches exception, write message to the log, re-raise.