nfd-worker: Watch features.d changes #2156

ozhuraki · 2025-05-12T19:05:35Z

Closes: #2075

netlify · 2025-05-12T19:05:41Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`7f64c8b`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-nfd/deploys/68b58812f5058600080d85ce
😎 Deploy Preview	https://deploy-preview-2156--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

k8s-ci-robot · 2025-05-12T19:05:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ozhuraki
Once this PR has been reviewed and has the lgtm label, please assign marquiz for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-05-12T19:05:45Z

Hi @ozhuraki. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

marquiz

Thanks @ozhuraki for taking a stab at this.

I think we should refactor the code and re-architecture this more to make the code more maintainable. There might be other sources we'd also make react to events in a similar way. Basically, it should be the source (source/local in this case) which should be able to notify the main event loop that features have been updated. Also, no need to run re-discovery of all features.

ozhuraki · 2025-05-13T08:55:12Z

@marquiz

Thanks, makes sense. I will move this into source/local.

ozhuraki · 2025-05-22T14:01:31Z

@marquiz

Moved into source/local, please take a look

ozhuraki · 2025-06-03T11:11:43Z

@ArangoGutierrez

Thanks, updated, please take a look.

marquiz

Progress in the right direction, but I maintain that we should aim for a more generic, maintainable solution. For example, who knows in the future we might want to do some uevent-based stuff or similar and it would be good to have the basics right for that, instead building of pile of one-off tricks.

Some specific observations:

We operate on interfaces in nfd-worker, IMO we better keep that to keep the design cleaner. E.g. introduce a new AsyncSource, EventSource or smth with a method to set the event channel, and then when configuring/enabling the feature sources check if the source implements the interface and if it does call the method
It should be the source/local who is internally setting up the the fswatcher and notifies nfd-worker. Then, we have two possibilities here:
- either nfd-worker does the source.Discover() and then advertises the updated features/labels
- or the source runs discovery internally and notifies nfd-worker just to re-advertise update features
When a source notifies the nfd-worker main loop, the main loop does not need to do full re-discovery of all feature sources
Some unit test for the local source would be nide 😊

source/local/local.go

source/source.go

pkg/nfd-worker/nfd-worker.go

marquiz · 2025-07-29T10:09:43Z

ping @ozhuraki any update on this?

ArangoGutierrez · 2025-08-14T12:39:57Z

/ok-to-test

ArangoGutierrez · 2025-08-14T12:47:22Z

/test pull-node-feature-discovery-verify-master

ArangoGutierrez · 2025-08-14T14:41:50Z

/retest

Copilot

Pull Request Overview

This PR implements filesystem watching functionality for the nfd-worker to automatically detect changes in the features.d directory. It introduces an EventSource interface to enable sources to send notifications when their underlying data changes, specifically targeting the local source to watch for file modifications.

Adds EventSource interface and related infrastructure for event-driven feature discovery
Implements filesystem watching in the local source using fsnotify
Integrates event handling into the nfd-worker main loop to trigger selective feature discovery

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
source/source.go	Adds EventSource interface and GetAllEventSources() function
source/local/local.go	Implements EventSource interface with fsnotify-based file watching
pkg/nfd-worker/nfd-worker.go	Integrates event handling and selective feature discovery by source

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.}

source/local/local.go

Copilot · 2025-08-14T14:43:44Z

source/local/local.go

+		case err := <-s.fsWatcher.Errors:
+			klog.ErrorS(err, "failed to to watch features.d changes")
+		}
+		time.Sleep(1 * time.Second)


The hardcoded 1-second sleep in the event loop may cause unnecessary delays in event processing. Consider removing this sleep or making it configurable, as fsnotify events should be processed immediately.

Suggested change

time.Sleep(1 * time.Second)

@ozhuraki I think copilot is up to something here :) If we get a burst of say 10 events, we'd read them one-per-second, causing a streak of 10 1-per-second updates in nfd-worker. We'd only want a 1 second total delay, i.e. read events as fast as we can but "group" them into one event that gets sent to the ch.

Elsewhere we've used e.g. a patter of rateLimit := time.After(time.Second)..., you could come up with something better, thoug...

source/local/local.go

pkg/nfd-worker/nfd-worker.go

Signed-off-by: Oleg Zhurakivskyy <[email protected]>

ozhuraki · 2025-09-01T12:40:54Z

@marquiz @ArangoGutierrez

Thanks for the review. Updated, please take a look.

marquiz

Thanks @ozhuraki for the update. Comments below

marquiz · 2025-09-09T12:58:18Z

source/local/local.go

+		case err := <-s.fsWatcher.Errors:
+			klog.ErrorS(err, "failed to to watch features.d changes")
+		}
+		time.Sleep(1 * time.Second)


@ozhuraki I think copilot is up to something here :) If we get a burst of say 10 events, we'd read them one-per-second, causing a streak of 10 1-per-second updates in nfd-worker. We'd only want a 1 second total delay, i.e. read events as fast as we can but "group" them into one event that gets sent to the ch.

