nfd-worker: Watch features.d changes

[ozhuraki]

Closes: #2075

[netlify]

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	7e7961c
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-nfd/deploys/694162efefa4f50007a99a3c
😎 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]

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.

Details

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]

@marquiz

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

[ozhuraki]

@marquiz

Moved into source/local, please take a look

[ozhuraki]

@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 😊

[marquiz]

ping @ozhuraki any update on this?

[ArangoGutierrez]

/ok-to-test

[ArangoGutierrez]

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

[Copilot]

Pull Request Overview

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

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[marquiz]

Thanks for the update @ozhuraki. And sorry this review pass took so long. A few more comments below

[ozhuraki]

@marquiz

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

[ozhuraki]

/retest

[marquiz]

Thanks for the update @ozhuraki. Small nits/suggestions below.

I'm still a bit concnerned about the handling of the notifier goroutine (leaking of goroutines if SetNotifyChannel gets called more than once. But I guess we can live with that for now as this is more of a concern for future

EDIT: Please rebase to get rid of the netlify failures

[ozhuraki]

@marquiz

Thanks, updated! Please take a look.

[marquiz]

Thanks for the update @ozhuraki, I think we could finally get this in.

/assign @fmuyassarov @ArangoGutierrez

[ArangoGutierrez]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Details

Git tree hash: cacfe0e1ad0c208e1345c7ba456cdb16bc37596d

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, marquiz, ozhuraki

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ArangoGutierrez,marquiz]

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