From 83a7fdf010ff11de3791bfaefe581e81bc2cce03 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 6 Sep 2022 11:56:28 +0100 Subject: [PATCH] db: Migrate "service" APIs to enginefacade Migrate service-related APIs from the legacy enginefacade to the modern context-based enginefacade. This one is a little tricky as it highlights two issues. * The models in 'manila.db.sqlalchemy.models' do not match those created by the migrations. This causes us issues since we apply the former rather than running migrations when creating our database via the DatabaseFixture, which means we're not actually testing against what our customers see. One gap is closed here in order to get newly added test working as expected but there's a larger issue that needs to be addressed at a later date. * We are using a pattern of "try to create and retrieve on failure" rather than "try to get and create on failure" when determining whether to create availability zone records. This works where each operation results in a new transaction but does not when the transaction is shared. We switch this. With these two issues addressed, the rest of the migration is relatively straightforward. Signed-off-by: Stephen Finucane Change-Id: Icfe4b225e216dfda46366af1eb3820b193befa0f --- manila/db/sqlalchemy/api.py | 157 +++++++++++++++---------- manila/db/sqlalchemy/models.py | 3 + manila/test.py | 8 ++ manila/tests/db/sqlalchemy/test_api.py | 40 +++++-- 4 files changed, 136 insertions(+), 72 deletions(-) diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 1b6fe649fe..b55fff5b50 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -500,21 +500,22 @@ def share_resources_host_update(context, current_host, new_host): @require_admin_context +@context_manager.writer def service_destroy(context, service_id): - session = get_session() - with session.begin(): - service_ref = service_get(context, service_id, session=session) - service_ref.soft_delete(session) + service_ref = _service_get(context, service_id) + service_ref.soft_delete(context.session) @require_admin_context -def service_get(context, service_id, session=None): - result = (model_query( - context, - models.Service, - session=session). - filter_by(id=service_id). - first()) +def _service_get(context, service_id): + result = ( + model_query( + context, + models.Service, + ).filter_by( + id=service_id, + ).first() + ) if not result: raise exception.ServiceNotFound(service_id=service_id) @@ -522,6 +523,13 @@ def service_get(context, service_id, session=None): @require_admin_context +@context_manager.reader +def service_get(context, service_id): + return _service_get(context, service_id) + + +@require_admin_context +@context_manager.reader def service_get_all(context, disabled=None): query = model_query(context, models.Service) @@ -532,6 +540,7 @@ def service_get_all(context, disabled=None): @require_admin_context +@context_manager.reader def service_get_all_by_topic(context, topic): return (model_query( context, models.Service, read_deleted="no"). @@ -541,6 +550,7 @@ def service_get_all_by_topic(context, topic): @require_admin_context +@context_manager.reader def service_get_by_host_and_topic(context, host, topic): result = (model_query( context, models.Service, read_deleted="no"). @@ -554,39 +564,53 @@ def service_get_by_host_and_topic(context, host, topic): @require_admin_context -def _service_get_all_topic_subquery(context, session, topic, subq, label): +def _service_get_all_topic_subquery(context, topic, subq, label): sort_value = getattr(subq.c, label) - return (model_query(context, models.Service, - func.coalesce(sort_value, 0), - session=session, read_deleted="no"). - filter_by(topic=topic). - filter_by(disabled=False). - outerjoin((subq, models.Service.host == subq.c.host)). - order_by(sort_value). - all()) + return ( + model_query( + context, models.Service, + func.coalesce(sort_value, 0), + read_deleted="no", + ).filter_by( + topic=topic, + ).filter_by( + disabled=False, + ).outerjoin( + (subq, models.Service.host == subq.c.host) + ).order_by( + sort_value + ).all() + ) @require_admin_context +@context_manager.reader def service_get_all_share_sorted(context): - session = get_session() - with session.begin(): - topic = CONF.share_topic - label = 'share_gigabytes' - subq = (model_query(context, models.Share, - func.sum(models.Share.size).label(label), - session=session, read_deleted="no"). - join(models.ShareInstance, - models.ShareInstance.share_id == models.Share.id). - group_by(models.ShareInstance.host). - subquery()) - return _service_get_all_topic_subquery(context, - session, - topic, - subq, - label) + topic = CONF.share_topic + label = 'share_gigabytes' + subq = ( + model_query( + context, + models.Share, + func.sum(models.Share.size).label(label), + read_deleted="no", + ).join( + models.ShareInstance, + models.ShareInstance.share_id == models.Share.id, + ).group_by( + models.ShareInstance.host + ).subquery() + ) + return _service_get_all_topic_subquery( + context, + topic, + subq, + label, + ) @require_admin_context +@context_manager.reader def service_get_by_args(context, host, binary): result = (model_query(context, models.Service). filter_by(host=host). @@ -600,32 +624,28 @@ def service_get_by_args(context, host, binary): @require_admin_context +@context_manager.writer def service_create(context, values): - session = get_session() - - _ensure_availability_zone_exists(context, values, session) + _ensure_availability_zone_exists(context, values) service_ref = models.Service() service_ref.update(values) if not CONF.enable_new_services: service_ref.disabled = True - with session.begin(): - service_ref.save(session) - return service_ref + service_ref.save(context.session) + return service_ref @require_admin_context @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True) +@context_manager.writer def service_update(context, service_id, values): - session = get_session() + _ensure_availability_zone_exists(context, values, strict=False) - _ensure_availability_zone_exists(context, values, session, strict=False) - - with session.begin(): - service_ref = service_get(context, service_id, session=session) - service_ref.update(values) - service_ref.save(session=session) + service_ref = _service_get(context, service_id) + service_ref.update(values) + service_ref.save(context.session) ################### @@ -5307,7 +5327,9 @@ def share_type_extra_specs_update_or_create(context, share_type_id, specs): return specs -def _ensure_availability_zone_exists(context, values, session, strict=True): +def _ensure_availability_zone_exists( + context, values, session=None, *, strict=True, +): az_name = values.pop('availability_zone', None) if strict and not az_name: @@ -5317,9 +5339,9 @@ def _ensure_availability_zone_exists(context, values, session, strict=True): return if uuidutils.is_uuid_like(az_name): - az_ref = availability_zone_get(context, az_name, session=session) + az_ref = _availability_zone_get(context, az_name, session=session) else: - az_ref = availability_zone_create_if_not_exist( + az_ref = _availability_zone_create_if_not_exist( context, az_name, session=session) values.update({'availability_zone_id': az_ref['id']}) @@ -5330,6 +5352,11 @@ def availability_zone_get(context, id_or_name, session=None): if session is None: session = get_session() + return _availability_zone_get(context, id_or_name, session=session) + + +@require_context +def _availability_zone_get(context, id_or_name, session=None): query = model_query(context, models.AvailabilityZone, session=session) if uuidutils.is_uuid_like(id_or_name): @@ -5345,21 +5372,23 @@ def availability_zone_get(context, id_or_name, session=None): return result +# TODO(stephenfin): Remove the 'session' argument once all callers have been +# converted @require_context @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True) -def availability_zone_create_if_not_exist(context, name, session=None): - if session is None: - session = get_session() - - az = models.AvailabilityZone() - az.update({'id': uuidutils.generate_uuid(), 'name': name}) +def _availability_zone_create_if_not_exist(context, name, session=None): try: - with session.begin(): - az.save(session) - # NOTE(u_glide): Do not catch specific exception here, because it depends - # on concrete backend used by SqlAlchemy - except Exception: - return availability_zone_get(context, name, session=session) + return _availability_zone_get(context, name, session=session) + except exception.AvailabilityZoneNotFound: + az = models.AvailabilityZone() + az.update({'id': uuidutils.generate_uuid(), 'name': name}) + # TODO(stephenfin): Remove this branch once all callers have been + # updated not to pass 'session' + if session is not None: + with session.begin(): + az.save(session) + else: + az.save(context.session) return az diff --git a/manila/db/sqlalchemy/models.py b/manila/db/sqlalchemy/models.py index 365fca4365..d4acc21f75 100644 --- a/manila/db/sqlalchemy/models.py +++ b/manila/db/sqlalchemy/models.py @@ -1202,6 +1202,9 @@ class DriverPrivateData(BASE, ManilaBase): class AvailabilityZone(BASE, ManilaBase): """Represents a private data as key-value pairs for a driver.""" __tablename__ = 'availability_zones' + __table_args__ = ( + schema.UniqueConstraint('name', 'deleted', name='az_name_uc'), + ) id = Column(String(36), primary_key=True, nullable=False) deleted = Column(String(36), default='False') name = Column(String(255), nullable=False) diff --git a/manila/test.py b/manila/test.py index 3851bbe321..0e0fa9bd60 100644 --- a/manila/test.py +++ b/manila/test.py @@ -72,6 +72,14 @@ class DatabaseFixture(fixtures.Fixture): self.engine = db_session.get_engine() self.engine.dispose() conn = self.engine.connect() + # FIXME(stephenfin): This is an issue. We're not applying our + # migrations on SQLite in-memory backends and because the model schemas + # and migration schemas don't currently match exactly, we are not + # testing against something resembling what our customers would see. + # We should (a) start applying the migrations for all backends (which + # will require reworking the migrations since SQLite doesn't support + # ALTER fully, meaning batch mode must be used) and (b) get the two + # different sets of schemas in sync and keep them in sync. if sql_connection == "sqlite://": self.setup_sqlite(db_migrate) else: diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index 3941f96d7c..94fbaa4a9f 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -3101,8 +3101,10 @@ class ShareNetworkSubnetDatabaseAPITestCase(BaseDatabaseAPITestCase): self.assertEqual(1, len(subnets)) def test_get_by_availability_zone_id(self): - az = db_api.availability_zone_create_if_not_exist(self.fake_context, - 'fake_zone_id') + with db_api.context_manager.writer.using(self.fake_context): + az = db_api._availability_zone_create_if_not_exist( + self.fake_context, 'fake_zone_id', + ) self.subnet_dict['availability_zone_id'] = az['id'] db_api.share_network_subnet_create(self.fake_context, self.subnet_dict) @@ -3112,8 +3114,10 @@ class ShareNetworkSubnetDatabaseAPITestCase(BaseDatabaseAPITestCase): self._check_fields(expected=self.subnet_dict, actual=result[0]) def test_get_az_subnets(self): - az = db_api.availability_zone_create_if_not_exist(self.fake_context, - 'fake_zone_id') + with db_api.context_manager.writer.using(self.fake_context): + az = db_api._availability_zone_create_if_not_exist( + self.fake_context, 'fake_zone_id', + ) self.subnet_dict['availability_zone_id'] = az['id'] db_api.share_network_subnet_create(self.fake_context, self.subnet_dict) @@ -3710,6 +3714,21 @@ class ServiceDatabaseAPITestCase(test.TestCase): self.assertEqual(az.id, service.availability_zone_id) self.assertSubDictMatch(self.service_data, service.to_dict()) + def test_create__az_exists(self): + + # there's no public AZ create method so we have to define one ourselves + @db_api.context_manager.writer + def availability_zone_create(context, name): + return db_api._availability_zone_create_if_not_exist( + context, name, + ) + + az = availability_zone_create(self.ctxt, 'fake_zone') + service = db_api.service_create(self.ctxt, self.service_data) + + self.assertEqual(az.id, service.availability_zone_id) + self.assertSubDictMatch(self.service_data, service.to_dict()) + def test_update(self): az_name = 'fake_zone2' update_data = {"availability_zone": az_name} @@ -3744,7 +3763,10 @@ class AvailabilityZonesDatabaseAPITestCase(test.TestCase): def test_az_get(self): az_name = 'test_az' - az = db_api.availability_zone_create_if_not_exist(self.ctxt, az_name) + with db_api.context_manager.writer.using(self.ctxt): + az = db_api._availability_zone_create_if_not_exist( + self.ctxt, az_name + ) az_by_id = db_api.availability_zone_get(self.ctxt, az['id']) az_by_name = db_api.availability_zone_get(self.ctxt, az_name) @@ -3755,9 +3777,11 @@ class AvailabilityZonesDatabaseAPITestCase(test.TestCase): self.assertEqual(az['id'], az_by_name['id']) def test_az_get_all(self): - db_api.availability_zone_create_if_not_exist(self.ctxt, 'test1') - db_api.availability_zone_create_if_not_exist(self.ctxt, 'test2') - db_api.availability_zone_create_if_not_exist(self.ctxt, 'test3') + with db_api.context_manager.writer.using(self.ctxt): + db_api._availability_zone_create_if_not_exist(self.ctxt, 'test1') + db_api._availability_zone_create_if_not_exist(self.ctxt, 'test2') + db_api._availability_zone_create_if_not_exist(self.ctxt, 'test3') + db_api.service_create(self.ctxt, {'availability_zone': 'test2'}) actual_result = db_api.availability_zone_get_all(self.ctxt)