Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove redfish credential validation in install event handler #192

Merged
merged 5 commits into from
Mar 26, 2024
Merged

fix: Remove redfish credential validation in install event handler #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f934bb
fix: Check exporter installed before healthcheck
dashmage Mar 19, 2024
47a403a
Return if exporter not installed, unit test fix
dashmage Mar 21, 2024
7351e70
Move exporter installed check to top of update status
dashmage Mar 21, 2024
16fd216
Fix unit test names and docstrings
dashmage Mar 23, 2024
fd7a71f
Remove validation in install event handler
dashmage Mar 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions src/charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,6 @@ def _on_install_or_upgrade(self, event: ops.InstallEvent) -> None:

# Install exporter
self.model.unit.status = MaintenanceStatus("Installing exporter...")
success, err_msg = self.validate_exporter_configs()
if not success:
self.model.unit.status = BlockedStatus(err_msg)
return

port = self.model.config.get("exporter-port", EXPORTER_DEFAULT_PORT)
level = self.model.config.get("exporter-log-level", "INFO")
redfish_conn_params = self.get_redfish_conn_params()
Expand Down Expand Up @@ -132,9 +127,9 @@ def _on_update_status(self, _: EventBase) -> None: # noqa: C901
self.model.unit.status = BlockedStatus("Cannot relate to more than one grafana-agent")
return

config_valied, confg_valid_message = self.validate_exporter_configs()
if not config_valied:
self.model.unit.status = BlockedStatus(confg_valid_message)
config_valid, config_valid_message = self.validate_exporter_configs()
gabrielcocenza marked this conversation as resolved.
Show resolved Hide resolved
if not config_valid:
self.model.unit.status = BlockedStatus(config_valid_message)
return

hw_tool_ok, error_msg = self.hw_tool_helper.check_installed()
Expand Down
73 changes: 41 additions & 32 deletions tests/unit/test_charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,16 @@ def _get_notice_count(self, hook):
notice_count += 1
return notice_count

