From dfb304355b46882696ef26386637836577be8db7 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Tue, 14 Dec 2021 17:25:16 +0100 Subject: [PATCH] Introduce @serial test execution decorator Tempest provides a LockFixture to avoid two potentially interfering tests to run in parallel. However, this solution does not scale when we want to separate a set of tests from many other test cases. For example, host aggregate and availability zone testing needs compute hosts without any nova servers to be able to test moving computes between aggregates but a lot of other tests are creating nova servers. To fully separate these aggregate tests from the rest of the tempest test cases, this patch proposes a @serial class decorator to mark a test class to be run totally independently of any other test classes. Under the hood, the @serial decorator is implemented with a tempest-wide interprocess read-write lock. The serial test classes always take the write lock, while the non-serial classes take the read lock. The lock allows in many readers OR a single writer. So the serial tests are run independently from the rest. To minimize the time a serial test blocks other tempest tests run in parallel, this patch also introduced a serial_tests test directory to store the serial tests. The current test ordering in a fresh env uses alphabetical order so the serial tests will run at the end of the execution not randomly in the middle. The gate uses fresh VMs for every run so we can rely on this optimization there. In local envs where tests are re-run, the subsequent runs will be ordered at runtime by stestr. Therfore, a longer runtime might be observed due to locking, but the correctness of the test execution is still kept. Related-Bug: #821732 Change-Id: I0181517edab75f586464a38c4811417f888783b1 --- HACKING.rst | 9 +- doc/source/write_tests.rst | 27 +++++ requirements.txt | 1 + tempest/lib/decorators.py | 7 ++ tempest/serial_tests/__init__.py | 0 tempest/serial_tests/api/__init__.py | 0 tempest/serial_tests/api/admin/__init__.py | 0 .../api}/admin/test_aggregates.py | 1 + tempest/serial_tests/scenario/__init__.py | 0 .../scenario/test_aggregates_basic_ops.py | 1 + tempest/test.py | 105 +++++++++++++----- tempest/test_discover/test_discover.py | 2 +- tempest/tests/test_test.py | 60 ++++++++++ tox.ini | 16 +-- 14 files changed, 190 insertions(+), 39 deletions(-) create mode 100644 tempest/serial_tests/__init__.py create mode 100644 tempest/serial_tests/api/__init__.py create mode 100644 tempest/serial_tests/api/admin/__init__.py rename tempest/{api/compute => serial_tests/api}/admin/test_aggregates.py (99%) create mode 100644 tempest/serial_tests/scenario/__init__.py rename tempest/{ => serial_tests}/scenario/test_aggregates_basic_ops.py (99%) diff --git a/HACKING.rst b/HACKING.rst index dc28e4e008..17e2a4908f 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -322,7 +322,14 @@ parallel. - If the execution of a set of tests is required to be serialized then locking can be used to perform this. See usage of ``LockFixture`` for examples of - using locking. + using locking. However, LockFixture only helps if you want to separate the + execution of two small sets of test cases. On the other hand, if you need to + run a set of tests separately from potentially all other tests then + ``LockFixture`` does not scale as you would need to take the lock in all the + other tests too. In this case, you can use the ``@serial`` decorator on top + of the test class holding the tests that need to run separately from the + potentially parallel test set. See more in :ref:`tempest_test_writing`. + Sample Configuration File ------------------------- diff --git a/doc/source/write_tests.rst b/doc/source/write_tests.rst index 34df089570..3626a3f33c 100644 --- a/doc/source/write_tests.rst +++ b/doc/source/write_tests.rst @@ -256,6 +256,33 @@ inheriting from classes other than the base TestCase in tempest/test.py it is worth checking the immediate parent for what is set to determine if your class needs to override that setting. +Running some tests in serial +---------------------------- +Tempest potentially runs test cases in parallel, depending on the configuration. +However, sometimes you need to make sure that tests are not interfering with +each other via OpenStack resources. Tempest creates separate projects for each +test class to separate project based resources between test cases. + +If your tests use resources outside of projects, e.g. host aggregates then +you might need to explicitly separate interfering test cases. If you only need +to separate a small set of testcases from each other then you can use the +``LockFixture``. + +However, in some cases a small set of tests needs to be run independently from +the rest of the test cases. For example, some of the host aggregate and +availability zone testing needs compute nodes without any running nova server +to be able to move compute hosts between availability zones. But many tempest +tests start one or more nova servers. In this scenario you can mark the small +set of tests that needs to be independent from the rest with the ``@serial`` +class decorator. This will make sure that even if tempest is configured to run +the tests in parallel the tests in the marked test class will always be executed +separately from the rest of the test cases. + +Please note that due to test ordering optimization reasons test cases marked +for ``@serial`` execution need to be put under ``tempest/serial_tests`` +directory. This will ensure that the serial tests will block the parallel tests +in the least amount of time. + Interacting with Credentials and Clients ======================================== diff --git a/requirements.txt b/requirements.txt index a118856a35..6e66046156 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,3 +22,4 @@ PrettyTable>=0.7.1 # BSD urllib3>=1.21.1 # MIT debtcollector>=1.2.0 # Apache-2.0 defusedxml>=0.7.1 # PSFL +fasteners>=0.16.0 # Apache-2.0 diff --git a/tempest/lib/decorators.py b/tempest/lib/decorators.py index bb588be437..7d54c1a74a 100644 --- a/tempest/lib/decorators.py +++ b/tempest/lib/decorators.py @@ -221,3 +221,10 @@ class cleanup_order: # class is the caller owner.cleanup = owner.addClassResourceCleanup return MethodType(self.func, owner) + + +def serial(cls): + """A decorator to mark a test class for serial execution""" + cls._serial = True + LOG.debug('marked %s for serial execution', cls.__name__) + return cls diff --git a/tempest/serial_tests/__init__.py b/tempest/serial_tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tempest/serial_tests/api/__init__.py b/tempest/serial_tests/api/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tempest/serial_tests/api/admin/__init__.py b/tempest/serial_tests/api/admin/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tempest/api/compute/admin/test_aggregates.py b/tempest/serial_tests/api/admin/test_aggregates.py similarity index 99% rename from tempest/api/compute/admin/test_aggregates.py rename to tempest/serial_tests/api/admin/test_aggregates.py index a6c6535736..2ca91aacec 100644 --- a/tempest/api/compute/admin/test_aggregates.py +++ b/tempest/serial_tests/api/admin/test_aggregates.py @@ -26,6 +26,7 @@ from tempest.lib import decorators CONF = config.CONF +@decorators.serial class AggregatesAdminTestBase(base.BaseV2ComputeAdminTest): """Tests Aggregates API that require admin privileges""" diff --git a/tempest/serial_tests/scenario/__init__.py b/tempest/serial_tests/scenario/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tempest/scenario/test_aggregates_basic_ops.py b/tempest/serial_tests/scenario/test_aggregates_basic_ops.py similarity index 99% rename from tempest/scenario/test_aggregates_basic_ops.py rename to tempest/serial_tests/scenario/test_aggregates_basic_ops.py index 58e234f70f..ba31d846e5 100644 --- a/tempest/scenario/test_aggregates_basic_ops.py +++ b/tempest/serial_tests/scenario/test_aggregates_basic_ops.py @@ -20,6 +20,7 @@ from tempest.lib import decorators from tempest.scenario import manager +@decorators.serial class TestAggregatesBasicOps(manager.ScenarioTest): """Creates an aggregate within an availability zone diff --git a/tempest/test.py b/tempest/test.py index dba2695138..d49458e728 100644 --- a/tempest/test.py +++ b/tempest/test.py @@ -18,7 +18,9 @@ import os import sys import debtcollector.moves +from fasteners import process_lock import fixtures +from oslo_concurrency import lockutils from oslo_log import log as logging import testtools @@ -123,6 +125,23 @@ class BaseTestCase(testtools.testcase.WithAttributes, # A way to adjust slow test classes TIMEOUT_SCALING_FACTOR = 1 + # An interprocess lock to implement serial test execution if requested. + # The serial test classes are the writers as only one of them can be + # executed. The rest of the test classes are the readers as many of them + # can be run in parallel. + # Only classes can be decorated with @serial decorator not individual test + # cases as tempest allows test class level resource setup which could + # interfere with serialized execution on test cases level. I.e. the class + # setup of one of the test cases could run before taking a test case level + # lock. + # We cannot init the lock here as external lock needs oslo configuration + # to be loaded first to get the lock_path + serial_rw_lock = None + + # Defines if the tests in this class should be run without any parallelism + # Use the @serial decorator on your test class to indicate such requirement + _serial = False + @classmethod def _reset_class(cls): cls.__setup_credentials_called = False @@ -133,15 +152,34 @@ class BaseTestCase(testtools.testcase.WithAttributes, # Stack of (name, callable) to be invoked in reverse order at teardown cls._teardowns = [] + @classmethod + def is_serial_execution_requested(cls): + return cls._serial + @classmethod def setUpClass(cls): cls.__setupclass_called = True + + if cls.serial_rw_lock is None: + path = os.path.join( + lockutils.get_lock_path(CONF), 'tempest-serial-rw-lock') + cls.serial_rw_lock = ( + process_lock.InterProcessReaderWriterLock(path) + ) + # Reset state cls._reset_class() # It should never be overridden by descendants if hasattr(super(BaseTestCase, cls), 'setUpClass'): super(BaseTestCase, cls).setUpClass() try: + if cls.is_serial_execution_requested(): + LOG.debug('%s taking the write lock', cls.__name__) + cls.serial_rw_lock.acquire_write_lock() + LOG.debug('%s took the write lock', cls.__name__) + else: + cls.serial_rw_lock.acquire_read_lock() + cls.skip_checks() if not cls.__skip_checks_called: @@ -184,35 +222,44 @@ class BaseTestCase(testtools.testcase.WithAttributes, # If there was no exception during setup we shall re-raise the first # exception in teardown re_raise = (etype is None) - while cls._teardowns: - name, teardown = cls._teardowns.pop() - # Catch any exception in tearDown so we can re-raise the original - # exception at the end - try: - teardown() - if name == 'resources': - if not cls.__resource_cleanup_called: - raise RuntimeError( - "resource_cleanup for %s did not call the " - "super's resource_cleanup" % cls.__name__) - except Exception as te: - sys_exec_info = sys.exc_info() - tetype = sys_exec_info[0] - # TODO(andreaf): Resource cleanup is often implemented by - # storing an array of resources at class level, and cleaning - # them up during `resource_cleanup`. - # In case of failure during setup, some resource arrays might - # not be defined at all, in which case the cleanup code might - # trigger an AttributeError. In such cases we log - # AttributeError as info instead of exception. Once all - # cleanups are migrated to addClassResourceCleanup we can - # remove this. - if tetype is AttributeError and name == 'resources': - LOG.info("tearDownClass of %s failed: %s", name, te) - else: - LOG.exception("teardown of %s failed: %s", name, te) - if not etype: - etype, value, trace = sys_exec_info + try: + while cls._teardowns: + name, teardown = cls._teardowns.pop() + # Catch any exception in tearDown so we can re-raise the + # original exception at the end + try: + teardown() + if name == 'resources': + if not cls.__resource_cleanup_called: + raise RuntimeError( + "resource_cleanup for %s did not call the " + "super's resource_cleanup" % cls.__name__) + except Exception as te: + sys_exec_info = sys.exc_info() + tetype = sys_exec_info[0] + # TODO(andreaf): Resource cleanup is often implemented by + # storing an array of resources at class level, and + # cleaning them up during `resource_cleanup`. + # In case of failure during setup, some resource arrays + # might not be defined at all, in which case the cleanup + # code might trigger an AttributeError. In such cases we + # log AttributeError as info instead of exception. Once all + # cleanups are migrated to addClassResourceCleanup we can + # remove this. + if tetype is AttributeError and name == 'resources': + LOG.info("tearDownClass of %s failed: %s", name, te) + else: + LOG.exception("teardown of %s failed: %s", name, te) + if not etype: + etype, value, trace = sys_exec_info + finally: + if cls.is_serial_execution_requested(): + LOG.debug('%s releasing the write lock', cls.__name__) + cls.serial_rw_lock.release_write_lock() + LOG.debug('%s released the write lock', cls.__name__) + else: + cls.serial_rw_lock.release_read_lock() + # If exceptions were raised during teardown, and not before, re-raise # the first one if re_raise and etype is not None: diff --git a/tempest/test_discover/test_discover.py b/tempest/test_discover/test_discover.py index a19f20b696..679d58bc6d 100644 --- a/tempest/test_discover/test_discover.py +++ b/tempest/test_discover/test_discover.py @@ -25,7 +25,7 @@ def load_tests(loader, tests, pattern): base_path = os.path.split(os.path.dirname(os.path.abspath(__file__)))[0] base_path = os.path.split(base_path)[0] # Load local tempest tests - for test_dir in ['api', 'scenario']: + for test_dir in ['api', 'scenario', 'serial_tests']: full_test_dir = os.path.join(base_path, 'tempest', test_dir) if not pattern: suite.addTests(loader.discover(full_test_dir, diff --git a/tempest/tests/test_test.py b/tempest/tests/test_test.py index cbb81e297d..26e80796f7 100644 --- a/tempest/tests/test_test.py +++ b/tempest/tests/test_test.py @@ -17,12 +17,14 @@ import os import unittest from unittest import mock +from oslo_concurrency import lockutils from oslo_config import cfg import testtools from tempest import clients from tempest import config from tempest.lib.common import validation_resources as vr +from tempest.lib import decorators from tempest.lib import exceptions as lib_exc from tempest.lib.services.compute import base_compute_client from tempest.lib.services.placement import base_placement_client @@ -33,6 +35,8 @@ from tempest.tests import fake_config from tempest.tests.lib import fake_credentials from tempest.tests.lib.services import registry_fixture +CONF = config.CONF + class LoggingTestResult(testtools.TestResult): @@ -594,6 +598,52 @@ class TestTempestBaseTestClass(base.TestCase): str(log[0][2]['traceback']).replace('\n', ' '), RuntimeError.__name__ + ': .* ' + OverridesSetup.__name__) + @mock.patch.object(test.process_lock, 'InterProcessReaderWriterLock') + def test_serial_execution_if_requested(self, mock_lock): + + @decorators.serial + class SerialTests(self.parent_test): + pass + + class ParallelTests(self.parent_test): + pass + + @decorators.serial + class SerialTests2(self.parent_test): + pass + + suite = unittest.TestSuite( + (SerialTests(), ParallelTests(), SerialTests2())) + log = [] + result = LoggingTestResult(log) + suite.run(result) + + expected_lock_path = os.path.join( + lockutils.get_lock_path(CONF), 'tempest-serial-rw-lock') + + # We except that each test class has a lock with the _same_ external + # path so that if they would run by different processes they would + # still use the same lock + # Also we expect that each serial class takes and releases the + # write-lock while each non-serial class takes and releases the + # read-lock. + self.assertEqual( + [ + mock.call(expected_lock_path), + mock.call().acquire_write_lock(), + mock.call().release_write_lock(), + + mock.call(expected_lock_path), + mock.call().acquire_read_lock(), + mock.call().release_read_lock(), + + mock.call(expected_lock_path), + mock.call().acquire_write_lock(), + mock.call().release_write_lock(), + ], + mock_lock.mock_calls + ) + class TestTempestBaseTestClassFixtures(base.TestCase): @@ -750,6 +800,11 @@ class TestTempestBaseTestClassFixtures(base.TestCase): class TestAPIMicroversionTest1(test.BaseTestCase): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.useFixture(fake_config.ConfigFixture()) + config.TempestConfigPrivate = fake_config.FakePrivate + @classmethod def resource_setup(cls): super(TestAPIMicroversionTest1, cls).resource_setup() @@ -812,6 +867,11 @@ class TestAPIMicroversionTest1(test.BaseTestCase): class TestAPIMicroversionTest2(test.BaseTestCase): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.useFixture(fake_config.ConfigFixture()) + config.TempestConfigPrivate = fake_config.FakePrivate + @classmethod def resource_setup(cls): super(TestAPIMicroversionTest2, cls).resource_setup() diff --git a/tox.ini b/tox.ini index c78429366c..618f9e007a 100644 --- a/tox.ini +++ b/tox.ini @@ -124,7 +124,7 @@ deps = {[tempestenv]deps} commands = find . -type f -name "*.pyc" -delete tempest run --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.api)' {posargs} - tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)' {posargs} + tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)|(^tempest\.serial_tests)' {posargs} [testenv:full-parallel] envdir = .tox/tempest @@ -135,7 +135,7 @@ deps = {[tempestenv]deps} # The regex below is used to select all tempest scenario and including the non slow api tests commands = find . -type f -name "*.pyc" -delete - tempest run --regex '(^tempest\.scenario.*)|(?!.*\[.*\bslow\b.*\])(^tempest\.api)' {posargs} + tempest run --regex '(^tempest\.scenario.*)|(^tempest\.serial_tests)|(?!.*\[.*\bslow\b.*\])(^tempest\.api)' {posargs} [testenv:api-microversion-tests] envdir = .tox/tempest @@ -160,7 +160,7 @@ deps = {[tempestenv]deps} commands = find . -type f -name "*.pyc" -delete tempest run --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.api)' --exclude-list ./tools/tempest-integrated-gate-networking-exclude-list.txt {posargs} - tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)' --exclude-list ./tools/tempest-integrated-gate-networking-exclude-list.txt {posargs} + tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)|(^tempest\.serial_tests)' --exclude-list ./tools/tempest-integrated-gate-networking-exclude-list.txt {posargs} [testenv:integrated-compute] envdir = .tox/tempest @@ -173,7 +173,7 @@ deps = {[tempestenv]deps} commands = find . -type f -name "*.pyc" -delete tempest run --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.api)' --exclude-list ./tools/tempest-integrated-gate-compute-exclude-list.txt {posargs} - tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)' --exclude-list ./tools/tempest-integrated-gate-compute-exclude-list.txt {posargs} + tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)|(^tempest\.serial_tests)' --exclude-list ./tools/tempest-integrated-gate-compute-exclude-list.txt {posargs} [testenv:integrated-placement] envdir = .tox/tempest @@ -186,7 +186,7 @@ deps = {[tempestenv]deps} commands = find . -type f -name "*.pyc" -delete tempest run --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.api)' --exclude-list ./tools/tempest-integrated-gate-placement-exclude-list.txt {posargs} - tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)' --exclude-list ./tools/tempest-integrated-gate-placement-exclude-list.txt {posargs} + tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)|(^tempest\.serial_tests)' --exclude-list ./tools/tempest-integrated-gate-placement-exclude-list.txt {posargs} [testenv:integrated-storage] envdir = .tox/tempest @@ -199,7 +199,7 @@ deps = {[tempestenv]deps} commands = find . -type f -name "*.pyc" -delete tempest run --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.api)' --exclude-list ./tools/tempest-integrated-gate-storage-exclude-list.txt {posargs} - tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)' --exclude-list ./tools/tempest-integrated-gate-storage-exclude-list.txt {posargs} + tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)|(^tempest\.serial_tests)' --exclude-list ./tools/tempest-integrated-gate-storage-exclude-list.txt {posargs} [testenv:integrated-object-storage] envdir = .tox/tempest @@ -212,7 +212,7 @@ deps = {[tempestenv]deps} commands = find . -type f -name "*.pyc" -delete tempest run --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.api)' --exclude-list ./tools/tempest-integrated-gate-object-storage-exclude-list.txt {posargs} - tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)' --exclude-list ./tools/tempest-integrated-gate-object-storage-exclude-list.txt {posargs} + tempest run --combine --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.scenario)|(^tempest\.serial_tests)' --exclude-list ./tools/tempest-integrated-gate-object-storage-exclude-list.txt {posargs} [testenv:full-serial] envdir = .tox/tempest @@ -225,7 +225,7 @@ deps = {[tempestenv]deps} # FIXME: We can replace it with the `--exclude-regex` option to exclude tests now. commands = find . -type f -name "*.pyc" -delete - tempest run --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.(api|scenario))' {posargs} + tempest run --serial --regex '(?!.*\[.*\bslow\b.*\])(^tempest\.(api|scenario|serial_tests))' {posargs} [testenv:scenario] envdir = .tox/tempest