From d0ad23950ffbbc87c176c008cd0180a0d579ddcd Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Mon, 18 Sep 2023 15:01:41 +0100 Subject: [PATCH] Fix local/remote assumptions in manila charm When the remote plugin was developed it made the assumption that the `enabled_share_backends` included the remote backends. This assumption is wrong, and it should only contain the locally defined drivers; i.e. ones that have a [driver] section in the manila.conf file. The cephfsnfs driver doesn't have a local configuration as the share-server is runs on the manila-ganesha unit and so shouldn't be local. This patch splits the config in local/remote and then uses the local and remote config in the correct places. Closes-Bug: #2012457 Change-Id: I436452208aaeaf08d1655da4fccbd7a89549b404 --- src/lib/charm/openstack/manila.py | 83 +++++++++++++++---- src/templates/mitaka/manila.conf | 2 +- src/templates/pike/manila.conf | 2 +- src/templates/queens/manila.conf | 2 +- src/templates/rocky/manila.conf | 2 +- test-requirements.txt | 1 + unit_tests/test_lib_charm_openstack_manila.py | 72 +++++++++++++--- 7 files changed, 133 insertions(+), 31 deletions(-) diff --git a/src/lib/charm/openstack/manila.py b/src/lib/charm/openstack/manila.py index 700bad7..869d2b7 100644 --- a/src/lib/charm/openstack/manila.py +++ b/src/lib/charm/openstack/manila.py @@ -44,8 +44,10 @@ MANILA_LOGGING_CONF = MANILA_DIR + "logging.conf" MANILA_API_PASTE_CONF = MANILA_DIR + "api-paste.ini" MANILA_WEBSERVER_SITE = 'manila-api' MANILA_WSGI_CONF = '/etc/apache2/sites-available/manila-api.conf' -PLUGIN_RELATIONS = ("manila-plugin.available", - "remote-manila-plugin.available",) +LOCAL_PLUGIN_RELATION = "manila-plugin.available" +REMOTE_PLUGIN_RELATION = "remote-manila-plugin.available" +PLUGIN_RELATIONS = (LOCAL_PLUGIN_RELATION, + REMOTE_PLUGIN_RELATION,) # select the default release function and ssl feature charms_openstack.charm.use_defaults('charm.default-select-release') @@ -66,17 +68,21 @@ def strip_join(s, divider=" "): ### # Compute some options to help with template rendering +### + @charms_openstack.adapters.config_property -def computed_share_backends(config): +def computed_local_share_backends(config): """Determine the backend protocols that are provided as a string. This asks the charm class what the backend protocols are, and then provides - it as a comma separated list of backends. + it as a comma separated list of backends for the local share service. This + does not include the remote backends (i.e. other non-local units running + manila-share.service) :param config: the config option on which to look up config options :returns: string """ - return ','.join(config.charm_instance.configured_backends) + return ','.join(config.charm_instance.configured_local_backends) @charms_openstack.adapters.config_property @@ -227,7 +233,9 @@ class ManilaCharm(charms_openstack.charm.HAOpenStackCharm): 'haproxy', 'manila-scheduler', 'manila-data'] - if not self.get_adapter('remote-manila-plugin.available'): + # BUG: #2012457: only start manila-share if a local share server is + # going to be configured. + if self.get_adapter(LOCAL_PLUGIN_RELATION): services.append('manila-share') return services @@ -267,7 +275,7 @@ class ManilaCharm(charms_openstack.charm.HAOpenStackCharm): there is a problem. Or (None, None) if there are no issues. """ options = self.options # tiny optimisation for less typing. - backends = options.computed_share_backends + backends = self.all_backends if not backends: return 'blocked', 'No share backends configured' default_share_backend = options.default_share_backend @@ -380,19 +388,40 @@ class ManilaCharm(charms_openstack.charm.HAOpenStackCharm): return super().internal_url + "/v2/%(tenant_id)s" @property - def configured_backends(self): + def configured_local_backends(self): """Return a list of configured backends that come from the associated 'manila-share.available' state.. + Note, that this mustn't contain the remote backends as they are share + server specific (e.g. running manila-share service) and if they are + included then the share-service wont' start. + :returns: list of strings: backend sections that are configured. """ # adapter.names is a property that provides a list of backend manila # plugin names for the sections + try: + # if self.local_adapter is None, the raises AttributeError when + # trying to access 'relation' + return sorted(set(self.local_manila_plugin_adapter.relation.names)) + except AttributeError: + return [] + @property + def all_backends(self): + """Return a list of all configured backends. + + This is obtained from both of the manila-share adapter and the + remote-manila-share adapter. + + :returns: list of strings: backend sections that are configured. + """ names = [] - for backend_relation in self.adapters: - names.extend(backend_relation.relation.names) - return sorted(list(set(names))) + for manila_plugin_adapter in [self.local_manila_plugin_adapter, + self.remote_manila_plugin_adapter]: + if manila_plugin_adapter: + names.extend(manila_plugin_adapter.relation.names) + return sorted(set(names)) def config_lines_for(self, config_file): """Return the list of configuration lines for `config_file` as returned @@ -415,7 +444,7 @@ class ManilaCharm(charms_openstack.charm.HAOpenStackCharm): """ config_lines = [] inverted_config_data = collections.defaultdict(dict) - for adapter in self.adapters: + for adapter in self.manila_plugin_adapters: # get the configuration data for all plugins config_data = adapter.relation.get_configuration_data() @@ -439,7 +468,7 @@ class ManilaCharm(charms_openstack.charm.HAOpenStackCharm): """ config_files = set() - for adapter in self.adapters: + for adapter in self.manila_plugin_adapters: # get the configuration data for all plugins config_data = adapter.relation.get_configuration_data() @@ -449,13 +478,37 @@ class ManilaCharm(charms_openstack.charm.HAOpenStackCharm): return list(config_files) @property - def adapters(self): + def manila_plugin_adapters(self): + """Return a list of manila-plugin adapters (local and remote). + + :returns: the list of adapters, if available. + :rtype: List[OpenStackRelationAdapter] + """ return [ adapter - for adapter in map(self.get_adapter, PLUGIN_RELATIONS) + for adapter in [self.local_manila_plugin_adapter, + self.remote_manila_plugin_adapter] if adapter is not None ] + @property + def local_manila_plugin_adapter(self): + """Return the local manila-plugin adapter or None. + + :returns: the adapters, if available. + :rtype: OpenStackRelationAdapter + """ + return self.get_adapter(LOCAL_PLUGIN_RELATION) + + @property + def remote_manila_plugin_adapter(self): + """Return the remote manila-plugin adapter or None. + + :returns: the adapters, if available. + :rtype: OpenStackRelationAdapter + """ + return self.get_adapter(REMOTE_PLUGIN_RELATION) + def enable_webserver_site(self): """Enable Manila API apache2 site if rendered or installed""" if os.path.exists(MANILA_WSGI_CONF): diff --git a/src/templates/mitaka/manila.conf b/src/templates/mitaka/manila.conf index c550462..0361d96 100644 --- a/src/templates/mitaka/manila.conf +++ b/src/templates/mitaka/manila.conf @@ -7,7 +7,7 @@ [DEFAULT] # This all needs to be configurable -enabled_share_backends = {{ options.computed_share_backends }} +enabled_share_backends = {{ options.computed_local_share_backends }} # enabled_share_protocols = NFS,CIFS enabled_share_protocols = {{ options.computed_share_protocols }} diff --git a/src/templates/pike/manila.conf b/src/templates/pike/manila.conf index b337392..b789c5b 100644 --- a/src/templates/pike/manila.conf +++ b/src/templates/pike/manila.conf @@ -7,7 +7,7 @@ [DEFAULT] # This all needs to be configurable -enabled_share_backends = {{ options.computed_share_backends }} +enabled_share_backends = {{ options.computed_local_share_backends }} # enabled_share_protocols = NFS,CIFS enabled_share_protocols = {{ options.computed_share_protocols }} diff --git a/src/templates/queens/manila.conf b/src/templates/queens/manila.conf index 0d1dbbe..3e569e4 100644 --- a/src/templates/queens/manila.conf +++ b/src/templates/queens/manila.conf @@ -7,7 +7,7 @@ [DEFAULT] # This all needs to be configurable -enabled_share_backends = {{ options.computed_share_backends }} +enabled_share_backends = {{ options.computed_local_share_backends }} # enabled_share_protocols = NFS,CIFS enabled_share_protocols = {{ options.computed_share_protocols }} diff --git a/src/templates/rocky/manila.conf b/src/templates/rocky/manila.conf index b337392..b789c5b 100644 --- a/src/templates/rocky/manila.conf +++ b/src/templates/rocky/manila.conf @@ -7,7 +7,7 @@ [DEFAULT] # This all needs to be configurable -enabled_share_backends = {{ options.computed_share_backends }} +enabled_share_backends = {{ options.computed_local_share_backends }} # enabled_share_protocols = NFS,CIFS enabled_share_protocols = {{ options.computed_share_protocols }} diff --git a/test-requirements.txt b/test-requirements.txt index a7936e6..bdb178e 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -6,6 +6,7 @@ pyparsing<3.0.0 # aodhclient is pinned in zaza and needs pyparsing < 3.0.0, but cffi also needs it, so pin here. setuptools<50.0.0 # https://github.com/pypa/setuptools/commit/04e3df22df840c6bb244e9b27bc56750c44b7c85 +testtools<2.6.0 stestr>=2.2.0 # Dependency of stestr. Workaround for diff --git a/unit_tests/test_lib_charm_openstack_manila.py b/unit_tests/test_lib_charm_openstack_manila.py index 201271d..e2c0032 100644 --- a/unit_tests/test_lib_charm_openstack_manila.py +++ b/unit_tests/test_lib_charm_openstack_manila.py @@ -47,10 +47,10 @@ class TestManilaCharmUtilities(Helper): class TestManilaCharmConfigProperties(Helper): - def test_computed_share_backends(self): + def test_computed_local_share_backends(self): config = mock.MagicMock() - config.charm_instance.configured_backends = ["a", "c", "b"] - self.assertEqual(manila.computed_share_backends(config), "a,c,b") + config.charm_instance.configured_local_backends = ["a", "c", "b"] + self.assertEqual(manila.computed_local_share_backends(config), "a,c,b") def test_computed_share_protocols(self): config = mock.MagicMock() @@ -107,7 +107,7 @@ class TestManilaCharm(Helper): def _patch_get_adapter(self, c, adapters=None): self.patch_object(c, 'get_adapter') if adapters is None: - adapters = ['remote-manila-plugin.available'] + adapters = ['manila-plugin.available'] def _helper(x): if x not in adapters: @@ -125,7 +125,7 @@ class TestManilaCharm(Helper): self._patch_get_adapter(c) self.out = None - self.assertEqual(c.configured_backends, []) + self.assertEqual(c.configured_local_backends, []) self.assertEqual(c.custom_assess_status_check(), ('blocked', 'No share backends configured')) self._patch_get_adapter(c) @@ -133,7 +133,7 @@ class TestManilaCharm(Helper): self.out.relation.names = ['name1'] self.assertEqual(c.custom_assess_status_check(), ('blocked', "'default-share-backend' is not set")) - self.assertEqual(self.var, 'remote-manila-plugin.available') + self.assertEqual(self.var, 'manila-plugin.available') def test_custom_assess_status_check2(self): config = { @@ -237,24 +237,36 @@ class TestManilaCharm(Helper): remote_adapter = mock.Mock() remote_adapter.relation.names = ['remote'] self.get_adapter.side_effect = [local_adapter, remote_adapter] - self.assertEqual(c.configured_backends, ['local', 'remote']) + # note that the ONLY local backends show up for the share-server + self.assertEqual(c.configured_local_backends, ['local']) - def test_configured_backends(self): + def test_configured_local_backends(self): c = self._patch_config_and_charm({}) self._patch_get_adapter(c) self.out = None - self.assertEqual(c.configured_backends, []) - self.assertEqual(self.var, 'remote-manila-plugin.available') + self.assertEqual(c.configured_local_backends, []) + self.assertEqual(self.var, 'manila-plugin.available') self.out = mock.Mock() self.out.relation.names = ['a', 'b'] - self.assertEqual(c.configured_backends, ['a', 'b']) + self.assertEqual(c.configured_local_backends, ['a', 'b']) + + def test_all_backends(self): + c = self._patch_config_and_charm({}) + self.patch_object(c, 'get_adapter') + local_adapter = mock.Mock() + local_adapter.relation.names = ['local'] + remote_adapter = mock.Mock() + remote_adapter.relation.names = ['remote'] + self.get_adapter.side_effect = [local_adapter, remote_adapter] + # note that the ONLY local backends show up for the share-server + self.assertEqual(c.all_backends, ['local', 'remote']) def test_config_lines_for(self): c = self._patch_config_and_charm({}) self._patch_get_adapter(c) self.out = None self.assertEqual(c.config_lines_for('conf'), []) - self.assertEqual(self.var, 'remote-manila-plugin.available') + self.assertEqual(self.var, 'manila-plugin.available') self.out = mock.Mock() self.out.relation.get_configuration_data.return_value = {} self.assertEqual(c.config_lines_for('conf'), []) @@ -288,3 +300,39 @@ class TestManilaCharm(Helper): self.NRPE.assert_has_calls([ mock.call().write(), ]) + + def test_manila_plugin_adapters__local(self): + c = self._patch_config_and_charm({}) + self.patch_object(c, 'get_adapter') + local_adapter = mock.Mock() + local_adapter.relation.names = ['local'] + self.get_adapter.side_effect = [local_adapter, None] + self.assertEqual(c.manila_plugin_adapters, [local_adapter]) + self.get_adapter.assert_has_calls([ + mock.call(manila.LOCAL_PLUGIN_RELATION), + mock.call(manila.REMOTE_PLUGIN_RELATION)]) + + def test_manila_plugin_adapters__remote(self): + c = self._patch_config_and_charm({}) + self.patch_object(c, 'get_adapter') + remote_adapter = mock.Mock() + remote_adapter.relation.names = ['remote'] + self.get_adapter.side_effect = [None, remote_adapter] + self.assertEqual(c.manila_plugin_adapters, [remote_adapter]) + self.get_adapter.assert_has_calls([ + mock.call(manila.LOCAL_PLUGIN_RELATION), + mock.call(manila.REMOTE_PLUGIN_RELATION)]) + + def test_manila_plugin_adapters__both(self): + c = self._patch_config_and_charm({}) + self.patch_object(c, 'get_adapter') + local_adapter = mock.Mock() + local_adapter.relation.names = ['local'] + remote_adapter = mock.Mock() + remote_adapter.relation.names = ['remote'] + self.get_adapter.side_effect = [local_adapter, remote_adapter] + self.assertEqual(c.manila_plugin_adapters, + [local_adapter, remote_adapter]) + self.get_adapter.assert_has_calls([ + mock.call(manila.LOCAL_PLUGIN_RELATION), + mock.call(manila.REMOTE_PLUGIN_RELATION)])