Protect against '.delete()' for quota and revisions

The devref was suggesting to do exactly the wrong thing for cases where
we have SQLAlchemy event listeners waiting for deleted objects.
DELETE statements fail to trigger any kind of SQLAlchemy event
listeners and the majority of our delete operations are small
(<10 items at once) so the performance difference is negligible
(especially since we do it so infrequently).

Change-Id: Id142179418a7b94d8d9695871b3fcd5dcc64730c

doc/source/devref/effective_neutron.rst

@@ -87,8 +87,11 @@ Document common pitfalls as well as good practices done during database developm
 
 * `first() <http://docs.sqlalchemy.org/en/rel_1_0/orm/query.html#sqlalchemy.orm.query.Query.first>`_
   does not raise an exception.
-* Do not get an object to delete it. If you can `delete() <http://docs.sqlalchemy.org/en/rel_1_0/orm/query.html#sqlalchemy.orm.query.Query.delete>`_
-  on the query object. Read the warnings for more details about in-python cascades.
+* Do not use `delete() <http://docs.sqlalchemy.org/en/rel_1_0/orm/query.html#sqlalchemy.orm.query.Query.delete>`_
+  to remove objects. A delete query does not load the object so no sqlalchemy events
+  can be triggered that would do things like recalculate quotas or update revision
+  numbers of parent objects. For more details on all of the things that can go wrong
+  using bulk delete operations, see the "Warning" sections in the link above.
 * For PostgreSQL if you're using GROUP BY everything in the SELECT list must be
   an aggregate SUM(...), COUNT(...), etc or used in the GROUP BY.
 

neutron/db/standard_attr.py

@@ -16,8 +16,10 @@ from neutron_lib.db import constants as db_const
 from neutron_lib.db import model_base
 from oslo_utils import timeutils
 import sqlalchemy as sa
+from sqlalchemy import event
 from sqlalchemy.ext.associationproxy import association_proxy
 from sqlalchemy.ext import declarative
+from sqlalchemy.orm import session as se
 
 from neutron._i18n import _LE
 from neutron.db import sqlalchemytypes
@@ -170,3 +172,13 @@ def get_standard_attr_resource_model_map():
                                         res=resource))
             rs_map[resource] = subclass
     return rs_map
+
+
+@event.listens_for(se.Session, 'after_bulk_delete')
+def throw_exception_on_bulk_delete_of_listened_for_objects(delete_context):
+    if hasattr(delete_context.mapper.class_, 'revises_on_change'):
+        raise RuntimeError(_LE("%s may not be deleted in bulk because it "
+                               "bumps the revision of other resources via "
+                               "SQLAlchemy event handlers, which are not "
+                               "compatible with bulk deletes.") %
+                           delete_context.mapper.class_)

neutron/quota/resource.py

@@ -17,6 +17,7 @@ from oslo_log import log
 from oslo_utils import excutils
 from sqlalchemy import event
 from sqlalchemy import exc as sql_exc
+from sqlalchemy.orm import session as se
 
 from neutron._i18n import _LE, _LW
 from neutron.db import api as db_api
@@ -289,9 +290,18 @@ class TrackedResource(BaseResource):
                        'used': usage_info.used})
         return usage_info.used + reserved
 
+    def _except_bulk_delete(self, delete_context):
+        if delete_context.mapper.class_ == self._model_class:
+            raise RuntimeError(_LE("%s may not be deleted in bulk because "
+                                   "it is tracked by the quota engine via "
+                                   "SQLAlchemy event handlers, which are not "
+                                   "compatible with bulk deletes.") %
+                               self._model_class)
+
     def register_events(self):
         event.listen(self._model_class, 'after_insert', self._db_event_handler)
         event.listen(self._model_class, 'after_delete', self._db_event_handler)
+        event.listen(se.Session, 'after_bulk_delete', self._except_bulk_delete)
 
     def unregister_events(self):
         try:
@@ -299,6 +309,8 @@ class TrackedResource(BaseResource):
                          self._db_event_handler)
             event.remove(self._model_class, 'after_delete',
                          self._db_event_handler)
+            event.remove(se.Session, 'after_bulk_delete',
+                         self._except_bulk_delete)
         except sql_exc.InvalidRequestError:
             LOG.warning(_LW("No sqlalchemy event for resource %s found"),
                         self.name)

neutron/tests/unit/db/test_standard_attr.py

@@ -17,6 +17,7 @@ import gc
 from sqlalchemy.ext import declarative
 import testtools
 
+from neutron import context
 from neutron.db import standard_attr
 from neutron.tests import base
 from neutron.tests.unit import testlib_api
@@ -73,3 +74,15 @@ class StandardAttrAPIImapctTestCase(testlib_api.SqlTestCase):
             set(expected),
             set(standard_attr.get_standard_attr_resource_model_map().keys())
         )
+
+
+class StandardAttrRevisesBulkDeleteTestCase(testlib_api.SqlTestCase):
+
+    def test_bulk_delete_protection(self):
+        # security group rules increment security groups so they must not be
+        # allowed to be deleted in bulk
+        mm = standard_attr.get_standard_attr_resource_model_map()
+        sg_rule_model = mm['security_group_rules']
+        with testtools.ExpectedException(RuntimeError):
+            ctx = context.get_admin_context()
+            ctx.session.query(sg_rule_model).delete()

neutron/tests/unit/quota/test_resource.py

@@ -15,6 +15,7 @@
 import mock
 from oslo_config import cfg
 from oslo_utils import uuidutils
+import testtools
 
 from neutron import context
 from neutron.db import api as db_api
@@ -113,6 +114,12 @@ class TestTrackedResource(testlib_api.SqlTestCaseLight):
         self._register_events(res)
         return res
 
+    def test_bulk_delete_protection(self):
+        self._create_resource()
+        with testtools.ExpectedException(RuntimeError):
+            ctx = context.get_admin_context()
+            ctx.session.query(test_quota.MehModel).delete()
+
     def test_count_first_call_with_dirty_false(self):
         quota_api.set_quota_usage(
             self.context, self.resource, self.tenant_id, in_use=1)