add configurable pagination to nfd-master #2000

ivelichkovich · 2025-01-04T19:27:50Z

addresses scalability and api-server load concerns for large clusters by adding configurable pagination to the informer cache of nfd-master

related to: #1998

k8s-ci-robot · 2025-01-04T19:27:59Z

Welcome @ivelichkovich!

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 · 2025-01-04T19:27:59Z

Hi @ivelichkovich. 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.

netlify · 2025-01-04T19:28:05Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`07c4318`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/67f6a88abcbdcb0009890510
😎 Deploy Preview	https://deploy-preview-2000--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 configuration.

ivelichkovich · 2025-01-04T19:28:18Z

/ok-to-test

k8s-ci-robot · 2025-01-04T19:28:32Z

@ivelichkovich: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

TessaIO

Thanks for the work, small comment out there. Can you please add the documentation for this new option?

TessaIO · 2025-01-04T23:13:24Z

cmd/nfd-master/main.go

@@ -122,6 +122,8 @@ func initFlags(flagset *flag.FlagSet) (*master.Args, *master.ConfigOverrideArgs)
 			"in the same format as in the config file (i.e. json or yaml). These options")
 	flagset.BoolVar(&args.EnableLeaderElection, "enable-leader-election", false,
 		"Enables a leader election. Enable this when running more than one replica on nfd master.")
