Skip to content

feat(cni): add file watcher for CNI config file #1566

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.

Sign up for GitHub

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

Open
AAdIprog wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(cni): add file watcher for CNI config file #1566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions pkg/cni/chained.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,10 +210,6 @@ func (i *Installer) chainedKmeshCniPlugin(mode string, cniMountNetEtcDIR string)
}
log.Infof("cni config file: %s", cniConfigFilePath)

/*
TODO: add watcher for cniConfigFile
*/

existCNIConfig, err := os.ReadFile(cniConfigFilePath)
if err != nil {
err = fmt.Errorf("failed to read cni config file %v : %v", cniConfigFilePath, err)
Expand Down
52 changes: 52 additions & 0 deletions pkg/cni/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,54 @@ func (i *Installer) WatchServiceAccountToken() error {
return nil
}

// WatchCNIConfigFile watches the CNI config file for external modifications
// and re-applies Kmesh CNI configuration when changes are detected.
// This ensures Kmesh CNI plugin remains installed even if other CNI plugins
// modify the config file.
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 {
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Member

@hzxuzhonghu hzxuzhonghu Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check, this is a dead loop

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)
Copy link
Contributor

@LiZhenCheng9527 LiZhenCheng9527 Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be preferable to use an easily understandable constant name.

}
}
Comment on lines +154 to +158
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

case err := <-i.Watcher.Errors(cniConfigPath):
if err != nil {
log.Errorf("error from CNI config file watcher: %v", err)
return
}
}
}
Comment on lines +143 to +166
Copy link

@gemini-code-assist gemini-code-assist bot Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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

}()

return nil
}
Comment on lines +128 to +170
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

func (i *Installer) Start() error {
if i.Mode == constants.KernelNativeMode || i.Mode == constants.DualEngineMode {
log.Info("start write CNI config")
Expand All @@ -133,6 +181,10 @@ func (i *Installer) Start() error {
if err := i.WatchServiceAccountToken(); err != nil {
return err
}

if err := i.WatchCNIConfigFile(); err != nil {
return err
}
}

return nil
Expand Down
Loading