-
Notifications
You must be signed in to change notification settings - Fork 0
use fsnotify and klog #5
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
Conversation
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.
Pull request overview
This pull request modernizes the device plugin's logging and kubelet restart detection by replacing the standard log package with k8s.io/klog/v2 and introducing fsnotify for event-based monitoring instead of polling.
Key changes:
- Migration from standard library
logto structuredk8s.io/klog/v2logging with verbosity levels - Introduction of
fsnotifyfor efficient filesystem event monitoring with polling fallback - Enhancement of health check logic to update all devices when the underlying hypervisor device becomes unavailable
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| device-plugin/main.go | Replaced all log calls with klog, added fsnotify-based kubelet restart detection with polling fallback, improved health checking to update all devices |
| device-plugin/go.mod | Added dependencies for fsnotify v1.8.0 and klog/v2 v2.130.1 |
| device-plugin/go.sum | Added checksums for new dependencies |
Comments suppressed due to low confidence (1)
device-plugin/main.go:209
- The health check logic has a potential race condition. The p.devices slice is being read and modified without synchronization in ListAndWatch, while it could potentially be accessed from other goroutines (e.g., during Allocate calls). Although the Kubernetes device plugin framework typically serializes these calls, accessing and modifying the shared devices slice without proper synchronization could lead to data races in edge cases.
// Check if health changed (compare against first device as representative)
if p.devices[0].Health != newHealth {
// Update ALL devices - they all share the same underlying hypervisor device
for i := range p.devices {
p.devices[i].Health = newHealth
}
klog.Infof("Device health changed to %s for all %d devices", newHealth, len(p.devices))
if err := srv.Send(&pluginapi.ListAndWatchResponse{Devices: p.devices}); err != nil {
return err
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
468f79b to
e9c496d
Compare
Signed-off-by: Simon Davies <[email protected]>
- Rename newHealth to health (Go naming conventions) - Remove dead kubeletSock check (wrong directory being watched) - Handle closed fsnotify channels with fallback to polling - Update comment to reflect actual behavior Signed-off-by: Simon Davies <[email protected]>
e9c496d to
9ccbb33
Compare
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.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.