From 8f3dfb184bede367d8bf8df88e6f32071caec36d Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Fri, 28 Aug 2020 15:18:29 +0200 Subject: [PATCH] Add Ceph BlueStore Compression support Func-Test-Pr: https://github.com/openstack-charmers/zaza-openstack-tests/pull/398 Change-Id: I997bb15c692c442db2db79d96882a2204186dbe1 --- charmhelpers/contrib/openstack/context.py | 12 ++++ charmhelpers/contrib/storage/linux/ceph.py | 14 +++-- config.yaml | 66 ++++++++++++++++++++++ hooks/glance_relations.py | 56 +++++++++++++----- hooks/glance_utils.py | 22 ++++++-- test-requirements.txt | 1 - tests/tests.yaml | 1 + unit_tests/test_glance_relations.py | 39 ++++++++++++- unit_tests/test_glance_utils.py | 2 +- 9 files changed, 185 insertions(+), 28 deletions(-) diff --git a/charmhelpers/contrib/openstack/context.py b/charmhelpers/contrib/openstack/context.py index 0e41a9f3..54aed7ff 100644 --- a/charmhelpers/contrib/openstack/context.py +++ b/charmhelpers/contrib/openstack/context.py @@ -3245,6 +3245,18 @@ class CephBlueStoreCompressionContext(OSContextGenerator): """ return self.op + def get_kwargs(self): + """Get values for use as keyword arguments. + + :returns: Context values with key suitable for use as kwargs to + CephBrokerRq add_op_create_*_pool methods. + :rtype: Dict[str,any] + """ + return { + k.replace('-', '_'): v + for k, v in self.op.items() + } + def validate(self): """Validate options. diff --git a/charmhelpers/contrib/storage/linux/ceph.py b/charmhelpers/contrib/storage/linux/ceph.py index d9d43578..526b95ad 100644 --- a/charmhelpers/contrib/storage/linux/ceph.py +++ b/charmhelpers/contrib/storage/linux/ceph.py @@ -705,12 +705,12 @@ class ErasurePool(BasePool): # from different handling of this in the `charms.ceph` library. self.erasure_code_profile = op.get('erasure-profile', 'default-canonical') + self.allow_ec_overwrites = op.get('allow-ec-overwrites') else: # We keep the class default when initialized from keyword arguments # to not break the API for any other consumers. self.erasure_code_profile = erasure_code_profile or 'default' - - self.allow_ec_overwrites = allow_ec_overwrites + self.allow_ec_overwrites = allow_ec_overwrites def _create(self): # Try to find the erasure profile information in order to properly @@ -1972,12 +1972,14 @@ class CephBrokerRq(object): 'request-id': self.request_id}) def _ops_equal(self, other): + keys_to_compare = [ + 'replicas', 'name', 'op', 'pg_num', 'group-permission', + 'object-prefix-permissions', + ] + keys_to_compare += list(self._partial_build_common_op_create().keys()) if len(self.ops) == len(other.ops): for req_no in range(0, len(self.ops)): - for key in [ - 'replicas', 'name', 'op', 'pg_num', 'weight', - 'group', 'group-namespace', 'group-permission', - 'object-prefix-permissions']: + for key in keys_to_compare: if self.ops[req_no].get(key) != other.ops[req_no].get(key): return False else: diff --git a/config.yaml b/config.yaml index 088e6753..1b198f08 100644 --- a/config.yaml +++ b/config.yaml @@ -459,3 +459,69 @@ options: override YAML files in the service's policy.d directory. The resource file should be a ZIP file containing at least one yaml file with a .yaml or .yml extension. If False then remove the overrides. + bluestore-compression-algorithm: + type: string + default: + description: | + Compressor to use (if any) for pools requested by this charm. + . + NOTE: The ceph-osd charm sets a global default for this value (defaults + to 'lz4' unless configured by the end user) which will be used unless + specified for individual pools. + bluestore-compression-mode: + type: string + default: + description: | + Policy for using compression on pools requested by this charm. + . + 'none' means never use compression. + 'passive' means use compression when clients hint that data is + compressible. + 'aggressive' means use compression unless clients hint that + data is not compressible. + 'force' means use compression under all circumstances even if the clients + hint that the data is not compressible. + bluestore-compression-required-ratio: + type: float + default: + description: | + The ratio of the size of the data chunk after compression relative to the + original size must be at least this small in order to store the + compressed version on pools requested by this charm. + bluestore-compression-min-blob-size: + type: int + default: + description: | + Chunks smaller than this are never compressed on pools requested by + this charm. + bluestore-compression-min-blob-size-hdd: + type: int + default: + description: | + Value of bluestore compression min blob size for rotational media on + pools requested by this charm. + bluestore-compression-min-blob-size-ssd: + type: int + default: + description: | + Value of bluestore compression min blob size for solid state media on + pools requested by this charm. + bluestore-compression-max-blob-size: + type: int + default: + description: | + Chunks larger than this are broken into smaller blobs sizing bluestore + compression max blob size before being compressed on pools requested by + this charm. + bluestore-compression-max-blob-size-hdd: + type: int + default: + description: | + Value of bluestore compression max blob size for rotational media on + pools requested by this charm. + bluestore-compression-max-blob-size-ssd: + type: int + default: + description: | + Value of bluestore compression max blob size for solid state media on + pools requested by this charm. diff --git a/hooks/glance_relations.py b/hooks/glance_relations.py index 4a583183..080137da 100755 --- a/hooks/glance_relations.py +++ b/hooks/glance_relations.py @@ -126,7 +126,8 @@ from charmhelpers.contrib.openstack.ip import ( PUBLIC, INTERNAL, ADMIN ) from charmhelpers.contrib.openstack.context import ( - ADDRESS_TYPES + ADDRESS_TYPES, + CephBlueStoreCompressionContext, ) from charmhelpers.contrib.charmsupport import nrpe from charmhelpers.contrib.hardening.harden import harden @@ -310,6 +311,7 @@ def get_ceph_request(): rq = CephBrokerRq() weight = config('ceph-pool-weight') replicas = config('ceph-osd-replication-count') + bluestore_compression = CephBlueStoreCompressionContext() if config('pool-type') == 'erasure-coded': # General EC plugin config @@ -361,17 +363,35 @@ def get_ceph_request(): ) # Create EC data pool - rq.add_op_create_erasure_pool( - name=pool_name, - erasure_profile=profile_name, - weight=weight, - group="images", - app_name="rbd", - allow_ec_overwrites=True - ) + + # NOTE(fnordahl): once we deprecate Python 3.5 support we can do + # the unpacking of the BlueStore compression arguments as part of + # the function arguments. Until then we need to build the dict + # prior to the function call. + kwargs = { + 'name': pool_name, + 'erasure_profile': profile_name, + 'weight': weight, + 'group': "images", + 'app_name': "rbd", + 'allow_ec_overwrites': True, + } + kwargs.update(bluestore_compression.get_kwargs()) + rq.add_op_create_erasure_pool(**kwargs) else: - rq.add_op_create_pool(name=pool_name, replica_count=replicas, - weight=weight, group='images', app_name='rbd') + # NOTE(fnordahl): once we deprecate Python 3.5 support we can do + # the unpacking of the BlueStore compression arguments as part of + # the function arguments. Until then we need to build the dict + # prior to the function call. + kwargs = { + 'name': pool_name, + 'replica_count': replicas, + 'weight': weight, + 'group': 'images', + 'app_name': 'rbd', + } + kwargs.update(bluestore_compression.get_kwargs()) + rq.add_op_create_replicated_pool(**kwargs) if config('restrict-ceph-pools'): rq.add_op_request_access_to_group( @@ -498,8 +518,18 @@ def config_changed(): # NOTE(jamespage): trigger any configuration related changes # for cephx permissions restrictions - ceph_changed() - update_image_location_policy(CONFIGS) + try: + ceph_changed() + update_image_location_policy(CONFIGS) + except ValueError as e: + # The end user has most likely provided a invalid value for a + # configuration option. Just log the traceback here, the end user will + # be notified by assess_status() called at the end of the hook + # execution. + juju_log( + 'Caught ValueError, invalid value provided for configuration?: ' + '"{}"'.format(str(e)), + level=DEBUG) # call the policy overrides handler which will install any policy overrides maybe_do_policyd_overrides_on_config_changed( diff --git a/hooks/glance_utils.py b/hooks/glance_utils.py index c3ef0bd8..42cedb72 100644 --- a/hooks/glance_utils.py +++ b/hooks/glance_utils.py @@ -462,9 +462,8 @@ def get_optional_interfaces(): return optional_interfaces -def check_optional_relations(configs): - """Check that if we have a relation_id for high availability that we can - get the hacluster config. If we can't then we are blocked. +def check_optional_config_and_relations(configs): + """Validate optional configuration and relations when present. This function is called from assess_status/set_os_workload_status as the charm_func and needs to return either None, None if there is no problem or @@ -473,6 +472,8 @@ def check_optional_relations(configs): :param configs: an OSConfigRender() instance. :return 2-tuple: (string, string) = (status, message) """ + # Check that if we have a relation_id for high availability that we can + # get the hacluster config. If we can't then we are blocked. if relation_ids('ha'): try: get_hacluster_config() @@ -480,6 +481,19 @@ def check_optional_relations(configs): return ('blocked', 'hacluster missing configuration: ' 'vip, vip_iface, vip_cidr') + + if relation_ids('ceph'): + # Check that provided Ceph BlueStoe configuration is valid. + try: + bluestore_compression = context.CephBlueStoreCompressionContext() + bluestore_compression.validate() + except AttributeError: + # The charm does late installation of the `ceph-common` package and + # the class initializer above will throw an exception until it is. + pass + except ValueError as e: + return ('blocked', 'Invalid configuration: {}'.format(str(e))) + # return 'unknown' as the lowest priority to not clobber an existing # status. return "unknown", "" @@ -522,7 +536,7 @@ def assess_status_func(configs): _services, _ = get_managed_services_and_ports(services(), []) return make_assess_status_func( configs, required_interfaces, - charm_func=check_optional_relations, + charm_func=check_optional_config_and_relations, services=_services, ports=None) diff --git a/test-requirements.txt b/test-requirements.txt index 3e9f5cbc..9cb9048a 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -7,7 +7,6 @@ # requirements. They are intertwined. Also, Zaza itself should specify # all of its own requirements and if it doesn't, fix it there. # -setuptools<50.0.0 # https://github.com/pypa/setuptools/commit/04e3df22df840c6bb244e9b27bc56750c44b7c85 charm-tools>=2.4.4 requests>=2.18.4 mock>=1.2 diff --git a/tests/tests.yaml b/tests/tests.yaml index 36e95d52..29986017 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -39,6 +39,7 @@ tests: - zaza.openstack.charm_tests.glance.tests.GlanceTest - zaza.openstack.charm_tests.policyd.tests.GlanceTests - zaza.openstack.charm_tests.ceph.tests.CheckPoolTypes + - zaza.openstack.charm_tests.ceph.tests.BlueStoreCompressionCharmOperation - zaza.openstack.charm_tests.glance.tests.GlanceTest - zaza.openstack.charm_tests.policyd.tests.GlanceTests - full_run: diff --git a/unit_tests/test_glance_relations.py b/unit_tests/test_glance_relations.py index 8edbfd0e..5e32910b 100644 --- a/unit_tests/test_glance_relations.py +++ b/unit_tests/test_glance_relations.py @@ -354,12 +354,14 @@ class GlanceRelationTests(CharmTestCase): for c in [call('/etc/glance/glance.conf')]: self.assertNotIn(c, configs.write.call_args_list) + @patch.object(relations, 'CephBlueStoreCompressionContext') @patch('charmhelpers.contrib.storage.linux.ceph.CephBrokerRq' '.add_op_request_access_to_group') @patch('charmhelpers.contrib.storage.linux.ceph.CephBrokerRq' - '.add_op_create_pool') + '.add_op_create_replicated_pool') def test_create_pool_op(self, mock_create_pool, - mock_request_access): + mock_request_access, + mock_bluestore_compression): self.service_name.return_value = 'glance' self.test_config.set('ceph-osd-replication-count', 3) self.test_config.set('ceph-pool-weight', 6) @@ -384,6 +386,21 @@ class GlanceRelationTests(CharmTestCase): permission='rwx'), ]) + # confirm operation with bluestore compression + mock_create_pool.reset_mock() + mock_bluestore_compression().get_kwargs.return_value = { + 'compression_mode': 'fake', + } + relations.get_ceph_request() + mock_create_pool.assert_called_once_with( + name='glance', + replica_count=3, + weight=6, + group='images', + app_name='rbd', + compression_mode='fake') + + @patch.object(relations, 'CephBlueStoreCompressionContext') @patch('charmhelpers.contrib.storage.linux.ceph.CephBrokerRq' '.add_op_create_erasure_pool') @patch('charmhelpers.contrib.storage.linux.ceph.CephBrokerRq' @@ -395,7 +412,8 @@ class GlanceRelationTests(CharmTestCase): def test_create_ec_pool_op(self, mock_create_pool, mock_request_access, mock_create_erasure_profile, - mock_create_erasure_pool): + mock_create_erasure_pool, + mock_bluestore_compression): self.service_name.return_value = 'glance' self.test_config.set('ceph-osd-replication-count', 3) self.test_config.set('ceph-pool-weight', 6) @@ -431,6 +449,21 @@ class GlanceRelationTests(CharmTestCase): allow_ec_overwrites=True) mock_request_access.assert_not_called() + # confirm operation with bluestore compression + mock_create_erasure_pool.reset_mock() + mock_bluestore_compression().get_kwargs.return_value = { + 'compression_mode': 'fake', + } + relations.get_ceph_request() + mock_create_erasure_pool.assert_called_once_with( + name='glance', + erasure_profile='glance-profile', + weight=5.94, + group='images', + app_name='rbd', + allow_ec_overwrites=True, + compression_mode='fake') + @patch.object(relations, 'get_ceph_request') @patch.object(relations, 'send_request_if_needed') @patch.object(relations, 'is_request_complete') diff --git a/unit_tests/test_glance_utils.py b/unit_tests/test_glance_utils.py index f5d016a4..c778de0c 100644 --- a/unit_tests/test_glance_utils.py +++ b/unit_tests/test_glance_utils.py @@ -309,7 +309,7 @@ class TestGlanceUtils(CharmTestCase): make_assess_status_func.assert_called_once_with( 'test-config', {'int': ['test 1'], 'opt': ['test 2']}, - charm_func=utils.check_optional_relations, + charm_func=utils.check_optional_config_and_relations, services=['s1'], ports=None) def test_pause_unit_helper(self):