topology-updater: introduce exclude-list #949

Tal-or · 2022-11-02T15:11:10Z

The exclude-list allows filtering specific resource accounting
from NRT's objects per node basis.

The CRs created by the topology-updater are used by the scheduler-plugin
as a source of truth for making scheduling decisions.
As such, this feature allows hiding specific information
from the scheduler, which in turn
will affect the scheduling decision.
A common use case is when a user would like to perform scheduling
decisions that are based on a specific resource.
In that case, we can exclude all the other resources
which we don't want the scheduler to examine.

The exclude-list is provided to the topology-updater via a ConfigMap.
Resource type's names specified in the list should match the names
as shown here: https://pkg.go.dev/k8s.io/api/core/v1#ResourceName

This is a resurrection of an old work started here:
#545

k8s-ci-robot · 2022-11-02T15:11:19Z

Hi @Tal-or. 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.

netlify · 2022-11-02T15:11:32Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`d495376`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/637bd2563d86300008d4ef95
😎 Deploy Preview	https://deploy-preview-949--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.

marquiz · 2022-11-04T09:21:04Z

Thanks @Tal-or for the patch. On a very very quick look it looks generally good 😄 I'll take a closer look next week

/ok-to-test

marquiz

I left a few comments, still a little bit of work but nothing major. Basically we should take care of the Helm deployment, too, and update documentation.

In the documentation we should somewhere describe this new feature, e.g. in docs/usage/nfd-topology-updater.md. We could also add topology-updater config reference (similar to docs/reference/worker-configuration-reference.md). We don't support dynamic configuration file update (similar to nfd-worker) and I think that's fine but I think that should be noted in the documentation as well.

Albout the commit message(s). It would be nice if the commit message would explain a bit why this is added. E.g. an example of an intended usage scenario (it's still a bit unclear to me 😅)

Also, could you wrap the body of commit messages to 72 characters (https://www.kubernetes.dev/docs/guide/pull-requests/#wrap-the-commit-message-body-at-72-characters)

deployment/components/topology-updater-config/kustomization.yaml

deployment/components/topology-updater/topologyupdater-mounts.yaml

pkg/resourcemonitor/excludelist.go

cmd/nfd-topology-updater/main.go

Tal-or · 2022-11-15T20:15:45Z

Explained the rationale for this feature and added an e2e test

marquiz · 2022-11-16T10:17:39Z

added an e2e test

👍👍 just need to fix the one linter problem there

Tal-or · 2022-11-16T10:33:31Z

added an e2e test

+1+1 just need to fix the one linter problem there

On it. Already been fixed locally I'll just address the last comment from you and upload a new revision

marquiz

Good progress @Tal-or 👍 Just a few more small comments but almost there, I think

docs/reference/topology-updater-commandline-reference.md

docs/reference/topology-updater-configuration-reference.md

docs/usage/nfd-topology-updater.md

marquiz

Thanks @Tal-or. Good that you spotted the helm chart parameters table in docs, too.

Just two more small nits 🙄 But after those I think I'm good

docs/usage/nfd-topology-updater.md

pkg/resourcemonitor/excludelist.go

marquiz · 2022-11-21T11:58:05Z

@Tal-or #961 is merged. Could you rebase this one?

The exclude-list allows to filter specific resource accounting from NRT's objects per node basis. The CRs created by the topology-updater are used by the scheduler-plugin as a source of truth for making scheduling decisions. As such, this feature allows to hide specific information from the scheduler, which in turn will affect the scheduling decision. A common use case is when user would like to perform scheduling decisions which are based on a specific resource. In that case, we can exclude all the other resources which we don't want the scheduler to exemine. The exclude-list is provided to the topology-updater via a ConfigMap. Resource type's names specified in the list should match the names as shown here: https://pkg.go.dev/k8s.io/api/core/v1#ResourceName This is a resurrection of an old work started here: kubernetes-sigs#545 Signed-off-by: Talor Itzhak <titzhak@redhat.com>

Different tests requires different configuration of the topology-updater DaemonSet. Here, we decouple the configuration from the creation part using `JustBeforeEach` so that each test container will has its own configuration. Additional reading: https://onsi.github.io/ginkgo/#separating-creation-and-configuration-justbeforeeach Signed-off-by: Talor Itzhak <titzhak@redhat.com>

Tal-or · 2022-11-21T13:30:12Z

/hold cancel #961 merged

Tal-or · 2022-11-21T15:36:38Z

/test pull-node-feature-discovery-build-image-cross-generic

marquiz

Thanks @Tal-or.

I have one more nit about the commit message title(s)

e2e: topology-updater: add e2e test

add e2e test for what, not the whole topology updater, I think 😄

Same thing with the commits updating deployment and docs.

Tal-or · 2022-11-21T16:59:01Z

Thanks @Tal-or.

I have one more nit about the commit message title(s)

e2e: topology-updater: add e2e test

add e2e test for what, not the whole topology updater, I think 😄

Same thing with the commits updating deployment and docs.

Oops :) I thought that because it is under the topology update PR it's clear. I'll address that no problem

marquiz · 2022-11-21T19:08:18Z

I thought that because it is under the topology update PR it's clear. I'll address that no problem

NP. Yeah, it's clear now but later on when reading the Git log it's not so obvious anymore :)

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

Add a kustomization file with a config example for the exclude-list. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

- Add a helm template with a config example for the exclude-list. - Add mount for the topology-updater.conf file - Update the templates Makefile target Signed-off-by: Talor Itzhak <titzhak@redhat.com>

Update the docs with explanations and examples about the exclude-list feature. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

Tal-or · 2022-11-21T19:33:05Z

