diff --git a/lib/charms/opensearch/v0/opensearch_plugin_manager.py b/lib/charms/opensearch/v0/opensearch_plugin_manager.py index ab7814701..4d63809e4 100644 --- a/lib/charms/opensearch/v0/opensearch_plugin_manager.py +++ b/lib/charms/opensearch/v0/opensearch_plugin_manager.py @@ -11,7 +11,7 @@ """ import logging -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, Optional, Tuple from charms.opensearch.v0.constants_charm import PluginConfigStart from charms.opensearch.v0.helper_cluster import ClusterTopology @@ -256,6 +256,42 @@ def _disable_if_needed(self, plugin: OpenSearchPlugin) -> bool: except KeyError as e: raise OpenSearchPluginMissingConfigError(e) + def _compute_settings( + self, config: OpenSearchPluginConfig + ) -> Tuple[Dict[str, str], Dict[str, str]]: + """Returns the current and the new configuration.""" + current_settings = ClusterTopology.get_cluster_settings( + self._charm.opensearch, + include_defaults=True, + ) + to_remove = dict( + zip(config.config_entries_to_del, [None] * len(config.config_entries_to_del)) + ) + new_conf = { + **current_settings, + **to_remove, + **config.config_entries_to_add, + } + logger.debug( + "Difference between current and new configuration: \n" + + str( + { + "in current but not in new_conf": { + k: v for k, v in current_settings.items() if k not in new_conf.keys() + }, + "in new_conf but not in current": { + k: v for k, v in new_conf.items() if k not in current_settings.keys() + }, + "in both but different values": { + k: v + for k, v in new_conf.items() + if k in current_settings.keys() and current_settings[k] != v + }, + } + ) + ) + return current_settings, new_conf + def apply_config(self, config: OpenSearchPluginConfig) -> bool: """Runs the configuration changes as passed via OpenSearchPluginConfig. @@ -270,39 +306,7 @@ def apply_config(self, config: OpenSearchPluginConfig) -> bool: if config.secret_entries_to_del or config.secret_entries_to_add: self._keystore.reload_keystore() - # Add and remove configuration if applies - current_settings = ClusterTopology.get_cluster_settings( - self._charm.opensearch, - include_defaults=True, - ) - to_remove = config.config_entries_to_del - if isinstance(config.config_entries_to_del, list): - # No keys should have value "None", therefore, setting them to None means - # the original current_settings will be changed. - to_remove = dict( - zip(config.config_entries_to_del, [None] * len(config.config_entries_to_del)) - ) - - new_conf = { - **current_settings, - **to_remove, - **config.config_entries_to_add, - } - - diffs = { - "in current but not in new_conf": { - k: v for k, v in current_settings.items() if k not in new_conf.keys() - }, - "in new_conf but not in current": { - k: v for k, v in new_conf.items() if k not in current_settings.keys() - }, - "in both but different values": { - k: v - for k, v in new_conf.items() - if k in current_settings.keys() and current_settings[k] != v - }, - } - logger.debug(f"Difference between current and new configuration: \n{diffs}") + current_settings, new_conf = self._compute_settings(config) if current_settings == new_conf: # Nothing to do here logger.info("apply_config: nothing to do, return") @@ -344,32 +348,28 @@ def _user_requested_to_enable(self, plugin: OpenSearchPlugin) -> bool: def _is_enabled(self, plugin: OpenSearchPlugin) -> bool: """Returns true if plugin is enabled. - The main question to answer is if we would remove the configuration from - opensearch.yaml and/or add some other configuration instead. - If yes, then we know that the service is enabled. + The main question to answer is if we would have it in the configuration + from cluster settings. If yes, then we know that the service is enabled. - The disable() method is used, as a given entry will be removed from opensearch.yml. - - If disable() returns a list, then check if the keys in the stored_plugin_conf - matches the list values; otherwise, if a dict, then check if the keys and values match. - If they match in any of the cases above, then return True. + Check if the configuration from enable() is present or not. """ try: - plugin_conf = plugin.disable().config_entries_to_del - keystore_conf = plugin.disable().secret_entries_to_del - stored_plugin_conf = self._opensearch_config.get_plugin(plugin_conf) - - if any(k not in self._keystore.list() for k in keystore_conf): + current_settings, new_conf = self._compute_settings(plugin.config()) + if current_settings != new_conf: return False - # Using sets to guarantee matches; stored_plugin_conf will be a dict - if isinstance(plugin_conf, list): - return set(plugin_conf) == set(stored_plugin_conf) - else: # plugin_conf is a dict - return plugin_conf == stored_plugin_conf + # Now, focus in on the keystore part + keys_available = self._keystore.list() + keys_to_add = plugin.config().secret_entries_to_add + if any(k not in keys_available for k in keys_to_add): + return False + keys_to_del = plugin.config().secret_entries_to_del + if any(k in keys_available for k in keys_to_del): + return False except (KeyError, OpenSearchPluginError) as e: logger.warning(f"_is_enabled: error with {e}") return False + return True def _needs_upgrade(self, plugin: OpenSearchPlugin) -> bool: """Returns true if plugin needs upgrade.""" diff --git a/tests/integration/plugins/test_plugins.py b/tests/integration/plugins/test_plugins.py index 1c9ec5aa2..da209abf6 100644 --- a/tests/integration/plugins/test_plugins.py +++ b/tests/integration/plugins/test_plugins.py @@ -79,6 +79,7 @@ async def test_build_and_deploy(ops_test: OpsTest) -> None: assert len(ops_test.model.applications[APP_NAME].units) == 3 +@pytest.mark.abort_on_fail async def test_prometheus_exporter_enabled_by_default(ops_test): """Test that Prometheus Exporter is running before the relation is there.""" leader_unit_ip = await get_leader_unit_ip(ops_test, app=APP_NAME) @@ -90,6 +91,7 @@ async def test_prometheus_exporter_enabled_by_default(ops_test): assert len(response_str.split("\n")) > 500 +@pytest.mark.abort_on_fail async def test_prometheus_exporter_cos_relation(ops_test): await ops_test.model.deploy(COS_APP_NAME, channel="edge"), await ops_test.model.integrate(APP_NAME, COS_APP_NAME) @@ -119,6 +121,7 @@ async def test_prometheus_exporter_cos_relation(ops_test): assert relation_data["scheme"] == "https" +@pytest.mark.abort_on_fail async def test_monitoring_user_fetch_prometheus_data(ops_test): leader_unit_ip = await get_leader_unit_ip(ops_test, app=APP_NAME) endpoint = f"https://{leader_unit_ip}:9200/_prometheus/metrics" @@ -139,6 +142,7 @@ async def test_monitoring_user_fetch_prometheus_data(ops_test): assert len(response_str.split("\n")) > 500 +@pytest.mark.abort_on_fail async def test_prometheus_monitor_user_password_change(ops_test): # Password change applied as expected leader_id = await get_leader_unit_id(ops_test, APP_NAME) @@ -161,6 +165,7 @@ async def test_prometheus_monitor_user_password_change(ops_test): assert relation_data["password"] == new_password +@pytest.mark.abort_on_fail async def test_knn_enabled_disabled(ops_test): config = await ops_test.model.applications[APP_NAME].get_config() assert config["plugin_opensearch_knn"]["default"] is True @@ -177,6 +182,7 @@ async def test_knn_enabled_disabled(ops_test): config = await ops_test.model.applications[APP_NAME].get_config() assert config["plugin_opensearch_knn"]["value"] is True + await ops_test.model.wait_for_idle(apps=[APP_NAME], status="active", idle_period=45) @pytest.mark.abort_on_fail @@ -218,7 +224,7 @@ async def test_knn_search_with_hnsw_faiss(ops_test: OpsTest) -> None: # Insert data in bulk await bulk_insert(ops_test, app, leader_unit_ip, payload) query = {"size": 2, "query": {"knn": {vector_name: {"vector": payload_list[0], "k": 2}}}} - docs = await search(ops_test, app, leader_unit_ip, index_name, query) + docs = await search(ops_test, app, leader_unit_ip, index_name, query, retries=30) assert len(docs) == 2 @@ -261,7 +267,7 @@ async def test_knn_search_with_hnsw_nmslib(ops_test: OpsTest) -> None: # Insert data in bulk await bulk_insert(ops_test, app, leader_unit_ip, payload) query = {"size": 2, "query": {"knn": {vector_name: {"vector": payload_list[0], "k": 2}}}} - docs = await search(ops_test, app, leader_unit_ip, index_name, query) + docs = await search(ops_test, app, leader_unit_ip, index_name, query, retries=30) assert len(docs) == 2