Add more details for Errors and ensure they are triaged at def run()

lib/charms/opensearch/v0/opensearch_plugin_manager.py

@@ -20,6 +20,7 @@
 from charms.opensearch.v0.opensearch_health import HealthColors
 from charms.opensearch.v0.opensearch_keystore import OpenSearchKeystore
 from charms.opensearch.v0.opensearch_plugins import (
+    OpenSearchBackupPlugin,
     OpenSearchKnn,
     OpenSearchPlugin,
     OpenSearchPluginConfig,
@@ -52,6 +53,11 @@
         "config": "plugin_opensearch_knn",
         "relation": None,
     },
+    "repository-s3": {
+        "class": OpenSearchBackupPlugin,
+        "config": None,
+        "relation": "s3-credentials",
+    },
 }
 
 
@@ -165,9 +171,9 @@ def run(self) -> bool:
             ) as e:
                 # This is a more serious issue, as we are missing some input from
                 # the user. The charm should block.
-                raise e
-            except OpenSearchPluginError as e:
                 err_msgs.append(str(e))
+            except OpenSearchPluginError as e:
+                logger.error(f"Plugin {plugin.name} failed: {str(e)}")
 
         if err_msgs:
             raise OpenSearchPluginError("\n".join(err_msgs))
@@ -226,10 +232,7 @@ def _install_if_needed(self, plugin: OpenSearchPlugin) -> bool:
     def _configure_if_needed(self, plugin: OpenSearchPlugin) -> bool:
         """Gathers all the configuration changes needed and applies them."""
         try:
-            if (
-                not self._user_requested_to_enable(plugin)
-                or self.status(plugin) != PluginState.INSTALLED
-            ):
+            if self.status(plugin) != PluginState.INSTALLED:
                 # Leave this method if either user did not request to enable this plugin
                 # or plugin has been already enabled.
                 return False
@@ -285,13 +288,19 @@ def status(self, plugin: OpenSearchPlugin) -> PluginState:
         """Returns the status for a given plugin."""
         if not self._is_installed(plugin):
             return PluginState.MISSING
-        if not self._is_enabled(plugin):
-            if self._user_requested_to_enable(plugin):
-                return PluginState.INSTALLED
-            return PluginState.DISABLED
+
         if self._needs_upgrade(plugin):
             return PluginState.WAITING_FOR_UPGRADE
-        return PluginState.ENABLED
+
+        # The _user_request_to_enable comes first, as it ensures there is a relation/config
+        # set, which will be used by _is_enabled to determine if we are enabled or not.
+        if not self._user_requested_to_enable(plugin) and not self._is_enabled(plugin):
+            return PluginState.DISABLED
+
+        if self._is_enabled(plugin):
+            return PluginState.ENABLED
+
+        return PluginState.INSTALLED
 
     def _is_installed(self, plugin: OpenSearchPlugin) -> bool:
         """Returns true if plugin is installed."""
@@ -300,9 +309,10 @@ def _is_installed(self, plugin: OpenSearchPlugin) -> bool:
     def _user_requested_to_enable(self, plugin: OpenSearchPlugin) -> bool:
         """Returns True if user requested plugin to be enabled."""
         plugin_data = ConfigExposedPlugins[plugin.name]
-        if not self._charm.config.get(
-            plugin_data["config"], False
-        ) and not self._is_plugin_relation_set(plugin_data["relation"]):
+        if not (
+            self._charm.config.get(plugin_data["config"], False)
+            or self._is_plugin_relation_set(plugin_data["relation"])
+        ):
             # User asked to disable this plugin
             return False
         return True

tests/unit/lib/test_plugins.py

@@ -13,6 +13,7 @@
     OpenSearchPlugin,
     OpenSearchPluginConfig,
     OpenSearchPluginInstallError,
+    OpenSearchPluginMissingConfigError,
     OpenSearchPluginMissingDepsError,
     PluginState,
 )
@@ -120,6 +121,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"
 
     @patch("charms.opensearch.v0.opensearch_plugin_manager.OpenSearchPluginManager._is_enabled")
     @patch("charms.opensearch.v0.opensearch_plugin_manager.OpenSearchPluginManager._is_installed")
@@ -140,12 +142,8 @@ def test_plugin_process_plugin_properties_file(
         assert test_plugin.version == "2.9.0.0"
         assert self.plugin_manager.status(test_plugin) == PluginState.WAITING_FOR_UPGRADE
 
-    @patch(
-        "charms.opensearch.v0.opensearch_distro.OpenSearchDistribution.version",
-        new_callable=PropertyMock,
-    )
     @patch("charms.opensearch.v0.opensearch_config.OpenSearchConfig.load_node")
-    def test_failed_install_plugin(self, _, mock_version) -> None:
+    def test_failed_install_plugin(self, _) -> None:
         """Tests a failed command."""
         succeeded = False
         self.charm.opensearch._run_cmd = MagicMock(
@@ -156,18 +154,14 @@ def test_failed_install_plugin(self, _, mock_version) -> None:
             test_plugin = self.plugin_manager.plugins[0]
             self.plugin_manager._install_if_needed(test_plugin)
         except OpenSearchPluginInstallError as e:
-            assert str(e) == "Failed to install plugin test: this is a test"
+            assert str(e) == "Failed to install plugin: test"
             succeeded = True
         finally:
             # We may reach this point because of another exception, check it:
             assert succeeded is True
 
-    @patch(
-        "charms.opensearch.v0.opensearch_distro.OpenSearchDistribution.version",
-        new_callable=PropertyMock,
-    )
     @patch("charms.opensearch.v0.opensearch_config.OpenSearchConfig.load_node")
-    def test_failed_install_plugin_already_exists(self, _, mock_version) -> None:
+    def test_failed_install_plugin_already_exists(self, _) -> None:
         """Tests a failed command when the plugin already exists."""
         succeeded = True
         self.charm.opensearch._run_cmd = MagicMock(
@@ -198,7 +192,7 @@ def test_failed_install_plugin_missing_dependency(self, _, mock_version) -> None
         except OpenSearchPluginMissingDepsError as e:
             assert (
                 str(e)
-                == "Failed to install test, missing dependencies: ['test-plugin-dependency']"
+                == "Failed to install plugin: test - missing dependencies ['test-plugin-dependency']"
             )
             succeeded = True
         # Check if we had any other exception
@@ -324,6 +318,7 @@ def test_plugin_setup_with_relation(
             False,  # called by self._configure
             True,  # called by self.status, in self._disable
         ]
+
         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(
@@ -376,6 +371,7 @@ def test_disable_plugin(
         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
+
         self.assertTrue(self.plugin_manager.run())
         self.plugin_manager._keystore._add.assert_not_called()
         self.plugin_manager._keystore._delete.assert_called()
@@ -404,9 +400,14 @@ def test_config_missing_all_configs(self):
             plugins_path=self.plugin_manager._plugins_path,
             extra_config={},
         )
-        with patch("charms.opensearch.v0.opensearch_plugins.logger.error") as mock_logger:
+        try:
             plugin.config()
-            mock_logger.assert_called_with("Missing AWS access-key and secret-key configuration")
+        except OpenSearchPluginMissingConfigError as e:
+            assert (
+                str(e) == "Plugin repository-s3 is missing configs: ['access-key', 'secret-key']"
+            )
+        else:
+            assert False
 
     def test_config_with_valid_keys(self):
         plugin = OpenSearchBackupPlugin(