Merge "Fix bug and remove update lock in db.instance_test_and_set()"

2 files changed, 43 insertions, 20 deletions

diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 9cc3b64a1..8e454ec78 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -1768,38 +1768,40 @@ 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, session=None):
+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 session:
-        session = get_session()
+    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)
-
-        if uuidutils.is_uuid_like(instance_uuid):
-            query = query.filter_by(uuid=instance_uuid)
+                            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:
-            raise exception.InvalidUUID(instance_uuid)
-
-        # NOTE(vish): if with_lockmode isn't supported, as in sqlite,
-        #             then this has concurrency issues
-        instance = query.with_lockmode('update').first()
+            filter_op = attr_column.in_(ok_states)
 
-        state = instance[attr]
-        if state not 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['uuid'],
-                state=state,
+                instance_uuid=instance_ref['uuid'],
+                state=instance_ref[attr],
                 method='instance_test_and_set')
 
-        instance[attr] = new_state
-        instance.save(session=session)
-
 
 @require_context
 def instance_update(context, instance_uuid, values):
diff --git a/nova/tests/test_db_api.py b/nova/tests/test_db_api.py
index 29bce8bf5..ea6e9aea5 100644
--- a/nova/tests/test_db_api.py
+++ b/nova/tests/test_db_api.py
@@ -284,6 +284,27 @@ 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()