From 916eaec6b8daeb309f16297fd43b20b9f480f7e3 Mon Sep 17 00:00:00 2001 From: Elzbieta Kotulska Date: Tue, 26 Nov 2024 15:07:46 +0100 Subject: [PATCH 1/5] Catch OSError in ConfigManagerActor _read_config method Becasuse: * not all files in self._config_paths might exists * catching the watchfiles signal and file reading happen in separate async thread and the file could be removed between signal detection and file reading. This prevents unexpected actor crash. Signed-off-by: Elzbieta Kotulska --- src/frequenz/sdk/config/_config_managing.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/frequenz/sdk/config/_config_managing.py b/src/frequenz/sdk/config/_config_managing.py index 30ac708d3..081f0fd7b 100644 --- a/src/frequenz/sdk/config/_config_managing.py +++ b/src/frequenz/sdk/config/_config_managing.py @@ -130,6 +130,15 @@ def _read_config(self) -> abc.Mapping[str, Any]: except ValueError as err: _logger.error("%s: Can't read config file, err: %s", self, err) error_count += 1 + except OSError as err: + # It is ok for config file to don't exist. + _logger.error( + "%s: Error reading config file %s (%s). Ignoring it.", + self, + err, + config_path, + ) + error_count += 1 if error_count == len(self._config_paths): raise ValueError(f"{self}: Can't read any of the config files") From 4d384e53ccc86a27c87b7b2cc49ad6797ce0847a Mon Sep 17 00:00:00 2001 From: Elzbieta Kotulska Date: Tue, 26 Nov 2024 15:31:17 +0100 Subject: [PATCH 2/5] Fix ConfigManagingActor crashing when any file was deleted `event.path.samefile` raises error if: * `event.pathq` doesn't exists * path `p` doesn't exists In this case it raised exception: * if any file in parent directory was deleted * any path in self._config_paths didn't exist Signed-off-by: Elzbieta Kotulska --- src/frequenz/sdk/config/_config_managing.py | 27 +++++++++++++++------ 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/frequenz/sdk/config/_config_managing.py b/src/frequenz/sdk/config/_config_managing.py index 081f0fd7b..e66e2e461 100644 --- a/src/frequenz/sdk/config/_config_managing.py +++ b/src/frequenz/sdk/config/_config_managing.py @@ -74,7 +74,6 @@ def __init__( self, config_paths: abc.Sequence[pathlib.Path | str], output: Sender[abc.Mapping[str, Any]], - event_types: abc.Set[EventType] = frozenset(EventType), *, name: str | None = None, force_polling: bool = True, @@ -89,7 +88,6 @@ def __init__( the previous paths. Dict keys will be merged recursively, but other objects (like lists) will be replaced by the value in the last path. output: The sender to send the configuration to. - event_types: The set of event types to monitor. name: The name of the actor. If `None`, `str(id(self))` will be used. This is used mostly for debugging purposes. force_polling: Whether to force file polling to check for changes. @@ -106,7 +104,6 @@ def __init__( for config_path in config_paths ] self._output: Sender[abc.Mapping[str, Any]] = output - self._event_types: abc.Set[EventType] = event_types self._force_polling: bool = force_polling self._polling_interval: timedelta = polling_interval @@ -166,17 +163,32 @@ async def _run(self) -> None: # or it is deleted and recreated again. file_watcher = FileWatcher( paths=list(parent_paths), - event_types=self._event_types, + event_types={EventType.CREATE, EventType.MODIFY}, force_polling=self._force_polling, polling_interval=self._polling_interval, ) try: async for event in file_watcher: + if not event.path.exists(): + _logger.error( + "%s: Received event %s, but the watched path %s doesn't exist.", + self, + event, + event.path, + ) + continue # Since we are watching the whole parent directories, we need to make # sure we only react to events related to the configuration files we # are interested in. - if not any(event.path.samefile(p) for p in self._config_paths): + # + # pathlib.Path.samefile raises error if any path doesn't exist so we need to + # make sure the paths exists before calling it. This could happen as it is not + # required that all config files exist, only one is required but we don't know + # which. + if not any( + event.path.samefile(p) for p in self._config_paths if p.exists() + ): continue match event.type: @@ -195,8 +207,9 @@ async def _run(self) -> None: ) await self.send_config() case EventType.DELETE: - _logger.info( - "%s: The configuration file %s was deleted, ignoring...", + _logger.error( + "%s: Unexpected DELETE event for path %s. Please report this " + "issue to Frequenz.", self, event.path, ) From 2b60cf0aad54b60eefd59e5f0b3a9560c0d213fa Mon Sep 17 00:00:00 2001 From: Elzbieta Kotulska Date: Tue, 26 Nov 2024 15:36:44 +0100 Subject: [PATCH 3/5] Update release notes Signed-off-by: Elzbieta Kotulska --- RELEASE_NOTES.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 277a4f885..f822e0656 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -6,7 +6,8 @@ ## Upgrading - +- The `ConfigManagingActor` now only reacts to `CREATE` and `MODIFY` events. `DELETE` is not supported anymore and are ignored. +- Remove the `event_types` argument from the `ConfigManagingActor` constructor. ## New Features @@ -15,3 +16,8 @@ ## Bug Fixes - Fix a bug in the resampler that could end up with an *IndexError: list index out of range* exception when a new resampler was added while awaiting the existing resampler to finish resampling. + +- Fix bugs with `ConfigManagingActor`: + - Raising unhandled exceptions when any file in config directory was deleted. + - Raising unhandled exception if not all config files exist. + - Eliminate recursive actor crashes when all config files were missing. \ No newline at end of file From 003ac257de90914aac2f7ab6976d74ee799a31f6 Mon Sep 17 00:00:00 2001 From: Elzbieta Kotulska Date: Tue, 26 Nov 2024 16:56:23 +0100 Subject: [PATCH 4/5] Add tests for ConfigManagingActor Signed-off-by: Elzbieta Kotulska --- tests/config/test_config_manager.py | 93 +++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/tests/config/test_config_manager.py b/tests/config/test_config_manager.py index 2ef1c68d9..c22ef3d4f 100644 --- a/tests/config/test_config_manager.py +++ b/tests/config/test_config_manager.py @@ -8,9 +8,12 @@ from collections.abc import Mapping, MutableMapping from dataclasses import dataclass from typing import Any +from unittest.mock import MagicMock import pytest from frequenz.channels import Broadcast +from frequenz.channels.file_watcher import Event, EventType +from pytest_mock import MockerFixture from frequenz.sdk.config import ConfigManagingActor from frequenz.sdk.config._config_managing import _recursive_update @@ -265,6 +268,96 @@ async def test_update_multiple_files(self, config_file: pathlib.Path) -> None: "dict_str_int": {"a": 1, "b": 2, "c": 4}, } + async def test_actor_works_if_not_all_config_files_exist( + self, config_file: pathlib.Path + ) -> None: + """Test ConfigManagingActor works if not all config files exist.""" + config_channel: Broadcast[Mapping[str, Any]] = Broadcast( + name="Config Channel", resend_latest=True + ) + config_receiver = config_channel.new_receiver() + + # This file does not exist + config_file2 = config_file.parent / "config2.toml" + + async with ConfigManagingActor( + [config_file, config_file2], + config_channel.new_sender(), + force_polling=False, + ): + config = await config_receiver.receive() + assert config is not None + assert config.get("var2") is None + + number = 5 + config_file.write_text(create_content(number=number)) + + config = await config_receiver.receive() + assert config is not None + assert config.get("var2") == str(number) + + # Create second config file that overrides the value from the first one + number = 42 + config_file2.write_text(create_content(number=number)) + + config = await config_receiver.receive() + assert config is not None + assert config.get("var2") == str(number) + + async def test_actor_does_not_crash_if_file_is_deleted( + self, config_file: pathlib.Path, mocker: MockerFixture + ) -> None: + """Test ConfigManagingActor does not crash if a file is deleted.""" + config_channel: Broadcast[Mapping[str, Any]] = Broadcast( + name="Config Channel", resend_latest=True + ) + config_receiver = config_channel.new_receiver() + + number = 5 + config_file2 = config_file.parent / "config2.toml" + config_file2.write_text(create_content(number=number)) + + # Not config file but existing in the same directory + any_file = config_file.parent / "any_file.txt" + any_file.write_text("content") + + file_watcher_mock = MagicMock() + file_watcher_mock.__anext__.side_effect = [ + Event(EventType.DELETE, any_file), + Event(EventType.MODIFY, config_file2), + Event(EventType.MODIFY, config_file), + ] + + mocker.patch( + "frequenz.channels.file_watcher.FileWatcher", return_value=file_watcher_mock + ) + + async with ConfigManagingActor( + [config_file, config_file2], + config_channel.new_sender(), + force_polling=False, + ) as actor: + send_config_spy = mocker.spy(actor, "send_config") + + config = await config_receiver.receive() + assert config is not None + assert config.get("var2") == str(number) + send_config_spy.assert_called_once() + send_config_spy.reset_mock() + + # Remove file and send DELETE events + any_file.unlink() + config_file2.unlink() + number = 101 + config_file.write_text(create_content(number=number)) + + config = await config_receiver.receive() + assert config is not None + assert config.get("var2") == str(number) + # Config should be updated only once on MODIFY event + # DELETE events are ignored + send_config_spy.assert_called_once() + @dataclass(frozen=True, kw_only=True) class RecursiveUpdateTestCase: From d8655e3b3d856af32a0102782aa55db01df845bc Mon Sep 17 00:00:00 2001 From: Elzbieta Kotulska Date: Thu, 28 Nov 2024 12:44:09 +0100 Subject: [PATCH 5/5] Fix ConfigManagingActor to handle missing config files gracefully Eliminate recursive actor crashes when config files are missing. The previous implementation suffered from a critical error pattern: - The `_read_config` method is called at the start of the `_run` method and on FileWatcher event. - When no config files existed, an exception would be raised - This exception caused the actor to crash and immediately restart - Restarting triggered the same `_read_config` method - The cycle repeated, creating a persistent crash loop This fix introduces a more robust approach: - Detect missing config files without throwing exceptions - Set up a FileWatcher to monitor for future config file creation - call `_read_config` method as soon as any config file is crated. Signed-off-by: Elzbieta Kotulska --- src/frequenz/sdk/config/_config_managing.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/frequenz/sdk/config/_config_managing.py b/src/frequenz/sdk/config/_config_managing.py index e66e2e461..b54d07890 100644 --- a/src/frequenz/sdk/config/_config_managing.py +++ b/src/frequenz/sdk/config/_config_managing.py @@ -107,14 +107,11 @@ def __init__( self._force_polling: bool = force_polling self._polling_interval: timedelta = polling_interval - def _read_config(self) -> abc.Mapping[str, Any]: + def _read_config(self) -> abc.Mapping[str, Any] | None: """Read the contents of the configuration file. Returns: A dictionary containing configuration variables. - - Raises: - ValueError: If config file cannot be read. """ error_count = 0 config: dict[str, Any] = {} @@ -138,14 +135,18 @@ def _read_config(self) -> abc.Mapping[str, Any]: error_count += 1 if error_count == len(self._config_paths): - raise ValueError(f"{self}: Can't read any of the config files") + _logger.error( + "%s: Can't read any of the config files, ignoring config update.", self + ) + return None return config async def send_config(self) -> None: """Send the configuration to the output sender.""" config = self._read_config() - await self._output.send(config) + if config is not None: + await self._output.send(config) async def _run(self) -> None: """Monitor for and send configuration file updates.