Clean up compute API image_create

The _image_create() code in compute manager is the only place where
db.instance_test_and_set is used.. which was added before
'expected_task_state' support was added to instance_update.  By
switching to use self.update(), we can also take advantage of it doing
the state change notification for us.

The instance update and the compute RPC call have been moved out of
_create_image() and into snapshot() and backup() to support a future
cells patch.

Change-Id: Ib6603f732d28e49cb6f351447da5f6e41615dea5

5 files changed, 27 insertions, 99 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index d0a039644..344761801 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1274,10 +1274,16 @@ class API(base.Base):
             None if rotation shouldn't be used (as in the case of snapshots)
         :param extra_properties: dict of extra image properties to include
         """
-        recv_meta = self._create_image(context, instance, name, 'backup',
+        instance = self.update(context, instance,
+                               task_state=task_states.IMAGE_BACKUP,
+                               expected_task_state=None)
+        image_meta = self._create_image(context, instance, name, 'backup',
                             backup_type=backup_type, rotation=rotation,
                             extra_properties=extra_properties)
-        return recv_meta
+        self.compute_rpcapi.snapshot_instance(context, instance=instance,
+                image_id=image_meta['id'], image_type='backup',
+                backup_type=backup_type, rotation=rotation)
+        return image_meta
 
     @wrap_check_policy
     @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED])
@@ -1290,12 +1296,20 @@ class API(base.Base):
 
         :returns: A dict containing image metadata
         """
-        return self._create_image(context, instance, name, 'snapshot',
-                                  extra_properties=extra_properties)
+        instance = self.update(context, instance,
+                               task_state=task_states.IMAGE_SNAPSHOT,
+                               expected_task_state=None)
+        image_meta = self._create_image(context, instance, name,
+                'snapshot', extra_properties=extra_properties)
+        self.compute_rpcapi.snapshot_instance(context, instance=instance,
+                image_id=image_meta['id'], image_type='snapshot')
+        return image_meta
 
     def _create_image(self, context, instance, name, image_type,
                       backup_type=None, rotation=None, extra_properties=None):
-        """Create snapshot or backup for an instance on this host.
+        """Create new image entry in the image service.  This new image
+        will be reserved for the compute manager to upload a snapshot
+        or backup.
 
         :param context: security context
         :param instance: nova.db.sqlalchemy.models.Instance
@@ -1309,29 +1323,6 @@ class API(base.Base):
         """
         instance_uuid = instance['uuid']
 
-        if image_type == "snapshot":
-            task_state = task_states.IMAGE_SNAPSHOT
-        elif image_type == "backup":
-            task_state = task_states.IMAGE_BACKUP
-        else:
-            raise Exception(_('Image type not recognized %s') % image_type)
-
-        # change instance state and notify
-        old_vm_state = instance["vm_state"]
-        old_task_state = instance["task_state"]
-
-        self.db.instance_test_and_set(
-                context, instance_uuid, 'task_state', [None], task_state)
-
-        # NOTE(sirp): `instance_test_and_set` only sets the task-state in the
-        # DB, but we also need to set it on the current instance so that the
-        # correct value is passed down to the compute manager.
-        instance['task_state'] = task_state
-
-        notifications.send_update_with_states(context, instance, old_vm_state,
-                instance["vm_state"], old_task_state, instance["task_state"],
-                service="api", verify_states=True)
-
         properties = {
             'instance_uuid': instance_uuid,
             'user_id': str(context.user_id),
@@ -1378,11 +1369,7 @@ class API(base.Base):
             # up above will not be overwritten by inherited values
             properties.setdefault(key, value)
 
-        recv_meta = self.image_service.create(context, sent_meta)
-        self.compute_rpcapi.snapshot_instance(context, instance=instance,
-                image_id=recv_meta['id'], image_type=image_type,
-                backup_type=backup_type, rotation=rotation)
-        return recv_meta
+        return self.image_service.create(context, sent_meta)
 
     @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED])
     def snapshot_volume_backed(self, context, instance, image_meta, name,
diff --git a/nova/compute/rpcapi.py b/nova/compute/rpcapi.py
index ae283283b..1c53b6084 100644
--- a/nova/compute/rpcapi.py
+++ b/nova/compute/rpcapi.py
@@ -525,7 +525,7 @@ class ComputeAPI(nova.openstack.common.rpc.proxy.RpcProxy):
                 version='2.3')
 
     def snapshot_instance(self, ctxt, instance, image_id, image_type,
-            backup_type, rotation):
+            backup_type=None, rotation=None):
         instance_p = jsonutils.to_primitive(instance)
         self.cast(ctxt, self.make_msg('snapshot_instance',
                 instance=instance_p, image_id=image_id,
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 8930f6ccc..22d39e892 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -1787,42 +1787,6 @@ def instance_get_all_hung_in_rebooting(context, reboot_window):
 
 
 @require_context
-def instance_test_and_set(context, instance_uuid, attr, ok_states, new_state):
-    """Atomically check if an instance is in a valid state, and if it is, set
-    the instance into a new state.
-    """
-    if not uuidutils.is_uuid_like(instance_uuid):
-        raise exception.InvalidUUID(instance_uuid)
-
-    session = get_session()
-    with session.begin():
-        query = model_query(context, models.Instance, session=session,
-                            project_only=True).\
-                    filter_by(uuid=instance_uuid)
-
-        attr_column = getattr(models.Instance, attr)
-        filter_op = None
-        # NOTE(boris-42): `SELECT IN` doesn't work with None values because
-        #                 they are incomparable.
-        if None in ok_states:
-            filter_op = or_(attr_column == None,
-                            attr_column.in_(filter(lambda x: x is not None,
-                                                   ok_states)))
-        else:
-            filter_op = attr_column.in_(ok_states)
-
-        count = query.filter(filter_op).\
-                        update({attr: new_state}, synchronize_session=False)
-        if count == 0:
-            instance_ref = query.first()
-            raise exception.InstanceInvalidState(
-                attr=attr,
-                instance_uuid=instance_ref['uuid'],
-                state=instance_ref[attr],
-                method='instance_test_and_set')
-
-
-@require_context
 def instance_update(context, instance_uuid, values):
     instance_ref = _instance_update(context, instance_uuid, values)[1]
     return instance_ref
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 08d9451b3..b79fbd042 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -4383,27 +4383,25 @@ class ComputeAPITestCase(BaseTestCase):
         and min_disk set to that of the original instances flavor.
         """
 
