From 951b806347adce32fa7f99b441b9e973af1e789d Mon Sep 17 00:00:00 2001 From: Benny Kopilov Date: Wed, 4 May 2022 08:50:03 +0300 Subject: [PATCH] Decorate volume.base functions - fix cleanup There are functions set as classmethod while used in testcase The cleanup done on class level and not part of the testcase The problem is that we may see testcase pass while the resource_cleanup fails on classlevel instead of the test fail. Maintaine and support proper cleanup ordering when functions are classbase while other instances. Current fix contains class descriptor that learn caller in case its a class it will use the class cleanup in case its an instance it will use instance cleanup. Expecting to see failures due to wrong cleanup in tests result from the wrong impelementaions. Added unittests for cleanup_order decorator Updated test_groups cleanup , volume can not be deleted before group, class level cleanup hides it because group clean was instance level. See scenario methods as reference (manager) Change-Id: I27328bf6c176840c7762bd97f596481ffa6f5736 --- .../api/volume/admin/test_group_snapshots.py | 47 ++++++-- tempest/api/volume/admin/test_groups.py | 23 +++- tempest/api/volume/base.py | 74 ++++++------ tempest/lib/decorators.py | 32 ++++++ tempest/tests/lib/test_decorators.py | 107 ++++++++++++++++++ 5 files changed, 230 insertions(+), 53 deletions(-) diff --git a/tempest/api/volume/admin/test_group_snapshots.py b/tempest/api/volume/admin/test_group_snapshots.py index 73903cf52a..8af8435530 100644 --- a/tempest/api/volume/admin/test_group_snapshots.py +++ b/tempest/api/volume/admin/test_group_snapshots.py @@ -91,9 +91,15 @@ class GroupSnapshotsTest(BaseGroupSnapshotsTest): grp = self.create_group(group_type=group_type['id'], volume_types=[volume_type['id']]) - # Create volume - vol = self.create_volume(volume_type=volume_type['id'], - group_id=grp['id']) + # Create volume is instance level, can not be deleted before group. + # Volume delete handled by delete_group method, cleanup method. + params = {'name': data_utils.rand_name("volume"), + 'volume_type': volume_type['id'], + 'group_id': grp['id'], + 'size': CONF.volume.volume_size} + vol = self.volumes_client.create_volume(**params)['volume'] + waiters.wait_for_volume_resource_status( + self.volumes_client, vol['id'], 'available') # Create group snapshot group_snapshot_name = data_utils.rand_name('group_snapshot') @@ -153,9 +159,15 @@ class GroupSnapshotsTest(BaseGroupSnapshotsTest): grp = self.create_group(group_type=group_type['id'], volume_types=[volume_type['id']]) - # Create volume - vol = self.create_volume(volume_type=volume_type['id'], - group_id=grp['id']) + # Create volume is instance level, can not be deleted before group. + # Volume delete handled by delete_group method, cleanup method. + params = {'name': data_utils.rand_name("volume"), + 'volume_type': volume_type['id'], + 'group_id': grp['id'], + 'size': CONF.volume.volume_size} + vol = self.volumes_client.create_volume(**params)['volume'] + waiters.wait_for_volume_resource_status( + self.volumes_client, vol['id'], 'available') # Create group_snapshot group_snapshot_name = data_utils.rand_name('group_snapshot') @@ -215,8 +227,15 @@ class GroupSnapshotsTest(BaseGroupSnapshotsTest): # volume-type and group id. volume_list = [] for _ in range(2): - volume = self.create_volume(volume_type=volume_type['id'], - group_id=grp['id']) + # Create volume is instance level, can't be deleted before group. + # Volume delete handled by delete_group method, cleanup method. + params = {'name': data_utils.rand_name("volume"), + 'volume_type': volume_type['id'], + 'group_id': grp['id'], + 'size': CONF.volume.volume_size} + volume = self.volumes_client.create_volume(**params)['volume'] + waiters.wait_for_volume_resource_status( + self.volumes_client, volume['id'], 'available') volume_list.append(volume['id']) for vol in volume_list: @@ -268,9 +287,15 @@ class GroupSnapshotsV319Test(BaseGroupSnapshotsTest): group = self.create_group(group_type=group_type['id'], volume_types=[volume_type['id']]) - # Create volume - volume = self.create_volume(volume_type=volume_type['id'], - group_id=group['id']) + # Create volume is instance level, can not be deleted before group. + # Volume delete handled by delete_group method, cleanup method. + params = {'name': data_utils.rand_name("volume"), + 'volume_type': volume_type['id'], + 'group_id': group['id'], + 'size': CONF.volume.volume_size} + volume = self.volumes_client.create_volume(**params)['volume'] + waiters.wait_for_volume_resource_status( + self.volumes_client, volume['id'], 'available') # Create group snapshot group_snapshot = self._create_group_snapshot(group_id=group['id']) diff --git a/tempest/api/volume/admin/test_groups.py b/tempest/api/volume/admin/test_groups.py index f16e4d2d27..094f1424c5 100644 --- a/tempest/api/volume/admin/test_groups.py +++ b/tempest/api/volume/admin/test_groups.py @@ -108,11 +108,17 @@ class GroupsTest(base.BaseVolumeAdminTest): grp = self.create_group(group_type=group_type['id'], volume_types=[volume_type['id']]) - # Create volumes + # Create volume is instance level, can not be deleted before group. + # Volume delete handled by delete_group method, cleanup method. grp_vols = [] for _ in range(2): - vol = self.create_volume(volume_type=volume_type['id'], - group_id=grp['id']) + params = {'name': data_utils.rand_name("volume"), + 'volume_type': volume_type['id'], + 'group_id': grp['id'], + 'size': CONF.volume.volume_size} + vol = self.volumes_client.create_volume(**params)['volume'] + waiters.wait_for_volume_resource_status( + self.volumes_client, vol['id'], 'available') grp_vols.append(vol) vol2 = grp_vols[1] @@ -171,8 +177,15 @@ class GroupsV314Test(base.BaseVolumeAdminTest): grp = self.create_group(group_type=group_type['id'], volume_types=[volume_type['id']]) - # Create volume - self.create_volume(volume_type=volume_type['id'], group_id=grp['id']) + # Create volume is instance level, can not be deleted before group. + # Volume delete handled by delete_group method, cleanup method. + params = {'name': data_utils.rand_name("volume"), + 'volume_type': volume_type['id'], + 'group_id': grp['id'], + 'size': CONF.volume.volume_size} + vol = self.volumes_client.create_volume(**params)['volume'] + waiters.wait_for_volume_resource_status( + self.volumes_client, vol['id'], 'available') # Create Group from Group grp_name2 = data_utils.rand_name('Group_from_grp') diff --git a/tempest/api/volume/base.py b/tempest/api/volume/base.py index 172b6ed727..49f9e2217e 100644 --- a/tempest/api/volume/base.py +++ b/tempest/api/volume/base.py @@ -19,6 +19,7 @@ from tempest import config from tempest.lib.common import api_version_utils from tempest.lib.common.utils import data_utils from tempest.lib.common.utils import test_utils +from tempest.lib.decorators import cleanup_order import tempest.test CONF = config.CONF @@ -94,8 +95,8 @@ class BaseVolumeTest(api_version_utils.BaseMicroversionTest, cls.build_interval = CONF.volume.build_interval cls.build_timeout = CONF.volume.build_timeout - @classmethod - def create_volume(cls, wait_until='available', **kwargs): + @cleanup_order + def create_volume(self, wait_until='available', **kwargs): """Wrapper utility that returns a test volume. :param wait_until: wait till volume status, None means no wait. @@ -104,12 +105,12 @@ class BaseVolumeTest(api_version_utils.BaseMicroversionTest, kwargs['size'] = CONF.volume.volume_size if 'imageRef' in kwargs: - image = cls.images_client.show_image(kwargs['imageRef']) + image = self.images_client.show_image(kwargs['imageRef']) min_disk = image['min_disk'] kwargs['size'] = max(kwargs['size'], min_disk) if 'name' not in kwargs: - name = data_utils.rand_name(cls.__name__ + '-Volume') + name = data_utils.rand_name(self.__name__ + '-Volume') kwargs['name'] = name if CONF.volume.volume_type and 'volume_type' not in kwargs: @@ -123,27 +124,26 @@ class BaseVolumeTest(api_version_utils.BaseMicroversionTest, kwargs.setdefault('availability_zone', CONF.compute.compute_volume_common_az) - volume = cls.volumes_client.create_volume(**kwargs)['volume'] - cls.addClassResourceCleanup(test_utils.call_and_ignore_notfound_exc, - cls.delete_volume, cls.volumes_client, - volume['id']) + volume = self.volumes_client.create_volume(**kwargs)['volume'] + self.cleanup(test_utils.call_and_ignore_notfound_exc, + self.delete_volume, self.volumes_client, volume['id']) if wait_until: - waiters.wait_for_volume_resource_status(cls.volumes_client, + waiters.wait_for_volume_resource_status(self.volumes_client, volume['id'], wait_until) return volume - @classmethod - def create_snapshot(cls, volume_id=1, **kwargs): + @cleanup_order + def create_snapshot(self, volume_id=1, **kwargs): """Wrapper utility that returns a test snapshot.""" if 'name' not in kwargs: - name = data_utils.rand_name(cls.__name__ + '-Snapshot') + name = data_utils.rand_name(self.__name__ + '-Snapshot') kwargs['name'] = name - snapshot = cls.snapshots_client.create_snapshot( + snapshot = self.snapshots_client.create_snapshot( volume_id=volume_id, **kwargs)['snapshot'] - cls.addClassResourceCleanup(test_utils.call_and_ignore_notfound_exc, - cls.delete_snapshot, snapshot['id']) - waiters.wait_for_volume_resource_status(cls.snapshots_client, + self.cleanup(test_utils.call_and_ignore_notfound_exc, + self.delete_snapshot, snapshot['id']) + waiters.wait_for_volume_resource_status(self.snapshots_client, snapshot['id'], 'available') return snapshot @@ -175,11 +175,11 @@ class BaseVolumeTest(api_version_utils.BaseMicroversionTest, client.delete_volume(volume_id) client.wait_for_resource_deletion(volume_id) - @classmethod - def delete_snapshot(cls, snapshot_id, snapshots_client=None): + @cleanup_order + def delete_snapshot(self, snapshot_id, snapshots_client=None): """Delete snapshot by the given client""" if snapshots_client is None: - snapshots_client = cls.snapshots_client + snapshots_client = self.snapshots_client snapshots_client.delete_snapshot(snapshot_id) snapshots_client.wait_for_resource_deletion(snapshot_id) @@ -278,23 +278,23 @@ class BaseVolumeAdminTest(BaseVolumeTest): cls.admin_scheduler_stats_client = \ cls.os_admin.volume_scheduler_stats_client_latest - @classmethod - def create_test_qos_specs(cls, name=None, consumer=None, **kwargs): + @cleanup_order + def create_test_qos_specs(self, name=None, consumer=None, **kwargs): """create a test Qos-Specs.""" - name = name or data_utils.rand_name(cls.__name__ + '-QoS') + name = name or data_utils.rand_name(self.__name__ + '-QoS') consumer = consumer or 'front-end' - qos_specs = cls.admin_volume_qos_client.create_qos( + qos_specs = self.admin_volume_qos_client.create_qos( name=name, consumer=consumer, **kwargs)['qos_specs'] - cls.addClassResourceCleanup(cls.clear_qos_spec, qos_specs['id']) + self.cleanup(self.clear_qos_spec, qos_specs['id']) return qos_specs - @classmethod - def create_volume_type(cls, name=None, **kwargs): + @cleanup_order + def create_volume_type(self, name=None, **kwargs): """Create a test volume-type""" - name = name or data_utils.rand_name(cls.__name__ + '-volume-type') - volume_type = cls.admin_volume_types_client.create_volume_type( + name = name or data_utils.rand_name(self.__name__ + '-volume-type') + volume_type = self.admin_volume_types_client.create_volume_type( name=name, **kwargs)['volume_type'] - cls.addClassResourceCleanup(cls.clear_volume_type, volume_type['id']) + self.cleanup(self.clear_volume_type, volume_type['id']) return volume_type def create_encryption_type(self, type_id=None, provider=None, @@ -328,19 +328,19 @@ class BaseVolumeAdminTest(BaseVolumeTest): group_type['id']) return group_type - @classmethod - def clear_qos_spec(cls, qos_id): + @cleanup_order + def clear_qos_spec(self, qos_id): test_utils.call_and_ignore_notfound_exc( - cls.admin_volume_qos_client.delete_qos, qos_id) + self.admin_volume_qos_client.delete_qos, qos_id) test_utils.call_and_ignore_notfound_exc( - cls.admin_volume_qos_client.wait_for_resource_deletion, qos_id) + self.admin_volume_qos_client.wait_for_resource_deletion, qos_id) - @classmethod - def clear_volume_type(cls, vol_type_id): + @cleanup_order + def clear_volume_type(self, vol_type_id): test_utils.call_and_ignore_notfound_exc( - cls.admin_volume_types_client.delete_volume_type, vol_type_id) + self.admin_volume_types_client.delete_volume_type, vol_type_id) test_utils.call_and_ignore_notfound_exc( - cls.admin_volume_types_client.wait_for_resource_deletion, + self.admin_volume_types_client.wait_for_resource_deletion, vol_type_id) diff --git a/tempest/lib/decorators.py b/tempest/lib/decorators.py index a4633cac3e..bb588be437 100644 --- a/tempest/lib/decorators.py +++ b/tempest/lib/decorators.py @@ -13,6 +13,7 @@ # under the License. import functools +from types import MethodType import uuid from oslo_log import log as logging @@ -189,3 +190,34 @@ def unstable_test(*args, **kwargs): raise e return inner return decor + + +class cleanup_order: + """Descriptor for base create function to cleanup based on caller. + + There are functions created as classmethod and the cleanup + was managed by the class with addClassResourceCleanup, + In case the function called from a class level (resource_setup) its ok + But when it is called from testcase level there is no reson to delete the + resource when class tears down. + + The testcase results will not reflect the resources cleanup because test + may pass but the class cleanup fails. if the resources were created by + testcase its better to let the testcase delete them and report failure + part of the testcase + """ + + def __init__(self, func): + self.func = func + functools.update_wrapper(self, func) + + def __get__(self, instance, owner): + if instance: + # instance is the caller + instance.cleanup = instance.addCleanup + instance.__name__ = owner.__name__ + return MethodType(self.func, instance) + elif owner: + # class is the caller + owner.cleanup = owner.addClassResourceCleanup + return MethodType(self.func, owner) diff --git a/tempest/tests/lib/test_decorators.py b/tempest/tests/lib/test_decorators.py index fc93f76a84..f9a12b6be7 100644 --- a/tempest/tests/lib/test_decorators.py +++ b/tempest/tests/lib/test_decorators.py @@ -21,6 +21,7 @@ import testtools from tempest.lib import base as test from tempest.lib.common.utils import data_utils from tempest.lib import decorators +from tempest.lib import exceptions from tempest.lib import exceptions as lib_exc from tempest.tests import base @@ -289,3 +290,109 @@ class TestRelatedBugDecorator(base.TestCase): with mock.patch.object(decorators.LOG, 'error'): self.assertRaises(lib_exc.InvalidParam, test_foo, object()) + + +class TestCleanupOrderDecorator(base.TestCase): + + @decorators.cleanup_order + def _create_volume(self, raise_exception=False): + """Test doc""" + vol_id = "487ef6b6-546a-40c7-bc3f-b405d6239fc8" + self.cleanup(self._delete_dummy, vol_id) + if raise_exception: + raise exceptions.NotFound("Not found") + return "volume" + + def _delete_dummy(self, vol_id): + pass + + class DummyClassResourceCleanup(list): + """dummy list class simulate ClassResourceCleanup""" + + def __call__(self, func, vol_id): + self.append((func, vol_id)) + + @classmethod + def resource_setup(cls): + cls.addClassResourceCleanup = cls.DummyClassResourceCleanup() + cls.volume = cls._create_volume() + + @classmethod + def resource_setup_exception(cls): + cls.addClassResourceCleanup = cls.DummyClassResourceCleanup() + cls.volume = cls._create_volume(raise_exception=True) + + def setUp(self): + super().setUp() + self.volume_instance = self._create_volume() + + def test_cleanup_order_when_called_from_instance_testcase(self): + # create a volume + my_vol = self._create_volume() + # Verify method runs and return value + self.assertEqual(my_vol, "volume") + # Verify __doc__ exists from original function + self.assertEqual(self._create_volume.__doc__, "Test doc") + # New cleanup created and refers to addCleanup + self.assertTrue(hasattr(self, "cleanup")) + self.assertEqual(self.cleanup, self.addCleanup) + # New __name__ created from type(self) + self.assertEqual(self.__name__, type(self).__name__) + # Verify function added to instance _cleanups + self.assertIn(self._delete_dummy, [e[0] for e in self._cleanups]) + + def test_cleanup_order_when_called_from_setup_instance(self): + # create a volume + my_vol = self.volume_instance + # Verify method runs and return value + self.assertEqual(my_vol, "volume") + # Verify __doc__ exists from original function + self.assertEqual(self._create_volume.__doc__, "Test doc") + # New cleanup created and refers to addCleanup + self.assertTrue(hasattr(self, "cleanup")) + self.assertEqual(self.cleanup, self.addCleanup) + # New __name__ created from type(self) + self.assertEqual(self.__name__, type(self).__name__) + # Verify function added to instance _cleanups + self.assertIn(self._delete_dummy, [e[0] for e in self._cleanups]) + + def test_cleanup_order_when_called_from_instance_raise(self): + # create a volume when raised exceptions + self.assertRaises(exceptions.NotFound, self._create_volume, + raise_exception=True) + # before raise exceptions + self.assertTrue(hasattr(self, "cleanup")) + self.assertEqual(self.cleanup, self.addCleanup) + # New __name__ created from type(self) + self.assertEqual(self.__name__, type(self).__name__) + # Verify function added to instance _cleanups before exception + self.assertIn(self._delete_dummy, [e[0] for e in self._cleanups]) + + def test_cleanup_order_when_called_from_class_method(self): + # call class method + type(self).resource_setup() + # create a volume + my_vol = self.volume + # Verify method runs and return value + self.assertEqual(my_vol, "volume") + # Verify __doc__ exists from original function + self.assertEqual(self._create_volume.__doc__, "Test doc") + # New cleanup created and refers to addClassResourceCleanup + self.assertTrue(hasattr(self, "cleanup")) + self.assertEqual(type(self).cleanup, self.addClassResourceCleanup) + # Verify function added to instance addClassResourceCleanup + self.assertIn(type(self)._delete_dummy, + [e[0] for e in self.addClassResourceCleanup]) + + def test_cleanup_order_when_called_from_class_method_raise(self): + # call class method + self.assertRaises(exceptions.NotFound, + type(self).resource_setup_exception) + # Verify __doc__ exists from original function + self.assertEqual(self._create_volume.__doc__, "Test doc") + # New cleanup created and refers to addClassResourceCleanup + self.assertTrue(hasattr(self, "cleanup")) + self.assertEqual(type(self).cleanup, self.addClassResourceCleanup) + # Verify function added to instance addClassResourceCleanup + self.assertIn(type(self)._delete_dummy, + [e[0] for e in self.addClassResourceCleanup])