-
Notifications
You must be signed in to change notification settings - Fork 4
DRAFT: Add Configuration Management with Hot Reload Support #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
+1,005
−9
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
13c42e3
Add Configuration Management with Hot Reload Support
ChrisJBurns edfbf09
removes write
ChrisJBurns 40aca25
Merge branch 'main' into hot-reload-config-file
ChrisJBurns aa61309
removes duplicate test
ChrisJBurns 328d632
oops
ChrisJBurns da03764
Merge branch 'main' into hot-reload-config-file
ChrisJBurns 05f2fc0
Merge branch 'main' into hot-reload-config-file
ChrisJBurns File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,224 @@ | ||
| package config | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "sync" | ||
|
|
||
| "github.com/fsnotify/fsnotify" | ||
| "github.com/stacklok/toolhive/pkg/logger" | ||
| ) | ||
|
|
||
| // ConfigManager provides thread-safe, read-only configuration management. | ||
| // Configuration files are never modified by the application - all updates come from | ||
| // external sources (Kubernetes ConfigMaps, Docker volume mounts, orchestration tools). | ||
| // | ||
| // Design for read-only operation: | ||
| // - No file locking needed - we only observe external changes | ||
| // - No write coordination required - we never modify files | ||
| // - Uses sync.RWMutex optimized for concurrent reads | ||
| // - Validates externally-updated configs before applying | ||
| // - Preserves last known good configuration on invalid updates | ||
| // - Optimized for container environments with atomic file updates | ||
| type ConfigManager interface { | ||
| // GetConfig safely retrieves the current configuration | ||
| GetConfig() *Config | ||
|
|
||
| // 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 | ||
|
|
||
| // WatchConfig observes the configuration file for external changes. | ||
| // Automatically reloads when the file is updated by external systems. | ||
| // Blocks until context is cancelled. | ||
| WatchConfig(ctx context.Context) error | ||
|
|
||
| // Close releases the file watcher resources | ||
| Close() error | ||
| } | ||
|
|
||
| // ConfigValidator defines the interface for validating configurations | ||
| // This allows custom validation logic to be injected beyond the basic validation | ||
| type ConfigValidator interface { | ||
| Validate(config *Config) error | ||
| } | ||
|
|
||
| // defaultValidator uses the Config's built-in validation | ||
| type defaultValidator struct{} | ||
|
|
||
| // Validate delegates to the Config's own Validate method | ||
| func (v *defaultValidator) Validate(config *Config) error { | ||
| return config.Validate() | ||
| } | ||
|
|
||
| // configManager is the concrete implementation of ConfigManager | ||
| type configManager struct { | ||
| mu sync.RWMutex // Protects concurrent access to config | ||
| config *Config // Current active configuration | ||
| configPath string // Path to configuration file | ||
| loader ConfigLoader // Loader for reading config files | ||
| validator ConfigValidator // Validator for checking config validity | ||
| watcher *fsnotify.Watcher // File system watcher (nil if not watching) | ||
| watcherMu sync.Mutex // Protects watcher field | ||
| } | ||
|
|
||
| // ConfigManagerOption allows customizing ConfigManager behavior | ||
| type ConfigManagerOption func(*configManager) | ||
|
|
||
| // WithValidator sets a custom validator for the config manager | ||
| func WithValidator(validator ConfigValidator) ConfigManagerOption { | ||
| return func(cm *configManager) { | ||
| cm.validator = validator | ||
| } | ||
| } | ||
|
|
||
| // WithLoader sets a custom config loader for the config manager | ||
| func WithLoader(loader ConfigLoader) ConfigManagerOption { | ||
| return func(cm *configManager) { | ||
| cm.loader = loader | ||
| } | ||
| } | ||
|
|
||
| // NewConfigManager creates a new ConfigManager with the given configuration file path. | ||
| // It loads and validates the initial configuration. | ||
| // Returns error if initial load or validation fails. | ||
| func NewConfigManager(configPath string, opts ...ConfigManagerOption) (ConfigManager, error) { | ||
| cm := &configManager{ | ||
| configPath: configPath, | ||
| loader: NewConfigLoader(), | ||
| validator: &defaultValidator{}, // Uses Config.Validate() by default | ||
| } | ||
|
|
||
| // Apply options | ||
| for _, opt := range opts { | ||
| opt(cm) | ||
| } | ||
|
|
||
| // Load initial configuration | ||
| if err := cm.ReloadConfig(); err != nil { | ||
| return nil, fmt.Errorf("failed to load initial configuration: %w", err) | ||
| } | ||
|
|
||
| return cm, nil | ||
| } | ||
|
|
||
| // GetConfig safely retrieves the current configuration | ||
| // Multiple goroutines can safely call this method concurrently | ||
| func (cm *configManager) GetConfig() *Config { | ||
| cm.mu.RLock() | ||
| defer cm.mu.RUnlock() | ||
|
|
||
| // Return a copy to prevent external modifications | ||
| // This is a shallow copy, which is sufficient for our use case | ||
| // as the Config struct fields are not modified in place | ||
| configCopy := *cm.config | ||
| return &configCopy | ||
| } | ||
|
|
||
| // ReloadConfig reads the configuration file and applies it if valid. | ||
| // The file is treated as read-only - we never modify it. | ||
| // If the new configuration is invalid, the previous configuration remains active. | ||
| func (cm *configManager) ReloadConfig() error { | ||
| // Read the configuration file (read-only operation) | ||
| newConfig, err := cm.loader.LoadConfig(cm.configPath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read config file: %w", err) | ||
| } | ||
|
|
||
| // Validate before applying | ||
| if err := cm.validator.Validate(newConfig); err != nil { | ||
| return fmt.Errorf("invalid configuration: %w", err) | ||
| } | ||
|
|
||
| // Atomically update the in-memory configuration | ||
| cm.mu.Lock() | ||
| cm.config = newConfig | ||
| cm.mu.Unlock() | ||
|
|
||
| logger.Infof("Configuration reloaded from %s", cm.configPath) | ||
| return nil | ||
| } | ||
|
|
||
| // WatchConfig observes the configuration file for external changes. | ||
| // Since we never write to the file, all changes come from external sources: | ||
| // - Kubernetes ConfigMap updates (atomic via symlink swaps) | ||
| // - Docker volume mount updates | ||
| // - Configuration management tools | ||
| // | ||
| // This method blocks until the context is cancelled. | ||
| func (cm *configManager) WatchConfig(ctx context.Context) error { | ||
| cm.watcherMu.Lock() | ||
| if cm.watcher != nil { | ||
| cm.watcherMu.Unlock() | ||
| return fmt.Errorf("config watcher is already running") | ||
| } | ||
|
|
||
| watcher, err := fsnotify.NewWatcher() | ||
| if err != nil { | ||
| cm.watcherMu.Unlock() | ||
| return fmt.Errorf("failed to create file watcher: %w", err) | ||
| } | ||
| cm.watcher = watcher | ||
| cm.watcherMu.Unlock() | ||
|
|
||
| // Add config file to watcher | ||
| if err := watcher.Add(cm.configPath); err != nil { | ||
| return fmt.Errorf("failed to watch config file %s: %w", cm.configPath, err) | ||
| } | ||
|
|
||
| logger.Infof("Started watching configuration file: %s", cm.configPath) | ||
|
|
||
| // Watch loop | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| logger.Info("Stopping config file watcher due to context cancellation") | ||
| return ctx.Err() | ||
|
|
||
| case event, ok := <-watcher.Events: | ||
| if !ok { | ||
| return fmt.Errorf("watcher event channel closed") | ||
| } | ||
|
|
||
| // Detect external file modifications | ||
| if event.Has(fsnotify.Write) || event.Has(fsnotify.Create) { | ||
| logger.Infof("External config update detected, reloading") | ||
|
|
||
| if err := cm.ReloadConfig(); err != nil { | ||
| logger.Errorf("Failed to reload config: %v", err) | ||
| // Continue observing - previous config remains active | ||
| } | ||
| } | ||
|
|
||
| // Handle K8s ConfigMap updates (may remove/recreate symlinks) | ||
| if event.Has(fsnotify.Remove) { | ||
| logger.Debugf("Config file removed (K8s update), re-watching") | ||
| _ = watcher.Add(cm.configPath) | ||
| } | ||
|
|
||
| case err, ok := <-watcher.Errors: | ||
| if !ok { | ||
| return fmt.Errorf("watcher error channel closed") | ||
| } | ||
| // Log error but continue watching | ||
| logger.Errorf("File watcher error: %v", err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Close releases resources held by the config manager | ||
| // Specifically, it closes the file watcher if active | ||
| func (cm *configManager) Close() error { | ||
| cm.watcherMu.Lock() | ||
| defer cm.watcherMu.Unlock() | ||
|
|
||
| if cm.watcher != nil { | ||
| if err := cm.watcher.Close(); err != nil { | ||
| return fmt.Errorf("failed to close file watcher: %w", err) | ||
| } | ||
| cm.watcher = nil | ||
| logger.Info("Config watcher closed") | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By
cold reloadyou mean an API like thePOST /registry/syncthat 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
MCPRegistryspecs during the reconciliation cycle. Optimizations could be tracked by a separate issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how can we change the configuration in K8s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#59 waits for another approval and we can merge