-        self.fake_image['disk_format'] = 'vhd'
+        self.fake_image.update(disk_format='vhd',
+                               min_ram=1, min_disk=1)
         self.stubs.Set(fake_image._FakeImageService, 'show', self.fake_show)
 
-        instance = self._create_fake_instance()
-        inst_params = {'root_gb': 2, 'memory_mb': 256}
-        instance['instance_type'].update(inst_params)
+        instance = self._create_fake_instance(type_name='m1.small')
 
         image = self.compute_api.snapshot(self.context, instance, 'snap1',
                                           {'extra_param': 'value1'})
 
         self.assertEqual(image['name'], 'snap1')
-        self.assertEqual(image['min_ram'], 256)
-        self.assertEqual(image['min_disk'], 2)
+        instance_type = instance['instance_type']
+        self.assertEqual(image['min_ram'], instance_type['memory_mb'])
+        self.assertEqual(image['min_disk'], instance_type['root_gb'])
         properties = image['properties']
         self.assertTrue('backup_type' not in properties)
         self.assertEqual(properties['image_type'], 'snapshot')
         self.assertEqual(properties['instance_uuid'], instance['uuid'])
         self.assertEqual(properties['extra_param'], 'value1')
 
-        db.instance_destroy(self.context, instance['uuid'])
-
     def test_snapshot_minram_mindisk(self):
         """Ensure a snapshots min_ram and min_disk are correct.
 
diff --git a/nova/tests/test_db_api.py b/nova/tests/test_db_api.py
index 7df28bfcb..c70e96cdc 100644
--- a/nova/tests/test_db_api.py
+++ b/nova/tests/test_db_api.py
@@ -299,27 +299,6 @@ class DbApiTestCase(test.TestCase):
         self.assertRaises(exception.DuplicateVlan,
                           db.network_create_safe, ctxt, values2)
 
-    def test_instance_test_and_set(self):
-        ctxt = context.get_admin_context()
-        states = [
-            (None, [None, 'some'], 'building'),
-            (None, [None], 'building'),
-            ('building', ['building'], 'ready'),
-            ('building', [None, 'building'], 'ready')]
-        for st in states:
-            inst = db.instance_create(ctxt, {'vm_state': st[0]})
-            uuid = inst['uuid']
-            db.instance_test_and_set(ctxt, uuid, 'vm_state', st[1], st[2])
-            inst = db.instance_get_by_uuid(ctxt, uuid)
-            self.assertEqual(inst["vm_state"], st[2])
-
-    def test_instance_test_and_set_exception(self):
-        ctxt = context.get_admin_context()
-        inst = db.instance_create(ctxt, {'vm_state': 'building'})
-        self.assertRaises(exception.InstanceInvalidState,
-                          db.instance_test_and_set, ctxt,
-                          inst['uuid'], 'vm_state', [None, 'disable'], 'run')
-
     def test_instance_update_with_instance_uuid(self):
         # test instance_update() works when an instance UUID is passed.
         ctxt = context.get_admin_context()