From 563e06c3c69888e4166b10c1d9f50fe0e820c02d Mon Sep 17 00:00:00 2001 From: TommyLike Date: Thu, 2 Mar 2017 14:43:15 +0800 Subject: [PATCH] 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 --- ...142971c4ef_add_reservation_expire_index.py | 43 +++++++++++++++++++ manila/db/sqlalchemy/api.py | 10 +++-- manila/db/sqlalchemy/models.py | 6 --- manila/scheduler/manager.py | 8 +++- .../alembic/migrations_data_checks.py | 22 ++++++++++ manila/tests/db/sqlalchemy/test_api.py | 31 +++++++++++++ manila/tests/scheduler/test_manager.py | 34 +++++++++++++++ .../add-perodic-task-7454sd45deopb13e.yaml | 4 ++ 8 files changed, 148 insertions(+), 10 deletions(-) create mode 100644 manila/db/migrations/alembic/versions/7d142971c4ef_add_reservation_expire_index.py create mode 100644 releasenotes/notes/add-perodic-task-7454sd45deopb13e.yaml diff --git a/manila/db/migrations/alembic/versions/7d142971c4ef_add_reservation_expire_index.py b/manila/db/migrations/alembic/versions/7d142971c4ef_add_reservation_expire_index.py new file mode 100644 index 0000000000..6a1a2b9a0f --- /dev/null +++ b/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') diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index ecc779e8e4..b0802ce4ac 100644 --- a/manila/db/sqlalchemy/api.py +++ b/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) diff --git a/manila/db/sqlalchemy/models.py b/manila/db/sqlalchemy/models.py index 17c1948dfe..67d5d66f7e 100644 --- a/manila/db/sqlalchemy/models.py +++ b/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.""" diff --git a/manila/scheduler/manager.py b/manila/scheduler/manager.py index 93a9ad12d2..78853db67d 100644 --- a/manila/scheduler/manager.py +++ b/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: diff --git a/manila/tests/db/migrations/alembic/migrations_data_checks.py b/manila/tests/db/migrations/alembic/migrations_data_checks.py index 69c62c8dea..e15f084065 100644 --- a/manila/tests/db/migrations/alembic/migrations_data_checks.py +++ b/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)) diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index 54d1bfdb74..e6dc4a40ed 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/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): diff --git a/manila/tests/scheduler/test_manager.py b/manila/tests/scheduler/test_manager.py index 4ef53550f8..7734f3a67f 100644 --- a/manila/tests/scheduler/test_manager.py +++ b/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, diff --git a/releasenotes/notes/add-perodic-task-7454sd45deopb13e.yaml b/releasenotes/notes/add-perodic-task-7454sd45deopb13e.yaml new file mode 100644 index 0000000000..c24bbabd17 --- /dev/null +++ b/releasenotes/notes/add-perodic-task-7454sd45deopb13e.yaml @@ -0,0 +1,4 @@ +--- +features: + - Added perodic task to clean up expired reservation + at manila scheduler service.