-
Notifications
You must be signed in to change notification settings - Fork 10
adding secret watcher to restart ptp4l process #111
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for your PR, |
pkg/daemon/daemon.go
Outdated
| chronydProcessName, // there can be only one chronyd process in the system | ||
| } | ||
|
|
||
| // saFileInfo tracks authentication file information for a profile |
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.
What specifically is sa short for in this context?
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.
security association, sa_file is an option inside the ptp4lconf's global section where we can add a filepath to mount a specific secret inside the linuxptp-daemon-container.
basically I converted some of the code from the ptpconfig_controller here because the file watcher forces a restart on the pods in case a change is detected on the mounted secret, so instead we're having a restart on the ptp4l process.
pkg/daemon/daemon.go
Outdated
| } | ||
| } | ||
|
|
||
| return changed |
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.
Perhaps you could return the new hash that way you don't have to recompute them again in the update function?
- Replace 5-second ticker with fsnotify event-driven monitoring - Add fsnotify watcher initialization with graceful degradation - Implement restart flood protection using atomic.Bool flag - Filter fsnotify events for Write/Create and ignore hidden files - Setup directory watches for Kubernetes symlink compatibility - Optimize hash computation by reusing hashes for unchanged file paths - Handle fsnotify channel closures and errors gracefully - Add watcher cleanup on daemon shutdown
98fdb45 to
a3fcf96
Compare
pkg/daemon/daemon.go
Outdated
| } | ||
| // Filter for relevant events (Write, Create - Kubernetes atomic updates) | ||
| // Ignore events on temporary/hidden files | ||
| if event.Op&(fsnotify.Write|fsnotify.Create) == 0 { |
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.
Should delete also be handled as a change?
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.
If you mean "deleting the sa_file" then the controller actually removes the volumeMount in case the user delete it from the ptp4lconf or manually from the file system of the container. which forces a restart on the daemonset.
| case err, ok := <-watcherErrors: | ||
| // fsnotify watcher error | ||
| if !ok { | ||
| watcherErrors = nil |
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.
Should you recreate the watcher here?
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.
we should keep recreate until watcher is not having error
pkg/daemon/daemon.go
Outdated
|
|
||
| // Setup fsnotify watch on directory (not file, due to Kubernetes symlinks) | ||
| if dn.saFileWatcher != nil { | ||
| dirPath := filepath.Dir(saFilePath) |
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.
There should be location where all sa_files are mounted. This path should controlled by the ptp-operator so is known ahead of time.
Instead of relying on the ptpconfig it should just recursively watch all dirs within that path.
That way theirs no need to have this late binding approach it can just be there from the start.
| } | ||
| glog.Info("PtpConfig change detected, restarting PTP processes") | ||
| dn.restartInProgress.Store(true) | ||
| err := dn.applyNodePTPProfiles() |
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.
These are not run as goroutines so they are blocking meaning there is no need for this flag.
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.
That's right, there is no concurrent access.. I will fix that
d4bdb68 to
7cbf06b
Compare
- Add PTP_SEC_FOLDER constant for /etc/ptp-secret-mount/ - Watch security mount folder from daemon startup instead of per-profile - Remove saFileTracker, saFileMutex, and saFileInfo (no longer needed) - Remove checkSaFileChanges, updateSaFileTracking, extractSaFileFromConf - Simplify fsnotify handler: any file change triggers PTP restart - Eliminates late binding complexity since mount path is known ahead of time
16ed150 to
915d95f
Compare
…profiles Add extractAuthSettingsForPhc2sys function to parse sa_file, spp, and active_key_id from ptp4lConf [global] section. For grandmaster clock types, automatically inject these authentication settings into the phc2sys configuration to ensure consistent TLV authentication across PTP processes.
Add sa_file Change Detection for Authentication Secret Updates
Summary
Implement automatic detection of PTP authentication file (sa_file) changes to enable ptp4l process restarts without pod restarts when authentication secrets are updated.
Motivation
When PTP authentication secrets are updated in Kubernetes, the mounted sa_file content changes but linuxptp-daemon had no mechanism to detect this, requiring manual intervention or pod restarts to apply new authentication keys.
Changes
New Components
Modified Functions
updateSaFileTracking()after profiles are appliedNew Functions
computeFileHash(): Generic SHA256 hash computation (works with any file content/format)extractSaFileFromConf(): Parses sa_file path from ptp4lConf global sectioncheckSaFileChanges(): Compares current file hash with stored hash for all tracked filesupdateSaFileTracking(): Initializes/updates tracking when profiles changeBehavior
applyNodePTPProfiles()to restart ptp4l processDetection Timeline
Benefits