Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apis/nfd: add matchName field in feature matcher terms #788

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Mar 17, 2022

Extend NodeFeatureRule API

Extend the format of feature matcher terms (the elements of the
arrayspecified under under matchFeatures field) with new matchName
field. The value of this field is an expression that is evaluated
against the names of feature elements instead of their values (values
are matched with the matchExpressions field, instead).

The matchName field is useful e.g. in template rules for creating
per-feature-element labels based on feature names (instead of values)
and in non-template rules for checking if (at least) one of certain
feature element names are present.

If both matchExpressions and matchName for certain feature matcher term
is specified, they both must match in order to get an overall match.
Also, in this case the list of matched features (used in templating) is
the union of the results from matchExpressions and matchName.

An example of creating an "avx512" label if any AVX512* CPUID feature is
present:

  - name: "avx wildcard rule"
    labels:
        avx512: "true"
    matchFeatures:
      - feature: cpu.cpuid
        matchName: {op: InRegexp, value: ["^AVX512"]}

An example of a template rule creating a dynamic set of labels based on
the existence of certain kconfig options.

  - name: "kconfig template rule"
    labelsTemplate: |
      {{ range .kernel.config }}kconfig-{{ .Name }}={{ .Value }}
      {{ end }}
    matchFeatures:
      - feature: kernel.config
        matchName: {op: In, value: ["SWAP", "X86", "ARM"]}

Sample rules

This PR contains also another patch that adds sample NodeFeatureRules that correspond the built-in
labels created by nfd-worker (with its default configuration).

The samples provide examples on how to utilize NodeFeatureRules and an
easy way to modify the labels being generated. In order to replace the
built-in node labeling of nfd-worker with these sample rules, all node
labeling from nfd-worker must be disabled by setting the
core.labelSources configuration option to an empty list (or specify
-label-sources= on the command line). Then, with all nfd-worker side
labeling disabled, just apply the rules with

kubectl apply -f samples/

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2022
@marquiz
Copy link
Contributor Author

marquiz commented Mar 17, 2022

Still a POC/RFC, missing e.g. documentation
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 17, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2022
@marquiz marquiz force-pushed the devel/wildcard-rule-ng branch from ba69b27 to 727f86f Compare April 21, 2022 11:09
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2022
@marquiz marquiz force-pushed the devel/wildcard-rule-ng branch from 727f86f to ff69ea4 Compare June 15, 2022 10:04
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2022
@marquiz marquiz force-pushed the devel/wildcard-rule-ng branch from ff69ea4 to 62f3672 Compare August 12, 2022 06:53
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2022
@ArangoGutierrez
Copy link
Contributor

still a hold?

@marquiz
Copy link
Contributor Author

marquiz commented Aug 24, 2022

still a hold?

Heh, I'm still a bit unsure do we need this, if anybody wants this 😄 OTOH, it would make it possible to turn all "built-in" labels into NodeFeatureRules 🧐

In addition, docs are still missing.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2022
@marquiz marquiz force-pushed the devel/wildcard-rule-ng branch from 62f3672 to bc90a9a Compare August 24, 2022 08:08
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2022
@marquiz marquiz force-pushed the devel/wildcard-rule-ng branch from bc90a9a to 3595986 Compare August 24, 2022 08:13
@marquiz
Copy link
Contributor Author

marquiz commented Aug 30, 2022

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2022
@marquiz marquiz force-pushed the devel/wildcard-rule-ng branch from 3595986 to e6a2f3e Compare October 7, 2022 12:21
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2022
@marquiz
Copy link
Contributor Author

marquiz commented Oct 7, 2022

Rebased

@netlify
Copy link

netlify bot commented Oct 7, 2022

Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit 49886ed
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/657c1d4ea48f910008af4ab5
😎 Deploy Preview https://deploy-preview-788--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2022
@marquiz marquiz force-pushed the devel/wildcard-rule-ng branch from e6a2f3e to b28c310 Compare December 20, 2022 09:20
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2022
@marquiz marquiz force-pushed the devel/wildcard-rule-ng branch from aacea3f to 8b9a3c8 Compare December 12, 2023 15:55
@marquiz
Copy link
Contributor Author

marquiz commented Dec 12, 2023

Update:

  • rebased
  • fixed a bug where the matchNames never failed
  • added e2e tests

I think this would be ready for review, no reason to keep it hanging open forever. At least should not have any effect (cause problems) if not used 😉
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2023
@marquiz
Copy link
Contributor Author

marquiz commented Dec 12, 2023

/assign @ArangoGutierrez

@marquiz
Copy link
Contributor Author

marquiz commented Dec 12, 2023

