diff --git a/requirements.txt b/requirements.txt index 1a3b33cc7..992af69e8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -50,7 +50,10 @@ python-novaclient python-octaviaclient python-swiftclient python-watcherclient -tenacity +# Due to https://github.com/jd/tenacity/pull/479 the strategy for mocking out tenacity +# waits/times/etc no longer works. Pin to 8.4.1 until it is solved. +# Bug in tenacity tracking issue: https://github.com/jd/tenacity/issues/482 +tenacity<8.4.2 paramiko # Documentation requirements diff --git a/unit_tests/utilities/test_utilities.py b/unit_tests/utilities/test_utilities.py index 6af6089b3..7eb99276d 100644 --- a/unit_tests/utilities/test_utilities.py +++ b/unit_tests/utilities/test_utilities.py @@ -113,6 +113,28 @@ def func(self): mock_sleep.assert_not_called() + @mock.patch("time.sleep") + def test_object_wrap_multilevel_with_exception(self, mock_sleep): + + class A: + + def func(self): + raise SomeException() + + class B: + + def __init__(self): + self.a = A() + + b = B() + # retry on a specific exception + wrapped_b = utilities.ObjectRetrierWraps( + b, num_retries=1, retry_exceptions=[SomeException]) + with self.assertRaises(SomeException): + wrapped_b.a.func() + + mock_sleep.assert_called_once_with(5) + @mock.patch("time.sleep") def test_log_called(self, mock_sleep): @@ -128,9 +150,7 @@ def func(self): with self.assertRaises(SomeException): wrapped_a.func() - # there should be two calls; one for the single retry and one for the - # failure. - self.assertEqual(mock_log.call_count, 2) + mock_log.assert_called() @mock.patch("time.sleep") def test_back_off_maximum(self, mock_sleep): diff --git a/unit_tests/utilities/test_zaza_utilities_openstack.py b/unit_tests/utilities/test_zaza_utilities_openstack.py index 222aab2f8..e00f713b1 100644 --- a/unit_tests/utilities/test_zaza_utilities_openstack.py +++ b/unit_tests/utilities/test_zaza_utilities_openstack.py @@ -99,6 +99,9 @@ def setUp(self): self.neutronclient.list_agents.return_value = self.agents self.neutronclient.list_bgp_speaker_on_dragent.return_value = \ self.bgp_speakers + self.patch("zaza.openstack.utilities.ObjectRetrierWraps", + name="_object_retrier_wraps", + new=lambda x, *_, **__: x) def test_create_port(self): self.patch_object(openstack_utils, "get_net_uuid") diff --git a/zaza/openstack/charm_tests/cinder_backup/tests.py b/zaza/openstack/charm_tests/cinder_backup/tests.py index 97b365808..258c7bb77 100644 --- a/zaza/openstack/charm_tests/cinder_backup/tests.py +++ b/zaza/openstack/charm_tests/cinder_backup/tests.py @@ -22,6 +22,7 @@ import zaza.model import zaza.openstack.charm_tests.test_utils as test_utils +from zaza.openstack.utilities import retry_on_connect_failure import zaza.openstack.utilities.ceph as ceph_utils import zaza.openstack.utilities.openstack as openstack_utils @@ -35,8 +36,9 @@ class CinderBackupTest(test_utils.OpenStackBaseTest): def setUpClass(cls): """Run class setup for running Cinder Backup tests.""" super(CinderBackupTest, cls).setUpClass() - cls.cinder_client = openstack_utils.get_cinder_session_client( - cls.keystone_session) + cls.cinder_client = retry_on_connect_failure( + openstack_utils.get_cinder_session_client(cls.keystone_session), + log=logging.warn) @property def services(self): @@ -101,7 +103,7 @@ def test_410_cinder_vol_create_backup_delete_restore_pool_inspect(self): self.cinder_client.volumes, cinder_vol.id, wait_iteration_max_time=180, - stop_after_attempt=15, + stop_after_attempt=30, expected_status='available', msg='ceph-backed cinder volume') @@ -123,7 +125,7 @@ def test_410_cinder_vol_create_backup_delete_restore_pool_inspect(self): self.cinder_client.backups, vol_backup.id, wait_iteration_max_time=180, - stop_after_attempt=15, + stop_after_attempt=30, expected_status='available', msg='Backup volume') diff --git a/zaza/openstack/charm_tests/manila/tests.py b/zaza/openstack/charm_tests/manila/tests.py index ea4073891..3a3f54c00 100644 --- a/zaza/openstack/charm_tests/manila/tests.py +++ b/zaza/openstack/charm_tests/manila/tests.py @@ -359,7 +359,7 @@ def test_manila_share(self): self.manila_client.shares, share.id, wait_iteration_max_time=120, - stop_after_attempt=2, + stop_after_attempt=10, expected_status="available", msg="Waiting for a share to become available") diff --git a/zaza/openstack/utilities/__init__.py b/zaza/openstack/utilities/__init__.py index 5798f2650..02826ff7f 100644 --- a/zaza/openstack/utilities/__init__.py +++ b/zaza/openstack/utilities/__init__.py @@ -15,10 +15,30 @@ """Collection of utilities to support zaza tests etc.""" +import logging import time from keystoneauth1.exceptions.connection import ConnectFailure +NEVER_RETRY_EXCEPTIONS = ( + AssertionError, + AttributeError, + ImportError, + IndexError, + KeyError, + NotImplementedError, + OverflowError, + RecursionError, + ReferenceError, + RuntimeError, + SyntaxError, + IndentationError, + SystemExit, + TypeError, + UnicodeError, + ZeroDivisionError, +) + class ObjectRetrierWraps(object): """An automatic retrier for an object. @@ -76,10 +96,19 @@ def __init__(self, obj, num_retries=3, initial_interval=5.0, backoff=1.0, If a list, then it will only retry if the exception is one of the ones in the list. :type retry_exceptions: List[Exception] + :param log: If False, disable logging; if None (the default) or True, + use logging.warn; otherwise use the passed param `log`. + :type param: None | Boolean | Callable """ # Note we use semi-private variable names that shouldn't clash with any # on the actual object. self.__obj = obj + if log in (None, True): + _log = logging.warning + elif log is False: + _log = lambda *_, **__: None # noqa + else: + _log = log self.__kwargs = { 'num_retries': num_retries, 'initial_interval': initial_interval, @@ -87,24 +116,19 @@ def __init__(self, obj, num_retries=3, initial_interval=5.0, backoff=1.0, 'max_interval': max_interval, 'total_wait': total_wait, 'retry_exceptions': retry_exceptions, - 'log': log or (lambda x: None), + 'log': _log, } + _log(f"ObjectRetrierWraps: wrapping {self.__obj}") def __getattr__(self, name): """Get attribute; delegates to wrapped object.""" - # Note the above may generate an attribute error; we expect this and - # will fail with an attribute error. - attr = getattr(self.__obj, name) - if callable(attr) or hasattr(attr, "__getattr__"): + obj = self.__obj + attr = getattr(obj, name) + if callable(attr): return ObjectRetrierWraps(attr, **self.__kwargs) - else: + if attr.__class__.__module__ == 'builtins': return attr - # TODO(ajkavanagh): Note detecting a property is a bit trickier. we - # can do isinstance(attr, property), but then the act of accessing it - # is what calls it. i.e. it would fail at the getattr(self.__obj, - # name) stage. The solution is to check first, and if it's a property, - # then treat it like the retrier. However, I think this is too - # complex for the first go, and to use manual retries in that instance. + return ObjectRetrierWraps(attr, **self.__kwargs) def __call__(self, *args, **kwargs): """Call the object; delegates to the wrapped object.""" @@ -126,16 +150,20 @@ def __call__(self, *args, **kwargs): # is not in the list of retries, then raise an exception # immediately. This means that if retry_exceptions is None, # then the method is always retried. + if isinstance(e, NEVER_RETRY_EXCEPTIONS): + log("ObjectRetrierWraps: error {} is never caught; raising" + .format(str(e))) + raise if (retry_exceptions is not None and type(e) not in retry_exceptions): raise retry += 1 if retry > num_retries: - log("{}: exceeded number of retries, so erroring out" - .format(str(obj))) + log("ObjectRetrierWraps: exceeded number of retries, " + "so erroring out") raise e - log("{}: call failed: retrying in {} seconds" - .format(str(obj), wait)) + log("ObjectRetrierWraps: call failed: retrying in {} " + "seconds" .format(wait)) time.sleep(wait) wait_so_far += wait if wait_so_far >= total_wait: