Discover node features as annotations #1093

bebc · 2023-03-19T06:50:49Z

resolve issue #863

linux-foundation-easycla · 2023-03-19T06:50:55Z

The committers listed above are authorized under a signed CLA.

✅ login: bebc (a5702d6)

netlify · 2023-03-19T06:50:55Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`8b8e627`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/6457aa590903960008ba2bdb
😎 Deploy Preview	https://deploy-preview-1093--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.

k8s-ci-robot · 2023-03-19T06:50:57Z

Welcome @bebc!

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-03-19T06:50:58Z

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

marquiz

Thanks @bebc for the PR. Looks pretty good already.

In the docs we want to mention this new feature in the annotations section of get-started/introduction.md and docs/usage/nfd-master.md at least, I think.

Also, could you improve the commit message. Describe what is being added and e.g. how it can be used. You can take a look at the K8s commit message guidelines for some inspiration

docs/usage/customization-guide.md

pkg/apis/nfd/v1alpha1/annotations_labels.go

pkg/nfd-master/nfd-master.go

test/e2e/data/nodefeaturerule-4.yaml

test/e2e/node_feature_discovery_test.go

marquiz · 2023-03-20T13:14:28Z

/ok-to-test

bebc · 2023-03-26T13:24:05Z

I meet an intractability problem.The WorkerVersionAnnotation added to the annotions in the SetLabels function(517).So the filterFeatureAnnotations function will deny it.
I think WorkerVersionAnnotation should just transfer parameters in an extra struct(I don't quit sure the MasterVersionAnnotation and etc. will meet the problem ) to the refreshNodeFeatures function and add to the annotions in the updateNodeObject function like FeatureLabelsAnnotation.This can solve the problem.
It's a bit complex(When I see the issue I think it should be an easy one. T_T) and I maybe solving it next week if needed.

marquiz · 2023-03-30T18:13:17Z

I meet an intractability problem.The WorkerVersionAnnotation added to the annotions in the SetLabels function(517).So the filterFeatureAnnotations function will deny it.

Exactly. That's why we need to treat "feature annotations" from the NodeFeatureRule separately. I.e. apply the filtering to crAnnotations

I think WorkerVersionAnnotation should just transfer parameters in an extra struct(I don't quit sure the MasterVersionAnnotation and etc. will meet the problem ) to the refreshNodeFeatures function and add to the annotions in the updateNodeObject function like FeatureLabelsAnnotation.This can solve the problem.

Yeah, that would probably make sense, have "nfd annotations" and "feature annotations" as separate args to the updateNodeObject method, i.e. smth like:

func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string, labels Labels, nfdAnnotations, featureAnnnotations Annotations, extendedResources ExtendedResources, taints []corev1.Taint) error

It's a bit complex(When I see the issue I think it should be an easy one. T_T) and I maybe solving it next week if needed.

Yes, there are devils in the details, a bit more than I originally imagined 😬 Big thanks for your efforts on this @bebc.

marquiz

Thanks @bebc 👍 A few comments below, I didn't have time to check pkg/nfd-master/nfd-master.go yet so need to do that a bit later

docs/usage/customization-guide.md

pkg/apis/nfd/v1alpha1/annotations_labels.go

docs/get-started/introduction.md

test/e2e/data/nodefeaturerule-1.yaml

pkg/nfd-master/nfd-master.go

test/e2e/node_feature_discovery_test.go

.gitignore

pkg/apis/nfd/v1alpha1/annotations_labels.go

pkg/nfd-master/nfd-master.go

ArangoGutierrez

Please remove all the files .idea

marquiz

@bebc could you rebase the PR on top of the latest master to make it possible to review. Currently the "Files changed" shows a lot of stuff merged in other PRs

.idea/.gitignore

Add FeatureAnnotationsTrackingAnnotation to oldAnnotations.So that the patches can compare and restore the annotation Signed-off-by: bebc <mchf1990212@gmail.com>

codecov · 2023-05-07T13:42:09Z

Codecov Report

Merging #1093 (41f0487) into master (ef92b2f) will increase coverage by 0.16%.
The diff coverage is 55.93%.

❗ Current head 41f0487 differs from pull request most recent head 8b8e627. Consider uploading reports for the commit 8b8e627 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1093      +/-   ##
==========================================
+ Coverage   28.79%   28.95%   +0.16%     
==========================================
  Files          51       51              
  Lines        7120     7145      +25     
==========================================
+ Hits         2050     2069      +19     
- Misses       4857     4863       +6     
  Partials      213      213

Impacted Files	Coverage Δ
pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go	`23.98% <0.00%> (-0.37%)`	⬇️
pkg/nfd-master/nfd-master.go	`44.57% <61.53%> (+1.16%)`	⬆️
pkg/apis/nfd/v1alpha1/rule.go	`79.76% <100.00%> (ø)`

... and 2 files with indirect coverage changes

k8s-ci-robot · 2023-05-07T13:47:04Z

@bebc: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-node-feature-discovery-verify-master	`8b8e627`	link	true	`/test pull-node-feature-discovery-verify-master`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

marquiz · 2023-05-08T07:21:53Z

Sorry When I resolve the conflict.I click the merge button on github

Rebase by doing git rebase origin/master, resolve conflicts manually and then git push -f

k8s-ci-robot · 2023-05-22T20:49:51Z

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

marquiz · 2023-05-23T07:35:08Z

Hi @bebc, could you rebase the PR

ArangoGutierrez · 2023-05-30T10:26:57Z

test/e2e/node_feature_discovery_test.go

+		return nil
+	}
+
+	// Simple and stupid re-try loop


We now have a more elegant way of running this loop, look at #1184

marquiz · 2023-06-22T11:25:42Z

ping @bebc are you still working on this? If not, would you be comfortable for somebody else picking it up from here and finalizing the PR?

ArangoGutierrez · 2023-06-22T16:06:50Z

ping @bebc are you still working on this? If not, would you be comfortable for somebody else picking it up from here and finalizing the PR?

I am more than happy to fork this and work upon this. work from @bebc so far is awesome, and what's left is really minimal

marquiz · 2023-06-22T16:37:02Z

Yeah, I do think that most of the work is already done. Just rebase (that will be some work) and polishing

ArangoGutierrez · 2023-10-13T15:38:35Z

Rebase and updated version at #1417
/close

k8s-ci-robot · 2023-10-13T15:38:40Z

@ArangoGutierrez: Closed this PR.

In response to this:

Rebase and updated version at #1417
/close

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.

k8s-ci-robot requested review from ArangoGutierrez and fmuyassarov March 19, 2023 06:50

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 19, 2023

marquiz reviewed Mar 20, 2023

View reviewed changes

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 Mar 20, 2023

bebc requested a review from marquiz April 3, 2023 12:39

marquiz reviewed Apr 3, 2023

View reviewed changes

ArangoGutierrez requested changes Apr 4, 2023

View reviewed changes

test/e2e/node_feature_discovery_test.go Outdated Show resolved Hide resolved

ArangoGutierrez requested changes Apr 4, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

pkg/apis/nfd/v1alpha1/annotations_labels.go Outdated Show resolved Hide resolved

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

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

bebc requested review from marquiz and ArangoGutierrez April 5, 2023 09:26

ArangoGutierrez requested changes Apr 5, 2023

View reviewed changes

marquiz reviewed Apr 6, 2023

View reviewed changes

.idea/.gitignore Outdated Show resolved Hide resolved

Don't create empty annotaionKeys When featureAnnotations is empty

735c097

Add FeatureAnnotationsTrackingAnnotation to oldAnnotations.So that the patches can compare and restore the annotation Signed-off-by: bebc <mchf1990212@gmail.com>

bebc force-pushed the issue#863 branch from 4057320 to 752831f Compare May 7, 2023 13:25

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 7, 2023

bebc force-pushed the issue#863 branch from 752831f to 735c097 Compare May 7, 2023 13:28

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 7, 2023

Merge branch 'master' into issue#863

8b8e627

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

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

ArangoGutierrez requested changes May 30, 2023

View reviewed changes

k8s-ci-robot closed this Oct 13, 2023

marquiz mentioned this pull request Oct 26, 2023

Discover node features as annotations #1417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discover node features as annotations #1093

Discover node features as annotations #1093

bebc commented Mar 19, 2023

linux-foundation-easycla bot commented Mar 19, 2023 •

edited

Loading

netlify bot commented Mar 19, 2023 •

edited

Loading

k8s-ci-robot commented Mar 19, 2023

k8s-ci-robot commented Mar 19, 2023

marquiz left a comment

marquiz commented Mar 20, 2023

bebc commented Mar 26, 2023 •

edited

Loading

marquiz commented Mar 30, 2023

marquiz left a comment

ArangoGutierrez left a comment

marquiz left a comment

codecov bot commented May 7, 2023

k8s-ci-robot commented May 7, 2023 •

edited

Loading

marquiz commented May 8, 2023

k8s-ci-robot commented May 22, 2023

marquiz commented May 23, 2023

ArangoGutierrez May 30, 2023

marquiz commented Jun 22, 2023

ArangoGutierrez commented Jun 22, 2023

marquiz commented Jun 22, 2023

ArangoGutierrez commented Oct 13, 2023

k8s-ci-robot commented Oct 13, 2023

Discover node features as annotations #1093

Discover node features as annotations #1093

Conversation

bebc commented Mar 19, 2023

linux-foundation-easycla bot commented Mar 19, 2023 • edited Loading

netlify bot commented Mar 19, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

k8s-ci-robot commented Mar 19, 2023

k8s-ci-robot commented Mar 19, 2023

marquiz left a comment

Choose a reason for hiding this comment

marquiz commented Mar 20, 2023

bebc commented Mar 26, 2023 • edited Loading

marquiz commented Mar 30, 2023

marquiz left a comment

Choose a reason for hiding this comment

ArangoGutierrez left a comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

codecov bot commented May 7, 2023

Codecov Report

k8s-ci-robot commented May 7, 2023 • edited Loading

marquiz commented May 8, 2023

k8s-ci-robot commented May 22, 2023

marquiz commented May 23, 2023

ArangoGutierrez May 30, 2023

Choose a reason for hiding this comment

marquiz commented Jun 22, 2023

ArangoGutierrez commented Jun 22, 2023

marquiz commented Jun 22, 2023

ArangoGutierrez commented Oct 13, 2023

k8s-ci-robot commented Oct 13, 2023

linux-foundation-easycla bot commented Mar 19, 2023 •

edited

Loading

netlify bot commented Mar 19, 2023 •

edited

Loading

bebc commented Mar 26, 2023 •

edited

Loading

k8s-ci-robot commented May 7, 2023 •

edited

Loading