Skip to content

feat: enable configuration hot-reloading - #231

Merged
ltitanb merged 26 commits into
mainfrom
mb/hot_reloading
Jan 22, 2025
Merged

feat: enable configuration hot-reloading#231
ltitanb merged 26 commits into
mainfrom
mb/hot_reloading

Conversation

@ManuelBilbao

@ManuelBilbao ManuelBilbao commented Jan 10, 2025

Copy link
Copy Markdown
Collaborator

Added hot-reloading for configuration in PBS and signer modules. This expose a /reload endpoint on each module that re-parse the configuration file and apply the changes to their config/manager structures.
Changes to listening hosts and ports will not be applied as they require the server to restart. Also, path-related changes may not work if the module is running in Docker, as they require the container to be recreated.

Close #219

@ManuelBilbao
ManuelBilbao marked this pull request as ready for review January 14, 2025 16:58
@ManuelBilbao
ManuelBilbao requested a review from ltitanb January 14, 2025 16:58
@ltitanb

ltitanb commented Jan 14, 2025

Copy link
Copy Markdown
Collaborator

mm let's check with @blevkivskyi-everstake what would be his preferred use case but this feels a bit overly complicated to me? why not just have a watcher to the file (eg with notify) and trigger a config update when it's modified?

@blevkivskyi-everstake

Copy link
Copy Markdown

I'm not sure if it will work correctly with container environments.
Also, there is a corner case where the config is invalid, and commit-boost will crash on the next reload.

@ltitanb

ltitanb commented Jan 15, 2025

Copy link
Copy Markdown
Collaborator

I would say that if the reload fails, then the config is not updated. At startup it would fail ofc which is the normal behaviour

@blevkivskyi-everstake

Copy link
Copy Markdown

If it works with files from includes - ok

image

@ManuelBilbao

Copy link
Copy Markdown
Collaborator Author

why not just have a watcher to the file (eg with notify) and trigger a config update when it's modified?

I thought that instant updates can cause some problems, as you might save the file several times before having a desirable version? Having an endpoint instead gives you control over when the config is updated.
Let me know if you consider that this is not a problem and I'll take that approach

I would say that if the reload fails, then the config is not updated. At startup it would fail ofc which is the normal behaviour

That is correct. If the reload fails (i.e. some config is wrong), the config is not updated and the request returns a 500 error.

If it works with files from includes - ok

Just to clarify, do you want to hot-reload the server when pubkey_commit_boost/test_mux.json change in addition to the config file?

@blevkivskyi-everstake

Copy link
Copy Markdown

Just to clarify, do you want to hot-reload the server when pubkey_commit_boost/test_mux.json change in addition to the config file?

Yes.

@ManuelBilbao

Copy link
Copy Markdown
Collaborator Author

The current implementation should work then, using the /reload endpoint. Let me know if you prefer the file watcher option (automatic updates on save) instead.

@ank-everstake

Copy link
Copy Markdown

Yes, reloading all included files is required.
We prefer /reload option that gives more flexibility and control then the file watcher.

@ltitanb ltitanb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall! Some smaller changes/suggestions

Comment thread crates/pbs/src/state.rs Outdated
Comment thread docs/docs/get_started/configuration.md
Comment thread crates/signer/src/service.rs Outdated
Comment thread api/signer-api.yml Outdated
Comment thread crates/pbs/src/routes/register_validator.rs Outdated
Comment thread crates/pbs/src/routes/reload.rs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEAT] Add hot-reload via api

4 participants