feat(cni): add file watcher for CNI config file#1566
feat(cni): add file watcher for CNI config file#1566AAdIprog wants to merge 1 commit intokmesh-net:mainfrom
Conversation
Add WatchCNIConfigFile() function to monitor the CNI config file for external modifications and re-apply Kmesh CNI configuration when changes are detected. This ensures Kmesh CNI plugin remains installed even if other CNI plugins modify the config file. - Remove TODO comment for cniConfigFile watcher in chained.go - Add WatchCNIConfigFile() function in install.go - Integrate watcher in Start() function Signed-off-by: Aadi Shah <your-email@example.com> Signed-off-by: AAdIprog <aadishah132@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @AAdIprog, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the Kmesh CNI plugin by implementing a file watcher for its configuration file. This ensures that even if other CNI plugins modify the shared CNI configuration, the Kmesh plugin's settings are automatically restored, preventing unintended disruptions to its operation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Config file shifts, Watcher wakes, Kmesh restores, Order reigns again. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a file watcher for the CNI configuration file, which is a great addition for ensuring the Kmesh CNI plugin remains active even when other plugins modify the configuration. The implementation follows the existing pattern for watching the service account token.
My review focuses on a potential issue in the new watcher's goroutine. I've identified a scenario that could lead to high CPU usage when the watcher is stopped and have provided a code suggestion to address it. This fix would also be applicable to the existing service account token watcher.
| for { | ||
| select { | ||
| case <-timerC: | ||
| timerC = nil | ||
| log.Infof("CNI config file changed, re-applying Kmesh CNI configuration") | ||
| if err := i.chainedKmeshCniPlugin(i.Mode, i.CniMountNetEtcDIR); err != nil { | ||
| log.Errorf("failed to re-apply Kmesh CNI config: %v", err) | ||
| } | ||
|
|
||
| case event := <-i.Watcher.Events(cniConfigPath): | ||
| log.Debugf("got CNI config event %s", event.String()) | ||
| if event.Has(fsnotify.Write) || event.Has(fsnotify.Create) { | ||
| if timerC == nil { | ||
| timerC = time.After(100 * time.Millisecond) | ||
| } | ||
| } | ||
|
|
||
| case err := <-i.Watcher.Errors(cniConfigPath): | ||
| if err != nil { | ||
| log.Errorf("error from CNI config file watcher: %v", err) | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of the file watcher goroutine has a potential issue. When i.Watcher.Close() is called (in Stop()), the Events and Errors channels will be closed. Reading from a closed channel returns a zero value immediately without blocking. This will cause the select statement inside the for loop to spin, consuming a CPU core.
To fix this, you should check if the channels are closed when reading from them. You can do this using the two-variable assignment value, ok := <-channel. If ok is false, the channel is closed, and the goroutine should exit.
This issue also exists in WatchServiceAccountToken. While it's outside the scope of this PR's changes, it would be good to fix it there as well for consistency and correctness.
for {
select {
case <-timerC:
timerC = nil
log.Infof("CNI config file changed, re-applying Kmesh CNI configuration")
if err := i.chainedKmeshCniPlugin(i.Mode, i.CniMountNetEtcDIR); err != nil {
log.Errorf("failed to re-apply Kmesh CNI config: %v", err)
}
case event, ok := <-i.Watcher.Events(cniConfigPath):
if !ok {
log.Info("CNI config file watcher stopped")
return
}
log.Debugf("got CNI config event %s", event.String())
if event.Has(fsnotify.Write) || event.Has(fsnotify.Create) {
if timerC == nil {
timerC = time.After(100 * time.Millisecond)
}
}
case err, ok := <-i.Watcher.Errors(cniConfigPath):
if !ok {
log.Info("CNI config file watcher stopped")
return
}
if err != nil {
log.Errorf("error from CNI config file watcher: %v", err)
return
}
}
}There was a problem hiding this comment.
Pull request overview
This PR adds a file watcher for the CNI config file to automatically detect and respond to external modifications by re-applying the Kmesh CNI configuration. This addresses a TODO comment in the codebase and ensures the Kmesh CNI plugin remains properly configured even when other CNI plugins modify the shared config file.
Changes:
- Added
WatchCNIConfigFile()function that monitors the CNI config file and re-applies Kmesh configuration when changes are detected - Integrated the watcher into the
Start()function to begin monitoring after initial setup - Removed the TODO comment that was addressed
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/cni/install.go | Adds new WatchCNIConfigFile() function following the same pattern as WatchServiceAccountToken(), with file watching, debounce logic, and automatic re-application of config; integrates the watcher into the Start() function |
| pkg/cni/chained.go | Removes the TODO comment about adding a watcher for the CNI config file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if event.Has(fsnotify.Write) || event.Has(fsnotify.Create) { | ||
| if timerC == nil { | ||
| timerC = time.After(100 * time.Millisecond) | ||
| } | ||
| } |
There was a problem hiding this comment.
The watcher does not handle Remove or Rename events for the CNI config file. If the file is deleted or renamed by another CNI plugin, the watcher will stop receiving events but the goroutine will continue running. Consider adding event handlers for fsnotify.Remove and fsnotify.Rename events to either re-add the watch or take appropriate action.
| func (i *Installer) WatchCNIConfigFile() error { | ||
| cniConfigPath, err := i.getCniConfigPath() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get CNI config path: %v", err) | ||
| } | ||
|
|
||
| if err := i.Watcher.Add(cniConfigPath); err != nil { | ||
| return fmt.Errorf("failed to add %s to file watcher: %v", cniConfigPath, err) | ||
| } | ||
|
|
||
| // Start listening for events. | ||
| go func() { | ||
| log.Infof("start watching CNI config file %s", cniConfigPath) | ||
|
|
||
| var timerC <-chan time.Time | ||
| for { | ||
| select { | ||
| case <-timerC: | ||
| timerC = nil | ||
| log.Infof("CNI config file changed, re-applying Kmesh CNI configuration") | ||
| if err := i.chainedKmeshCniPlugin(i.Mode, i.CniMountNetEtcDIR); err != nil { | ||
| log.Errorf("failed to re-apply Kmesh CNI config: %v", err) | ||
| } | ||
|
|
||
| case event := <-i.Watcher.Events(cniConfigPath): | ||
| log.Debugf("got CNI config event %s", event.String()) | ||
| if event.Has(fsnotify.Write) || event.Has(fsnotify.Create) { | ||
| if timerC == nil { | ||
| timerC = time.After(100 * time.Millisecond) | ||
| } | ||
| } | ||
|
|
||
| case err := <-i.Watcher.Errors(cniConfigPath): | ||
| if err != nil { | ||
| log.Errorf("error from CNI config file watcher: %v", err) | ||
| return | ||
| } | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The WatchCNIConfigFile function lacks test coverage. Similar to WatchServiceAccountToken which has a test in install_test.go, this new function should have corresponding test coverage to ensure the file watching and re-apply logic works correctly.
| case <-timerC: | ||
| timerC = nil | ||
| log.Infof("CNI config file changed, re-applying Kmesh CNI configuration") | ||
| if err := i.chainedKmeshCniPlugin(i.Mode, i.CniMountNetEtcDIR); err != nil { |
There was a problem hiding this comment.
Calling chainedKmeshCniPlugin will trigger a write to the CNI config file being watched, which will cause this watcher to detect its own write and trigger another re-apply. This creates an infinite loop of file writes. Consider checking if the Kmesh plugin is already present in the config before re-applying, or implement a mechanism to distinguish between external changes and self-triggered changes.
There was a problem hiding this comment.
Please check, this is a dead loop
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| log.Debugf("got CNI config event %s", event.String()) | ||
| if event.Has(fsnotify.Write) || event.Has(fsnotify.Create) { | ||
| if timerC == nil { | ||
| timerC = time.After(100 * time.Millisecond) |
There was a problem hiding this comment.
It would be preferable to use an easily understandable constant name.
|
@YaoZengzeng @Okabe-Rintarou-0 Please take a look at this |
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds a file watcher for the CNI config file to address the TODO comment in
pkg/cni/chained.go.Changes:
WatchCNIConfigFile()function to monitor the CNI config file for external modificationsThe watcher uses the same pattern as the existing
WatchServiceAccountToken()function, with a 100ms debounce timer to handle rapid file changes efficiently.Which issue(s) this PR fixes:
Fixes #1565
Special notes for your reviewer:
fsnotifyvia thefilewatcherpackage already in use by the projectWriteandCreateevents on the CNI config fileDoes this PR introduce a user-facing change?: