From cfdc0181084c6243df33c0afac88d0f71b7d76da Mon Sep 17 00:00:00 2001 From: Pedro Guimaraes Date: Thu, 29 Feb 2024 11:59:47 +0100 Subject: [PATCH] Add /cluster/settings support and fix plugin status --- lib/charms/opensearch/v0/helper_cluster.py | 17 +++ .../opensearch/v0/opensearch_backups.py | 13 +- .../opensearch/v0/opensearch_base_charm.py | 19 +-- .../v0/opensearch_plugin_manager.py | 127 ++++++++++++------ .../opensearch/v0/opensearch_plugins.py | 58 ++++---- .../v0/opensearch_relation_provider.py | 22 +-- .../integration/ha/test_large_deployments.py | 4 + tests/integration/plugins/test_plugins.py | 10 +- tests/unit/lib/test_backups.py | 12 +- tests/unit/lib/test_helper_cluster.py | 35 +++++ tests/unit/lib/test_ml_plugins.py | 14 +- tests/unit/lib/test_plugins.py | 48 +++++-- 12 files changed, 261 insertions(+), 118 deletions(-) diff --git a/lib/charms/opensearch/v0/helper_cluster.py b/lib/charms/opensearch/v0/helper_cluster.py index af60ffa66..1b2baf779 100644 --- a/lib/charms/opensearch/v0/helper_cluster.py +++ b/lib/charms/opensearch/v0/helper_cluster.py @@ -57,6 +57,23 @@ def suggest_roles(nodes: List[Node], planned_units: int) -> List[str]: return base_roles + ["cluster_manager"] + @staticmethod + def get_cluster_settings( + opensearch: OpenSearchDistribution, + host: Optional[str] = None, + alt_hosts: Optional[List[str]] = None, + include_defaults: bool = False, + ) -> Dict[str, any]: + """Get the cluster settings.""" + settings = opensearch.request( + "GET", + f"/_cluster/settings?flat_settings=true&include_defaults={str(include_defaults).lower()}", + host=host, + alt_hosts=alt_hosts, + ) + + return dict(settings["defaults"] | settings["persistent"] | settings["transient"]) + @staticmethod def recompute_nodes_conf(app_name: str, nodes: List[Node]) -> Dict[str, Node]: """Recompute the configuration of all the nodes (cluster set to auto-generate roles).""" diff --git a/lib/charms/opensearch/v0/opensearch_backups.py b/lib/charms/opensearch/v0/opensearch_backups.py index 7da706816..47267e1e5 100644 --- a/lib/charms/opensearch/v0/opensearch_backups.py +++ b/lib/charms/opensearch/v0/opensearch_backups.py @@ -3,8 +3,8 @@ """OpenSearch Backup. -This file holds the implementation of the OpenSearchBackup, OpenSearchBackupPlugin classes -as well as the configuration and state enum. +This file holds the implementation of the OpenSearchBackup class, as well as the state enum +and configuration. The OpenSearchBackup class listens to both relation changes from S3_RELATION and API calls and responses. The OpenSearchBackupPlugin holds the configuration info. The classes together @@ -467,7 +467,7 @@ def _query_backup_status(self, backup_id=None) -> BackupServiceState: return BackupServiceState.RESPONSE_FAILED_NETWORK return self.get_service_status(output) - def _on_s3_credentials_changed(self, event: EventBase) -> None: + def _on_s3_credentials_changed(self, event: EventBase) -> None: # noqa: C901 """Calls the plugin manager config handler. This method will iterate over the s3 relation and check: @@ -489,8 +489,14 @@ def _on_s3_credentials_changed(self, event: EventBase) -> None: self.charm.status.set(MaintenanceStatus(BackupSetupStart)) try: + if not self.charm.plugin_manager.check_plugin_manager_ready(): + raise OpenSearchNotFullyReadyError() + plugin = self.charm.plugin_manager.get_plugin(OpenSearchBackupPlugin) if self.charm.plugin_manager.status(plugin) == PluginState.ENABLED: + # We need to explicitly disable the plugin before reconfiguration + # That happens because, differently from the actual configs, we cannot + # retrieve the key values and check if they changed. self.charm.plugin_manager.apply_config(plugin.disable()) self.charm.plugin_manager.apply_config(plugin.config()) except OpenSearchError as e: @@ -507,6 +513,7 @@ def _on_s3_credentials_changed(self, event: EventBase) -> None: PluginState.ENABLED, PluginState.WAITING_FOR_UPGRADE, ]: + logger.warning("_on_s3_credentials_changed: plugin is not enabled.") event.defer() return diff --git a/lib/charms/opensearch/v0/opensearch_base_charm.py b/lib/charms/opensearch/v0/opensearch_base_charm.py index 90b33e3dc..8bc29efe7 100644 --- a/lib/charms/opensearch/v0/opensearch_base_charm.py +++ b/lib/charms/opensearch/v0/opensearch_base_charm.py @@ -493,22 +493,21 @@ def _on_config_changed(self, event: ConfigChangedEvent): event.defer() return - self.status.set(MaintenanceStatus(PluginConfigStart)) try: + self.status.set(MaintenanceStatus(PluginConfigStart)) if self.plugin_manager.run(): self.on[self.service_manager.name].acquire_lock.emit( callback_override="_restart_opensearch" ) - except OpenSearchNotFullyReadyError: - logger.warning("Plugin management: cluster not ready yet at config changed") - event.defer() - return - except OpenSearchPluginError: - self.status.set(BlockedStatus(PluginConfigChangeError)) + self.status.clear(PluginConfigStart) + except (OpenSearchNotFullyReadyError, OpenSearchPluginError) as e: + if isinstance(e, OpenSearchNotFullyReadyError): + logger.warning("Plugin management: cluster not ready yet at config changed") + else: + self.status.set(BlockedStatus(PluginConfigChangeError)) event.defer() return self.status.clear(PluginConfigChangeError) - self.status.clear(PluginConfigStart) def _on_set_password_action(self, event: ActionEvent): """Set new admin password from user input or generate if not passed.""" @@ -619,7 +618,6 @@ def _start_opensearch(self, event: EventBase) -> None: # noqa: C901 event.defer() self.defer_trigger_event.emit() return - if not self._can_service_start(): self.peers_data.delete(Scope.UNIT, "starting") event.defer() @@ -632,9 +630,7 @@ def _start_opensearch(self, event: EventBase) -> None: # noqa: C901 self.peers_data.delete(Scope.UNIT, "starting") event.defer() return - self.unit.status = WaitingStatus(WaitingToStart) - rel = self.model.get_relation(PeerRelationName) for unit in rel.units.union({self.unit}): if rel.data[unit].get("starting") == "True": @@ -646,7 +642,6 @@ def _start_opensearch(self, event: EventBase) -> None: # noqa: C901 try: # Retrieve the nodes of the cluster, needed to configure this node nodes = self._get_nodes(False) - # validate the roles prior to starting self.opensearch_peer_cm.validate_roles(nodes, on_new_unit=True) diff --git a/lib/charms/opensearch/v0/opensearch_plugin_manager.py b/lib/charms/opensearch/v0/opensearch_plugin_manager.py index 256c5c6b2..7dc737f13 100644 --- a/lib/charms/opensearch/v0/opensearch_plugin_manager.py +++ b/lib/charms/opensearch/v0/opensearch_plugin_manager.py @@ -10,9 +10,12 @@ config-changed, upgrade, s3-credentials-changed, etc. """ +import copy +import functools import logging -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, Optional, Tuple +from charms.opensearch.v0.helper_cluster import ClusterTopology from charms.opensearch.v0.opensearch_exceptions import ( OpenSearchCmdError, OpenSearchNotFullyReadyError, @@ -78,6 +81,11 @@ def __init__(self, charm): self._keystore = OpenSearchKeystore(self._charm) self._event_scope = OpenSearchPluginEventScope.DEFAULT + @functools.cached_property + def cluster_config(self): + """Returns the cluster configuration.""" + return ClusterTopology.get_cluster_settings(self._charm.opensearch, include_defaults=True) + def set_event_scope(self, event_scope: OpenSearchPluginEventScope) -> None: """Sets the event scope of the plugin manager. @@ -129,25 +137,31 @@ def _extra_conf(self, plugin_data: Dict[str, Any]) -> Optional[Dict[str, Any]]: } return {**self._charm_config, "opensearch-version": self._opensearch.version} + def check_plugin_manager_ready(self) -> bool: + """Checks if the plugin manager is ready to run.""" + return ( + self._charm.opensearch.is_node_up() + and len(self._charm._get_nodes(True)) == self._charm.app.planned_units() + and self._charm.health.apply() + in [ + HealthColors.GREEN, + HealthColors.YELLOW, + ] + ) + def run(self) -> bool: """Runs a check on each plugin: install, execute config changes or remove. This method should be called at config-changed event. Returns if needed restart. """ - if not self._charm.opensearch.is_started() or self._charm.health.apply() not in [ - HealthColors.GREEN, - HealthColors.YELLOW, - ]: - # If the health is not green, then raise a cluster-not-ready error - # The classes above should then defer their own events in waiting. - # Defer is important as next steps to configure plugins will involve - # calls to the APIs of the cluster. - logger.info("Cluster not ready, wait for the next event...") + if not self.check_plugin_manager_ready(): raise OpenSearchNotFullyReadyError() err_msgs = [] restart_needed = False for plugin in self.plugins: + logger.info(f"Checking plugin {plugin.name}...") + logger.debug(f"Status: {self.status(plugin)}") # These are independent plugins. # Any plugin that errors, if that is an OpenSearchPluginError, then # it is an "expected" error, such as missing additional config; should @@ -172,8 +186,8 @@ def run(self) -> bool: # This is a more serious issue, as we are missing some input from # the user. The charm should block. err_msgs.append(str(e)) - except OpenSearchPluginError as e: - logger.error(f"Plugin {plugin.name} failed: {str(e)}") + logger.debug(f"Finished Plugin {plugin.name} status: {self.status(plugin)}") + logger.info(f"Plugin check finished, restart needed: {restart_needed}") if err_msgs: raise OpenSearchPluginError("\n".join(err_msgs)) @@ -255,6 +269,39 @@ def _disable_if_needed(self, plugin: OpenSearchPlugin) -> bool: except KeyError as e: raise OpenSearchPluginMissingConfigError(plugin.name, configs=[f"{e}"]) + def _compute_settings( + self, config: OpenSearchPluginConfig + ) -> Tuple[Dict[str, str], Dict[str, str]]: + """Returns the current and the new configuration.""" + current_settings = self.cluster_config + # We use current_settings and new_conf and check for any differences + # therefore, we need to make a deepcopy here before editing new_conf + new_conf = copy.deepcopy(current_settings) + for key_to_del in config.config_entries_to_del: + if key_to_del in new_conf: + del new_conf[key_to_del] + new_conf |= 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. @@ -262,27 +309,25 @@ def apply_config(self, config: OpenSearchPluginConfig) -> bool: 1) Remove the entries to be deleted 2) Add entries, if available - For example: - KNN needs to: - 1) Remove from configuration: {"knn.plugin.enabled": "True"} - 2) Add to configuration: {"knn.plugin.enabled": "False"} - Returns True if a configuration change was performed. """ self._keystore.delete(config.secret_entries_to_del) self._keystore.add(config.secret_entries_to_add) - # Add and remove configuration if applies + if config.secret_entries_to_del or config.secret_entries_to_add: + self._keystore.reload_keystore() + + 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") + return False + + # Update the configuration if config.config_entries_to_del: self._opensearch_config.delete_plugin(config.config_entries_to_del) - if config.config_entries_to_add: self._opensearch_config.add_plugin(config.config_entries_to_add) - - if config.secret_entries_to_del or config.secret_entries_to_add: - self._keystore.reload_keystore() - - # Return True if some configuration entries changed - return True if config.config_entries_to_add or config.config_entries_to_del else False + return True def status(self, plugin: OpenSearchPlugin) -> PluginState: """Returns the status for a given plugin.""" @@ -320,32 +365,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 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/lib/charms/opensearch/v0/opensearch_plugins.py b/lib/charms/opensearch/v0/opensearch_plugins.py index 52b333d9a..1cf4c594e 100644 --- a/lib/charms/opensearch/v0/opensearch_plugins.py +++ b/lib/charms/opensearch/v0/opensearch_plugins.py @@ -277,6 +277,7 @@ def _on_update_status(self, event): from charms.opensearch.v0.helper_enums import BaseStrEnum from charms.opensearch.v0.opensearch_exceptions import OpenSearchError from jproperties import Properties +from pydantic import BaseModel, validator # The unique Charmhub library identifier, never change it LIBID = "3b05456c6e304680b4af8e20dae246a2" @@ -331,20 +332,28 @@ class PluginState(BaseStrEnum): WAITING_FOR_UPGRADE = "waiting-for-upgrade" -class OpenSearchPluginConfig: - """Represent the configuration of a plugin to be applied when configuring or disabling it.""" +class OpenSearchPluginConfig(BaseModel): + """Represent the configuration of a plugin to be applied when configuring or disabling it. - def __init__( - self, - config_entries_to_add: Optional[Dict[str, str]] = None, - config_entries_to_del: Optional[List[str]] = None, - secret_entries_to_add: Optional[Dict[str, str]] = None, - secret_entries_to_del: Optional[List[str]] = None, - ): - self.config_entries_to_add = config_entries_to_add or {} - self.config_entries_to_del = config_entries_to_del or [] - self.secret_entries_to_add = secret_entries_to_add or {} - self.secret_entries_to_del = secret_entries_to_del or [] + The config may receive any type of data, but will convert everything to strings and + pay attention to special types, such as booleans, which need to be "true" or "false". + """ + + config_entries_to_add: Optional[Dict[str, str]] = {} + config_entries_to_del: Optional[List[str]] = [] + secret_entries_to_add: Optional[Dict[str, str]] = {} + secret_entries_to_del: Optional[List[str]] = [] + + @validator("config_entries_to_add", "secret_entries_to_add", allow_reuse=True, pre=True) + def convert_values_to_add(cls, conf) -> Dict[str, str]: # noqa N805 + """Converts the object to a dictionary. + + Respects the conversion for boolean to {"true", "false"}. + """ + return { + key: str(val).lower() if isinstance(val, bool) else str(val) + for key, val in conf.items() + } class OpenSearchPlugin: @@ -389,9 +398,9 @@ def config(self) -> OpenSearchPluginConfig: Format: OpenSearchPluginConfig( config_entries_to_add = {...}, - config_entries_to_del = [...]|{...}, + config_entries_to_del = [...], secret_entries_to_add = {...}, - secret_entries_to_del = [...]|{...}, + secret_entries_to_del = [...], ) May throw KeyError if accessing some source, such as self._extra_config, but the @@ -406,9 +415,9 @@ def disable(self) -> OpenSearchPluginConfig: Format: OpenSearchPluginConfig( config_entries_to_add = {...}, - config_entries_to_del = [...]|{...}, + config_entries_to_del = [...], secret_entries_to_add = {...}, - secret_entries_to_del = [...]|{...}, + secret_entries_to_del = [...], ) May throw KeyError if accessing some source, such as self._extra_config, but the @@ -429,14 +438,12 @@ def config(self) -> OpenSearchPluginConfig: """Returns a plugin config object to be applied for enabling the current plugin.""" return OpenSearchPluginConfig( config_entries_to_add={"knn.plugin.enabled": True}, - config_entries_to_del={"knn.plugin.enabled": False}, ) def disable(self) -> OpenSearchPluginConfig: """Returns a plugin config object to be applied for disabling the current plugin.""" return OpenSearchPluginConfig( config_entries_to_add={"knn.plugin.enabled": False}, - config_entries_to_del={"knn.plugin.enabled": True}, ) @property @@ -462,9 +469,9 @@ def config(self) -> OpenSearchPluginConfig: Format: OpenSearchPluginConfig( config_entries_to_add = {...}, - config_entries_to_del = [...]|{...}, + config_entries_to_del = [...], secret_entries_to_add = {...}, - secret_entries_to_del = [...]|{...}, + secret_entries_to_del = [...], ) """ if not self._extra_config.get("access-key") or not self._extra_config.get("secret-key"): @@ -480,11 +487,6 @@ def config(self) -> OpenSearchPluginConfig: ) return OpenSearchPluginConfig( - # Try to remove the previous values - secret_entries_to_del=[ - "s3.client.default.access_key", - "s3.client.default.secret_key", - ], secret_entries_to_add={ # Remove any entries with None value k: v @@ -502,9 +504,9 @@ def disable(self) -> OpenSearchPluginConfig: Format: OpenSearchPluginConfig( config_entries_to_add = {...}, - config_entries_to_del = [...]|{...}, + config_entries_to_del = [...], secret_entries_to_add = {...}, - secret_entries_to_del = [...]|{...}, + secret_entries_to_del = [...], ) """ return OpenSearchPluginConfig( diff --git a/lib/charms/opensearch/v0/opensearch_relation_provider.py b/lib/charms/opensearch/v0/opensearch_relation_provider.py index 570d90f99..6fbbbf068 100644 --- a/lib/charms/opensearch/v0/opensearch_relation_provider.py +++ b/lib/charms/opensearch/v0/opensearch_relation_provider.py @@ -328,18 +328,18 @@ def update_certs(self, relation_id, ca_chain=None): """ if not self.unit.is_leader(): # We're updating app databags in this function, so it can't be called on follower - # units. + # units. This is not checked in `set_tls_ca`, in data-interfaces. return - if not ca_chain: - try: - ca_chain = self.charm.secrets.get_object(Scope.APP, CertType.APP_ADMIN.val).get( - "chain" - ) - except AttributeError: - # cert doesn't exist - presumably we don't yet have a TLS relation. - return - - self.opensearch_provides.set_tls_ca(relation_id, ca_chain) + try: + # If the ca_chain=None, then we try to load the CA from the app-admin secret. + _ch_chain = ca_chain or self.charm.secrets.get_object( + Scope.APP, CertType.APP_ADMIN.val + ).get("chain") + except AttributeError: + # cert doesn't exist - presumably we don't yet have a TLS relation. + logger.warning("unable to get ca_chain") + return + self.opensearch_provides.set_tls_ca(relation_id, _ch_chain) def _on_relation_changed(self, event: RelationChangedEvent) -> None: if not self.unit.is_leader(): diff --git a/tests/integration/ha/test_large_deployments.py b/tests/integration/ha/test_large_deployments.py index 0eec7a8dc..7326fb29e 100644 --- a/tests/integration/ha/test_large_deployments.py +++ b/tests/integration/ha/test_large_deployments.py @@ -120,6 +120,8 @@ async def test_set_roles_manually( assert node.temperature is None, "Node temperature was erroneously set." # change cluster name and roles + temperature, should trigger a rolling restart + + logger.info("Changing cluster name and roles + temperature.") await ops_test.model.applications[app].set_config( {"cluster_name": "new_cluster_name", "roles": "cluster_manager, data.cold"} ) @@ -131,6 +133,8 @@ async def test_set_roles_manually( wait_for_exact_units=len(nodes), idle_period=IDLE_PERIOD, ) + + logger.info("Checking if the cluster name and roles + temperature were changed.") assert await check_cluster_formation_successful( ops_test, leader_unit_ip, get_application_unit_names(ops_test, app=app) ) 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 diff --git a/tests/unit/lib/test_backups.py b/tests/unit/lib/test_backups.py index 6f09fad52..9c77b97cf 100644 --- a/tests/unit/lib/test_backups.py +++ b/tests/unit/lib/test_backups.py @@ -490,12 +490,18 @@ def test_get_endpoint_protocol(self) -> None: assert self.charm.backup._get_endpoint_protocol("https://10.0.0.2:8000") == "https" assert self.charm.backup._get_endpoint_protocol("test.not-valid-url") == "https" + @patch( + "charms.opensearch.v0.opensearch_plugin_manager.OpenSearchPluginManager.check_plugin_manager_ready" + ) @patch("charms.opensearch.v0.opensearch_plugin_manager.OpenSearchPluginManager.status") @patch("charms.opensearch.v0.opensearch_backups.OpenSearchBackup.apply_api_config_if_needed") @patch("charms.opensearch.v0.opensearch_plugin_manager.OpenSearchPluginManager.apply_config") @patch("charms.opensearch.v0.opensearch_distro.OpenSearchDistribution.version") - def test_00_update_relation_data(self, __, mock_apply_config, _, mock_status) -> None: + def test_00_update_relation_data( + self, __, mock_apply_config, _, mock_status, mock_pm_ready + ) -> None: """Tests if new relation without data returns.""" + mock_pm_ready.return_value = True mock_status.return_value = PluginState.INSTALLED self.harness.update_relation_data( self.s3_rel_id, @@ -513,10 +519,6 @@ def test_00_update_relation_data(self, __, mock_apply_config, _, mock_status) -> assert ( mock_apply_config.call_args[0][0].__dict__ == OpenSearchPluginConfig( - secret_entries_to_del=[ - "s3.client.default.access_key", - "s3.client.default.secret_key", - ], secret_entries_to_add={ "s3.client.default.access_key": "aaaa", "s3.client.default.secret_key": "bbbb", diff --git a/tests/unit/lib/test_helper_cluster.py b/tests/unit/lib/test_helper_cluster.py index 9e1ba10ab..93eca237d 100644 --- a/tests/unit/lib/test_helper_cluster.py +++ b/tests/unit/lib/test_helper_cluster.py @@ -241,3 +241,38 @@ def test_node_obj_creation_from_json(self): self.assertEqual(raw_node.name, from_json_node.name) self.assertEqual(raw_node.roles, from_json_node.roles) self.assertEqual(raw_node.ip, from_json_node.ip) + + @patch("charms.opensearch.v0.helper_cluster.OpenSearchDistribution.request") + def test_get_cluster_settings(self, request_mock): + """Test the get_cluster_settings method.""" + request_mock.return_value = { + "defaults": { + "knn.plugin.enabled": "false", + }, + "persistent": { + "knn.plugin.enabled": "true", + "cluster.routing.allocation.enable": "all", + }, + "transient": { + "indices.recovery.max_bytes_per_sec": "50mb", + }, + } + + expected_settings = { + "knn.plugin.enabled": "true", + "cluster.routing.allocation.enable": "all", + "indices.recovery.max_bytes_per_sec": "50mb", + } + + settings = ClusterTopology.get_cluster_settings( + self.opensearch, + include_defaults=True, + ) + + self.assertEqual(settings, expected_settings) + request_mock.assert_called_once_with( + "GET", + "/_cluster/settings?flat_settings=true&include_defaults=true", + host=None, + alt_hosts=None, + ) diff --git a/tests/unit/lib/test_ml_plugins.py b/tests/unit/lib/test_ml_plugins.py index d1aa9e1c2..ac26c77e4 100644 --- a/tests/unit/lib/test_ml_plugins.py +++ b/tests/unit/lib/test_ml_plugins.py @@ -54,7 +54,12 @@ def setUp(self) -> None: } self.charm.opensearch.is_started = MagicMock(return_value=True) self.charm.health.apply = MagicMock(return_value=HealthColors.GREEN) + self.plugin_manager._is_cluster_ready = MagicMock(return_value=True) + charms.opensearch.v0.helper_cluster.ClusterTopology.get_cluster_settings = MagicMock( + return_value={} + ) + @patch(f"{BASE_LIB_PATH}.opensearch_distro.OpenSearchDistribution.is_node_up") @patch( f"{BASE_LIB_PATH}.opensearch_peer_clusters.OpenSearchPeerClustersManager.deployment_desc" ) @@ -78,6 +83,7 @@ def test_disable_via_config_change( mock_version, mock_acquire_lock, ___, + mock_is_node_up, ) -> None: """Tests entire config_changed event with KNN plugin.""" mock_status.return_value = PluginState.ENABLED @@ -89,12 +95,12 @@ def test_disable_via_config_change( self.plugin_manager._opensearch_config.delete_plugin = MagicMock() self.plugin_manager._opensearch_config.add_plugin = MagicMock() self.charm.status = MagicMock() + mock_is_node_up.return_value = True + self.charm._get_nodes = MagicMock(return_value=[1]) + self.charm.planned_units = MagicMock(return_value=1) self.harness.update_config({"plugin_opensearch_knn": False}) mock_acquire_lock.assert_called_once() - self.plugin_manager._opensearch_config.delete_plugin.assert_called_once_with( - {"knn.plugin.enabled": True} - ) self.plugin_manager._opensearch_config.add_plugin.assert_called_once_with( - {"knn.plugin.enabled": False} + {"knn.plugin.enabled": "false"} ) diff --git a/tests/unit/lib/test_plugins.py b/tests/unit/lib/test_plugins.py index d22d83b26..dd9b31242 100644 --- a/tests/unit/lib/test_plugins.py +++ b/tests/unit/lib/test_plugins.py @@ -122,6 +122,7 @@ def setUp(self) -> None: self.charm.opensearch.is_started = MagicMock(return_value=True) self.charm.health.apply = MagicMock(return_value=HealthColors.GREEN) self.charm.opensearch.version = "2.9.0" + self.plugin_manager._is_cluster_ready = MagicMock(return_value=True) @patch("charms.opensearch.v0.opensearch_plugin_manager.OpenSearchPluginManager._is_enabled") @patch("charms.opensearch.v0.opensearch_plugin_manager.OpenSearchPluginManager._is_installed") @@ -242,6 +243,16 @@ def test_reconfigure_and_add_keystore_plugin( # Mock _installed_plugins to return test mock_installed_plugins.return_value = ["test"] + self.charm._get_nodes = MagicMock( + return_value={ + "1": {}, + "2": {}, + "3": {}, + } + ) + self.charm.app.planned_units = MagicMock(return_value=3) + self.charm.opensearch.is_node_up = MagicMock(return_value=True) + mock_load.return_value = {} # run is called, but only _configure method really matter: # Set install to false, so only _configure is evaluated @@ -291,6 +302,16 @@ def test_plugin_setup_with_relation( # Return a fake content of the relation mock_process_relation.return_value = {"param": "tested"} + self.charm._get_nodes = MagicMock( + return_value={ + "1": {}, + "2": {}, + "3": {}, + } + ) + self.charm.app.planned_units = MagicMock(return_value=3) + self.charm.opensearch.is_node_up = MagicMock(return_value=True) + # Keystore-related mocks self.plugin_manager._keystore._add = MagicMock() self.plugin_manager._opensearch.request = MagicMock(return_value={"status": 200}) @@ -311,11 +332,13 @@ def test_plugin_setup_with_relation( mock_plugin_relation.return_value = True # plugin is initially disabled and enabled when method self._disable calls self.status mock_is_enabled.side_effect = [ + False, # called by logger False, # called by self.status, in self._install False, # called by self._configure True, # called by self.status, in self._disable + True, # called by logger ] - + charms.opensearch.v0.opensearch_plugin_manager.logger = MagicMock() self.assertTrue(self.plugin_manager.run()) self.plugin_manager._keystore._add.assert_has_calls([call("key1", "secret1")]) self.charm.opensearch.config.put.assert_has_calls( @@ -326,6 +349,7 @@ def test_plugin_setup_with_relation( [call("POST", "_nodes/reload_secure_settings")] ) + @patch("charms.opensearch.v0.opensearch_plugin_manager.ClusterTopology.get_cluster_settings") @patch("charms.opensearch.v0.opensearch_plugin_manager.OpenSearchPluginManager._extra_conf") @patch("charms.opensearch.v0.opensearch_plugin_manager.OpenSearchPluginManager._is_enabled") @patch( @@ -334,16 +358,15 @@ def test_plugin_setup_with_relation( @patch( "charms.opensearch.v0.opensearch_plugin_manager.OpenSearchPluginManager._installed_plugins" ) - @patch("charms.opensearch.v0.opensearch_config.OpenSearchConfig.load_node") @patch("charms.opensearch.v0.opensearch_distro.OpenSearchDistribution.version") def test_disable_plugin( self, _, - mock_load, mock_installed_plugins, mock_plugin_relation, mock_is_enabled, - mock_extra_conf, + __, + mock_get_cluster_settings, ) -> None: """Tests end-to-end the disable of a plugin.""" # Keystore-related mocks @@ -363,8 +386,17 @@ def test_disable_plugin( # Mock _installed_plugins to return test mock_installed_plugins.return_value = ["test"] - # load_node will be called multiple times - mock_load.side_effect = {"param": "tested"} + self.charm._get_nodes = MagicMock( + return_value={ + "1": {}, + "2": {}, + "3": {}, + } + ) + self.charm.app.planned_units = MagicMock(return_value=3) + self.charm.opensearch.is_node_up = MagicMock(return_value=True) + + mock_get_cluster_settings.return_value = {"param": "tested"} mock_plugin_relation.return_value = False # plugin is initially disabled and enabled when method self._disable calls self.status mock_is_enabled.return_value = True @@ -414,10 +446,6 @@ def test_config_with_valid_keys(self): "secret-key": "SECRET_KEY", } expected_config = OpenSearchPluginConfig( - secret_entries_to_del=[ - "s3.client.default.access_key", - "s3.client.default.secret_key", - ], secret_entries_to_add={ "s3.client.default.access_key": "ACCESS_KEY", "s3.client.default.secret_key": "SECRET_KEY",