Change nfd-worker to use Ticker instead of After. #1050

VillePihlava · 2023-02-06T10:13:13Z

This pull request changes the nfd-worker to use time.Ticker instead of time.After to implement core.sleepInterval. An infinite interval is represented by stopping the Ticker.

Issue: #899

netlify · 2023-02-06T10:13:18Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`2101cb2`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/63e50f0a244d7e0008b61c1d
😎 Deploy Preview	https://deploy-preview-1050--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 site settings.

linux-foundation-easycla · 2023-02-06T10:13:18Z

The committers listed above are authorized under a signed CLA.

✅ login: VillePihlava / name: Ville Pihlava (4d1fe19)

k8s-ci-robot · 2023-02-06T10:13:21Z

Welcome @VillePihlava!

It looks like this is your first PR to kubernetes-sigs/node-feature-discovery 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/node-feature-discovery has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-02-06T10:13:23Z

Hi @VillePihlava. 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/test-infra repository.

ArangoGutierrez

Patch looks good, thanks for this @VillePihlava !!
Just a small comment
/ok-to-test
The rest I'll leave it to
/assign @marquiz

ArangoGutierrez · 2023-02-06T10:57:23Z

pkg/nfd-worker/nfd-worker.go

+func (w *nfdWorker) createTicker() *time.Ticker {
+	if w.config.Core.SleepInterval.Duration > 0 {
+		return time.NewTicker(w.config.Core.SleepInterval.Duration)
+	} else {


This Else is not needed

The else is now removed

TO me this looks better like a https://gobyexample.com/switch since the func doesn't have a return a Switch will be more elegant here

pkg/nfd-worker/nfd-worker.go

ArangoGutierrez · 2023-02-07T08:15:16Z

/assign @marquiz

fmuyassarov

Looks good. Can you perhaps squash the commits?

marquiz · 2023-02-07T08:31:10Z

pkg/nfd-worker/nfd-worker.go

+	t := time.NewTicker(time.Hour)
+	t.Stop()
+	return t


I think you could simplify this a bit with just

Suggested change

t := time.NewTicker(time.Hour)

t.Stop()

return t

return &time.Ticker{}

I'm not entirely sure what happens when calling Stop() with an uninitialized ticker, so I changed the implementation a little to not call it on an uninitialized one and incorporated this change in a sense

Mm, that's a fair point. Even if it works now it's not clearly specified (from what I can tell) if calling Stop() on uninitialized Ticker is legit which might cause a breakage later.

VillePihlava · 2023-02-07T10:21:14Z

Looks good. Can you perhaps squash the commits?

Done

marquiz · 2023-02-08T17:57:17Z

pkg/nfd-worker/nfd-worker.go

+func (infiniteTicker *infiniteTicker) startTicker(w *nfdWorker) {
+	// If the sleep interval is not a positive number, the ticker will act as if it was set to an infinite duration by not ticking.
+	if w.config.Core.SleepInterval.Duration > 0 {
+		infiniteTicker.T = time.NewTicker(w.config.Core.SleepInterval.Duration)
+		infiniteTicker.initialized = true
+	}
+}


Thanks for the update @VillePihlava. Quite a bike-shedding on this PR, but somehow this latest version feels just a tad too complicated (in terms of lines-of-code) for such a simple thing 😇

How about using struct composition to wrap the Ticker type, with something like

type infiniteTicker struct { *time.Ticker } func (i *infiniteTicker) Reset(d time.Duration) { if d > 0 { i.Ticker.Reset(d) } else { i.Ticker.Stop() } }

...then you'd initialize with smth like (you could specify a dedicated create function if this looks too bad):

labelTrigger := infiniteTicker{Ticker: time.NewTicker(1)} // just any non-zero duration here, we'll do the actual configuration below labelTrigger.Reset(w.config.Core.SleepInterval.Duration)

use the channel directly:

case <-labelTrigger.C:

and simply use reset for re-configuring (instead of creating a new Ticker instance)

labelTrigger.Reset(w.config.Core.SleepInterval.Duration)

WDYT? Haven't tested but I think that should work out

Yeah this makes sense, I made these changes now. Also tested manually that making changes to the sleepinterval in the config worked

marquiz

Thanks @VillePihlava! Looks good to me 🤓

marquiz · 2023-02-09T08:30:37Z

/assign @ArangoGutierrez

ArangoGutierrez · 2023-02-09T13:29:15Z

pkg/nfd-worker/nfd-worker.go

+func (w *nfdWorker) createTicker() *time.Ticker {
+	if w.config.Core.SleepInterval.Duration > 0 {
+		return time.NewTicker(w.config.Core.SleepInterval.Duration)
+	} else {


TO me this looks better like a https://gobyexample.com/switch since the func doesn't have a return a Switch will be more elegant here

ArangoGutierrez · 2023-02-09T13:29:16Z

pkg/nfd-worker/nfd-worker.go

+		i.Ticker.Reset(d)
+	} else {
+		// If the sleep interval is not a positive number the ticker will act as if it was set to an infinite duration by not ticking.
+		i.Ticker.Stop()


Could you format this comment to fit in 80 columns, see how other comments when too long they are several lines. makes it easier to read

I changed the if-else to a switch and made the comment fit into 80 columns

ArangoGutierrez

Thanks for the effort @VillePihlava !

/lgtm

k8s-ci-robot · 2023-02-09T15:54:14Z

LGTM label has been added.

Git tree hash: 922bfb8952a57ef5029c26b9e548a31ed048895e

k8s-ci-robot · 2023-02-09T15:54:15Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [marquiz]

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 requested review from ArangoGutierrez and kad February 6, 2023 10:13

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 6, 2023

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 6, 2023

ArangoGutierrez requested changes Feb 6, 2023

View reviewed changes

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Feb 6, 2023

k8s-ci-robot assigned marquiz Feb 6, 2023

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 6, 2023

PiotrProkop reviewed Feb 6, 2023

View reviewed changes

pkg/nfd-worker/nfd-worker.go Outdated Show resolved Hide resolved

pkg/nfd-worker/nfd-worker.go Outdated Show resolved Hide resolved

fmuyassarov reviewed Feb 7, 2023

View reviewed changes

marquiz reviewed Feb 7, 2023

View reviewed changes

VillePihlava force-pushed the interval-fix branch from 7450a0b to 6e473e8 Compare February 7, 2023 10:17

marquiz reviewed Feb 8, 2023

View reviewed changes

VillePihlava force-pushed the interval-fix branch from 6e473e8 to 803c7e6 Compare February 9, 2023 07:00

marquiz approved these changes Feb 9, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2023

k8s-ci-robot assigned ArangoGutierrez Feb 9, 2023

ArangoGutierrez requested changes Feb 9, 2023

View reviewed changes

Change nfd-worker to use Ticker instead of After.

2101cb2

VillePihlava force-pushed the interval-fix branch from 803c7e6 to 2101cb2 Compare February 9, 2023 15:19

ArangoGutierrez approved these changes Feb 9, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2023

k8s-ci-robot merged commit ac271b3 into kubernetes-sigs:master Feb 9, 2023

VillePihlava mentioned this pull request Feb 10, 2023

Add discovery duration logging #1055

Merged

marquiz mentioned this pull request Apr 12, 2023

Release v0.13.0 #1068

Closed

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change nfd-worker to use Ticker instead of After. #1050

Change nfd-worker to use Ticker instead of After. #1050

VillePihlava commented Feb 6, 2023

netlify bot commented Feb 6, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 6, 2023 •

edited

Loading

k8s-ci-robot commented Feb 6, 2023

k8s-ci-robot commented Feb 6, 2023

ArangoGutierrez left a comment

ArangoGutierrez Feb 6, 2023

VillePihlava Feb 7, 2023

ArangoGutierrez Feb 9, 2023

ArangoGutierrez commented Feb 7, 2023

fmuyassarov left a comment

marquiz Feb 7, 2023

VillePihlava Feb 7, 2023

marquiz Feb 8, 2023

VillePihlava commented Feb 7, 2023

marquiz Feb 8, 2023

VillePihlava Feb 9, 2023

marquiz left a comment

marquiz commented Feb 9, 2023

ArangoGutierrez Feb 9, 2023

ArangoGutierrez Feb 9, 2023

VillePihlava Feb 9, 2023

ArangoGutierrez left a comment

k8s-ci-robot commented Feb 9, 2023

k8s-ci-robot commented Feb 9, 2023

Change nfd-worker to use Ticker instead of After. #1050

Change nfd-worker to use Ticker instead of After. #1050

Conversation

VillePihlava commented Feb 6, 2023

netlify bot commented Feb 6, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

linux-foundation-easycla bot commented Feb 6, 2023 • edited Loading

k8s-ci-robot commented Feb 6, 2023

k8s-ci-robot commented Feb 6, 2023

ArangoGutierrez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArangoGutierrez commented Feb 7, 2023

fmuyassarov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VillePihlava commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

marquiz commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArangoGutierrez left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 9, 2023

k8s-ci-robot commented Feb 9, 2023

netlify bot commented Feb 6, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 6, 2023 •

edited

Loading