Ach, docs is missing 🙈 (plus unit tests fail)
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2023
@marquiz marquiz force-pushed the devel/wildcard-rule-ng branch 2 times, most recently from 03fd03b to 64e88a4 Compare December 12, 2023 17:50
@marquiz
Copy link
Contributor Author

marquiz commented Dec 12, 2023

Rebased. Added documentation.
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2023
@@ -0,0 +1,112 @@
#
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with this Samples, should we remove examples ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good question where we should put these 🤔 The examples/ are really supposed to be examples but these are "fully usable" samples that replicate the built-in labels. Maybe these should be put somewhere under deployment/ instead, e.g. deployment/nodefeaturerule/. Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like deployment/nodefeaturerule/samples

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do that, then. Better I think

@ArangoGutierrez
Copy link
Contributor

/retest

@ArangoGutierrez
Copy link
Contributor

/needs-rebase

Extend the format of feature matcher terms (the elements of the
arrayspecified under under matchFeatures field) with new matchName
field. The value of this field is an expression that is evaluated
against the names of feature elements instead of their values (values
are matched with the matchExpressions field, instead).

The matchName field is useful e.g. in template rules for creating
per-feature-element labels based on feature names (instead of values)
and in non-template rules for checking if (at least) one of certain
feature element names are present.

If both matchExpressions and matchName for certain feature matcher term
is specified, they both must match in order to get an overall match.
Also, in this case the list of matched features (used in templating) is
the union of the results from matchExpressions and matchName.

An example of creating an "avx512" label if any AVX512* CPUID feature is
present:

  - name: "avx wildcard rule"
    labels:
        avx512: "true"
    matchFeatures:
      - feature: cpu.cpuid
        matchName: {op: InRegexp, value: ["^AVX512"]}

An example of a template rule creating a dynamic set of labels  based on
the existence of certain kconfig options.

  - name: "kconfig template rule"
    labelsTemplate: |
      {{ range .kernel.config }}kconfig-{{ .Name }}={{ .Value }}
      {{ end }}
    matchFeatures:
      - feature: kernel.config
        matchName: {op: In, value: ["SWAP", "X86", "ARM"]}

NOTE: this patch changes the corner case of nil/null match expressions
with instance features (i.e. "matchExpressions: null"). Previously, we
returned all instances for templating but now a nil match expression is
not evaluated and no instances for templating are returned.
Work around a bug in k8s deepcopy-gen.
This patch adds sample NodeFeatureRules that correspond the built-in
labels created by nfd-worker (with its default configuration).

The samples provide examples on how to utilize NodeFeatureRules and an
easy way to modify the labels being generated. In order to replace the
built-in node labeling of nfd-worker with these sample rules, all node
labeling from nfd-worker must be disabled by setting the
"core.labelSources" configuration option to an empty list (or specify
"-label-sources= " on the command line). Then, with all nfd-worker side
labeling disabled, just apply the rules with

  kubectl apply -f samples/
@marquiz marquiz force-pushed the devel/wildcard-rule-ng branch from 64e88a4 to 49886ed Compare December 15, 2023 09:33
Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v0.15 here we gooo!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 15, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 4714b3d1701ce3b7624f3ff116fc7e8d584dcaea

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, 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 [ArangoGutierrez,marquiz]

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

@marquiz
Copy link
Contributor Author

marquiz commented Dec 15, 2023

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2023
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Merging #788 (49886ed) into master (443ff80) will decrease coverage by 0.07%.
The diff coverage is 34.25%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #788      +/-   ##
==========================================
- Coverage   31.39%   31.33%   -0.07%     
==========================================
  Files          59       59              
  Lines        7555     7647      +92     
==========================================
+ Hits         2372     2396      +24     
- Misses       4939     5001      +62     
- Partials      244      250       +6     
Files Coverage Δ
pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go 26.14% <57.89%> (+0.25%) ⬆️
pkg/apis/nfd/v1alpha1/rule.go 78.12% <64.28%> (-2.95%) ⬇️
pkg/apis/nfd/v1alpha1/expression.go 66.66% <13.11%> (-10.12%) ⬇️

@k8s-ci-robot k8s-ci-robot merged commit 119e2a3 into kubernetes-sigs:master Dec 15, 2023
14 checks passed
@marquiz marquiz deleted the devel/wildcard-rule-ng branch December 15, 2023 11:30
@marquiz
Copy link
Contributor Author

marquiz commented Dec 15, 2023

Too slow 😿

@marquiz
Copy link
Contributor Author

marquiz commented Dec 15, 2023

OK, see #1504

@marquiz marquiz mentioned this pull request Dec 20, 2023
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants