Skip to content

Conversation

@ManuelBilbao
Copy link
Collaborator

@ManuelBilbao ManuelBilbao commented Jan 10, 2025

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
Copy link
Collaborator

ltitanb commented Jan 14, 2025

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

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
Copy link
Collaborator

ltitanb commented Jan 15, 2025

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

If it works with files from includes - ok

image

@ManuelBilbao
Copy link
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

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
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

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

Copy link
Collaborator

@ltitanb ltitanb left a comment

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

@ltitanb ltitanb merged commit 248538a into main Jan 22, 2025
4 checks passed
@ltitanb ltitanb deleted the mb/hot_reloading branch January 22, 2025 20:41
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

5 participants