Eliminate a race condition on instance deletes.

- Add constraint and equality conditions to nova.db[.sqlalchemy].api
- Use host constraints to ensure the compute api doesn't simply delete
  an instance from the database that a compute manager has already
  started to run.

This race condition is associated with bug #998117

Change-Id: Id74192d3e66bea073327342f57ce0f26987efd2d

7 files changed, 154 insertions, 19 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 9f96b8e6b..a3bd93bac 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -980,9 +980,15 @@ class API(base.Base):
         try:
             if not instance['host']:
                 # Just update database, nothing else we can do
-                result = self.db.instance_destroy(context, instance['id'])
-                QUOTAS.commit(context, reservations)
-                return result
+                constraint = self.db.constraint(host=self.db.equal_any(host))
+                try:
+                    result = self.db.instance_destroy(
+                            context, instance['uuid'], constraint)
+                    QUOTAS.commit(context, reservations)
+                    return result
+                except exception.ConstraintNotMet:
+                    # Refresh to get new host information
+                    instance = self.get(context, instance['uuid'])
 
             self.update(context,
                         instance,
diff --git a/nova/db/api.py b/nova/db/api.py
index d33584036..2fff1988d 100644
--- a/nova/db/api.py
+++ b/nova/db/api.py
@@ -87,6 +87,32 @@ class NoMoreTargets(exception.NovaException):
 ###################
 
 
+def constraint(**conditions):
+    """Return a constraint object suitable for use with some updates."""
+    return IMPL.constraint(**conditions)
+
+
+def equal_any(*values):
+    """Return an equality condition object suitable for use in a constraint.
+
+    Equal_any conditions require that a model object's attribute equal any
+    one of the given values.
+    """
+    return IMPL.equal_any(*values)
+
+
+def not_equal(*values):
+    """Return an inequality condition object suitable for use in a constraint.
+
+    Not_equal conditions require that a model object's attribute differs from
+    all of the given values.
+    """
+    return IMPL.not_equal(*values)
+
+
+###################
+
+
 def service_destroy(context, instance_id):
     """Destroy the service or raise if it does not exist."""
     return IMPL.service_destroy(context, instance_id)
@@ -527,9 +553,9 @@ def instance_data_get_for_project(context, project_id, session=None):
                                               session=session)
 
 
-def instance_destroy(context, instance_id):
+def instance_destroy(context, instance_id, constraint=None):
     """Destroy the instance or raise if it does not exist."""
-    return IMPL.instance_destroy(context, instance_id)
+    return IMPL.instance_destroy(context, instance_id, constraint)
 
 
 def instance_get_by_uuid(context, uuid):
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 30c556b4d..0881af62f 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -43,6 +43,7 @@ from sqlalchemy.orm import joinedload_all
 from sqlalchemy.sql.expression import asc
 from sqlalchemy.sql.expression import desc
 from sqlalchemy.sql.expression import literal_column
+from sqlalchemy.sql.expression import or_
 from sqlalchemy.sql import func
 
 FLAGS = flags.FLAGS
@@ -263,6 +264,52 @@ def exact_filter(query, model, filters, legal_keys):
 ###################
 
 
+def constraint(**conditions):
+    return Constraint(conditions)
+
+
+def equal_any(*values):
+    return EqualityCondition(values)
+
+
+def not_equal(*values):
+    return InequalityCondition(values)
+
+
+class Constraint(object):
+
+    def __init__(self, conditions):
+        self.conditions = conditions
+
+    def apply(self, model, query):
+        clauses = []
+        for key, condition in self.conditions.iteritems():
+            for clause in condition.clauses(getattr(model, key)):
+                query = query.filter(clause)
+        return query
+
+
+class EqualityCondition(object):
+
+    def __init__(self, values):
+        self.values = values
+
+    def clauses(self, field):
+        return or_([field == value for value in self.values])
+
+
+class InequalityCondition(object):
+
+    def __init__(self, values):
+        self.values = values
+
+    def clauses(self, field):
+        return [field != value for value in self.values]
+
+
+###################
+
+
 @require_admin_context
 def service_destroy(context, service_id):
     session = get_session()
@@ -1311,7 +1358,7 @@ def instance_data_get_for_project(context, project_id, session=None):
 
 
 @require_context
-def instance_destroy(context, instance_id):
+def instance_destroy(context, instance_id, constraint=None):
     session = get_session()
     with session.begin():
         if utils.is_uuid_like(instance_id):
@@ -1321,11 +1368,14 @@ def instance_destroy(context, instance_id):
         else:
             instance_ref = instance_get(context, instance_id,
                     session=session)
-        session.query(models.Instance).\
-                filter_by(id=instance_id).\
-                update({'deleted': True,
-                        'deleted_at': utils.utcnow(),
-                        'updated_at': literal_column('updated_at')})
+        query = session.query(models.Instance).filter_by(id=instance_id)
+        if constraint is not None:
+            query = constraint.apply(models.Instance, query)
+        count = query.update({'deleted': True,
+                              'deleted_at': utils.utcnow(),
+                              'updated_at': literal_column('updated_at')})
+        if count == 0:
+            raise exception.ConstraintNotMet()
         session.query(models.SecurityGroupInstanceAssociation).\
                 filter_by(instance_id=instance_id).\
                 update({'deleted': True,
diff --git a/nova/exception.py b/nova/exception.py
index 1e1818e7d..aff188d01 100644
--- a/nova/exception.py
+++ b/nova/exception.py
@@ -419,6 +419,11 @@ class InvalidUUID(Invalid):
     message = _("Expected a uuid but received %(uuid).")
 
 
+class ConstraintNotMet(NovaException):
+    message = _("Constraint not met.")
+    code = 412
+
+
 class NotFound(NovaException):
     message = _("Resource could not be found.")
     code = 404
diff --git a/nova/tests/api/openstack/compute/test_servers.py b/nova/tests/api/openstack/compute/test_servers.py
index 83a8963a5..f9e815c20 100644
--- a/nova/tests/api/openstack/compute/test_servers.py
+++ b/nova/tests/api/openstack/compute/test_servers.py
@@ -1298,7 +1298,7 @@ class ServersControllerTest(test.TestCase):
         self.stubs.Set(nova.db, 'instance_get_by_uuid',
                 fakes.fake_instance_get(vm_state=vm_states.ACTIVE))
 
-        def instance_destroy_mock(context, id):
+        def instance_destroy_mock(*args, **kwargs):
             self.server_delete_called = True
         self.stubs.Set(nova.db, 'instance_destroy', instance_destroy_mock)
 
@@ -1313,7 +1313,7 @@ class ServersControllerTest(test.TestCase):
 
         self.server_delete_called = False
 
-        def instance_destroy_mock(context, id):
+        def instance_destroy_mock(*args, **kwargs):
             self.server_delete_called = True
         self.stubs.Set(nova.db, 'instance_destroy', instance_destroy_mock)
 
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index f1903bd4c..8530eca7b 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -2270,6 +2270,22 @@ class ComputeAPITestCase(BaseTestCase):
 
         db.instance_destroy(self.context, instance['id'])
 
+    def test_delete_fast_if_host_not_set(self):
+        instance = self._create_fake_instance({'host': None})
+        self.compute_api.delete(self.context, instance)
+        self.assertRaises(exception.InstanceNotFound, db.instance_get_by_uuid,
+                          self.context, instance['uuid'])
+
+    def test_delete_handles_host_setting_race_condition(self):
+        instance, instance_uuid = self._run_instance()
+        instance['host'] = None  # make it think host was never set
+        self.compute_api.delete(self.context, instance)
+
+        instance = db.instance_get_by_uuid(self.context, instance_uuid)
+        self.assertEqual(instance['task_state'], task_states.DELETING)
+
+        db.instance_destroy(self.context, instance['id'])
+
     def test_delete_fail(self):
         instance, instance_uuid = self._run_instance()
 
@@ -3950,12 +3966,7 @@ class ComputePolicyTestCase(BaseTestCase):
         nova.compute.api.check_policy(self.context, 'reboot', {})
 
     def test_wrapped_method(self):
-        instance = self._create_fake_instance()
-        # Reset this to None for this policy check. If it's set, it
-        # tries to do a compute_api.update() and we're not testing for
-        # that here.
-        instance['host'] = None
-        self.compute.run_instance(self.context, instance['uuid'])
+        instance = self._create_fake_instance(params={'host': None})
 
         # force delete to fail
         rules = {"compute:delete": [["false:false"]]}
diff --git a/nova/tests/test_db_api.py b/nova/tests/test_db_api.py
index 8ce2ab6ee..26feb0fc2 100644
--- a/nova/tests/test_db_api.py
+++ b/nova/tests/test_db_api.py
@@ -840,3 +840,40 @@ class TestIpAllocation(test.TestCase):
         fixed_ip = db.fixed_ip_get_by_address(self.ctxt, address)
         self.assertEqual(fixed_ip.instance_id, self.instance.id)
         self.assertEqual(fixed_ip.network_id, self.network.id)
+
+
+class InstanceDestroyConstraints(test.TestCase):
+
+    def test_destroy_with_equal_any_constraint_met(self):
+        ctx = context.get_admin_context()
+        instance = db.instance_create(ctx, {'task_state': 'deleting'})
+        constraint = db.constraint(task_state=db.equal_any('deleting'))
+        db.instance_destroy(ctx, instance['uuid'], constraint)
+        self.assertRaises(exception.InstanceNotFound, db.instance_get_by_uuid,
+                          ctx, instance['uuid'])
+
+    def test_destroy_with_equal_any_constraint_not_met(self):
+        ctx = context.get_admin_context()
+        instance = db.instance_create(ctx, {'vm_state': 'resize'})
+        constraint = db.constraint(vm_state=db.equal_any('active', 'error'))
+        self.assertRaises(exception.ConstraintNotMet, db.instance_destroy,
+                          ctx, instance['uuid'], constraint)
+        instance = db.instance_get_by_uuid(ctx, instance['uuid'])
+        self.assertFalse(instance['deleted'])
+
+    def test_destroy_with_not_equal_constraint_met(self):
+        ctx = context.get_admin_context()
+        instance = db.instance_create(ctx, {'task_state': 'deleting'})
+        constraint = db.constraint(task_state=db.not_equal('error', 'resize'))
+        db.instance_destroy(ctx, instance['uuid'], constraint)
+        self.assertRaises(exception.InstanceNotFound, db.instance_get_by_uuid,
+                          ctx, instance['uuid'])
+
+    def test_destroy_with_not_equal_constraint_not_met(self):
+        ctx = context.get_admin_context()
+        instance = db.instance_create(ctx, {'vm_state': 'active'})
+        constraint = db.constraint(vm_state=db.not_equal('active', 'error'))
+        self.assertRaises(exception.ConstraintNotMet, db.instance_destroy,
+                          ctx, instance['uuid'], constraint)
+        instance = db.instance_get_by_uuid(ctx, instance['uuid'])
+        self.assertFalse(instance['deleted'])