From 52a2aabf57e7c563827fe55f5a779934c42f5251 Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Fri, 28 Aug 2020 11:28:42 +0200 Subject: [PATCH 1/6] utilities/ceph: Add helper for retrieving pools from broker req --- .../utilities/test_zaza_utilities_ceph.py | 17 +++++++++ zaza/openstack/utilities/ceph.py | 38 ++++++++++++++++++- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/unit_tests/utilities/test_zaza_utilities_ceph.py b/unit_tests/utilities/test_zaza_utilities_ceph.py index 0855e41..ea718b7 100644 --- a/unit_tests/utilities/test_zaza_utilities_ceph.py +++ b/unit_tests/utilities/test_zaza_utilities_ceph.py @@ -116,3 +116,20 @@ class TestCephUtils(ut_utils.BaseTestCase): with self.assertRaises(model.CommandRunFailed): ceph_utils.get_rbd_hash('aunit', 'apool', 'aimage', model_name='amodel') + + def test_pools_from_broker_req(self): + self.patch_object(ceph_utils.zaza_juju, 'get_relation_from_unit') + self.get_relation_from_unit.return_value = { + 'broker_req': ( + '{"api-version": 1, "ops": [' + '{"op": "create-pool", "name": "cinder-ceph", ' + '"compression-mode": null},' + '{"op": "create-pool", "name": "cinder-ceph", ' + '"compression-mode": "aggressive"}]}'), + } + self.assertEquals( + ceph_utils.get_pools_from_broker_req( + 'anApplication', 'aModelName'), + ['cinder-ceph']) + self.get_relation_from_unit.assert_called_once_with( + 'ceph-mon', 'anApplication', None, model_name='aModelName') diff --git a/zaza/openstack/utilities/ceph.py b/zaza/openstack/utilities/ceph.py index 893ea5e..6613154 100644 --- a/zaza/openstack/utilities/ceph.py +++ b/zaza/openstack/utilities/ceph.py @@ -2,8 +2,10 @@ import json import logging -import zaza.openstack.utilities.openstack as openstack_utils import zaza.model as zaza_model +import zaza.utilities.juju as zaza_juju + +import zaza.openstack.utilities.openstack as openstack_utils REPLICATED_POOL_TYPE = 'replicated' ERASURE_POOL_TYPE = 'erasure-coded' @@ -204,3 +206,37 @@ def get_rbd_hash(unit_name, pool, image, model_name=None): if result.get('Code') != '0': raise zaza_model.CommandRunFailed(cmd, result) return result.get('Stdout').rstrip() + + +def get_pools_from_broker_req(application_or_unit, model_name=None): + """Get pools requested by application or unit. + + By retrieving and parsing broker request from relation data we can get a + list of pools a unit has requested. + + :param application_or_unit: Name of application or unit that is at the + other end of a ceph-mon relation. + :type application_or_unit: str + :param model_name: Name of Juju model to operate on + :type model_name: Optional[str] + :returns: List of pools requested. + :rtype: List[str] + :raises: KeyError + """ + # NOTE: we do not pass on a name for the remote_interface_name as that + # varies between the Ceph consuming applications. + relation_data = zaza_juju.get_relation_from_unit( + 'ceph-mon', application_or_unit, None, model_name=model_name) + + # NOTE: we probably should consume the Ceph broker code from c-h but c-h is + # such a beast of a dependency so let's defer adding it to Zaza if we can. + broker_req = json.loads(relation_data['broker_req']) + + # A charm may request modifications to an existing pool by adding multiple + # 'create-pool' broker requests so we need to deduplicate the list before + # returning it. + return list(set([ + op['name'] + for op in broker_req['ops'] + if op['op'] == 'create-pool' + ])) From 5b2a2fee9ca3a1b36808677eec45f1e686d1c2d0 Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Mon, 31 Aug 2020 15:49:14 +0200 Subject: [PATCH 2/6] unit_tests: Replace patch decorators with BaseTestCase helpers --- unit_tests/charm_tests/test_utils.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/unit_tests/charm_tests/test_utils.py b/unit_tests/charm_tests/test_utils.py index 2a494d6..8e0661b 100644 --- a/unit_tests/charm_tests/test_utils.py +++ b/unit_tests/charm_tests/test_utils.py @@ -12,10 +12,9 @@ # See the License for the specific language governing permissions and # limitations under the License. -import unittest import zaza.openstack.charm_tests.test_utils as test_utils -from unittest.mock import patch +import unit_tests.utils as ut_utils class TestBaseCharmTest(unittest.TestCase): @@ -38,15 +37,16 @@ class TestBaseCharmTest(unittest.TestCase): }), 'aValue') -class TestOpenStackBaseTest(unittest.TestCase): +class TestOpenStackBaseTest(ut_utils.BaseTestCase): - @patch.object(test_utils.openstack_utils, 'get_cacert') - @patch.object(test_utils.openstack_utils, 'get_overcloud_keystone_session') - @patch.object(test_utils.BaseCharmTest, 'setUpClass') - def test_setUpClass(self, _setUpClass, _get_ovcks, _get_cacert): + def test_setUpClass(self): + self.patch_object(test_utils.openstack_utils, 'get_cacert') + self.patch_object(test_utils.openstack_utils, + 'get_overcloud_keystone_session') + self.patch_object(test_utils.BaseCharmTest, 'setUpClass') class MyTestClass(test_utils.OpenStackBaseTest): model_name = 'deadbeef' MyTestClass.setUpClass('foo', 'bar') - _setUpClass.assert_called_with('foo', 'bar') + self.setUpClass.assert_called_with('foo', 'bar') From 8a8178738d05a0003a6784acea6ba3e7f58a19d2 Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Mon, 31 Aug 2020 16:26:14 +0200 Subject: [PATCH 3/6] Add support for resetting back to charm default on config_change Due to python-libjuju's requirement for coercing every config value into strings, some configuration type/values are not possible to specify the default for. Allow the config_change helper to optionally use reset back to charm default instead of specifying default config values in a dictionary. --- unit_tests/charm_tests/test_utils.py | 73 +++++++++++++++++++++++- zaza/openstack/charm_tests/test_utils.py | 27 +++++++-- 2 files changed, 93 insertions(+), 7 deletions(-) diff --git a/unit_tests/charm_tests/test_utils.py b/unit_tests/charm_tests/test_utils.py index 8e0661b..7ca3ba5 100644 --- a/unit_tests/charm_tests/test_utils.py +++ b/unit_tests/charm_tests/test_utils.py @@ -12,12 +12,26 @@ # See the License for the specific language governing permissions and # limitations under the License. +from unittest import mock + import zaza.openstack.charm_tests.test_utils as test_utils import unit_tests.utils as ut_utils -class TestBaseCharmTest(unittest.TestCase): +class TestBaseCharmTest(ut_utils.BaseTestCase): + + def setUp(self): + super(TestBaseCharmTest, self).setUp() + self.target = test_utils.BaseCharmTest() + + def patch_target(self, attr, return_value=None): + mocked = mock.patch.object(self.target, attr) + self._patches[attr] = mocked + started = mocked.start() + started.return_value = return_value + self._patches_start[attr] = started + setattr(self, attr, started) def test_get_my_tests_options(self): @@ -36,6 +50,63 @@ class TestBaseCharmTest(unittest.TestCase): }, }), 'aValue') + def test_config_change(self): + default_config = {'fakeKey': 'testProvidedDefault'} + alterna_config = {'fakeKey': 'testProvidedAlterna'} + self.target.model_name = 'aModel' + self.target.test_config = {} + self.patch_target('config_current') + self.config_current.return_value = default_config + self.patch_object(test_utils.model, 'set_application_config') + self.patch_object(test_utils.model, 'wait_for_agent_status') + self.patch_object(test_utils.model, 'wait_for_application_states') + self.patch_object(test_utils.model, 'block_until_all_units_idle') + with self.target.config_change( + default_config, alterna_config, application_name='anApp'): + self.set_application_config.assert_called_once_with( + 'anApp', alterna_config, model_name='aModel') + self.wait_for_agent_status.assert_called_once_with( + model_name='aModel') + self.wait_for_application_states.assert_called_once_with( + model_name='aModel', states={}) + self.block_until_all_units_idle.assert_called_once_with() + # after yield we will have different calls than the above, measure both + self.set_application_config.assert_has_calls([ + mock.call('anApp', alterna_config, model_name='aModel'), + mock.call('anApp', default_config, model_name='aModel'), + ]) + self.wait_for_application_states.assert_has_calls([ + mock.call(model_name='aModel', states={}), + mock.call(model_name='aModel', states={}), + ]) + self.block_until_all_units_idle.assert_has_calls([ + mock.call(), + mock.call(), + ]) + # confirm operation with `reset_to_charm_default` + self.set_application_config.reset_mock() + self.wait_for_agent_status.reset_mock() + self.wait_for_application_states.reset_mock() + self.patch_object(test_utils.model, 'reset_application_config') + with self.target.config_change( + default_config, alterna_config, application_name='anApp', + reset_to_charm_default=True): + self.set_application_config.assert_called_once_with( + 'anApp', alterna_config, model_name='aModel') + # we want to assert this not to be called after yield + self.set_application_config.reset_mock() + self.assertFalse(self.set_application_config.called) + self.reset_application_config.assert_called_once_with( + 'anApp', alterna_config.keys(), model_name='aModel') + self.wait_for_application_states.assert_has_calls([ + mock.call(model_name='aModel', states={}), + mock.call(model_name='aModel', states={}), + ]) + self.block_until_all_units_idle.assert_has_calls([ + mock.call(), + mock.call(), + ]) + class TestOpenStackBaseTest(ut_utils.BaseTestCase): diff --git a/zaza/openstack/charm_tests/test_utils.py b/zaza/openstack/charm_tests/test_utils.py index ee61336..9ba6955 100644 --- a/zaza/openstack/charm_tests/test_utils.py +++ b/zaza/openstack/charm_tests/test_utils.py @@ -182,7 +182,7 @@ class BaseCharmTest(unittest.TestCase): @contextlib.contextmanager def config_change(self, default_config, alternate_config, - application_name=None): + application_name=None, reset_to_charm_default=False): """Run change config tests. Change config to `alternate_config`, wait for idle workload status, @@ -202,6 +202,12 @@ class BaseCharmTest(unittest.TestCase): by a charm under test other than the object's application. :type application_name: str + :param reset_to_charm_default: When True we will ask Juju to reset each + configuration option mentioned in the + `alternate_config` dictionary back to + the charm default and ignore the + `default_config` dictionary. + :type reset_to_charm_default: bool """ if not application_name: application_name = self.application_name @@ -246,11 +252,20 @@ class BaseCharmTest(unittest.TestCase): yield - logging.debug('Restoring charm setting to {}'.format(default_config)) - model.set_application_config( - application_name, - self._stringed_value_config(default_config), - model_name=self.model_name) + if reset_to_charm_default: + logging.debug('Resetting these charm configuration options to the ' + 'charm default: "{}"' + .format(alternate_config.keys())) + model.reset_application_config(application_name, + alternate_config.keys(), + model_name=self.model_name) + else: + logging.debug('Restoring charm setting to {}' + .format(default_config)) + model.set_application_config( + application_name, + self._stringed_value_config(default_config), + model_name=self.model_name) logging.debug( 'Waiting for units to reach target states') From c5a3f832d014b28dd997356416af3ca082434d4c Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Wed, 16 Sep 2020 12:04:42 +0200 Subject: [PATCH 4/6] test_utils/config_change: Wait for agent to start executing Avoid returning prior to the config change has even started. Fixes: #419 --- zaza/openstack/charm_tests/test_utils.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/zaza/openstack/charm_tests/test_utils.py b/zaza/openstack/charm_tests/test_utils.py index 9ba6955..e6eca75 100644 --- a/zaza/openstack/charm_tests/test_utils.py +++ b/zaza/openstack/charm_tests/test_utils.py @@ -267,6 +267,9 @@ class BaseCharmTest(unittest.TestCase): self._stringed_value_config(default_config), model_name=self.model_name) + logging.debug( + 'Waiting for units to execute config-changed hook') + model.wait_for_agent_status(model_name=self.model_name) logging.debug( 'Waiting for units to reach target states') model.wait_for_application_states( From a1f3a8710fe916025f2c9aadbf26415d6d155074 Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Mon, 14 Sep 2020 12:45:11 +0200 Subject: [PATCH 5/6] Teach Zaza to determine release pair for Ceph deployments As Ceph is distributed as part of Ubuntu Cloud Archive, the pockets lend their name from the accompanying OpenStack release. --- zaza/openstack/utilities/openstack.py | 5 +++++ zaza/openstack/utilities/os_versions.py | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/zaza/openstack/utilities/openstack.py b/zaza/openstack/utilities/openstack.py index 0c31c05..cc07afb 100644 --- a/zaza/openstack/utilities/openstack.py +++ b/zaza/openstack/utilities/openstack.py @@ -119,6 +119,10 @@ CHARM_TYPES = { 'pkg': 'ovn-common', 'origin_setting': 'source' }, + 'ceph-mon': { + 'pkg': 'ceph-common', + 'origin_setting': 'source' + }, } # Older tests use the order the services appear in the list to imply @@ -138,6 +142,7 @@ UPGRADE_SERVICES = [ {'name': 'openstack-dashboard', 'type': CHARM_TYPES['openstack-dashboard']}, {'name': 'ovn-central', 'type': CHARM_TYPES['ovn-central']}, + {'name': 'ceph-mon', 'type': CHARM_TYPES['ceph-mon']}, ] diff --git a/zaza/openstack/utilities/os_versions.py b/zaza/openstack/utilities/os_versions.py index b46fc21..75dc723 100644 --- a/zaza/openstack/utilities/os_versions.py +++ b/zaza/openstack/utilities/os_versions.py @@ -249,4 +249,11 @@ PACKAGE_CODENAMES = { ('2', 'train'), ('20', 'ussuri'), ]), + 'ceph-common': OrderedDict([ + ('10', 'mitaka'), # jewel + ('12', 'queens'), # luminous + ('13', 'rocky'), # mimic + ('14', 'train'), # nautilus + ('15', 'ussuri'), # octopus + ]), } From 4db2202b1ec0818c91463759bacb25b834da54f3 Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Fri, 28 Aug 2020 14:19:26 +0200 Subject: [PATCH 6/6] ceph: Add functional tests for BlueStore compression --- zaza/openstack/charm_tests/ceph/tests.py | 138 +++++++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/zaza/openstack/charm_tests/ceph/tests.py b/zaza/openstack/charm_tests/ceph/tests.py index 99285a3..19ce1ec 100644 --- a/zaza/openstack/charm_tests/ceph/tests.py +++ b/zaza/openstack/charm_tests/ceph/tests.py @@ -872,3 +872,141 @@ def _get_mon_count_from_prometheus(prometheus_ip): response = client.get(url) logging.debug("Prometheus response: {}".format(response.json())) return response.json()['data']['result'][0]['value'][1] + + +class BlueStoreCompressionCharmOperation(test_utils.BaseCharmTest): + """Test charm handling of bluestore compression configuration options.""" + + @classmethod + def setUpClass(cls): + """Perform class one time initialization.""" + super(BlueStoreCompressionCharmOperation, cls).setUpClass() + cls.current_release = zaza_openstack.get_os_release( + zaza_openstack.get_current_os_release_pair( + application='ceph-mon')) + cls.bionic_rocky = zaza_openstack.get_os_release('bionic_rocky') + + def setUp(self): + """Perform common per test initialization steps.""" + super(BlueStoreCompressionCharmOperation, self).setUp() + + # determine if the tests should be run or not + logging.debug('os_release: {} >= {} = {}' + .format(self.current_release, + self.bionic_rocky, + self.current_release >= self.bionic_rocky)) + self.mimic_or_newer = self.current_release >= self.bionic_rocky + + def _assert_pools_properties(self, pools, pools_detail, + expected_properties, log_func=logging.info): + """Check properties on a set of pools. + + :param pools: List of pool names to check. + :type pools: List[str] + :param pools_detail: List of dictionaries with pool detail + :type pools_detail List[Dict[str,any]] + :param expected_properties: Properties to check and their expected + values. + :type expected_properties: Dict[str,any] + :returns: Nothing + :raises: AssertionError + """ + for pool in pools: + for pd in pools_detail: + if pd['pool_name'] == pool: + if 'options' in expected_properties: + for k, v in expected_properties['options'].items(): + self.assertEquals(pd['options'][k], v) + log_func("['options']['{}'] == {}".format(k, v)) + for k, v in expected_properties.items(): + if k == 'options': + continue + self.assertEquals(pd[k], v) + log_func("{} == {}".format(k, v)) + + def test_configure_compression(self): + """Enable compression and validate properties flush through to pool.""" + if not self.mimic_or_newer: + logging.info('Skipping test, Mimic or newer required.') + return + if self.application_name == 'ceph-osd': + # The ceph-osd charm itself does not request pools, neither does + # the BlueStore Compression configuration options it have affect + # pool properties. + logging.info('test does not apply to ceph-osd charm.') + return + elif self.application_name == 'ceph-radosgw': + # The Ceph RadosGW creates many light weight pools to keep track of + # metadata, we only compress the pool containing actual data. + app_pools = ['.rgw.buckets.data'] + else: + # Retrieve which pools the charm under test has requested skipping + # metadata pools as they are deliberately not compressed. + app_pools = [ + pool + for pool in zaza_ceph.get_pools_from_broker_req( + self.application_name, model_name=self.model_name) + if 'metadata' not in pool + ] + + ceph_pools_detail = zaza_ceph.get_ceph_pool_details( + model_name=self.model_name) + + logging.debug('BEFORE: {}'.format(ceph_pools_detail)) + try: + logging.info('Checking Ceph pool compression_mode prior to change') + self._assert_pools_properties( + app_pools, ceph_pools_detail, + {'options': {'compression_mode': 'none'}}) + except KeyError: + logging.info('property does not exist on pool, which is OK.') + logging.info('Changing "bluestore-compression-mode" to "force" on {}' + .format(self.application_name)) + with self.config_change( + {'bluestore-compression-mode': 'none'}, + {'bluestore-compression-mode': 'force'}): + # Retrieve pool details from Ceph after changing configuration + ceph_pools_detail = zaza_ceph.get_ceph_pool_details( + model_name=self.model_name) + logging.debug('CONFIG_CHANGE: {}'.format(ceph_pools_detail)) + logging.info('Checking Ceph pool compression_mode after to change') + self._assert_pools_properties( + app_pools, ceph_pools_detail, + {'options': {'compression_mode': 'force'}}) + ceph_pools_detail = zaza_ceph.get_ceph_pool_details( + model_name=self.model_name) + logging.debug('AFTER: {}'.format(ceph_pools_detail)) + logging.debug(zaza_juju.get_relation_from_unit( + 'ceph-mon', self.application_name, None, + model_name=self.model_name)) + logging.info('Checking Ceph pool compression_mode after restoring ' + 'config to previous value') + self._assert_pools_properties( + app_pools, ceph_pools_detail, + {'options': {'compression_mode': 'none'}}) + + def test_invalid_compression_configuration(self): + """Set invalid configuration and validate charm response.""" + if not self.mimic_or_newer: + logging.info('Skipping test, Mimic or newer required.') + return + stored_target_deploy_status = self.test_config.get( + 'target_deploy_status', {}) + new_target_deploy_status = stored_target_deploy_status.copy() + new_target_deploy_status[self.application_name] = { + 'workload-status': 'blocked', + 'workload-status-message': 'Invalid configuration', + } + if 'target_deploy_status' in self.test_config: + self.test_config['target_deploy_status'].update( + new_target_deploy_status) + else: + self.test_config['target_deploy_status'] = new_target_deploy_status + + with self.config_change( + {'bluestore-compression-mode': 'none'}, + {'bluestore-compression-mode': 'PEBCAK'}): + logging.info('Charm went into blocked state as expected, restore ' + 'configuration') + self.test_config[ + 'target_deploy_status'] = stored_target_deploy_status