From 1c4279cf9c6c4a150d6caeb4c175fd2a52879015 Mon Sep 17 00:00:00 2001 From: Corey Bryant Date: Wed, 11 Oct 2023 20:26:38 +0000 Subject: [PATCH 01/14] Add tenacity retries around ring sync check This fixes a race when checking if object ring is synced. (cherry picked from commit 0598ecfc9bebe1fe7264185d6e0027a926e1bad1) --- zaza/openstack/charm_tests/swift/tests.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/zaza/openstack/charm_tests/swift/tests.py b/zaza/openstack/charm_tests/swift/tests.py index 605b275..93bcc90 100644 --- a/zaza/openstack/charm_tests/swift/tests.py +++ b/zaza/openstack/charm_tests/swift/tests.py @@ -132,9 +132,15 @@ class SwiftProxyTests(test_utils.OpenStackBaseTest): zaza.model.wait_for_agent_status() zaza.model.block_until_all_units_idle() - self.assertTrue( - swift_utils.is_ring_synced('swift-proxy', 'object', - self.TEST_EXPECTED_RING_HOSTS)) + for attempt in tenacity.Retrying( + wait=tenacity.wait_fixed(2), + retry=tenacity.retry_if_exception_type(AssertionError), + reraise=True, + stop=tenacity.stop_after_attempt(10)): + with attempt: + self.assertTrue( + swift_utils.is_ring_synced('swift-proxy', 'object', + self.TEST_EXPECTED_RING_HOSTS)) def test_905_remove_device_action_and_validate_rebalance(self): """Remove device from object ring.""" From 045b115aa0fb4a6f3740a14def0f13439d86710d Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Tue, 31 Oct 2023 19:44:49 +0000 Subject: [PATCH 02/14] Add a retrier-wrap to the nova client for octavia tests The nova service may not be quite ready after vault initialisation for the basic network configuration, so add a retrier wrapper around the client for every test to make sure that it retries. (cherry picked from commit def0148642af16c7d32ccbb173639b2edba15543) --- zaza/openstack/utilities/openstack.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/zaza/openstack/utilities/openstack.py b/zaza/openstack/utilities/openstack.py index e548884..e3032d2 100644 --- a/zaza/openstack/utilities/openstack.py +++ b/zaza/openstack/utilities/openstack.py @@ -85,6 +85,7 @@ from zaza import model from zaza.openstack.utilities import ( exceptions, generic as generic_utils, + ObjectRetrierWraps, ) import zaza.utilities.networking as network_utils @@ -379,7 +380,8 @@ def get_nova_session_client(session, version=None): """ if not version: version = 2 - return novaclient_client.Client(version, session=session) + return ObjectRetrierWraps( + novaclient_client.Client(version, session=session)) def get_neutron_session_client(session): From fc508e8999fbb2d96a8dcfb484aee8293817f9a7 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Tue, 7 Nov 2023 15:24:03 +0200 Subject: [PATCH 03/14] [bobcat] Disable changing default_ttl test due to designate bug In designate bobcat the designate team re-organised the sql-alchemy code and this has resulted in the bug [1] that means that the default values for various zone creations are no longer used from the /etc/designate/designate.conf file. i.e. the defaults are fixed. Related-Bug: LP#2042944 (cherry picked from commit a365823de060e3f005a343a973bda6f184c118bb) --- zaza/openstack/charm_tests/designate/tests.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/zaza/openstack/charm_tests/designate/tests.py b/zaza/openstack/charm_tests/designate/tests.py index 73a434c..ed68a8c 100644 --- a/zaza/openstack/charm_tests/designate/tests.py +++ b/zaza/openstack/charm_tests/designate/tests.py @@ -134,6 +134,12 @@ class DesignateAPITests(BaseDesignateTest): def test_300_default_soa_config_options(self): """Configure default SOA options.""" + current_release = openstack_utils.get_os_release() + jammy_antelope = openstack_utils.get_os_release('jammy_antelope') + if current_release > jammy_antelope: + self.skipTest('changing default ttl is currently broken since ' + 'jammy_bobcat due to LP#2042944') + test_domain = "test_300_example.com." DEFAULT_TTL = 60 alternate_config = {'default-soa-minimum': 600, From e242121880ffa1535ad5ed89036a09cc3a6806af Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Mon, 9 Oct 2023 11:16:50 -0300 Subject: [PATCH 04/14] Add test to check ceph keys (new) For https://review.opendev.org/897549 (cherry picked from commit 912f33e0712034648d393874f14a27cf168bf68b) --- zaza/openstack/charm_tests/nova/tests.py | 58 ++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/zaza/openstack/charm_tests/nova/tests.py b/zaza/openstack/charm_tests/nova/tests.py index 34f0273..2c0286e 100644 --- a/zaza/openstack/charm_tests/nova/tests.py +++ b/zaza/openstack/charm_tests/nova/tests.py @@ -19,6 +19,7 @@ import json import logging import os +import re import tempfile import tenacity import unittest @@ -470,6 +471,63 @@ class NovaCompute(NovaCommonTests): with self.pause_resume(['nova-compute']): logging.info("Testing pause resume") + def test_904_test_ceph_keys(self): + """Test if the ceph keys in /etc/ceph are correct.""" + # only run if configured as rbd with ceph image backend + if zaza.model.get_application_config( + self.application_name)['libvirt-image-backend'].get( + 'value') != 'rbd': + return + + # Regex for + # [client.nova-compute] + # key = AQBm5xJl8CSnFxAACB9GVr2llNO0G8zWZuZnjQ == + regex = re.compile(r"^\[client.(.+)\]\n\tkey = (.+)$") + key_dict = {} + + # The new and correct behavior is to have + # "nova-compute-ceph-auth-" named keyring + # and one other named after the charm app. Example: + # for a charm app named "nova-compute-kvm", + # it should have both nova-compute-kvm and + # nova-compute-ceph-auth- keyrings. + # For a charm app named "nova-compute", + # it should have both nova-compute and + # nova-compute-ceph-auth- keyrings. + + # Previous behaviors: + # The old behavior is to have only 1 keyring named after the charm app. + + def check_keyring(key_name): + """Check matching keyring name and different from existing ones.""" + keyring_file = ( + '/etc/ceph/ceph.client.{}.keyring'.format(key_name)) + data = str(generic_utils.get_file_contents( + unit, keyring_file)) + + result = regex.findall(data)[0] + + # Assert keyring file name matches intended name + self.assertEqual(2, len(result)) + self.assertEqual(result[0], key_name) + + # Confirm the keys are different from each other and the + # same across all units + for k, v in key_dict.items(): + if k == result[0]: + self.assertEqual(v, result[1]) + else: + self.assertNotEqual(v, result[1]) + key_dict[result[0]] = result[1] + + for unit in zaza.model.get_units( + self.application_name, model_name=self.model_name): + + # old key + check_keyring(self.application_name) + # new key + check_keyring('nova-compute-ceph-auth-c91ce26f') + def test_930_check_virsh_default_network(self): """Test default virt network is not present.""" for unit in zaza.model.get_units('nova-compute', From 67aca7e44ebf27091c78758047a497bb42208626 Mon Sep 17 00:00:00 2001 From: Seyeong Kim Date: Tue, 9 Apr 2024 15:34:05 +0900 Subject: [PATCH 05/14] Adding fixing broken configuration test for mysql-router (cherry picked from commit 0a8889796605d33315e7fed93743c21eba1d2c6b) --- zaza/openstack/charm_tests/mysql/tests.py | 63 +++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/zaza/openstack/charm_tests/mysql/tests.py b/zaza/openstack/charm_tests/mysql/tests.py index 7101955..7c574a9 100644 --- a/zaza/openstack/charm_tests/mysql/tests.py +++ b/zaza/openstack/charm_tests/mysql/tests.py @@ -1145,3 +1145,66 @@ class MySQLRouterTests(test_utils.OpenStackBaseTest): {}, {}, self.services) logging.info("Passed restart on changed test.") + + def test_920_mysqlrouter_conf_broken(self): + """Checking conf broken case. + + Run the bootstrap when conf is broken + """ + # application_name on test is keystone-mysql-router + # using self.conf_file introduces error. + # instead of changing current self.conf_file, + # define one (it could introduce another issue) + config_file = ( + "/var/lib/mysql/{}/mysqlrouter.conf" + .format(self.application_name)) + + logging.info("Starting broken conf test") + + # put empty string to conf_file and make it wrong + logging.info("Breaking configuration file") + zaza.model.run_on_leader(self.application, + "echo '[DEFAULT]\n \ + [metadata_cache:[\\w$]+$] \ + ' > {}".format( + config_file)) + + logging.info("Getting configuration file") + recovered = zaza.model.run_on_leader(self.application, + "cat {}".format( + config_file))['Stdout'] + + # Checking conf file length, + # if file is broken it is around 250 + assert len(recovered) < 1000, ( + "Breaking mysqlrouter conf failed.") + + # verify it is in error state + for attempt in tenacity.Retrying( + reraise=True, + wait=tenacity.wait_fixed(10), + stop=tenacity.stop_after_attempt(30), + ): + with attempt: + # update status to make the status error + logging.info("Run update-status") + self.run_update_status_hooks(['keystone-mysql-router/0']) + + # get current status + unit_status = (zaza.model.get_status() + .applications + ['keystone-mysql-router']['status']) + logging.info("Status:{}".format(unit_status['status'])) + self.assertEqual(unit_status['status'], "active") + + logging.info("Getting configuration file") + recovered = zaza.model.run_on_leader(self.application, + "cat {}".format( + config_file))['Stdout'] + + # Checking conf file length, + # if file is broken it is around 250 + assert len(recovered) > 1000, ( + "Fixing mysqlrouter conf failed.") + + logging.info("Passed broken conf test.") From bca7a935fc87ffac1aab58983dd6fb5ccd23ef3b Mon Sep 17 00:00:00 2001 From: Myles Penner Date: Fri, 24 May 2024 15:27:39 -0700 Subject: [PATCH 06/14] Add class for keystone audit middleware testing Added general class for testing keystone audit middleware functionality in charms. Tests correct rendering of api-paste.ini file and allows charms without an api-paste.ini file to skip the tests. Example usage with charm Heat: tests/tests.yaml tests: - zaza.openstack.charm_tests.audit.tests.KeystoneAuditMiddlewareTest tests_options: audit-middleware: service: heat (cherry picked from commit ff5cdf51e6ab7536993ba4ebd15bdfaaa180122e) --- zaza/openstack/charm_tests/audit/__init__.py | 20 ++++ zaza/openstack/charm_tests/audit/tests.py | 119 +++++++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 zaza/openstack/charm_tests/audit/__init__.py create mode 100644 zaza/openstack/charm_tests/audit/tests.py diff --git a/zaza/openstack/charm_tests/audit/__init__.py b/zaza/openstack/charm_tests/audit/__init__.py new file mode 100644 index 0000000..0c3a4c5 --- /dev/null +++ b/zaza/openstack/charm_tests/audit/__init__.py @@ -0,0 +1,20 @@ +# Copyright 2024 Canonical Ltd. +# +# 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. + +""" +Keystone audit middleware. + +Collection of code for setting up and testing Keystone audit middleware +functionality. +""" diff --git a/zaza/openstack/charm_tests/audit/tests.py b/zaza/openstack/charm_tests/audit/tests.py new file mode 100644 index 0000000..b10c383 --- /dev/null +++ b/zaza/openstack/charm_tests/audit/tests.py @@ -0,0 +1,119 @@ +#!/usr/bin/env python3 +# +# Copyright 2024 Canonical Ltd. +# +# 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. + +""" +Keystone audit middleware API logging testing. + +These methods test the rendering of the charm api-paste.ini file to +ensure the appropriate sections are rendered or not rendered depending +on the state of the audit-middleware configuration option. +""" + +import textwrap +import logging +import zaza.model +import zaza.openstack.charm_tests.test_utils as test_utils + + +class KeystoneAuditMiddlewareTest(test_utils.OpenStackBaseTest): + """Keystone audit middleware test class.""" + + @classmethod + def setUpClass(cls): + """Run class setup for Keystone audit middleware tests.""" + super(KeystoneAuditMiddlewareTest, cls).setUpClass() + test_config = cls.test_config['tests_options']['audit-middleware'] + cls.service_name = test_config['service'] + + cls.application_name = test_config.get('application', cls.service_name) + logging.info('Using application name: %s', cls.application_name) + + cls.initial_audit_middleware = zaza.model.get_application_config( + cls.application_name)['audit-middleware']['value'] + + @classmethod + def tearDownClass(cls): + """Restore the audit-middleware configuration to its original state.""" + super(KeystoneAuditMiddlewareTest, cls).tearDownClass() + logging.info("Running teardown on %s" % cls.application_name) + zaza.model.set_application_config( + cls.application_name, + {'audit-middleware': str(cls.initial_audit_middleware)}, + model_name=cls.model_name + ) + zaza.model.wait_for_application_states( + states={cls.application_name: { + 'workload-status': 'active', + 'workload-status-message': 'Unit is ready'}}, + model_name=cls.model_name + ) + + def fetch_api_paste_content(self): + """Fetch content of api-paste.ini file.""" + api_paste_ini_path = f"/etc/{self.service_name}/api-paste.ini" + lead_unit = zaza.model.get_lead_unit_name( + self.application_name, + model_name=self.model_name + ) + try: + return zaza.model.file_contents( + lead_unit, + api_paste_ini_path, + ) + except zaza.model.CommandRunFailed as e: + self.fail("Error fetching api-paste.ini: %s" % e) + + def test_101_apipaste_includes_audit_section(self): + """Test api-paste.ini renders audit section when enabled.""" + expected_content = textwrap.dedent(f"""\ + [filter:audit] + paste.filter_factory = keystonemiddleware.audit:filter_factory + audit_map_file = /etc/{self.service_name}/api_audit_map.conf + service_name = {self.service_name} + """) + + set_default = {'audit-middleware': False} + set_alternate = {'audit-middleware': True} + + with self.config_change(default_config=set_default, + alternate_config=set_alternate, + application_name=self.application_name): + api_paste_content = self.fetch_api_paste_content() + self.assertIn(expected_content, api_paste_content) + + def test_102_apipaste_excludes_audit_section(self): + """Test api_paste.ini does not render audit section when disabled.""" + section_heading = '[filter:audit]' + set_default = {'audit-middleware': True} + set_alternate = {'audit-middleware': False} + + with self.config_change(default_config=set_default, + alternate_config=set_alternate, + application_name=self.application_name): + api_paste_content = self.fetch_api_paste_content() + self.assertNotIn(section_heading, api_paste_content) + + +class IronicAuditMiddlewareTest(KeystoneAuditMiddlewareTest): + """Ironic-API audit middleware test class.""" + + def test_101_apipaste_includes_audit_section(self): + """Test api-paste.ini renders audit section when enabled.""" + self.skipTest('ironic-api does not use an api-paste.ini file') + + def test_102_apipaste_excludes_audit_section(self): + """Test api_paste.ini does not render audit section when disabled.""" + self.skipTest('ironic-api does not use an api-paste.ini file') From af661259449bd3370e7df2b8f94234d6d210eedc Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Mon, 24 Jun 2024 18:49:03 +0100 Subject: [PATCH 07/14] Add ObjectRetrier to CinderaBackupTests This adds the auto-retrier to the cinder client to get past race hazards and other transient errors. --- zaza/openstack/charm_tests/cinder_backup/tests.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/zaza/openstack/charm_tests/cinder_backup/tests.py b/zaza/openstack/charm_tests/cinder_backup/tests.py index 97b3658..5201cbc 100644 --- a/zaza/openstack/charm_tests/cinder_backup/tests.py +++ b/zaza/openstack/charm_tests/cinder_backup/tests.py @@ -22,6 +22,7 @@ import tenacity import zaza.model import zaza.openstack.charm_tests.test_utils as test_utils +from zaza.openstack.utilities import ObjectRetrierWraps import zaza.openstack.utilities.ceph as ceph_utils import zaza.openstack.utilities.openstack as openstack_utils @@ -35,8 +36,8 @@ 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 = ObjectRetrierWraps( + openstack_utils.get_cinder_session_client(cls.keystone_session)) @property def services(self): From 3f18031cd2383e59ed2b33f90b2d6833ed300766 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Tue, 25 Jun 2024 12:25:01 +0100 Subject: [PATCH 08/14] Add additional debug for ObjectRetrier It wasn't capturing member variables on the wrapped object that would then be used to make the call; thus, wrap those. This also disables (temporarily) the long running cinder backup test deletion whilst checking whether retries are the problem. --- .../charm_tests/cinder_backup/tests.py | 8 +++-- zaza/openstack/utilities/__init__.py | 36 +++++++++++++++++-- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/zaza/openstack/charm_tests/cinder_backup/tests.py b/zaza/openstack/charm_tests/cinder_backup/tests.py index 5201cbc..9314cc0 100644 --- a/zaza/openstack/charm_tests/cinder_backup/tests.py +++ b/zaza/openstack/charm_tests/cinder_backup/tests.py @@ -22,7 +22,7 @@ import tenacity import zaza.model import zaza.openstack.charm_tests.test_utils as test_utils -from zaza.openstack.utilities import ObjectRetrierWraps +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 @@ -36,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 = ObjectRetrierWraps( - 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): @@ -73,6 +74,7 @@ class CinderBackupTest(test_utils.OpenStackBaseTest): inspect ceph cinder pool object count as the volume is created and deleted. """ + return unit_name = zaza.model.get_lead_unit_name('ceph-mon') obj_count_samples = [] pool_size_samples = [] diff --git a/zaza/openstack/utilities/__init__.py b/zaza/openstack/utilities/__init__.py index 5798f26..68b0ce8 100644 --- a/zaza/openstack/utilities/__init__.py +++ b/zaza/openstack/utilities/__init__.py @@ -19,6 +19,25 @@ 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. @@ -94,11 +113,19 @@ class ObjectRetrierWraps(object): """Get attribute; delegates to wrapped object.""" # Note the above may generate an attribute error; we expect this and # will fail with an attribute error. + __log = self.__kwargs['log'] + __log(f"__getattr__(..) called with {name}") attr = getattr(self.__obj, name) if callable(attr) or hasattr(attr, "__getattr__"): + __log(f"__getattr__(..): wrapping {attr}") return ObjectRetrierWraps(attr, **self.__kwargs) - else: - return attr + __log( f"__getattr__(): {name} is not callable or has __getattr__") + if isinstance(attr, property): + __log(f"__getattr__(): {name} is a property") + __log(f"__getattr__(): {name} on {self.__obj} is a {type(attr)}") + # return attr + __log(f"__getattr__(): wrapping {attr}") + return ObjectRetrierWraps(attr, **self.__kwargs) # 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, @@ -120,12 +147,17 @@ class ObjectRetrierWraps(object): wait_so_far = 0 while True: try: + log(f"Running {self.__name__}({args}, {kwargs})") return obj(*args, **kwargs) except Exception as e: # if retry_exceptions is not None, or the type of the exception # 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" + .format(str(e))) + raise if (retry_exceptions is not None and type(e) not in retry_exceptions): raise From 39b1f43b985fdffe1106f67354a3148ddd54e8e2 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Wed, 26 Jun 2024 10:07:37 +0100 Subject: [PATCH 09/14] Re-enable test 410 for cinder backups This is to verify that the retries really do work. --- zaza/openstack/charm_tests/cinder_backup/tests.py | 1 - 1 file changed, 1 deletion(-) diff --git a/zaza/openstack/charm_tests/cinder_backup/tests.py b/zaza/openstack/charm_tests/cinder_backup/tests.py index 9314cc0..fc73fdb 100644 --- a/zaza/openstack/charm_tests/cinder_backup/tests.py +++ b/zaza/openstack/charm_tests/cinder_backup/tests.py @@ -74,7 +74,6 @@ class CinderBackupTest(test_utils.OpenStackBaseTest): inspect ceph cinder pool object count as the volume is created and deleted. """ - return unit_name = zaza.model.get_lead_unit_name('ceph-mon') obj_count_samples = [] pool_size_samples = [] From 925540b457f459284bec487ad0d2415c7f936312 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Thu, 27 Jun 2024 12:33:36 +0100 Subject: [PATCH 10/14] Fix ObjectRetrierWraps recursive wrapping In order to ensure that an object that contains other objects that are called (e.g. the VolumeManager object on the Cinder client object), the ObjectRetrierWraps class needs to more agressively wrap non builtin classes. --- requirements.txt | 5 +- unit_tests/utilities/test_utilities.py | 24 ++++++++- .../test_zaza_utilities_openstack.py | 3 ++ zaza/openstack/utilities/__init__.py | 50 +++++++++---------- 4 files changed, 54 insertions(+), 28 deletions(-) diff --git a/requirements.txt b/requirements.txt index 1a3b33c..992af69 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 6af6089..2f6dcf6 100644 --- a/unit_tests/utilities/test_utilities.py +++ b/unit_tests/utilities/test_utilities.py @@ -113,6 +113,28 @@ class TestObjectRetrierWraps(ut_utils.BaseTestCase): 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): @@ -130,7 +152,7 @@ class TestObjectRetrierWraps(ut_utils.BaseTestCase): # there should be two calls; one for the single retry and one for the # failure. - self.assertEqual(mock_log.call_count, 2) + self.assertEqual(mock_log.call_count, 6) @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 358f15a..57544c1 100644 --- a/unit_tests/utilities/test_zaza_utilities_openstack.py +++ b/unit_tests/utilities/test_zaza_utilities_openstack.py @@ -99,6 +99,9 @@ class TestOpenStackUtils(ut_utils.BaseTestCase): 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/utilities/__init__.py b/zaza/openstack/utilities/__init__.py index 68b0ce8..7a8ca10 100644 --- a/zaza/openstack/utilities/__init__.py +++ b/zaza/openstack/utilities/__init__.py @@ -15,6 +15,7 @@ """Collection of utilities to support zaza tests etc.""" +import logging import time from keystoneauth1.exceptions.connection import ConnectFailure @@ -95,10 +96,19 @@ class ObjectRetrierWraps(object): 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, @@ -106,32 +116,19 @@ class ObjectRetrierWraps(object): '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. - __log = self.__kwargs['log'] - __log(f"__getattr__(..) called with {name}") - attr = getattr(self.__obj, name) - if callable(attr) or hasattr(attr, "__getattr__"): - __log(f"__getattr__(..): wrapping {attr}") + obj = self.__obj + attr = getattr(obj, name) + if callable(attr): return ObjectRetrierWraps(attr, **self.__kwargs) - __log( f"__getattr__(): {name} is not callable or has __getattr__") - if isinstance(attr, property): - __log(f"__getattr__(): {name} is a property") - __log(f"__getattr__(): {name} on {self.__obj} is a {type(attr)}") - # return attr - __log(f"__getattr__(): wrapping {attr}") + if attr.__class__.__module__ == 'builtins': + return attr return ObjectRetrierWraps(attr, **self.__kwargs) - # 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. def __call__(self, *args, **kwargs): """Call the object; delegates to the wrapped object.""" @@ -147,7 +144,7 @@ class ObjectRetrierWraps(object): wait_so_far = 0 while True: try: - log(f"Running {self.__name__}({args}, {kwargs})") + log(f"Running {self}({args}, {kwargs})") return obj(*args, **kwargs) except Exception as e: # if retry_exceptions is not None, or the type of the exception @@ -155,7 +152,7 @@ class ObjectRetrierWraps(object): # 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" + log("ObjectRetrierWraps: error {} is never caught; raising" .format(str(e))) raise if (retry_exceptions is not None and @@ -163,15 +160,16 @@ class ObjectRetrierWraps(object): 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" .format(str(obj))) raise e - log("{}: call failed: retrying in {} seconds" - .format(str(obj), wait)) + log("ObjectRetrierWraps: {}: call failed: retrying in {} " + "seconds" .format(str(obj), wait)) time.sleep(wait) wait_so_far += wait if wait_so_far >= total_wait: raise e + print('wait: ', wait, ' backoff:', backoff) wait = wait * backoff if wait > max_interval: wait = max_interval From fe1a6a550cc29e48e45ee4b5eef045c51eadceb9 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Thu, 27 Jun 2024 15:24:48 +0100 Subject: [PATCH 11/14] Double Cinder backup restore time This accounts for slow ServerStack when using ceph in the model. --- zaza/openstack/charm_tests/cinder_backup/tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zaza/openstack/charm_tests/cinder_backup/tests.py b/zaza/openstack/charm_tests/cinder_backup/tests.py index fc73fdb..258c7bb 100644 --- a/zaza/openstack/charm_tests/cinder_backup/tests.py +++ b/zaza/openstack/charm_tests/cinder_backup/tests.py @@ -103,7 +103,7 @@ class CinderBackupTest(test_utils.OpenStackBaseTest): 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') @@ -125,7 +125,7 @@ class CinderBackupTest(test_utils.OpenStackBaseTest): 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') From d4cc71964360259ab7a2e9fb204d88680fc632af Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Mon, 1 Jul 2024 16:11:54 +0100 Subject: [PATCH 12/14] Add more retries to allow manila backup restore to complete Due to slow 1GiB networking in ServerStack the restore often exceeds the retries. Increase them to enable the restore to succeed. --- zaza/openstack/charm_tests/manila/tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zaza/openstack/charm_tests/manila/tests.py b/zaza/openstack/charm_tests/manila/tests.py index ea40738..3a3f54c 100644 --- a/zaza/openstack/charm_tests/manila/tests.py +++ b/zaza/openstack/charm_tests/manila/tests.py @@ -359,7 +359,7 @@ packages: 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") From 7e6aba6c3fa8b3670e85f2fcf50242bc1e1806ad Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Mon, 1 Jul 2024 17:28:16 +0100 Subject: [PATCH 13/14] Reduce ObjectRetrierWraps logging noise during normal use --- zaza/openstack/utilities/__init__.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/zaza/openstack/utilities/__init__.py b/zaza/openstack/utilities/__init__.py index 7a8ca10..02826ff 100644 --- a/zaza/openstack/utilities/__init__.py +++ b/zaza/openstack/utilities/__init__.py @@ -144,7 +144,6 @@ class ObjectRetrierWraps(object): wait_so_far = 0 while True: try: - log(f"Running {self}({args}, {kwargs})") return obj(*args, **kwargs) except Exception as e: # if retry_exceptions is not None, or the type of the exception @@ -160,16 +159,15 @@ class ObjectRetrierWraps(object): raise retry += 1 if retry > num_retries: - log("ObjectRetrierWraps: {}: exceeded number of retries, " - "so erroring out" .format(str(obj))) + log("ObjectRetrierWraps: exceeded number of retries, " + "so erroring out") raise e - log("ObjectRetrierWraps: {}: 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: raise e - print('wait: ', wait, ' backoff:', backoff) wait = wait * backoff if wait > max_interval: wait = max_interval From 02d8d3fce8b1e54e679c00f05f7a903f85dc98e6 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Mon, 1 Jul 2024 20:30:05 +0100 Subject: [PATCH 14/14] Modify test code to be less brittle The test for log function against ObjectRetrierWraps class is too brittle to the number of log calls. This commit makes it less brittle. --- unit_tests/utilities/test_utilities.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/unit_tests/utilities/test_utilities.py b/unit_tests/utilities/test_utilities.py index 2f6dcf6..7eb9927 100644 --- a/unit_tests/utilities/test_utilities.py +++ b/unit_tests/utilities/test_utilities.py @@ -150,9 +150,7 @@ class TestObjectRetrierWraps(ut_utils.BaseTestCase): 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, 6) + mock_log.assert_called() @mock.patch("time.sleep") def test_back_off_maximum(self, mock_sleep):