ROX-30836: implement configuration hotreloading #110
Conversation
Molter73
force-pushed
the
mauro/ROX-30836/config-hotreload
branch
5 times, most recently
from
cdb1f97 to
d367f7c
Compare
Molter73
changed the title
Molter73
force-pushed
the
mauro/ROX-30836/config-hotreload
branch
from
d367f7c to
b4c7d6c
Compare
While testing #110, I realised when running fact with `--help` the usage message would start with the `Error: ` string, like this: ``` Error: Usage: fact [OPTIONS] [URL] ``` This is due to us incorrectly using the `try_parse` method on our CLI configuration and propagating the error up. Instead, we can call `parse` and the application will come to a full stop whenever a wrong argument is found or the `-h` or `--help` arguments are provided, showing the correct usage message. The change also makes it so configuration is parsed before dumping system information. This is more of a nitpick, having the system information dumped before the usage message is not a big deal, but I think this way makes more sense. Additionally, errors parsing configuration and CLI arguments _should not_ be caused by the actual system, so I don't think we are losing anything.
While testing #110, I realised when running fact with `--help` the usage message would start with the `Error: ` string, like this: ``` Error: Usage: fact [OPTIONS] [URL] ``` This is due to us incorrectly using the `try_parse` method on our CLI configuration and propagating the error up. Instead, we can call `parse` and the application will come to a full stop whenever a wrong argument is found or the `-h` or `--help` arguments are provided, showing the correct usage message. The change also makes it so configuration is parsed before dumping system information. This is more of a nitpick, having the system information dumped before the usage message is not a big deal, but I think this way makes more sense. Additionally, errors parsing configuration and CLI arguments _should not_ be caused by the actual system, so I don't think we are losing anything.
After implementing this patch it became apparent the effort to fully implement configuration reloading was too big for a single commit, so this will be patch 1/N. A new reloader submodule is implemented for the configuration which will be in charge of deciding if configuration has been changed, reload it and notify any relevant tasks that they should handle the new configuration as required. For testing purposes and simplicity, the first component to get hot reloading is the exposed hyper endpoints. The configuration for this module has been split into its own `EndpointConfig` structure, this makes it possible to notify of changes to just this subset of fields, which should mean other parts of the code will be unaffected when only the endpoint configuration is changed. Ultimately, all configuration will be split in such a way and tasks will get notified only when relevant configuration is modified. For simplicity, the reloader will iterate over the list of configuration files it is meant to monitor every 10 seconds and trigger a reloading when the modification time of at least one of these has changed. In the future we could use our own BPF hooks to trigger this condition, but for now I decided to do a simple as possible implementation. SIGHUP can also be used to force a reloading of configuration, this is an established pattern for daemons and also useful for faster testing.
Environment variables and CLI arguments can only be changed with a process restart, so there is no point in parsing them every time we hot reload our configuration. Instead, we can put the parsed configuration in a `LazyLock` which will be initialized when we first parse configuration on startup and then re-used when we hot reload.
Molter73
force-pushed
the
mauro/ROX-30836/config-hotreload
branch
from
8d0b047 to
29da1d6
Compare
| Ok(config) | ||
| } | ||
|
|
||
| fn build() -> anyhow::Result<FactConfig> { |
There was a problem hiding this comment.
Feel free to ignore my review. I think build should continue to take in paths. It makes it more flexible. CONFIG_PATHS can be passed to build. Or use Option to pass in the path and use CONFIG_PATHS if it is none.
There was a problem hiding this comment.
The approach taken here is somewhat akin to a builder pattern, meaning we should add anything we need before calling build() and this method will just output the resulting configuration. Currently the list of paths is defined statically, but in the future we might add a add_path(path: PathBuf) method in order to provide additional files if needed, build() will continue to only process and generate the resulting configuration.
There was a problem hiding this comment.
I've been looking into implementing a separate FactConfigBuilder struct that should make this pattern more obvious, because this PR is already quite bloated I will implement it in a follow up if that is ok with you.
There was a problem hiding this comment.
#135 is a draft of the FactConfigBuilder struct I mentioned above.
fact/src/config/reloader.rs
Outdated
| } | ||
| } else if self.files.contains_key(&path) { | ||
| res = true; | ||
| self.files.remove(&path); |
There was a problem hiding this comment.
It might be worth it to log what files are changed, added, and deleted.
There was a problem hiding this comment.
Added some debug statements on the latest commit.
ovalenti
left a comment
ovalenti
left a comment
There was a problem hiding this comment.
You mention the SIGUSR in the PR description and I did not see it in the code. We could probably go without it, but just to be sure.
I mentioned |
@Molter73 It is not you.... I missed the registration line 😅 |
Molter73
force-pushed
the
mauro/ROX-30836/config-hotreload
branch
from
01e187f to
6f07b6e
Compare
3 participants
Description
After implementing this patch it became apparent the effort to fully implement configuration reloading was too big for a single commit, so this will be patch 1/N.
A new reloader submodule is implemented for the configuration which will be in charge of deciding if configuration has been changed, reload it and notify any relevant tasks that they should handle the new configuration as required. For testing purposes and simplicity, the first component to get hot reloading is the exposed hyper endpoints. The configuration for this module has been split into its own
EndpointConfigstructure, this makes it possible to notify of changes to just this subset of fields, which should mean other parts of the code will be unaffected when only the endpoint configuration is changed. Ultimately, all configuration will be split in such a way and tasks will get notified only when relevant configuration is modified.For simplicity, the reloader will iterate over the list of configuration files it is meant to monitor every 10 seconds and trigger a reloading when the modification time of at least one of these has changed. In the future we could use our own BPF hooks to trigger this condition, but for now I decided to do a simple as possible implementation.
SIGHUP can also be used to force a reloading of configuration, this is an established pattern for daemons and also useful for faster testing.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Manually tested enabling/disabling endpoints, changing the address for them and added integration tests for those same use cases.