Elsewhere we've used e.g. a patter of rateLimit := time.After(time.Second)..., you could come up with something better, thoug...

marquiz · 2025-09-09T13:38:52Z

pkg/nfd-worker/nfd-worker.go

@@ -341,6 +381,12 @@ func (w *nfdWorker) Run() error {
 				return err
 			}

+		case sourceName := <-w.sourceEvent:
+			err = w.runFeatureDiscoveryBySourceName(sourceName)


I think we could simplify this quite bit by having sourceEvent of type chan FeatureSource. We could just do s.Discover() here and ditch runFeatureDiscoveryBySourceName altogether. Need to split runFeatureDiscovery() into two parts to avoid copy-pasting code: feature-discovery and feature-advertisement (create labels etc). Then call the feature-advertisement part here.

marquiz · 2025-09-09T13:40:13Z

pkg/nfd-worker/nfd-worker.go

+
+	discoveryDuration := time.Since(discoveryStart)
+	klog.V(2).InfoS("feature discovery of all sources completed", "duration", discoveryDuration)
+	featureDiscoveryDuration.WithLabelValues(utils.NodeName()).Observe(discoveryDuration.Seconds())


We shouldn't update the metrics here as the metric is for "all enabled feature sources" and we're only doing one here. We can think about per-source metrics in a separate PR

marquiz · 2025-09-09T13:46:00Z

pkg/nfd-worker/nfd-worker.go

+	for _, s := range eventSources {
+		if err := s.SetNotifyChannel(w.sourceEvent); err != nil {
+			klog.ErrorS(err, "failed to set notify channel for event source", "source", s.Name())
+			return fmt.Errorf("failed to set notify channel for event source %s: %w", s.Name(), err)


Let's not error out in this case (just log the error). The design pattern of nfd-worker has been that log errors if we cannot access some data but don't crash. Let's follow that

marquiz · 2025-09-09T13:46:55Z

pkg/nfd-worker/nfd-worker.go

+	eventSources := source.GetAllEventSources()
+	for _, s := range eventSources {


I think this stuff should be done in the configure() function. The other similar stuff is there, too.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 12, 2025

k8s-ci-robot requested review from ArangoGutierrez and marquiz May 12, 2025 19:05

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 12, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 12, 2025

marquiz reviewed May 13, 2025

View reviewed changes

ozhuraki force-pushed the watch-featuresd branch from 89606c0 to b3b1c64 Compare May 22, 2025 13:32

ArangoGutierrez requested a review from Copilot May 28, 2025 05:58

This comment was marked as outdated.

Sign in to view

ozhuraki force-pushed the watch-featuresd branch from b3b1c64 to 9a65598 Compare June 3, 2025 11:06

marquiz reviewed Jun 5, 2025

View reviewed changes

ozhuraki force-pushed the watch-featuresd branch from 9a65598 to 7c29f0a Compare June 18, 2025 09:27

marquiz reviewed Jun 25, 2025

View reviewed changes

ArangoGutierrez requested a review from Copilot August 14, 2025 12:39

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 14, 2025

This comment was marked as outdated.

Sign in to view

ozhuraki force-pushed the watch-featuresd branch from 7c29f0a to b9ea21a Compare August 14, 2025 14:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 14, 2025

ArangoGutierrez requested a review from Copilot August 14, 2025 14:36

ArangoGutierrez requested a review from marquiz August 14, 2025 14:36

Copilot AI reviewed Aug 14, 2025

View reviewed changes

nfd-worker: Watch features.d changes

7f64c8b

Signed-off-by: Oleg Zhurakivskyy <[email protected]>

ozhuraki force-pushed the watch-featuresd branch from b9ea21a to 7f64c8b Compare September 1, 2025 11:48

marquiz reviewed Sep 9, 2025

View reviewed changes

		eventSources := source.GetAllEventSources()
		for _, s := range eventSources {

nfd-worker: Watch features.d changes #2156

Are you sure you want to change the base?

nfd-worker: Watch features.d changes #2156

Conversation

ozhuraki commented May 12, 2025

Uh oh!

netlify bot commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Uh oh!

k8s-ci-robot commented May 12, 2025

Uh oh!

k8s-ci-robot commented May 12, 2025

Uh oh!

marquiz left a comment

Choose a reason for hiding this comment

Uh oh!

ozhuraki commented May 13, 2025

Uh oh!

ozhuraki commented May 22, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

ozhuraki commented Jun 3, 2025

Uh oh!

marquiz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marquiz commented Jul 29, 2025

Uh oh!

ArangoGutierrez commented Aug 14, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

ArangoGutierrez commented Aug 14, 2025

Uh oh!

ArangoGutierrez commented Aug 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

marquiz Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ozhuraki commented Sep 1, 2025

Uh oh!

marquiz left a comment

Choose a reason for hiding this comment

Uh oh!

marquiz Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

marquiz Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

marquiz Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

marquiz Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

marquiz Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

netlify bot commented May 12, 2025 •

edited

Loading