Merge "Defer image_ref update to manager on rebuild"

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.