Add periodic task to clean up expired reservation

Currently we would have uncommit reservations in database if
the commit operation failed. This could obstruct creating new
shares or others which would consume quota. This patch adds
a perodic task to clean this expired reservations.

Also fix bug in db.reservation_expire method(We dropped
attribute 'usage' from Reservation, but it's still used
in this method).

Closes-Bug: #1669449

Change-Id: I3dc0973ebac5eb33832e242c72059be1eb954369

manila/db/migrations/alembic/versions/7d142971c4ef_add_reservation_expire_index.py

@@ -0,0 +1,43 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+"""add_reservation_expire_index
+
+Revision ID: 7d142971c4ef
+Revises: d5db24264f5c
+Create Date: 2017-03-02 09:19:27.114719
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '7d142971c4ef'
+down_revision = 'd5db24264f5c'
+
+from alembic import op
+from sqlalchemy import Index, MetaData, Table
+
+
+def _reservation_index(method):
+    meta = MetaData()
+    meta.bind = op.get_bind().engine
+    reservations = Table('reservations', meta, autoload=True)
+    index = Index('reservations_deleted_expire_idx',
+                  reservations.c.deleted, reservations.c.expire)
+    getattr(index, method)(meta.bind)
+
+
+def upgrade():
+    _reservation_index('create')
+
+
+def downgrade():
+    _reservation_index('drop')

manila/db/sqlalchemy/api.py

@@ -1085,10 +1085,14 @@ def reservation_expire(context):
                                         session=session, read_deleted="no").\
             filter(models.Reservation.expire < current_time)
 
-        for reservation in reservation_query.join(models.QuotaUsage).all():
+        for reservation in reservation_query.all():
             if reservation.delta >= 0:
-                reservation.usage.reserved -= reservation.delta
-                session.add(reservation.usage)
+                quota_usage = model_query(context, models.QuotaUsage,
+                                          session=session,
+                                          read_deleted="no").filter(
+                    models.QuotaUsage.id == reservation.usage_id).first()
+                quota_usage.reserved -= reservation.delta
+                session.add(quota_usage)
 
         reservation_query.soft_delete(synchronize_session=False)
 

manila/db/sqlalchemy/models.py

@@ -177,12 +177,6 @@ class Reservation(BASE, ManilaBase):
     delta = Column(Integer)
     expire = Column(DateTime, nullable=False)
 
-#    usage = orm.relationship(
-#        "QuotaUsage",
-#        foreign_keys=usage_id,
-#        primaryjoin='and_(Reservation.usage_id == QuotaUsage.id,'
-#                         'QuotaUsage.deleted == 0)')
-
 
 class Share(BASE, ManilaBase):
     """Represents an NFS and CIFS shares."""

manila/scheduler/manager.py

@@ -21,6 +21,7 @@ Scheduler Service
 
 from oslo_config import cfg
 from oslo_log import log
+from oslo_service import periodic_task
 from oslo_utils import excutils
 from oslo_utils import importutils
 
@@ -29,6 +30,7 @@ from manila import context
 from manila import db
 from manila import exception
 from manila import manager
+from manila import quota
 from manila import rpc
 from manila.share import rpcapi as share_rpcapi
 
@@ -75,7 +77,7 @@ class SchedulerManager(manager.Manager):
             scheduler_driver = MAPPING[scheduler_driver]
 
         self.driver = importutils.import_object(scheduler_driver)
-        super(SchedulerManager, self).__init__(*args, **kwargs)
+        super(self.__class__, self).__init__(*args, **kwargs)
 
     def init_host(self):
         ctxt = context.get_admin_context()
@@ -222,6 +224,10 @@ class SchedulerManager(manager.Manager):
         if share_group_id:
             db.share_group_update(context, share_group_id, share_group_state)
 
+    @periodic_task.periodic_task(spacing=600, run_immediately=True)
+    def _expire_reservations(self, context):
+        quota.QUOTAS.expire(context)
+
     def create_share_group(self, context, share_group_id, request_spec=None,
                            filter_properties=None):
         try:

manila/tests/db/migrations/alembic/migrations_data_checks.py

@@ -2169,3 +2169,25 @@ class ShareGroupNewConsistentSnapshotSupportColumnChecks(BaseMigrationChecks):
         self.test_case.assertEqual(1, db_result.rowcount)
         for sg in db_result:
             self.test_case.assertFalse(hasattr(sg, self.new_attr_name))
+
+
+@map_to_migration('7d142971c4ef')
+class ReservationExpireIndexChecks(BaseMigrationChecks):
+
+    def setup_upgrade_data(self, engine):
+        pass
+
+    def _get_reservations_expire_delete_index(self, engine):
+        reservation_table = utils.load_table('reservations', engine)
+        members = ['deleted', 'expire']
+        for idx in reservation_table.indexes:
+            if sorted(idx.columns.keys()) == members:
+                return idx
+
+    def check_upgrade(self, engine, data):
+        self.test_case.assertTrue(
+            self._get_reservations_expire_delete_index(engine))
+
+    def check_downgrade(self, engine):
+        self.test_case.assertFalse(
+            self._get_reservations_expire_delete_index(engine))

manila/tests/db/sqlalchemy/test_api.py

@@ -2473,6 +2473,37 @@ class NetworkAllocationsDatabaseAPITestCase(test.TestCase):
             self.assertIn(na.label, ('admin', 'user', None))
 
 
+class ReservationDatabaseAPITest(test.TestCase):
+
+    def setUp(self):
+        super(ReservationDatabaseAPITest, self).setUp()
+        self.context = context.get_admin_context()
+
+    def test_reservation_expire(self):
+        quota_usage = db_api.quota_usage_create(self.context, 'fake_project',
+                                                'fake_user', 'fake_resource',
+                                                0, 12, until_refresh=None)
+        session = db_api.get_session()
+        for time_s in (-1, 1):
+            reservation = db_api._reservation_create(
+                self.context, 'fake_uuid',
+                quota_usage, 'fake_project',
+                'fake_user', 'fake_resource', 10,
+                timeutils.utcnow() +
+                datetime.timedelta(days=time_s),
+                session=session)
+
+        db_api.reservation_expire(self.context)
+
+        reservations = db_api._quota_reservations_query(session, self.context,
+                                                        ['fake_uuid']).all()
+        quota_usage = db_api.quota_usage_get(self.context, 'fake_project',
+                                             'fake_resource')
+        self.assertEqual(1, len(reservations))
+        self.assertEqual(reservation['id'], reservations[0]['id'])
+        self.assertEqual(2, quota_usage['reserved'])
+
+
 @ddt.ddt
 class PurgeDeletedTest(test.TestCase):
 

manila/tests/scheduler/test_manager.py

@@ -17,6 +17,12 @@
 Tests For Scheduler Manager
 """
 
