Advertise TopologyManger policy and scope as Attributes in NRT api v1alpha2 #1054

PiotrProkop · 2023-02-09T14:37:38Z

We want to advertise TopologyManager policy and scope as top level attributes in noderesourcetopology-api v1alpha2.
Based on this PR kubernetes-sigs/scheduler-plugins#488.

netlify · 2023-02-09T14:37:43Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`f76fc5b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/63ec977c18ee1d0008a28b99
😎 Deploy Preview	https://deploy-preview-1054--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-02-09T14:37:47Z

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

PiotrProkop · 2023-02-09T14:38:13Z

I'll add unit tests for this tomorrow. @ffromani @swatisehgal please review.

ffromani · 2023-02-09T14:40:32Z

/ok-to-test

ffromani

thanks for the quick PR! looks straightforward, suggest possible improvements inside.

cmd/nfd-topology-updater/main.go

ffromani · 2023-02-09T14:45:56Z

pkg/topologypolicy/topology-policy.go

@@ -63,3 +70,16 @@ func detectPolicyContainerScope(policy string) v1alpha2.TopologyManagerPolicy {
 		return v1alpha2.None
 	}
 }
+
+func DetectTopologyAttributes(policy string, scope string) v1alpha2.AttributeList {


codewise this is fine, but in the near future I'd love to deprecate pkg/topologypolicy and adding code here goes against this direction. So I'd move this code in pkg/nfd-topology-updater (possibly in nfd-topolog-updater.go) as private function.

Thanks! if you move, please also rename this function! The code is just fine, but it is not detecting, rather constructing/creating

moved and changed, and got rid of AttributeName type alias which was unused :)

swatisehgal · 2023-02-09T14:47:11Z

/cc

ffromani

looks fine. I'd like to see some form of test coverage (probably integration/e2e) before LGTM

ffromani · 2023-02-10T08:50:51Z

pkg/nfd-topology-updater/nfd-topology-updater.go

+	tmScope  string
+}
+
+func newStaticNodeInfo(policy, scope string) *staticNodeInfo {


nit/nonblocking: I'd just return a struct (not a pointer to a struct) and trust the compiler.
In general, returning pointers may allow faster copying of objects (but it should actually be measured) but also contributes to the GC pressure (which, OTOH, should also be measured).
What I'm trying to say is that returning pointers is not necessarily the faster option,

note: I'd NOT resubmit just for this.

I want to add tests, so I'll change it when I submit them.

Signed-off-by: pprokop <[email protected]>

…vertise as Attributes Signed-off-by: pprokop <[email protected]>

PiotrProkop · 2023-02-10T11:06:30Z

@ffromani I had to change the way how we load Kubelet config when read from file to make e2e test working, previously we were just reading config from file and unamarshalling it to struct. This approach didn't fill out default values but were skipping it based on omitempty tag. In e2e tests we are reading Kubelet config via http, which is serving Kubeletconfig with all values filled 😄

ffromani

/lgtm
very nice work @PiotrProkop !

test/e2e/utils/noderesourcetopology.go

pkg/utils/kubeconf/kubelet_config_file.go

k8s-ci-robot · 2023-02-10T12:51:28Z

LGTM label has been added.

Git tree hash: cf5ca6af5a40979ec18eeb7c6c25c66044a32135

PiotrProkop · 2023-02-10T15:39:53Z

/assign @marquiz for final approval

marquiz · 2023-02-10T15:42:43Z

Thanks @PiotrProkop for the PR. I'll check this next week

test/e2e/utils/noderesourcetopology.go

pkg/utils/kubeconf/kubelet_config_file.go

…alues Signed-off-by: PiotrProkop <[email protected]>

marquiz

Thanks @PiotrProkop, looks good to me now

k8s-ci-robot · 2023-02-15T08:44:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

PiotrProkop · 2023-02-15T08:46:08Z

@ffromani can you LGTM once more? I had to fix a small nit.

ffromani · 2023-02-15T09:12:04Z

/lgtm

k8s-ci-robot · 2023-02-15T09:12:09Z

LGTM label has been added.

Git tree hash: 53ec60386c31adafa4e240cd9c1ffeaa60a3a18f

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 Feb 9, 2023

k8s-ci-robot requested review from fmuyassarov and kad February 9, 2023 14:37

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 9, 2023

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 Feb 9, 2023

ffromani reviewed Feb 9, 2023

View reviewed changes

k8s-ci-robot requested a review from swatisehgal February 9, 2023 14:47

PiotrProkop force-pushed the use-new-nrt-api branch from 3c38cec to 04095cc Compare February 10, 2023 08:47

ffromani reviewed Feb 10, 2023

View reviewed changes

PiotrProkop force-pushed the use-new-nrt-api branch from 04095cc to e621e13 Compare February 10, 2023 11:02

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 10, 2023

PiotrProkop added 2 commits February 10, 2023 12:03

Advertise TopologyManger policy and scope as Attributes

5484bab

Signed-off-by: pprokop <[email protected]>

Modify e2e tests to check if Topology Manager policy and scope are ad…

b51e34d

…vertise as Attributes Signed-off-by: pprokop <[email protected]>

PiotrProkop force-pushed the use-new-nrt-api branch from e621e13 to 94468d8 Compare February 10, 2023 11:03

ffromani reviewed Feb 10, 2023

View reviewed changes

test/e2e/utils/noderesourcetopology.go Show resolved Hide resolved

pkg/utils/kubeconf/kubelet_config_file.go Show resolved Hide resolved

k8s-ci-robot assigned ffromani Feb 10, 2023

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

ffromani mentioned this pull request Feb 10, 2023

noderesourcetopology: overhaul Topology Manager configuration management kubernetes-sigs/scheduler-plugins#488

Merged

marquiz reviewed Feb 15, 2023

View reviewed changes

test/e2e/utils/noderesourcetopology.go Show resolved Hide resolved

pkg/utils/kubeconf/kubelet_config_file.go Outdated Show resolved Hide resolved

Read Kubelet configuration the same way as Kubelet to apply default v…

f76fc5b

…alues Signed-off-by: PiotrProkop <[email protected]>

PiotrProkop force-pushed the use-new-nrt-api branch from 94468d8 to f76fc5b Compare February 15, 2023 08:27

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

k8s-ci-robot requested a review from ffromani February 15, 2023 08:27

marquiz approved these changes Feb 15, 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 15, 2023

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

k8s-ci-robot merged commit 38cc370 into kubernetes-sigs:master Feb 15, 2023

jlojosnegros mentioned this pull request Feb 15, 2023

topology-updater:compute pod set fingerprint #1049

Merged

PiotrProkop mentioned this pull request Mar 3, 2023

REQUEST: New membership for PiotrProkop kubernetes/org#4063

Closed

9 tasks

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

Advertise TopologyManger policy and scope as Attributes in NRT api v1alpha2 #1054

Advertise TopologyManger policy and scope as Attributes in NRT api v1alpha2 #1054

PiotrProkop commented Feb 9, 2023

netlify bot commented Feb 9, 2023 •

edited

Loading

k8s-ci-robot commented Feb 9, 2023

PiotrProkop commented Feb 9, 2023

ffromani commented Feb 9, 2023

ffromani left a comment

ffromani Feb 9, 2023

PiotrProkop Feb 10, 2023

ffromani Feb 10, 2023

PiotrProkop Feb 10, 2023

swatisehgal commented Feb 9, 2023

ffromani left a comment •

edited

Loading

ffromani Feb 10, 2023

ffromani Feb 10, 2023

PiotrProkop Feb 10, 2023 •

edited

Loading

PiotrProkop commented Feb 10, 2023

ffromani left a comment

k8s-ci-robot commented Feb 10, 2023

PiotrProkop commented Feb 10, 2023

marquiz commented Feb 10, 2023

marquiz left a comment

k8s-ci-robot commented Feb 15, 2023

PiotrProkop commented Feb 15, 2023

ffromani commented Feb 15, 2023

k8s-ci-robot commented Feb 15, 2023

Advertise TopologyManger policy and scope as Attributes in NRT api v1alpha2 #1054

Advertise TopologyManger policy and scope as Attributes in NRT api v1alpha2 #1054

Conversation

PiotrProkop commented Feb 9, 2023

netlify bot commented Feb 9, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

k8s-ci-robot commented Feb 9, 2023

PiotrProkop commented Feb 9, 2023

ffromani commented Feb 9, 2023

ffromani left a comment

Choose a reason for hiding this comment

ffromani Feb 9, 2023

Choose a reason for hiding this comment

PiotrProkop Feb 10, 2023

Choose a reason for hiding this comment

ffromani Feb 10, 2023

Choose a reason for hiding this comment

PiotrProkop Feb 10, 2023

Choose a reason for hiding this comment

swatisehgal commented Feb 9, 2023

ffromani left a comment • edited Loading

Choose a reason for hiding this comment

ffromani Feb 10, 2023

Choose a reason for hiding this comment

ffromani Feb 10, 2023

Choose a reason for hiding this comment

PiotrProkop Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

PiotrProkop commented Feb 10, 2023

ffromani left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 10, 2023

PiotrProkop commented Feb 10, 2023

marquiz commented Feb 10, 2023

marquiz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 15, 2023

PiotrProkop commented Feb 15, 2023

ffromani commented Feb 15, 2023

k8s-ci-robot commented Feb 15, 2023

netlify bot commented Feb 9, 2023 •

edited

Loading

ffromani left a comment •

edited

Loading

PiotrProkop Feb 10, 2023 •

edited

Loading