-
Notifications
You must be signed in to change notification settings - Fork 268
nfd-worker: Watch features.d changes #2156
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: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -26,6 +26,7 @@ import ( | |||||
|
||||||
"k8s.io/klog/v2" | ||||||
|
||||||
"github.com/fsnotify/fsnotify" | ||||||
nfdv1alpha1 "sigs.k8s.io/node-feature-discovery/api/nfd/v1alpha1" | ||||||
"sigs.k8s.io/node-feature-discovery/pkg/utils" | ||||||
"sigs.k8s.io/node-feature-discovery/source" | ||||||
|
@@ -65,10 +66,11 @@ var ( | |||||
featureFilesDir = "/etc/kubernetes/node-feature-discovery/features.d/" | ||||||
) | ||||||
|
||||||
// localSource implements the FeatureSource and LabelSource interfaces. | ||||||
// localSource implements the FeatureSource, LabelSource, EventSource interfaces. | ||||||
type localSource struct { | ||||||
features *nfdv1alpha1.Features | ||||||
config *Config | ||||||
features *nfdv1alpha1.Features | ||||||
config *Config | ||||||
fsWatcher *fsnotify.Watcher | ||||||
} | ||||||
|
||||||
type Config struct { | ||||||
|
@@ -87,6 +89,7 @@ var ( | |||||
_ source.FeatureSource = &src | ||||||
_ source.LabelSource = &src | ||||||
_ source.ConfigurableSource = &src | ||||||
_ source.EventSource = &src | ||||||
) | ||||||
|
||||||
// Name method of the LabelSource interface | ||||||
|
@@ -318,6 +321,47 @@ func getFileContent(fileName string) ([][]byte, error) { | |||||
return lines, nil | ||||||
} | ||||||
|
||||||
func (s *localSource) runNotifier(ch chan struct{}) { | ||||||
for { | ||||||
select { | ||||||
case event := <-s.fsWatcher.Events: | ||||||
if event.Op&fsnotify.Create == fsnotify.Create || event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Remove == fsnotify.Remove || event.Op&fsnotify.Rename == fsnotify.Rename || event.Op&fsnotify.Chmod == fsnotify.Chmod { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small suggestion, could be more readable with something like: opAny := fsnotify.Create | fsnotify.Write | fsnotify.Remove | fsnotify.Rename | fsnotify.Chmod
if event.Op&opAny != 0 { WDYT? |
||||||
klog.InfoS("fsnotify event", event) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest not to log these as "debug", (to avoid potentially flooding the logs). Also the usage of klog.InfoS is not correct.
Suggested change
|
||||||
ch <- struct{}{} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at this I think we need some filtering of the events. For example, using temporary files in the dir (as recommended by our documentation, to avoid races) we will get a ton of events, each of them causing rediscovery. E.g. wait for one second (to get the slew of fsnotify events) and then send the feature event. |
||||||
} | ||||||
case err := <-s.fsWatcher.Errors: | ||||||
klog.ErrorS(err, "failed to to watch features.d changes") | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
// SetChannel method of the EventSource Interface | ||||||
func (s *localSource) SetChannel(ch chan struct{}) error { | ||||||
info, err := os.Stat(featureFilesDir) | ||||||
if err != nil { | ||||||
if !os.IsNotExist(err) { | ||||||
return err | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it does not exist we probably want to exit, too (return nil)? |
||||||
} | ||||||
|
||||||
if info != nil && info.IsDir() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe we should also check if |
||||||
watcher, err := fsnotify.NewWatcher() | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
|
||||||
err = watcher.Add(featureFilesDir) | ||||||
if err != nil { | ||||||
return fmt.Errorf("unable to access %v: %w", featureFilesDir, err) | ||||||
} | ||||||
s.fsWatcher = watcher | ||||||
} | ||||||
|
||||||
go s.runNotifier(ch) | ||||||
|
||||||
return nil | ||||||
} | ||||||
|
||||||
func init() { | ||||||
source.Register(&src) | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,14 @@ type SupplementalSource interface { | |
DisableByDefault() bool | ||
} | ||
|
||
// EventSource is an interface for a source that can send events | ||
type EventSource interface { | ||
Source | ||
|
||
// SetChannel sets the channel | ||
SetChannel(chan struct{}) error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit/suggestion: maybe indicate in the name what channel we're setting. E.g. |
||
} | ||
|
||
// FeatureLabelValue represents the value of one feature label | ||
type FeatureLabelValue interface{} | ||
|
||
|
@@ -155,6 +163,17 @@ func GetAllConfigurableSources() map[string]ConfigurableSource { | |
return all | ||
} | ||
|
||
// GetAllEventSources returns all registered event sources | ||
func GetAllEventSources() map[string]EventSource { | ||
all := make(map[string]EventSource) | ||
for k, v := range sources { | ||
if s, ok := v.(EventSource); ok { | ||
all[k] = s | ||
} | ||
} | ||
return all | ||
} | ||
|
||
// GetAllFeatures returns a combined set of all features from all feature | ||
// sources. | ||
func GetAllFeatures() *nfdv1alpha1.Features { | ||
|
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 don't want to run full feature discovery of all sources. Just the one that evented and then update the NodeFeature object. Prolly requires a bit of refactoring on the nfd-worker.go side.
Either do s.Discover() here or then in the source (and then just notify nfd-worker that it needs to re-advertise features).