add hot reloading for fs config #2179
Conversation
wswsmao
force-pushed
the
hotreload
branch
2 times, most recently
from
8486693 to
9615064
Compare
| defer w.mu.Unlock() | ||
|
|
||
| if w.lastConfig != nil { | ||
| revertBlacklistedChanges(ctx, w.lastConfig, &newFsConfig) |
There was a problem hiding this comment.
It looks like each package manages its own whitelist in UpdateConfig. Why do we need an additional blacklist here?
| The config file can be passed to stargz snapshotter using `containerd-stargz-grpc`'s `--config` option. | ||
|
|
||
| ## Configuration hot reload |
There was a problem hiding this comment.
The benefit of this feature compared to the FUSE manager (which already supports safe restarts) should be documented.
Signed-off-by: abushwang <[email protected]>
|
All reviews done @ktock |
ktock
left a comment
ktock
left a comment
There was a problem hiding this comment.
Can we make it optional and enable only when a flag is specified to contaienrd-stargz-grpc to keep the consistency with the behaviour of the previous versions of the snapshotter? We should also have tests for this feature and docs about which exact field support the hot reloading.
| if event.Name == absFilePath { | ||
| // Trigger on Write, Create, Rename, or Chmod events | ||
| // such as vim, nano, etc. | ||
| if event.Has(fsnotify.Write) || event.Has(fsnotify.Create) || | ||
| event.Has(fsnotify.Rename) || event.Has(fsnotify.Chmod) { |
There was a problem hiding this comment.
fsnotify.Rename
If the file is renamed, the next change won't be detected, will it? Is this an expected behaviour?
|
|
||
| // WatchConfig monitors the specified configuration file for changes. | ||
| // It triggers the config reload when a change is detected. | ||
| func WatchConfig( |
There was a problem hiding this comment.
You don't need to export this symbol.
| var ( | ||
| debounceTimer *time.Timer | ||
| mu sync.Mutex |
There was a problem hiding this comment.
Why is debounceTimer protected by the mutex? Is this accessed from multiple goroutines? I cound't find new goroutine creation from inside of this function.
|
|
||
| watchDir := filepath.Dir(absFilePath) | ||
|
|
||
| watcher, err := fsnotify.NewWatcher() |
2 participants
Fixes: #2178
This PR implements hot reloading, but only for fs configurations. My rationale is to reserve this feature for settings that are truly optional improvements.
The fs settings fit this model perfectly: the system is fully functional out-of-the-box, but users can tweak them for better results. Because the "best" value can be subjective or workload-dependent, users may want to adjust them frequently. Hot reloading makes this process of discovery and tuning seamless.
For higher-level settings that control fundamental behavior, hot reloading offers little benefit and introduces potential risk. A restart is the more appropriate mechanism for enabling or altering core features.