+	flagset.Int64Var(&args.ListSize, "node-feature-informer-list-size", 0,


I suggest a shorter way like this.

Suggested change

flagset.Int64Var(&args.ListSize, "node-feature-informer-list-size", 0,

flagset.Int64Var(&args.ListSize, "informer-list-size", 0,

Changed the flag and updated docs, let me know if you'd prefer the doc update somewhere else

ivelichkovich · 2025-01-05T01:41:02Z

I wasn't sure if we'd want to default to 500 (default list pagination size) to keep new default behavior consistent with old behavior or set it to default to 200 to match the gc pagination default size. I'm open to either but would lean towards making them consistent and setting the default here to 200, ref: https://github.com/kubernetes-sigs/node-feature-discovery/pull/2001/files

api/generated/informers/externalversions/nfd/v1alpha1/nodefeature.go

ivelichkovich · 2025-01-05T04:05:53Z

pkg/nfd-master/nfd-api-controller.go

@@ -101,6 +102,7 @@ func newNfdController(config *restclient.Config, nfdApiControllerOptions nfdApiC
 			if opts.ResourceVersion == "0" {


btw noticed the (TODO: find out why) about scalability of this resource version override. While researching the pagination stuff I think this is likely due to this snippet of code: https://github.com/kubernetes/kubernetes/blob/ace55542575fb098b3e413692bbe2bc20d2348ba/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go#L600-L616 if you set resource version to 0 it serves the request from apiservers cache and doesn't use pagination otherwise pagination will default to 500 so that may explain why it blows up on large clusters

So by setting this we're making it go to ETCD instead of from api-server cache, I found some WIP in k/k that seems to imply they're working on improving this behavior where you'll be able to paginate from apiserver cache but AFAICT it's not supported yet, would be good to track this though kubernetes/kubernetes#108003

marquiz

Thank you @ivelichkovich for the enhancement. A few small comments below
/ok-to-test
/cc @ArangoGutierrez @adrianchiris

cmd/nfd-master/main.go

docs/reference/master-commandline-reference.md

ivelichkovich · 2025-02-05T06:41:27Z

Thank you @ivelichkovich for the updates. To me, this looks pretty good, now.

One thing that would be nice would be to have the same setting as a config file option. I think that would also make this more usable with the helm chart (with like --set master.config.informerPageSize=100). We could track this separately (and as a separate PR) if you think that'd be better.

Also, thank you for investigating deeply the "find out why" part. Would it be possible to replace my TODO comment in the code with an explanation along the lines of these comments #2000 (comment) #2000 (comment)

WDYT?

Yeah that sounds good, I can update that

marquiz · 2025-02-05T12:57:52Z

Yeah that sounds good, I can update that

Thanks for working on this 👍

ivelichkovich · 2025-02-13T18:27:07Z

Okay updated, PTAL 😄

ArangoGutierrez · 2025-02-17T16:01:19Z

/test-all

ArangoGutierrez · 2025-02-17T16:01:35Z

/retest

ivelichkovich · 2025-02-19T05:21:28Z

it looks like code coverage may be complaining about the comment lines if I'm reading it correctly, can that be bypassed or configured to ignore comments?

marquiz · 2025-03-14T09:53:09Z

/retest

marquiz

@ivelichkovich please rebase the PR.

Also, you need to update deployment/components/master-config/nfd-master.conf.example (this is the "origin" for the conf) and after that run make templates to update the chart. This gets rid of the CI failure.

marquiz · 2025-03-27T12:17:00Z

/retest

marquiz · 2025-03-27T12:39:21Z

Argh, docs build failure. #2105 should fix this.

@ivelichkovich could you squash this into one commit?

marquiz · 2025-03-27T15:21:37Z

/retest

marquiz · 2025-03-28T08:02:40Z

@ivelichkovich rebase and squash, other than that, lgtm

ozhuraki

LGTM, just rebase and squash

marquiz

👍

@ArangoGutierrez PTAL

k8s-ci-robot · 2025-04-10T08:56:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivelichkovich, marquiz

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

marquiz · 2025-04-10T08:56:59Z

/cc @ArangoGutierrez

k8s-ci-robot · 2025-04-23T02:42:27Z

PR needs rebase.

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 · 2025-04-23T07:43:18Z

@ivelichkovich another rebase needed 🫤 When that's done let's merge this

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

k8s-ci-robot requested review from ArangoGutierrez and zvonkok January 4, 2025 19:27

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 Jan 4, 2025

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 4, 2025

ivelichkovich mentioned this pull request Jan 4, 2025

Pagination size cmd line arg #1998

Open

TessaIO reviewed Jan 4, 2025

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 5, 2025

ivelichkovich marked this pull request as draft January 5, 2025 03:16

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 5, 2025

ivelichkovich marked this pull request as ready for review January 5, 2025 03:38

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 5, 2025

k8s-ci-robot requested a review from marquiz January 5, 2025 03:38

ivelichkovich commented Jan 5, 2025

View reviewed changes

api/generated/informers/externalversions/nfd/v1alpha1/nodefeature.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 5, 2025

ivelichkovich commented Jan 5, 2025

View reviewed changes

marquiz reviewed Jan 8, 2025

View reviewed changes

cmd/nfd-master/main.go Outdated Show resolved Hide resolved

cmd/nfd-master/main.go Outdated Show resolved Hide resolved

docs/reference/master-commandline-reference.md Outdated Show resolved Hide resolved

k8s-ci-robot requested a review from adrianchiris January 8, 2025 12:35

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 Jan 8, 2025

marquiz mentioned this pull request Jan 22, 2025

add configurable pagination to nfd-gc #2001

Open

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 13, 2025

marquiz reviewed Mar 20, 2025

View reviewed changes

ivelichkovich force-pushed the master branch from 5ec7910 to dec4959 Compare March 26, 2025 19:16

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2025

ozhuraki reviewed Mar 31, 2025

View reviewed changes

ivelichkovich force-pushed the master branch 2 times, most recently from c9cc844 to aa5cad0 Compare April 9, 2025 16:44

add configurable pagination to nfd-master

07c4318

ivelichkovich force-pushed the master branch from aa5cad0 to 07c4318 Compare April 9, 2025 17:04

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2025

marquiz approved these changes Apr 10, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add configurable pagination to nfd-master #2000

add configurable pagination to nfd-master #2000

ivelichkovich commented Jan 4, 2025

k8s-ci-robot commented Jan 4, 2025

k8s-ci-robot commented Jan 4, 2025

netlify bot commented Jan 4, 2025 •

edited

Loading

ivelichkovich commented Jan 4, 2025

k8s-ci-robot commented Jan 4, 2025

TessaIO left a comment

TessaIO Jan 4, 2025

ivelichkovich Jan 5, 2025

ivelichkovich Jan 5, 2025

ivelichkovich commented Jan 5, 2025 •

edited

Loading

ivelichkovich Jan 5, 2025 •

edited

Loading

ivelichkovich Jan 5, 2025

marquiz left a comment

ivelichkovich commented Feb 5, 2025

marquiz commented Feb 5, 2025

ivelichkovich commented Feb 13, 2025

ArangoGutierrez commented Feb 17, 2025

ArangoGutierrez commented Feb 17, 2025

ivelichkovich commented Feb 19, 2025 •

edited

Loading

marquiz commented Mar 14, 2025

marquiz left a comment

marquiz commented Mar 27, 2025

marquiz commented Mar 27, 2025

marquiz commented Mar 27, 2025

marquiz commented Mar 28, 2025

ozhuraki left a comment

marquiz left a comment

k8s-ci-robot commented Apr 10, 2025

marquiz commented Apr 10, 2025

k8s-ci-robot commented Apr 23, 2025

marquiz commented Apr 23, 2025

	flagset.Int64Var(&args.ListSize, "node-feature-informer-list-size", 0,
	flagset.Int64Var(&args.ListSize, "informer-list-size", 0,

		@@ -101,6 +102,7 @@ func newNfdController(config *restclient.Config, nfdApiControllerOptions nfdApiC
		if opts.ResourceVersion == "0" {

add configurable pagination to nfd-master #2000

Are you sure you want to change the base?

add configurable pagination to nfd-master #2000

Conversation

ivelichkovich commented Jan 4, 2025

k8s-ci-robot commented Jan 4, 2025

k8s-ci-robot commented Jan 4, 2025

netlify bot commented Jan 4, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

ivelichkovich commented Jan 4, 2025

k8s-ci-robot commented Jan 4, 2025

TessaIO left a comment

Choose a reason for hiding this comment

TessaIO Jan 4, 2025

Choose a reason for hiding this comment

ivelichkovich Jan 5, 2025

Choose a reason for hiding this comment

ivelichkovich Jan 5, 2025

Choose a reason for hiding this comment

ivelichkovich commented Jan 5, 2025 • edited Loading

ivelichkovich Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

ivelichkovich Jan 5, 2025

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

ivelichkovich commented Feb 5, 2025

marquiz commented Feb 5, 2025

ivelichkovich commented Feb 13, 2025

ArangoGutierrez commented Feb 17, 2025

ArangoGutierrez commented Feb 17, 2025

ivelichkovich commented Feb 19, 2025 • edited Loading

marquiz commented Mar 14, 2025

marquiz left a comment

Choose a reason for hiding this comment

marquiz commented Mar 27, 2025

marquiz commented Mar 27, 2025

marquiz commented Mar 27, 2025

marquiz commented Mar 28, 2025

ozhuraki left a comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 10, 2025

marquiz commented Apr 10, 2025

k8s-ci-robot commented Apr 23, 2025

marquiz commented Apr 23, 2025

netlify bot commented Jan 4, 2025 •

edited

Loading

ivelichkovich commented Jan 5, 2025 •

edited

Loading

ivelichkovich Jan 5, 2025 •

edited

Loading

ivelichkovich commented Feb 19, 2025 •

edited

Loading