diff --git a/cinder/tests/unit/test.py b/cinder/tests/unit/test.py index 826f65e34d7..66c2d312b7a 100644 --- a/cinder/tests/unit/test.py +++ b/cinder/tests/unit/test.py @@ -144,6 +144,10 @@ class TestCase(testtools.TestCase): self.patch('cinder.rpc.get_notifier', side_effect=self._get_joined_notifier) + # Protect against any case where someone doesn't directly patch a retry + # decorated call. + self.patch('cinder.utils._time_sleep') + if self.MOCK_WORKER: # Mock worker creation for all tests that don't care about it clean_path = 'cinder.objects.cleanable.CinderCleanableObject.%s' diff --git a/cinder/tests/unit/test_utils.py b/cinder/tests/unit/test_utils.py index b73f56f3991..3e1d1107bd5 100644 --- a/cinder/tests/unit/test_utils.py +++ b/cinder/tests/unit/test_utils.py @@ -23,7 +23,6 @@ from unittest import mock import ddt from oslo_utils import timeutils import six -from six.moves import range import webob.exc import cinder @@ -939,7 +938,7 @@ class TestRetryDecorator(test.TestCase): def test_no_retry_required(self): self.counter = 0 - with mock.patch.object(time, 'sleep') as mock_sleep: + with mock.patch('cinder.utils._time_sleep') as mock_sleep: @utils.retry(exception.VolumeBackendAPIException, interval=2, retries=3, @@ -956,7 +955,7 @@ class TestRetryDecorator(test.TestCase): def test_no_retry_required_random(self): self.counter = 0 - with mock.patch.object(time, 'sleep') as mock_sleep: + with mock.patch('cinder.utils._time_sleep') as mock_sleep: @utils.retry(exception.VolumeBackendAPIException, interval=2, retries=3, @@ -977,7 +976,7 @@ class TestRetryDecorator(test.TestCase): backoff_rate = 2 retries = 3 - with mock.patch.object(time, 'sleep') as mock_sleep: + with mock.patch('cinder.utils._time_sleep') as mock_sleep: @utils.retry(exception.VolumeBackendAPIException, interval, retries, @@ -993,7 +992,7 @@ class TestRetryDecorator(test.TestCase): self.assertEqual('success', ret) self.assertEqual(2, self.counter) self.assertEqual(1, mock_sleep.call_count) - mock_sleep.assert_called_with(interval * backoff_rate) + mock_sleep.assert_called_with(interval) def test_retries_once_random(self): self.counter = 0 @@ -1001,7 +1000,7 @@ class TestRetryDecorator(test.TestCase): backoff_rate = 2 retries = 3 - with mock.patch.object(time, 'sleep') as mock_sleep: + with mock.patch('cinder.utils._time_sleep') as mock_sleep: @utils.retry(exception.VolumeBackendAPIException, interval, retries, @@ -1026,7 +1025,7 @@ class TestRetryDecorator(test.TestCase): interval = 2 backoff_rate = 4 - with mock.patch.object(time, 'sleep') as mock_sleep: + with mock.patch('cinder.utils._time_sleep') as mock_sleep: @utils.retry(exception.VolumeBackendAPIException, interval, retries, @@ -1040,17 +1039,17 @@ class TestRetryDecorator(test.TestCase): self.assertEqual(retries, self.counter) expected_sleep_arg = [] - for i in range(retries): if i > 0: - interval *= backoff_rate + interval *= (backoff_rate ** (i - 1)) expected_sleep_arg.append(float(interval)) - mock_sleep.assert_has_calls(map(mock.call, expected_sleep_arg)) + mock_sleep.assert_has_calls( + list(map(mock.call, expected_sleep_arg))) def test_wrong_exception_no_retry(self): - with mock.patch.object(time, 'sleep') as mock_sleep: + with mock.patch('cinder.utils._time_sleep') as mock_sleep: @utils.retry(exception.VolumeBackendAPIException) def raise_unexpected_error(): raise WrongException("wrong exception") diff --git a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py index e2fcce802d8..ed593ab7341 100644 --- a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py +++ b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py @@ -2383,13 +2383,6 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): expected + self.standard_logout) - def _FakeRetrying(wait_func=None, - original_retrying=hpecommon.utils.retrying.Retrying, - *args, **kwargs): - return original_retrying(wait_func=lambda *a, **k: 0, - *args, **kwargs) - - @mock.patch('retrying.Retrying', _FakeRetrying) def test_delete_volume_online_active_done(self): # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client diff --git a/cinder/tests/unit/volume/drivers/huawei/test_huawei_drivers.py b/cinder/tests/unit/volume/drivers/huawei/test_huawei_drivers.py index 929e73d9c7f..c0e135f6142 100644 --- a/cinder/tests/unit/volume/drivers/huawei/test_huawei_drivers.py +++ b/cinder/tests/unit/volume/drivers/huawei/test_huawei_drivers.py @@ -24,7 +24,6 @@ from xml.etree import ElementTree import ddt import requests -import retrying from cinder import context from cinder import exception @@ -2368,7 +2367,7 @@ class HuaweiTestBase(test.TestCase): @ddt.data('1', '', '0') def test_copy_volume(self, input_speed): - self.driver.configuration.lun_copy_wait_interval = 0 + self.driver.configuration.lun_copy_wait_interval = 1 self.volume.metadata = {'copyspeed': input_speed} mocker = self.mock_object( @@ -4121,7 +4120,7 @@ class HuaweiISCSIDriverTestCase(HuaweiTestBase): with mock.patch.object(rest_client.RestClient, 'get_lun_info', return_value=offline_status): - self.assertRaises(retrying.RetryError, + self.assertRaises(exception.VolumeDriverException, replica.wait_volume_online, self.driver.client, lun_info) @@ -4136,7 +4135,7 @@ class HuaweiISCSIDriverTestCase(HuaweiTestBase): return_value={'SECRESACCESS': access_ro}) common_driver.wait_second_access(pair_id, access_ro) - self.assertRaises(retrying.RetryError, + self.assertRaises(exception.VolumeDriverException, common_driver.wait_second_access, pair_id, access_rw) def test_wait_replica_ready(self): diff --git a/cinder/tests/unit/volume/drivers/test_rbd.py b/cinder/tests/unit/volume/drivers/test_rbd.py index 7128410a49f..15b36c4ed9b 100644 --- a/cinder/tests/unit/volume/drivers/test_rbd.py +++ b/cinder/tests/unit/volume/drivers/test_rbd.py @@ -91,14 +91,7 @@ def common_mocks(f): The point of doing these mocks here is so that we don't accidentally set mocks that can't/don't get unset. """ - def _FakeRetrying(wait_func=None, - original_retrying = driver.utils.retrying.Retrying, - *args, **kwargs): - return original_retrying(wait_func=lambda *a, **k: 0, - *args, **kwargs) - def _common_inner_inner1(inst, *args, **kwargs): - @mock.patch('retrying.Retrying', _FakeRetrying) @mock.patch.object(driver.RBDDriver, '_get_usage_info') @mock.patch('cinder.volume.drivers.rbd.RBDVolumeProxy') @mock.patch('cinder.volume.drivers.rbd.RADOSClient') diff --git a/cinder/utils.py b/cinder/utils.py index 0f101378408..5976108cc52 100644 --- a/cinder/utils.py +++ b/cinder/utils.py @@ -17,7 +17,6 @@ """Utilities and helper functions.""" - import abc from collections import OrderedDict import contextlib @@ -29,7 +28,6 @@ import math import operator import os import pyclbr -import random import re import shutil import stat @@ -50,12 +48,30 @@ from oslo_utils import excutils from oslo_utils import importutils from oslo_utils import strutils from oslo_utils import timeutils -import retrying import six from cinder import exception from cinder.i18n import _ +# The following section is needed to be able to mock out the sleep calls that +# happen in the tenacity retry handling. We save off the real time.sleep for +# later and so it can be mocked in unit tests for the sleep call that tenacity +# makes. But we don't want all time.sleep calls to be modified, so after +# loading the tenacity module, we restore things back to normal. End result is +# only the tenacity sleep calls go through the method that we can mock, +# everything else works as normal. +_time_sleep = time.sleep + + +def _sleep(duration): + """Helper class to make it easier to mock retry sleeping.""" + _time_sleep(duration) + + +time.sleep = _sleep +import tenacity # noqa +time.sleep = _time_sleep +# End of time CONF = cfg.CONF LOG = logging.getLogger(__name__) @@ -658,41 +674,27 @@ class ComparableMixin(object): def retry(exceptions, interval=1, retries=3, backoff_rate=2, wait_random=False): - def _retry_on_exception(e): - return isinstance(e, exceptions) - - def _backoff_sleep(previous_attempt_number, delay_since_first_attempt_ms): - exp = backoff_rate ** previous_attempt_number - wait_for = interval * exp - - if wait_random: - random.seed() - wait_val = random.randrange(interval * 1000.0, wait_for * 1000.0) - else: - wait_val = wait_for * 1000.0 - - LOG.debug("Sleeping for %s seconds", (wait_val / 1000.0)) - - return wait_val - - def _print_stop(previous_attempt_number, delay_since_first_attempt_ms): - delay_since_first_attempt = delay_since_first_attempt_ms / 1000.0 - LOG.debug("Failed attempt %s", previous_attempt_number) - LOG.debug("Have been at this for %s seconds", - delay_since_first_attempt) - return previous_attempt_number == retries - if retries < 1: raise ValueError('Retries must be greater than or ' 'equal to 1 (received: %s). ' % retries) + if wait_random: + wait = tenacity.wait_random_exponential(multiplier=interval) + else: + wait = tenacity.wait_exponential( + multiplier=interval, min=0, exp_base=backoff_rate) + def _decorator(f): @six.wraps(f) def _wrapper(*args, **kwargs): - r = retrying.Retrying(retry_on_exception=_retry_on_exception, - wait_func=_backoff_sleep, - stop_func=_print_stop) + r = tenacity.Retrying( + before_sleep=tenacity.before_sleep_log(LOG, logging.DEBUG), + after=tenacity.after_log(LOG, logging.DEBUG), + stop=tenacity.stop_after_attempt(retries), + reraise=True, + retry=tenacity.retry_if_exception_type(exceptions), + wait=wait) return r.call(f, *args, **kwargs) return _wrapper diff --git a/cinder/volume/drivers/huawei/huawei_utils.py b/cinder/volume/drivers/huawei/huawei_utils.py index e83e804f9e4..de838e96475 100644 --- a/cinder/volume/drivers/huawei/huawei_utils.py +++ b/cinder/volume/drivers/huawei/huawei_utils.py @@ -15,10 +15,10 @@ import hashlib import json +import math from oslo_log import log as logging from oslo_utils import strutils -import retrying import six from cinder import context @@ -26,6 +26,7 @@ from cinder import exception from cinder.i18n import _ from cinder import objects from cinder.objects import fields +from cinder import utils from cinder.volume.drivers.huawei import constants from cinder.volume import qos_specs from cinder.volume import volume_types @@ -65,17 +66,30 @@ def old_encode_host_name(name): def wait_for_condition(func, interval, timeout): - def _retry_on_result(result): - return not result + """Wait for ``func`` to return True. - def _retry_on_exception(result): - return False + This retries running func until it either returns True or raises an + exception. + :param func: The function to call. + :param interval: The interval to wait in seconds between calls. + :param timeout: The maximum time in seconds to wait. + """ + if interval == 0: + interval = 1 + if timeout == 0: + timeout = 1 - r = retrying.Retrying(retry_on_result=_retry_on_result, - retry_on_exception=_retry_on_exception, - wait_fixed=interval * 1000, - stop_max_delay=timeout * 1000) - r.call(func) + @utils.retry(exception.VolumeDriverException, + interval=interval, + backoff_rate=1, + retries=(math.ceil(timeout / interval))) + def _retry_call(): + result = func() + if not result: + raise exception.VolumeDriverException( + _('Timed out waiting for condition.')) + + _retry_call() def _get_volume_type(volume): diff --git a/cinder/volume/drivers/ibm/storwize_svc/storwize_svc_common.py b/cinder/volume/drivers/ibm/storwize_svc/storwize_svc_common.py index 7e79da37e23..4ec9deca2d0 100644 --- a/cinder/volume/drivers/ibm/storwize_svc/storwize_svc_common.py +++ b/cinder/volume/drivers/ibm/storwize_svc/storwize_svc_common.py @@ -31,7 +31,6 @@ from oslo_utils import excutils from oslo_utils import strutils from oslo_utils import units import paramiko -from retrying import retry import six from cinder import context @@ -1208,15 +1207,15 @@ class StorwizeHelpers(object): {'volume_name': volume_name, 'host_name': host_name}) return int(result_lun) - def _retry_on_exception(e): - if hasattr(e, 'msg') and 'CMMVC5879E' in e.msg: - return True - return False + class _RetryableVolumeDriverException( + exception.VolumeBackendAPIException): + """Exception to identify which types of errors to retry.""" + pass - @retry(retry_on_exception=_retry_on_exception, - stop_max_attempt_number=3, - wait_random_min=1, - wait_random_max=10) + @cinder_utils.retry(_RetryableVolumeDriverException, + interval=2, + retries=3, + wait_random=True) def make_vdisk_host_map(): try: result_lun = self._get_unused_lun_id(host_name) @@ -1233,6 +1232,8 @@ class StorwizeHelpers(object): message=_('CMMVC6071E The VDisk-to-host mapping was ' 'not created because the VDisk is already ' 'mapped to a host.')) + if hasattr(ex, 'msg') and 'CMMVC5879E' in ex.msg: + raise _RetryableVolumeDriverException(ex) with excutils.save_and_reraise_exception(): LOG.error('Error mapping VDisk-to-host.') diff --git a/lower-constraints.txt b/lower-constraints.txt index 28c90ee70d2..53e9e14dbd4 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -147,7 +147,7 @@ stevedore==1.20.0 tabulate==0.8.5 taskflow==3.2.0 Tempita==0.5.2 -tenacity==4.9.0 +tenacity==6.0.0 testrepository==0.0.20 testtools==2.2.0 tooz==1.58.0 diff --git a/requirements.txt b/requirements.txt index be0251685d2..b652e3239d8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -43,7 +43,6 @@ python-novaclient>=9.1.0 # Apache-2.0 python-swiftclient>=3.2.0 # Apache-2.0 pytz>=2013.6 # MIT requests>=2.14.2,!=2.20.0 # Apache-2.0 -retrying!=1.3.0,>=1.2.3 # Apache-2.0 Routes>=2.3.1 # MIT taskflow>=3.2.0 # Apache-2.0 rtslib-fb>=2.1.65 # Apache-2.0 @@ -52,6 +51,7 @@ SQLAlchemy!=1.1.5,!=1.1.6,!=1.1.7,!=1.1.8,>=1.0.10 # MIT sqlalchemy-migrate>=0.11.0 # Apache-2.0 stevedore>=1.20.0 # Apache-2.0 tabulate>=0.8.5 # MIT +tenacity>=6.0.0 # Apache-2.0 WebOb>=1.7.1 # MIT oslo.i18n>=3.15.3 # Apache-2.0 oslo.vmware>=2.35.0 # Apache-2.0