From f7a9111f094dd2814879f7fc585825fcdd0f552d Mon Sep 17 00:00:00 2001 From: whoami-rajat Date: Fri, 29 Nov 2019 09:43:57 +0000 Subject: [PATCH] Make volume soft delete more thorough When a volume record is soft-deleted in the database, dependent records in other tables (for example, Transfers, VolumeGlanceMetadata, etc.) must be soft deleted as well. Otherwise, we will get FK dependency errors when the database is purged. This patch adds that support for VolumeAttachment table. (other tables were already covered, just refactored) Also adds tests. Conflicts: test-requirements.txt The global constraint version of SQLAlchemy-Utils is 0.30.11 which doesn't match with the version specified in the patch i.e. 0.36.1 lower-constraints.txt Note: conflict is because lower-constraints.txt file doesn't exist in queens. Co-authored-by: Rajat Dhasmana Co-authored-by: Brian Rosmaita Change-Id: Ibfa6c4ba2f162681756ec3203991351345b65346 Related-Bug: #1542169 (cherry picked from commit a5bb17bdfc8af4df08ccce759bedcfd16bc7fe15) (cherry picked from commit dd2ba4900473869ef393b40d6a4b2ec6f7b035ac) --- cinder/db/sqlalchemy/api.py | 38 ++++++++------- .../tests/unit/db/test_orm_relationships.py | 46 +++++++++++++++++++ cinder/tests/unit/test_db_api.py | 7 +++ cinder/volume/manager.py | 6 --- test-requirements.txt | 1 + 5 files changed, 72 insertions(+), 26 deletions(-) create mode 100644 cinder/tests/unit/db/test_orm_relationships.py 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