I thought that because it is under the topology update PR it's clear. I'll address that no problem

NP. Yeah, it's clear now but later on when reading the Git log it's not so obvious anymore :)

Done!

marquiz

Looks good to me now 👍

ping @fromanirh @fmuyassarov wanna take a look?

k8s-ci-robot · 2022-11-22T06:04:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz, Tal-or

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

ffromani

LGTM!

fmuyassarov

/lgtm
thanks @Tal-or

ffromani · 2022-11-22T07:24:58Z

/hold cancel
per #949 (review) and #949 (review)

Tal-or · 2022-11-22T08:09:23Z

@marquiz Thank you for the time to review this work

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 2, 2022

k8s-ci-robot requested review from adrianchiris and ArangoGutierrez November 2, 2022 15:11

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 2, 2022

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 Nov 4, 2022

marquiz reviewed Nov 8, 2022

View reviewed changes

Tal-or force-pushed the exclude_list branch 2 times, most recently from 6722288 to ed0e102 Compare November 10, 2022 16:49

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

Tal-or force-pushed the exclude_list branch 2 times, most recently from bd681d9 to c5d2c0b Compare November 15, 2022 20:11

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

Tal-or force-pushed the exclude_list branch from c5d2c0b to beec618 Compare November 15, 2022 20:14

marquiz reviewed Nov 16, 2022

View reviewed changes

Tal-or force-pushed the exclude_list branch from beec618 to b921791 Compare November 16, 2022 14:22

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 16, 2022

Tal-or force-pushed the exclude_list branch 4 times, most recently from ec3e35c to 9cfa884 Compare November 16, 2022 15:13

marquiz reviewed Nov 16, 2022

View reviewed changes

docs/usage/nfd-topology-updater.md Outdated Show resolved Hide resolved

pkg/resourcemonitor/excludelist.go Show resolved Hide resolved

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

Tal-or added 2 commits November 21, 2022 14:08

Tal-or force-pushed the exclude_list branch from 569cf29 to ca7ac07 Compare November 21, 2022 12:08

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

marquiz reviewed Nov 21, 2022

View reviewed changes

Tal-or added 4 commits November 21, 2022 21:30

e2e: topology-updater: add e2e test for exclude-list feature

2896541

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

kustomize: topology-updater: enable the configuration via kustomization

8b5918a

Add a kustomization file with a config example for the exclude-list. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

helm: topology-updater: enable the configuration via helm

f832a7e

- Add a helm template with a config example for the exclude-list. - Add mount for the topology-updater.conf file - Update the templates Makefile target Signed-off-by: Talor Itzhak <titzhak@redhat.com>

docs: topology-updater: update docs for exclude-list feature

d495376

Update the docs with explanations and examples about the exclude-list feature. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

Tal-or force-pushed the exclude_list branch from ca7ac07 to d495376 Compare November 21, 2022 19:32

marquiz approved these changes Nov 22, 2022

View reviewed changes

ffromani reviewed Nov 22, 2022

View reviewed changes

fmuyassarov reviewed Nov 22, 2022

View reviewed changes

k8s-ci-robot assigned fmuyassarov Nov 22, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2022

k8s-ci-robot merged commit 592d6c6 into kubernetes-sigs:master Nov 22, 2022

Tal-or deleted the exclude_list branch November 22, 2022 08:09

marquiz mentioned this pull request Dec 20, 2022

Release v0.12.0 #985

Closed

22 tasks

Tal-or mentioned this pull request Mar 6, 2023

REQUEST: New membership for Tal-or kubernetes/org#4072

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

topology-updater: introduce exclude-list #949

topology-updater: introduce exclude-list #949

Tal-or commented Nov 2, 2022 •

edited

Loading

k8s-ci-robot commented Nov 2, 2022

netlify bot commented Nov 2, 2022 •

edited

Loading

marquiz commented Nov 4, 2022

marquiz left a comment

Tal-or commented Nov 15, 2022

marquiz commented Nov 16, 2022

Tal-or commented Nov 16, 2022

marquiz left a comment

marquiz left a comment

marquiz commented Nov 21, 2022

Tal-or commented Nov 21, 2022

Tal-or commented Nov 21, 2022

marquiz left a comment

Tal-or commented Nov 21, 2022

marquiz commented Nov 21, 2022

Tal-or commented Nov 21, 2022

marquiz left a comment

k8s-ci-robot commented Nov 22, 2022

ffromani left a comment

fmuyassarov left a comment

ffromani commented Nov 22, 2022

Tal-or commented Nov 22, 2022

topology-updater: introduce exclude-list #949

topology-updater: introduce exclude-list #949

Conversation

Tal-or commented Nov 2, 2022 • edited Loading

k8s-ci-robot commented Nov 2, 2022

netlify bot commented Nov 2, 2022 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

marquiz commented Nov 4, 2022

marquiz left a comment

Choose a reason for hiding this comment

Tal-or commented Nov 15, 2022

marquiz commented Nov 16, 2022

Tal-or commented Nov 16, 2022

marquiz left a comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

marquiz commented Nov 21, 2022

Tal-or commented Nov 21, 2022

Tal-or commented Nov 21, 2022

marquiz left a comment

Choose a reason for hiding this comment

Tal-or commented Nov 21, 2022

marquiz commented Nov 21, 2022

Tal-or commented Nov 21, 2022

marquiz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 22, 2022

ffromani left a comment

Choose a reason for hiding this comment

fmuyassarov left a comment

Choose a reason for hiding this comment

ffromani commented Nov 22, 2022

Tal-or commented Nov 22, 2022

Tal-or commented Nov 2, 2022 •

edited

Loading

netlify bot commented Nov 2, 2022 •

edited

Loading