Skip to content

Commit

Permalink
Remove unnecessary use of StoreState for configs (#209)
Browse files Browse the repository at this point in the history
* Drop `_store.config`.

* Remove unnecessary EXPORTER_DEFAULT_*.
  • Loading branch information
chanchiwai-ray authored Apr 5, 2024
1 parent 07056f4 commit 4a1b809
Show file tree
Hide file tree
Showing 7 changed files with 69 additions and 81 deletions.
77 changes: 20 additions & 57 deletions src/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@

from config import (
EXPORTER_CRASH_MSG,
EXPORTER_DEFAULT_COLLECT_TIMEOUT,
EXPORTER_DEFAULT_PORT,
EXPORTER_HEALTH_RETRY_COUNT,
EXPORTER_HEALTH_RETRY_TIMEOUT,
REDFISH_MAX_RETRY,
Expand All @@ -42,9 +40,6 @@ def __init__(self, *args: Any) -> None:
super().__init__(*args)
self.hw_tool_helper = HWToolHelper()

collect_timeout = self.model.config.get(
"collect-timeout", EXPORTER_DEFAULT_COLLECT_TIMEOUT
)
# Add refresh_events to COSAgentProvider to update relation data when
# config changed (default behavior) and upgrade charm. This is useful
# for updating alert rules.
Expand All @@ -56,7 +51,7 @@ def __init__(self, *args: Any) -> None:
],
# Setting scrape_timeout as collect_timeout in the `duration` format specified in
# https://prometheus.io/docs/prometheus/latest/configuration/configuration/#duration
scrape_configs=[{"scrape_timeout": f"{collect_timeout}s"}],
scrape_configs=[{"scrape_timeout": f"{int(self.model.config['collect-timeout'])}s"}],
)
self.exporter = Exporter(self.charm_dir)

Expand All @@ -73,7 +68,6 @@ def __init__(self, *args: Any) -> None:
)

self._stored.set_default(
config={},
exporter_installed=False,
resource_installed=False,
)
Expand All @@ -93,11 +87,12 @@ def _on_install_or_upgrade(self, event: ops.InstallEvent) -> None:

# Install exporter
self.model.unit.status = MaintenanceStatus("Installing exporter...")
port = self.model.config.get("exporter-port", EXPORTER_DEFAULT_PORT)
level = self.model.config.get("exporter-log-level", "INFO")
scrape_timeout = self.model.config.get("scrape-timeout", EXPORTER_DEFAULT_COLLECT_TIMEOUT)
redfish_conn_params = self.get_redfish_conn_params()
success = self.exporter.install(port, level, redfish_conn_params, int(scrape_timeout))
success = self.exporter.install(
int(self.model.config["exporter-port"]),
self.model.config["exporter-log-level"],
self.get_redfish_conn_params(),
int(self.model.config["collect-timeout"]),
)
self._stored.exporter_installed = success
if not success:
msg = "Failed to install exporter, please refer to `juju debug-log`"
Expand Down Expand Up @@ -171,20 +166,6 @@ def restart_exporter(self) -> None:

def _on_config_changed(self, event: EventBase) -> None:
"""Reconfigure charm."""
# Keep track of what model config options + some extra config related
# information are changed. This can be helpful when we want to respond
# to the change of a specific config option.
change_set = set()
model_config: Dict[str, Optional[str]] = dict(self.model.config.items())
for key, value in model_config.items():
if (
key not in self._stored.config # type: ignore[operator]
or self._stored.config[key] != value # type: ignore[index]
):
logger.info("Setting %s to: %s", key, value)
self._stored.config[key] = value # type: ignore
change_set.add(key)