def test_01_harness(self) -> None:
"""Test charm initialise."""
def test_harness(self) -> None:
"""Test charm initialize."""
self.harness.begin()
self.assertFalse(self.harness.charm._stored.resource_installed)
self.assertTrue(isinstance(self.harness.charm._stored.config, ops.framework.StoredDict))

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_02_install(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test event install."""
def test_install(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test install event handler."""
mock_hw_tool_helper.return_value.install.return_value = (True, "")
mock_exporter.return_value.install.return_value = True
self.harness.begin()
Expand All @@ -83,8 +83,8 @@ def test_02_install(self, mock_hw_tool_helper, mock_exporter) -> None:

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_03_upgrade_charm(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test event upgrade_charm."""
def test_upgrade_charm(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test upgrade_charm event handler."""
mock_hw_tool_helper.return_value.install.return_value = (True, "")
mock_exporter.return_value.install.return_value = True
self.harness.begin()
Expand All @@ -101,8 +101,8 @@ def test_03_upgrade_charm(self, mock_hw_tool_helper, mock_exporter) -> None:

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_04_install_missing_resources(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test event install."""
def test_install_missing_resources(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test install event handler when resources are missing."""
mock_hw_tool_helper.return_value.install.return_value = (
False,
"Missing resources: ['storcli-deb']",
Expand All @@ -116,8 +116,8 @@ def test_04_install_missing_resources(self, mock_hw_tool_helper, mock_exporter)

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_05_install_redfish_unavailable(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test event install."""
def test_install_redfish_unavailable(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test install event handler when redfish is unavailable."""
self.mock_bmc_hw_verifier.return_value = [
HWTool.IPMI_SENSOR,
HWTool.IPMI_SEL,
Expand All @@ -138,8 +138,8 @@ def test_05_install_redfish_unavailable(self, mock_hw_tool_helper, mock_exporter

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_06_install_failed(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test event install."""
def test_exporter_install_fail(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test exporter install failure."""
mock_hw_tool_helper.return_value.install.return_value = (True, "")
mock_exporter.return_value.install.return_value = False
self.harness.begin()
Expand All @@ -149,12 +149,15 @@ def test_06_install_failed(self, mock_hw_tool_helper, mock_exporter) -> None:

self.assertTrue(self.harness.charm._stored.resource_installed)

self.assertEqual(self.harness.charm.unit.status, BlockedStatus("error"))
self.assertEqual(
self.harness.charm.unit.status,
BlockedStatus("Failed to install exporter, please refer to `juju debug-log`"),
)

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_07_update_status_all_green(self, mock_hw_tool_helper, mock_exporter):
"""Test update_status when everything is okay."""
def test_update_status_all_green(self, mock_hw_tool_helper, mock_exporter):
"""Test update_status event handler when everything is okay."""
self.mock_bmc_hw_verifier.return_value = [
HWTool.IPMI_SENSOR,
HWTool.IPMI_SEL,
Expand All @@ -173,8 +176,8 @@ def test_07_update_status_all_green(self, mock_hw_tool_helper, mock_exporter):

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_08_update_status_check_installed_false(self, mock_hw_tool_helper, mock_exporter):
"""Test update_status when hw tool checks failed."""
def test_update_status_check_installed_false(self, mock_hw_tool_helper, mock_exporter):
"""Test update_status event handler when hw tool checks failed."""
self.mock_bmc_hw_verifier.return_value = [
HWTool.IPMI_SENSOR,
HWTool.IPMI_SEL,
Expand All @@ -193,7 +196,7 @@ def test_08_update_status_check_installed_false(self, mock_hw_tool_helper, mock_

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_09_update_status_exporter_crashed(self, mock_hw_tool_helper, mock_exporter):
def test_update_status_exporter_crashed(self, mock_hw_tool_helper, mock_exporter):
"""Test update_status."""
self.mock_bmc_hw_verifier.return_value = [
HWTool.IPMI_SENSOR,
Expand All @@ -210,7 +213,7 @@ def test_09_update_status_exporter_crashed(self, mock_hw_tool_helper, mock_expor
self.harness.charm.on.update_status.emit()

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
def test_10_config_changed(self, mock_exporter):
def test_config_changed(self, mock_exporter):
"""Test config change event updates the charm's internal store."""
self.harness.begin()
self.harness.charm._stored.resource_installed = True
Expand All @@ -227,8 +230,8 @@ def test_10_config_changed(self, mock_exporter):
)

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
def test_11_config_changed_before_install_complete(self, mock_exporter):
"""Test: config change event is deferred if charm not installed."""
def test_config_changed_before_install_complete(self, mock_exporter):
"""Test config change event is deferred if charm not installed."""
self.harness.begin()
self.harness.charm._stored.resource_installed = False

Expand All @@ -237,8 +240,8 @@ def test_11_config_changed_before_install_complete(self, mock_exporter):

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_12_upgrade_force_reconfig_exporter(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test event install."""
def test_upgrade_force_reconfig_exporter(self, mock_hw_tool_helper, mock_exporter) -> None:
"""Test upgrade event handler will reconfigure exporter."""
mock_hw_tool_helper.return_value.install.return_value = (True, "")
mock_exporter.return_value.install.return_value = True
self.harness.begin()
Expand All @@ -256,8 +259,8 @@ def test_12_upgrade_force_reconfig_exporter(self, mock_hw_tool_helper, mock_expo

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_13_update_status_config_invalid(self, mock_hw_tool_helper, mock_exporter):
"""Test update_status when everything is okay."""
def test_update_status_config_invalid(self, mock_hw_tool_helper, mock_exporter):
"""Test update_status event handler when config is invalid."""
self.mock_bmc_hw_verifier.return_value = [
HWTool.IPMI_SENSOR,
HWTool.IPMI_SEL,
Expand All @@ -279,7 +282,7 @@ def test_13_update_status_config_invalid(self, mock_hw_tool_helper, mock_exporte

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_14_config_changed_update_alert_rules(self, mock_hw_tool_helper, mock_exporter):
def test_config_changed_update_alert_rules(self, mock_hw_tool_helper, mock_exporter):
"""Test config changed will update alert rule."""
self.mock_bmc_hw_verifier.return_value = [
HWTool.IPMI_SENSOR,
Expand Down Expand Up @@ -315,8 +318,8 @@ def test_14_config_changed_update_alert_rules(self, mock_hw_tool_helper, mock_ex

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_15_upgrade_charm_update_alert_rules(self, mock_hw_tool_helper, mock_exporter):
"""Test upgrade charm will update alert rule."""
def test_upgrade_charm_update_alert_rules(self, mock_hw_tool_helper, mock_exporter):
"""Test upgrade charm event updates alert rule."""
self.mock_bmc_hw_verifier.return_value = [
HWTool.IPMI_SENSOR,
HWTool.IPMI_SEL,
Expand Down Expand Up @@ -351,10 +354,10 @@ def test_15_upgrade_charm_update_alert_rules(self, mock_hw_tool_helper, mock_exp

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
def test_16_install_redfish_enabled_with_correct_credential(
def test_install_redfish_enabled_with_correct_credential(
self, mock_hw_tool_helper, mock_exporter
) -> None:
"""Test event install when redfish is available and credential is correct."""
"""Test install event when redfish is available and credential is correct."""
self.mock_bmc_hw_verifier.return_value = [
HWTool.REDFISH,
]
Expand All @@ -373,7 +376,7 @@ def test_16_install_redfish_enabled_with_correct_credential(
@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
@mock.patch("charm.redfish_client", return_value=mock.MagicMock())
def test_17_install_redfish_enabled_with_incorrect_credential(
def test_install_redfish_enabled_with_incorrect_credential(
self, test_exception, mock_redfish_client, mock_hw_tool_helper, mock_exporter
) -> None:
"""Test event install when redfish is available but credential is wrong."""
Expand All @@ -383,11 +386,17 @@ def test_17_install_redfish_enabled_with_incorrect_credential(
mock_redfish_client.side_effect = test_exception()
mock_hw_tool_helper.return_value.install.return_value = (True, "")
mock_exporter.return_value.install.return_value = True
rid = self.harness.add_relation("cos-agent", "grafana-agent")
self.harness.begin()
self.harness.add_relation_unit(rid, "grafana-agent/0")
self.harness.charm.on.install.emit()

self.assertTrue(self.harness.charm._stored.resource_installed)

# ensure exporter is installed (not started/enabled)
# even when redfish credentials are wrong
mock_exporter.return_value.install.assert_called_once()
mock_exporter.reset_mock()
self.assertEqual(
self.harness.charm.unit.status,
BlockedStatus("Invalid config: 'redfish-username' or 'redfish-password'"),
Expand All @@ -397,7 +406,7 @@ def test_17_install_redfish_enabled_with_incorrect_credential(
@mock.patch("charm.Exporter", return_value=mock.MagicMock())
@mock.patch("charm.HWToolHelper", return_value=mock.MagicMock())
@mock.patch("charm.redfish_client", return_value=mock.MagicMock())
def test_18_config_changed_redfish_enabled_with_incorrect_credential(
def test_config_changed_redfish_enabled_with_incorrect_credential(
self, test_exception, mock_redfish_client, mock_hw_tool_helper, mock_exporter
) -> None:
"""Test event config changed when redfish is available but credential is wrong."""
Expand Down
Loading