Fix racey snapshots.

Fixes bug 949475

Atomically tests and sets the instance task_state before allowing a
snapshot or backup to be initiated.

Change-Id: I40671a80f5e75337e176a715837f62d400cc21b6

4 files changed, 56 insertions, 14 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index e24a3de5b..3279ca546 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1083,8 +1083,7 @@ class API(base.Base):
         return self.db.instance_get_all_by_filters(context, filters)
 
     @wrap_check_policy
-    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.SHUTOFF],
-                          task_state=[None, task_states.RESIZE_VERIFY])
+    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.SHUTOFF])
     def backup(self, context, instance, name, backup_type, rotation,
                extra_properties=None):
         """Backup the given instance
@@ -1102,8 +1101,7 @@ class API(base.Base):
         return recv_meta
 
     @wrap_check_policy
-    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.SHUTOFF],
-                          task_state=[None, task_states.RESIZE_VERIFY])
+    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.SHUTOFF])
     def snapshot(self, context, instance, name, extra_properties=None):
         """Snapshot the given instance.
 
@@ -1130,9 +1128,18 @@ class API(base.Base):
         :param extra_properties: dict of extra image properties to include
 
         """
-        task_state = instance["task_state"]
         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)
+
+        self.db.instance_test_and_set(
+                context, instance_uuid, 'task_state', [None], task_state)
+
         properties = {
             'instance_uuid': instance_uuid,
             'user_id': str(context.user_id),
diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index ef239ba50..1c6de672f 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -912,13 +912,6 @@ class ComputeManager(manager.SchedulerDependentManager):
         :param rotation: int representing how many backups to keep around;
             None if rotation shouldn't be used (as in the case of snapshots)
         """
-        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)
-
         context = context.elevated()
         instance_ref = self.db.instance_get_by_uuid(context, instance_uuid)
 
@@ -926,8 +919,7 @@ class ComputeManager(manager.SchedulerDependentManager):
         self._instance_update(context,
                               instance_ref['id'],
                               power_state=current_power_state,
-                              vm_state=vm_states.ACTIVE,
-                              task_state=task_state)
+                              vm_state=vm_states.ACTIVE)
 
         LOG.audit(_('instance %s: snapshotting'), instance_uuid,
                   context=context)
diff --git a/nova/db/api.py b/nova/db/api.py
index bf94747df..9d4fae11e 100644
--- a/nova/db/api.py
+++ b/nova/db/api.py
@@ -647,6 +647,15 @@ def instance_get_all_hung_in_rebooting(context, reboot_window):
     return IMPL.instance_get_all_hung_in_rebooting(context, reboot_window)
 
 
+def instance_test_and_set(context, instance_id, 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.
+    """
+    return IMPL.instance_test_and_set(
+            context, instance_id, attr, ok_states, new_state)
+
+
 def instance_update(context, instance_id, values):
     """Set the given properties on an instance and update it.
 
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 07d48a229..ff9ffd0bc 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -1691,6 +1691,40 @@ def instance_get_all_hung_in_rebooting(context, reboot_window, session=None):
 
 
 @require_context
+def instance_test_and_set(context, instance_id, attr, ok_states,
+                          new_state, session=None):
+    """Atomically check if an instance is in a valid state, and if it is, set
+    the instance into a new state.
+    """
+    if not session:
+        session = get_session()
+
+    with session.begin():
+        query = model_query(context, models.Instance, session=session,
+                            project_only=True)
+
+        if utils.is_uuid_like(instance_id):
+            query = query.filter_by(uuid=instance_id)
+        else:
+            query = query.filter_by(id=instance_id)
+
+        # NOTE(vish): if with_lockmode isn't supported, as in sqlite,
+        #             then this has concurrency issues
+        instance = query.with_lockmode('update').first()
+
+        state = instance[attr]
+        if state not in ok_states:
+            raise exception.InstanceInvalidState(
+                attr=attr,
+                instance_uuid=instance['uuid'],
+                state=state,
+                method='instance_test_and_set')
+
+        instance[attr] = new_state
+        instance.save(session=session)
+
+
+@require_context
 def instance_update(context, instance_id, values):
     session = get_session()