if not self._stored.resource_installed: # type: ignore[truthy-function]
logging.info( # type: ignore[unreachable]
"Config changed called before install complete, deferring event: %s",
Expand All @@ -198,35 +179,17 @@ def _on_config_changed(self, event: EventBase) -> None:
self.model.unit.status = BlockedStatus(message)
return

exporter_configs = {
"exporter-port",
"exporter-log-level",
"redfish-host",
"redfish-username",
"redfish-password",
"collect-timeout",
}
if exporter_configs.intersection(change_set):
logger.info("Detected changes in exporter config.")
port = self.model.config.get("exporter-port", EXPORTER_DEFAULT_PORT)
level = self.model.config.get("exporter-log-level", "INFO")
collect_timeout = self.model.config.get(
"collect-timeout", EXPORTER_DEFAULT_COLLECT_TIMEOUT
)
redfish_conn_params = self.get_redfish_conn_params()
success = self.exporter.template.render_config(
port=port,
level=level,
redfish_conn_params=redfish_conn_params,
collect_timeout=int(collect_timeout),
)
if not success:
message = (
"Failed to configure exporter, please check if the server is healthy."
)
self.model.unit.status = BlockedStatus(message)
return
self.exporter.restart()
success = self.exporter.template.render_config(
port=int(self.model.config["exporter-port"]),
level=self.model.config["exporter-log-level"],
redfish_conn_params=self.get_redfish_conn_params(),
collect_timeout=int(self.model.config["collect-timeout"]),
)
if not success:
message = "Failed to configure exporter, please check if the server is healthy."
self.model.unit.status = BlockedStatus(message)
return
self.exporter.restart()

self._on_update_status(event)

Expand Down Expand Up @@ -268,12 +231,12 @@ def get_redfish_conn_params(self) -> Dict[str, Any]:

def validate_exporter_configs(self) -> Tuple[bool, str]:
"""Validate the static and runtime config options for the exporter."""
port = int(self.model.config.get("exporter-port", EXPORTER_DEFAULT_PORT))
port = int(self.model.config["exporter-port"])
if not 1 <= port <= 65535:
logger.error("Invalid exporter-port: port must be in [1, 65535].")
return False, "Invalid config: 'exporter-port'"

level = self.model.config.get("exporter-log-level", "")
level = self.model.config["exporter-log-level"]
allowed_choices = {"DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"}
if level.upper() not in allowed_choices:
logger.error(
Expand Down
2 changes: 0 additions & 2 deletions src/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@
EXPORTER_SERVICE_TEMPLATE = f"{EXPORTER_NAME}.service.j2"
EXPORTER_HEALTH_RETRY_COUNT = 3
EXPORTER_HEALTH_RETRY_TIMEOUT = 3
EXPORTER_DEFAULT_PORT = "10200"
EXPORTER_CRASH_MSG = "Exporter crashed unexpectedly, please refer to systemd logs..."
EXPORTER_DEFAULT_COLLECT_TIMEOUT = 10

# Redfish
REDFISH_TIMEOUT = 10
Expand Down
4 changes: 2 additions & 2 deletions src/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def _uninstall(self, path: Path) -> bool:
return success

def render_config(
self, port: str, level: str, collect_timeout: int, redfish_conn_params: dict
self, port: int, level: str, collect_timeout: int, redfish_conn_params: dict
) -> bool:
"""Render and install exporter config file."""
hw_tools = get_hw_tool_white_list()
Expand Down Expand Up @@ -130,7 +130,7 @@ def __init__(self, charm_dir: Path) -> None:
self.template = ExporterTemplate(charm_dir)

def install(
self, port: str, level: str, redfish_conn_params: dict, collect_timeout: int
self, port: int, level: str, redfish_conn_params: dict, collect_timeout: int
) -> bool:
"""Install the exporter."""
logger.info("Installing %s.", EXPORTER_NAME)
Expand Down
4 changes: 1 addition & 3 deletions tests/functional/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@

from async_lru import alru_cache

from config import EXPORTER_DEFAULT_PORT

RESOURCES_DIR = Path("./resources/")


Expand Down Expand Up @@ -62,7 +60,7 @@ async def get_metrics_output(ops_test, unit_name) -> Optional[dict[str, list[Met
Raises MetricsFetchError if command to fetch metrics didn't execute successfully.
"""
command = f"curl -s localhost:{EXPORTER_DEFAULT_PORT}"
command = "curl -s localhost:10200" # curl at default port (see config.yaml)
results = await run_command_on_unit(ops_test, unit_name, command)
if results.get("return-code") > 0:
raise MetricsFetchError
Expand Down
4 changes: 1 addition & 3 deletions tests/integration/export_mock_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
from prometheus_client import REGISTRY, start_http_server
from prometheus_client.core import GaugeMetricFamily

from config import EXPORTER_DEFAULT_PORT


class SyntheticCollector:
"""Collector for creating synthetic(mock) metrics."""
Expand All @@ -30,7 +28,7 @@ def collect(self):


if __name__ == "__main__":
start_http_server(int(EXPORTER_DEFAULT_PORT))
start_http_server(10200) # start at default port (see config.yaml)
REGISTRY.register(SyntheticCollector())

while True:
Expand Down
52 changes: 40 additions & 12 deletions tests/unit/test_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

import charm
from charm import ExporterError, HardwareObserverCharm
from config import EXPORTER_DEFAULT_COLLECT_TIMEOUT, EXPORTER_DEFAULT_PORT, HWTool
from config import HWTool


class TestCharm(unittest.TestCase):
Expand Down Expand Up @@ -63,7 +63,6 @@ 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())
Expand Down Expand Up @@ -131,10 +130,10 @@ def test_install_redfish_unavailable(self, mock_hw_tool_helper, mock_exporter) -
self.assertTrue(self.harness.charm._stored.resource_installed)

self.harness.charm.exporter.install.assert_called_with(
int(EXPORTER_DEFAULT_PORT),
"INFO",
10200, # default in config.yaml
"INFO", # default in config.yaml
{},
int(EXPORTER_DEFAULT_COLLECT_TIMEOUT),
10, # default int config.yaml
)

@mock.patch("charm.Exporter", return_value=mock.MagicMock())
Expand Down Expand Up @@ -213,18 +212,47 @@ def test_update_status_exporter_crashed(self, mock_hw_tool_helper, mock_exporter
with self.assertRaises(ExporterError):
self.harness.charm.on.update_status.emit()

@mock.patch.object(charm, "bmc_hw_verifier", return_value=[])
@mock.patch("charm.HWToolHelper")
@mock.patch("charm.Exporter", return_value=mock.MagicMock())
def test_config_changed(self, mock_exporter):
"""Test config change event updates the charm's internal store."""
def test_config_changed(self, mock_exporter, mock_hw_tool_helper, mock_bmc_hw_verifier):
"""Test config change event renders config file."""
self.harness.begin()
self.harness.charm._stored.resource_installed = True
self.harness.charm.num_cos_agent_relations = 1 # exporter enabled
self.harness.charm.hw_tool_helper.check_installed.return_value = (
True,
"",
) # hw tool install ok

new_config = {"exporter-port": 80, "exporter-log-level": "DEBUG"}
self.harness.update_config(new_config)
self.harness.charm.on.config_changed.emit()

for k, v in self.harness.charm.model.config.items():
self.assertEqual(self.harness.charm._stored.config.get(k), v)
self.harness.charm.exporter.template.render_config.assert_called()

self.assertEqual(self.harness.charm.unit.status, ActiveStatus("Unit is ready"))

@mock.patch.object(charm, "bmc_hw_verifier", return_value=[])
@mock.patch("charm.HWToolHelper")
@mock.patch("charm.Exporter", return_value=mock.MagicMock())
def test_config_changed_without_cos_agent_relation(
self, mock_exporter, mock_hw_tool_helper, mock_bmc_hw_verifier
):
"""Test config change event don't render config file if cos_agent relation is missing."""
self.harness.begin()
self.harness.charm._stored.resource_installed = True
self.harness.charm.num_cos_agent_relations = 0 # exporter disabled
self.harness.charm.hw_tool_helper.check_installed.return_value = (
True,
"",
) # hw tool install ok

new_config = {"exporter-port": 80, "exporter-log-level": "DEBUG"}
self.harness.update_config(new_config)
self.harness.charm.on.config_changed.emit()

self.harness.charm.exporter.template.render_config.assert_not_called()

self.assertEqual(
self.harness.charm.unit.status, BlockedStatus("Missing relation: [cos-agent]")
Expand Down Expand Up @@ -370,10 +398,10 @@ def test_install_redfish_enabled_with_correct_credential(
self.assertTrue(self.harness.charm._stored.resource_installed)

self.harness.charm.exporter.install.assert_called_with(
int(EXPORTER_DEFAULT_PORT),
"INFO",
10200, # default in config.yaml
"INFO", # default in config.yaml
self.harness.charm.get_redfish_conn_params(),
int(EXPORTER_DEFAULT_COLLECT_TIMEOUT),
10, # default int config.yaml
)

@parameterized.expand([(InvalidCredentialsError), (Exception)])
Expand Down
7 changes: 5 additions & 2 deletions tests/unit/test_exporter.py
Original file line number Diff line number Diff line change
Expand Up @@ -275,10 +275,13 @@ def test_60_config_changed_log_level_okay(self, mock_service_installed):
self.mock_systemd.service_failed.return_value = False
self.harness.charm.on.install.emit()
self.harness.add_relation_unit(rid, "grafana-agent/0")
# this will trigger config changed event
self.harness.update_config({"exporter-log-level": "DEBUG"})
self.harness.charm.on.config_changed.emit()
self.assertEqual(self.harness.charm._stored.config.get("exporter-log-level"), "DEBUG")
self.mock_systemd.service_restart.assert_called_once()
self.assertEqual(
self.harness.charm.unit.status,
ActiveStatus("Unit is ready"),
)

@mock.patch.object(pathlib.Path, "exists", return_value=True)
def test_61_config_changed_not_okay(self, mock_service_installed):
Expand Down

0 comments on commit 4a1b809

Please sign in to comment.