+try:
+    # Python3 variant
+    from importlib import reload
+except ImportError:
+    pass
+
 import ddt
 import mock
 from oslo_config import cfg
@@ -25,6 +31,7 @@ from manila.common import constants
 from manila import context
 from manila import db
 from manila import exception
+from manila import quota
 from manila.scheduler.drivers import base
 from manila.scheduler.drivers import filter
 from manila.scheduler import manager
@@ -46,6 +53,19 @@ class SchedulerManagerTestCase(test.TestCase):
 
     def setUp(self):
         super(SchedulerManagerTestCase, self).setUp()
+        self.periodic_tasks = []
+
+        def _periodic_task(*args, **kwargs):
+            def decorator(f):
+                self.periodic_tasks.append(f)
+                return f
+            return mock.Mock(side_effect=decorator)
+
+        self.mock_periodic_task = self.mock_object(
+            manager.periodic_task, 'periodic_task',
+            mock.Mock(side_effect=_periodic_task))
+        reload(manager)
+
         self.flags(scheduler_driver=self.driver_cls_name)
         self.manager = self.manager_cls()
         self.context = context.RequestContext('fake_user', 'fake_project')
@@ -164,6 +184,20 @@ class SchedulerManagerTestCase(test.TestCase):
                 assert_called_once_with(self.context, request_spec, {}))
             manager.LOG.error.assert_called_once_with(mock.ANY, mock.ANY)
 
+    @mock.patch.object(quota.QUOTAS, 'expire')
+    def test__expire_reservations(self, mock_expire):
+        self.manager._expire_reservations(self.context)
+
+        mock_expire.assert_called_once_with(self.context)
+
+    def test_periodic_tasks(self):
+        self.mock_periodic_task.assert_called_once_with(
+            spacing=600, run_immediately=True)
+        self.assertEqual(1, len(self.periodic_tasks))
+        self.assertEqual(
+            self.periodic_tasks[0].__name__,
+            self.manager._expire_reservations.__name__)
+
     def test_get_pools(self):
         """Ensure get_pools exists and calls base_scheduler.get_pools."""
         mock_get_pools = self.mock_object(self.manager.driver,

releasenotes/notes/add-perodic-task-7454sd45deopb13e.yaml

@@ -0,0 +1,4 @@
+---
+features:
+  - Added perodic task to clean up expired reservation
+    at manila scheduler service.