DRAFT: Add Configuration Management with Hot Reload Support #47
Conversation
- pkg/config/config.go - ConfigLoader interface and YAML parsing for the specified config schema - pkg/config/manager.go - Thread-safe ConfigManager with file watching capabilities - pkg/config/config_test.go - Tests for config loading and validation - pkg/config/manager_test.go - Tests for ConfigManager including concurrent access and file watching - pkg/config/example.yaml - Example configuration file Signed-off-by: ChrisJBurns <[email protected]>
Signed-off-by: ChrisJBurns <[email protected]>
|
|
||
| // ReloadConfig reads the latest configuration from disk and applies it if valid. | ||
| // The file is only read, never written. Returns error if the new config is invalid. | ||
| ReloadConfig() error |
There was a problem hiding this comment.
Do you think we /need/ the hot reloading? I'm concerned that this will force us to manage requests in flight, e.g. for synchronization of different data sources for sources that might have dissapeared etc.
Isn't it in the end just cleaner to force a cold reload?
There was a problem hiding this comment.
By cold reload you mean an API like the POST /registry/sync that we already planned for manual sync?
In any case, even this solution would require some serialization of sync requests to avoid the sync issues you just mentioned: but the main goal here was to expose the same features as today, e.g. propagate the changes to the MCPRegistry specs during the reconciliation cycle. Optimizations could be tracked by a separate issue.
There was a problem hiding this comment.
No, by cold reload I meant that the registry server only reads its configuration on startup. The sync is orthogonal, it syncs an existing registry, the part of the hot reload I was concerned about was when a new registry is added or worse an existing removed.
There was a problem hiding this comment.
So how can we change the configuration in K8s?
There was a problem hiding this comment.
I believe the configuration will get updated automatically via the volumeMount, however the registry server would have already started. I suspect we will have to "restart" the registry server pod when the configmap has been updated?
There was a problem hiding this comment.
so the controller has to restart the server's Deployment any time the spec changes?
if we agree, let's add a task under the thv-operator to implement this behavior
There was a problem hiding this comment.
I think it would make for an easier and less error-prone implementation over the long term. I think it's also easier to start this way - hopefully changes to the spec by the admin would be minimal after some initial deployment going back and forth. If there is a need for more complexity and on-the-fly config changes, I'd rather implement that later.
There was a problem hiding this comment.
@ChrisJBurns if this is decided, do you want to review this PR and just remove the ReloadConfig method or should we keep the original design?
Also, I tracked #60 to postpone the hot reload function
There was a problem hiding this comment.
Yep, when #59 has been merged, i'll rebase this PR and take the hot reload stuff out. This wasn't 100% done in the first place, I just like getting drafts up because (for me) reading the diffs in github is easier than locally :D and helps me keep track of the amount of changes I'm making so I can keep them below 1k for easier review
There was a problem hiding this comment.
#59 waits for another approval and we can merge
Signed-off-by: ChrisJBurns <[email protected]>
Signed-off-by: ChrisJBurns <[email protected]>
Signed-off-by: ChrisJBurns <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
+ Coverage 60.80% 61.56% +0.75%
==========================================
Files 33 34 +1
Lines 2133 2209 +76
==========================================
+ Hits 1297 1360 +63
- Misses 736 744 +8
- Partials 100 105 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: ChrisJBurns <[email protected]>
The registry server will need the ability to reload the config on the fly via hot reloads. This PR adds the base of that functionality. The next PR will be the use of it.
Ref: #40