diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index a003a890812..69fe9f7b174 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -1710,35 +1710,33 @@ def volume_data_get_for_project(context, project_id, volume_type_id, host=host) +VOLUME_DEPENDENT_MODELS = frozenset([models.VolumeMetadata, + models.VolumeAdminMetadata, + models.Transfer, + models.VolumeGlanceMetadata, + models.VolumeAttachment]) + + @require_admin_context @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True) def volume_destroy(context, volume_id): session = get_session() now = timeutils.utcnow() + updated_values = {'status': 'deleted', + 'deleted': True, + 'deleted_at': now, + 'updated_at': literal_column('updated_at'), + 'migration_status': None} with session.begin(): - updated_values = {'status': 'deleted', - 'deleted': True, - 'deleted_at': now, - 'updated_at': literal_column('updated_at'), - 'migration_status': None} model_query(context, models.Volume, session=session).\ filter_by(id=volume_id).\ update(updated_values) - model_query(context, models.VolumeMetadata, session=session).\ - filter_by(volume_id=volume_id).\ - update({'deleted': True, - 'deleted_at': now, - 'updated_at': literal_column('updated_at')}) - model_query(context, models.VolumeAdminMetadata, session=session).\ - filter_by(volume_id=volume_id).\ - update({'deleted': True, - 'deleted_at': now, - 'updated_at': literal_column('updated_at')}) - model_query(context, models.Transfer, session=session).\ - filter_by(volume_id=volume_id).\ - update({'deleted': True, - 'deleted_at': now, - 'updated_at': literal_column('updated_at')}) + for model in VOLUME_DEPENDENT_MODELS: + model_query(context, model, session=session).\ + filter_by(volume_id=volume_id).\ + update({'deleted': True, + 'deleted_at': now, + 'updated_at': literal_column('updated_at')}) del updated_values['updated_at'] return updated_values diff --git a/cinder/tests/unit/db/test_orm_relationships.py b/cinder/tests/unit/db/test_orm_relationships.py new file mode 100644 index 00000000000..fa76479ce4b --- /dev/null +++ b/cinder/tests/unit/db/test_orm_relationships.py @@ -0,0 +1,46 @@ +# Copyright 2020 Red Hat, Inc. +# +# 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. + +"""Tests for code that makes assumptions about ORM relationships.""" + +from sqlalchemy_utils import functions as saf + +from cinder.db.sqlalchemy import api as db_api +from cinder.db.sqlalchemy import models +from cinder import test + + +class VolumeRelationshipsTestCase(test.TestCase): + """Test cases for Volume ORM model relationshps.""" + + def test_volume_dependent_models_list(self): + """Make sure the volume dependent tables list is accurate.""" + # Addresses LP Bug #1542169 + + volume_declarative_base = saf.get_declarative_base(models.Volume) + volume_fks = saf.get_referencing_foreign_keys(models.Volume) + + dependent_tables = [] + for table, fks in saf.group_foreign_keys(volume_fks): + dependent_tables.append(table) + + found_dependent_models = [] + for table in dependent_tables: + found_dependent_models.append(saf.get_class_by_table( + volume_declarative_base, table)) + + self.assertEqual(len(found_dependent_models), + len(db_api.VOLUME_DEPENDENT_MODELS)) + for model in found_dependent_models: + self.assertIn(model, db_api.VOLUME_DEPENDENT_MODELS) diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index 9dea45baf43..450e638ff53 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -764,6 +764,13 @@ class DBAPIVolumeTestCase(BaseTest): self.assertRaises(exception.VolumeNotFound, db.volume_get, self.ctxt, volume['id']) + @mock.patch('cinder.db.sqlalchemy.api.model_query') + def test_volume_destroy_deletes_dependent_data(self, mock_model_query): + """Addresses LP Bug #1542169.""" + db.volume_destroy(self.ctxt, fake.VOLUME_ID) + expected_call_count = 1 + len(sqlalchemy_api.VOLUME_DEPENDENT_MODELS) + self.assertEqual(expected_call_count, mock_model_query.call_count) + def test_volume_get_all(self): volumes = [db.volume_create(self.ctxt, {'host': 'h%d' % i, 'size': i}) diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 608061b4bb4..583e3ecee10 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -877,9 +877,6 @@ class VolumeManager(manager.CleanableManager, LOG.exception("Failed to update usages deleting volume.", resource=volume) - # Delete glance metadata if it exists - self.db.volume_glance_metadata_delete_by_volume(context, volume.id) - volume.destroy() # If deleting source/destination volume in a migration or a temp @@ -3448,9 +3445,6 @@ class VolumeManager(manager.CleanableManager, resource={'type': 'group', 'id': group.id}) - # Delete glance metadata if it exists - self.db.volume_glance_metadata_delete_by_volume(context, vol.id) - vol.destroy() # Commit the reservations diff --git a/test-requirements.txt b/test-requirements.txt index 7993564b0c9..811f7dded89 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -13,6 +13,7 @@ os-api-ref>=1.4.0 # Apache-2.0 oslotest>=3.2.0 # Apache-2.0 PyMySQL>=0.7.6 # MIT License psycopg2>=2.6.2 # LGPL/ZPL +SQLAlchemy-Utils>=0.30.11 # BSD License testtools>=2.2.0 # MIT testresources>=2.0.0 # Apache-2.0/BSD testscenarios>=0.4 # Apache-2.0/BSD