Merge pull request #398 from openstack-charmers/ceph-bluestore-compression

Ceph bluestore compression

unit_tests/charm_tests/test_utils.py

@@ -12,13 +12,26 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import unittest
+from unittest import mock
+
 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):
+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):
 
@@ -37,16 +50,74 @@ 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(unittest.TestCase):
 
-    @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):
+class TestOpenStackBaseTest(ut_utils.BaseTestCase):
+
+    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')

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')

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

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,12 +252,24 @@ 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 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(

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'
+    ]))

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']},
 ]
 
 

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
+    ]),
 }