Revamp config management #1134
Merged
Revamp config management #1134
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
part:docs
|
This is a draft because I need to complete/improve documentation and add tests, but the code should be pretty solid, and the existing tests pass (the CI is red because I left a couple of |
llucax
force-pushed
the
config-manager-3
branch
from
b7f55dd to
73e647e
Compare
|
If there are no major complains about this approach, I will move forward to clean it up so we can merge it soon. I will start doing that, so if anyone wants to review this, please do it sooner than later @frequenz-floss/python-sdk-team . |
Maybe too late for this, but these are the main changes compared to the old PR (I had them written down and forgot to put them in the PR when I created it):
|
Signed-off-by: Leandro Lucarella <[email protected]>
This is to be able to use type guards in with `Receiver.filter` to narrow the received type and the new `WithPrevious` class. Signed-off-by: Leandro Lucarella <[email protected]>
This class instantiates and starts the `ConfigManagingActor` and creates the channel needed to communicate with it. It offers a convenience method to get receivers to receive configuration updates. Signed-off-by: Leandro Lucarella <[email protected]>
When this option is enabled, configurations will be sent to the receiver only if they are different from the previously received configuration. Signed-off-by: Leandro Lucarella <[email protected]>
This forces a better configuration structure, by always having separate sections for separate entities, and avoids spurious updates and noise in the received configuration for other parties. Signed-off-by: Leandro Lucarella <[email protected]>
Configuration can be nested, and actors could have sub-actors that need
their own configuration, so we need to support filtering the
configuration by a sequence of keys.
For example, if the configuration is `{"key": {"subkey": "value"}}`, and
the key is `["key", "subkey"]`, then the receiver will get only
`"value"`.
Signed-off-by: Leandro Lucarella <[email protected]>
This commit adds support for validating configurations with a schema. This is useful to ensure the configuration is correct and to receive it as an instance of a dataclass. The `new_receiver` method now requires a `config_class` argument that is used to validate the configuration. If the configuration doesn't pass the validation, it will be ignored and an error will be logged. The schema is expected to be a dataclass, which is used to create a marshmallow schema to validate the configuration. To customize the schema, you can use `marshmallow_dataclass` to specify extra metadata. Validation errors are logged and the `ValidationError` instance is sent to the receiver. Signed-off-by: Leandro Lucarella <[email protected]>
If `unkown` is not specified in the `marshmallow_load_kwargs`, it will default to `marshmallow.EXCLUDE` instead of `marshmallow.RAISE`, as this is what most users will need when loading to a dataclass. But when `marshmallow.EXCLUDE` is used, a warning will be logged if there are fields in the configuration that are being excluded, so the user can still be aware of it and catch typos in the configuration file. Signed-off-by: Leandro Lucarella <[email protected]>
This schema is provided to use as a default, and might be extended in the future to support more commonly used fields that are not provided by marshmallow by default. To use the quantity schema we need to bump the `frequenz-quantities` dependency and add the optional `marshmallow` dependency. Signed-off-by: Leandro Lucarella <[email protected]>
The `LoggingConfigUpdatingActor` can now also receive `None` as configuration (for example if the configuration is removed), in which case it will go back to the default configuration. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
We were using `str(self)` as prefix, which prints the class name, but the logger name already has enough information to figure out this log comes from the config managing actor, so we now use only the unique ID (name) of the actor in the logs. Signed-off-by: Leandro Lucarella <[email protected]>
This function has a default timeout, so waiting for the first configuration can be automatically aborted when it takes too long. The method will also reports some progress, logging errors if there were issues receiving or loading the configuration, or if the configuration is missing, setting a logging baseline for all actors. Signed-off-by: Leandro Lucarella <[email protected]>
We now wait for the first configuration using the new `wait_for_first()` utility function. We also make the logging more explicity about when the old configuration is being kept and why. Signed-off-by: Leandro Lucarella <[email protected]>
Both the `ConfigManager` and the `ConfigManagingActor` can now take a single configuration file, both as a `str` or a `Path`. This also fixes a bug where `str` was still accepted, but taken as a `Sequence[str]`. This is a known issue with Python typing: python/mypy#11001 And while we are it, we now also explicitly reject empty sequences, raising a `ValueError` when one is passed. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
llucax
force-pushed
the
config-manager-3
branch
from
73e647e to
0d54f4f
Compare
|
OK, this is ready for review. I completed the release notes, added tests, added comprehensive documentation, including recommended practices when writing actors and apps (please review!) and a user guide, and fixed a bug in the actor (and manager) when passing a string as a config file (now both accept also a single file). I prepared the release notes to release after this is merged. |
Add module-level documentation to the `frequenz.sdk.config` module and include it in the generated documentation's user guide. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
llucax
force-pushed
the
config-manager-3
branch
from
0d54f4f to
4c87e6a
Compare
ela-kotulska-frequenz
approved these changes
Jan 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request adds a new
ConfigManagerclass that takes care of the client-side of theConfigManagingActor, providing the common tasks users of the config system need to do.The class owns the config channel and provides a convenient method to get new receivers that validate the configuration using a
dataclassas amarshmallowschema, and allow filtering to receive only configuration for a specific key. The receiver also reports errors when the configuration can't be properly loaded, and avoid spurious updates when the configuration didn't change compared to the previous one.The manager also takes care of instantiating both the
ConfigManagingActorand theLoggingConfigUpdatingActorand provides a utility function to wait for the first configuration to be received.