topology-updater:compute pod set fingerprint #1049

jlojosnegros · 2023-02-03T14:21:47Z

Add an option to compute the fingerprint of the current pod set on each node.
Report this new fingerprint using an attribute in new released v1alpha2 NRT object.

needs: #1053
address: #1048
see: this issue for more info about new NRT api version

linux-foundation-easycla · 2023-02-03T14:21:53Z

The committers listed above are authorized under a signed CLA.

✅ login: jlojosnegros / name: Jose Luis Ojosnegros (dd36774)

netlify · 2023-02-03T14:21:54Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`b650150`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/63f5def97b61b300082c122a
😎 Deploy Preview	https://deploy-preview-1049--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-03T14:21:55Z

Welcome @jlojosnegros!

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-02-03T14:21:56Z

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

ffromani · 2023-02-03T14:32:08Z

/cc

ffromani · 2023-02-03T14:33:37Z

/ok-to-test

ffromani

thanks for the PR! initial review.
Do we have plans to add integration/e2e tests?

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

ffromani · 2023-02-06T12:30:49Z

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

@@ -172,7 +172,7 @@ func (w *nfdTopologyUpdater) Stop() {
 	}
 }

-func (w *nfdTopologyUpdater) updateNodeResourceTopology(zoneInfo v1alpha1.ZoneList) error {
+func (w *nfdTopologyUpdater) updateNodeResourceTopology(zoneInfo v1alpha1.ZoneList, annotations map[string]string) error {


we shoudl evaluate here to wrap the args in a struct

I have changed it to use ScanResponse
I could have created a new struct including all the params ( right now only ZoneList) but as this is a "private" method that is called only once it does not seems worthy.

ffromani · 2023-02-06T12:31:36Z

pkg/resourcemonitor/podresourcesscanner.go

 }

 // NewPodResourcesScanner creates a new ResourcesScanner instance
-func NewPodResourcesScanner(namespace string, podResourceClient podresourcesapi.PodResourcesListerClient, kubeApihelper apihelper.APIHelpers) (ResourcesScanner, error) {
+func NewPodResourcesScanner(namespace string, podResourceClient podresourcesapi.PodResourcesListerClient, kubeApihelper apihelper.APIHelpers, podFingerprint bool) (ResourcesScanner, error) {


also here we should consider to move args in a struct, but this can be deferred in a later PR

pkg/resourcemonitor/podresourcesscanner.go

pkg/resourcemonitor/podresourcesscanner_test.go

go.mod

pkg/resourcemonitor/podresourcesscanner_test.go

PiotrProkop · 2023-02-09T08:30:58Z

I think we should create a seperate PR that upgrades NRT api to v1alpha2 and not bundle it in this one(you are missing changes to kustomize and helm deployment to install new CRDs).

ffromani · 2023-02-09T08:35:13Z

I think we should create a seperate PR that upgrades NRT api to v1alpha2 and not bundle it in this one(you are missing changes to kustomize and helm deployment to install new CRDs).

good point! I'm fine with both approaches (and yes your suggestion is cleaner indeed)

PiotrProkop · 2023-02-09T08:36:32Z

I think we should create a seperate PR that upgrades NRT api to v1alpha2 and not bundle it in this one(you are missing changes to kustomize and helm deployment to install new CRDs).

good point! I'm fine with both approaches (and yes your suggestion is cleaner indeed)

I just feel like we can easily miss important things we need to do with an upgrade like advertising Policies and Scope with Attributes 😄

ffromani · 2023-02-09T08:39:00Z

I think we should create a seperate PR that upgrades NRT api to v1alpha2 and not bundle it in this one(you are missing changes to kustomize and helm deployment to install new CRDs).

good point! I'm fine with both approaches (and yes your suggestion is cleaner indeed)

I just feel like we can easily miss important things we need to do with an upgrade like advertising Policies and Scope with Attributes smile

Yes, thinking about it you're right. Let's split the update to its own PR

jlojosnegros · 2023-02-09T09:04:22Z

Let's split this PR and create a new one only for NRT API update to v1alpha2
/hold

marquiz · 2023-02-20T15:28:21Z

pkg/resourcemonitor/podresourcesscanner.go

+		var status podfingerprint.Status
+		podFingerprintSign, err := computePodFingerprint(respPodResources, &status)
+		if err != nil {
+			klog.Errorf("podFingerprint: Unable to compute fingerprint %v", err)


Should we in this case remove the existing pod fingerprint attribute?

Should we in this case remove the existing pod fingerprint attribute?

I for myself I'm fine both ways. From the scheduler plugin perspective, clients consuming this attribute must tolerate either attribute disappearing (a previous update added, a later update removes) values and obsolete values.

Here we are computing the ScanResponse. Podfingerprint attribute will not be added to the ScanResponse in case of an error.

Removing the existing attribute in the NTR should be done in the nfd-topology-updater's updateNodeResourceTopology function, when updating the attribute list.
That can be done but will go agains this (#1049 (comment)) and will require some changes in the updateAttribute function.
I understood that those changes will wait for the new utility functions in @ffromani 's PR

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

ffromani · 2023-02-21T10:13:08Z

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

+	for k, v := range check {
+		ret = append(ret, v1alpha2.AttributeInfo{Name: k, Value: v})
+	}


In general I'd have squashed the fixing commit here. Codewise looks reasonnable.

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

ffromani

/lgtm

k8s-ci-robot · 2023-02-21T12:44:52Z

LGTM label has been added.

Git tree hash: f46014c9128494f91dde8889772ada0f4f27f3c4

marquiz

Thanks for working on this @jlojosnegros. I think the code is basically ready to be merged (I had one nit about the cmdline flag description).

However, I probably forgot to ask this before, but we need to update the documentation (docs/reference/topology-updater-commandline-reference.md) to describe the new command line flag.

Also, could you add support for the new flag in the help chart:

add smth like topologyUpdater.podSetFingerprint into deployment/helm/node-feature-discovery/values.yaml
use it in deployment/helm/node-feature-discovery/templates/topologyupdater.yaml
document the new helm parameter in docs/deployment/helm.md

cmd/nfd-topology-updater/main.go

jlojosnegros · 2023-02-21T14:50:27Z

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

We are gonna add new data to Scan response so better introduce a new ScanResponse struct as Scan return value to make it easier.

deployment/helm/node-feature-discovery/templates/topologyupdater.yaml

Add an option to compute the fingerprint of the current pod set on each node. Report this new fingerprint using an attribute in NRT object.

ffromani

/lgtm

updates to docs and helm chart seem good

k8s-ci-robot · 2023-02-22T09:50:48Z

LGTM label has been added.

Git tree hash: 3a4c5d93b06792657da6f0b572eb7aa16ffb34ce

marquiz

Thanks @jlojosnegros for the quick update. Looks good to me now 👍 If something's missing let's fix it in subsequent PRs

k8s-ci-robot · 2023-02-22T10:04:57Z

[APPROVALNOTIFIER] This PR is APPROVED

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

k8s-ci-robot requested review from ArangoGutierrez and zvonkok February 3, 2023 14:21

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

k8s-ci-robot requested a review from ffromani February 3, 2023 14:32

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 3, 2023

jlojosnegros force-pushed the node-signature branch from dd36774 to 6877401 Compare February 3, 2023 15:18

ffromani reviewed Feb 6, 2023

View reviewed changes

jlojosnegros force-pushed the node-signature branch 3 times, most recently from 28a6141 to abab034 Compare February 7, 2023 15:57

ffromani reviewed Feb 7, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/resourcemonitor/podresourcesscanner_test.go Outdated Show resolved Hide resolved

jlojosnegros force-pushed the node-signature branch 3 times, most recently from fcb05bb to f941cd7 Compare February 8, 2023 11:49

marquiz reviewed Feb 20, 2023

View reviewed changes

PiotrProkop reviewed Feb 20, 2023

View reviewed changes

pkg/nfd-topology-updater/nfd-topology-updater.go Outdated Show resolved Hide resolved

pkg/nfd-topology-updater/nfd-topology-updater.go Show resolved Hide resolved

jlojosnegros force-pushed the node-signature branch from 1bec609 to d7106dd Compare February 21, 2023 09:19

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

jlojosnegros force-pushed the node-signature branch from d7106dd to f09aafb Compare February 21, 2023 09:24

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

ffromani reviewed Feb 21, 2023

View reviewed changes

jlojosnegros force-pushed the node-signature branch from f09aafb to f720f60 Compare February 21, 2023 11:17

ffromani reviewed Feb 21, 2023

View reviewed changes

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

marquiz reviewed Feb 21, 2023

View reviewed changes

cmd/nfd-topology-updater/main.go Outdated Show resolved Hide resolved

topology-updater: Refactor Scan to expand response

1a687cb

We are gonna add new data to Scan response so better introduce a new ScanResponse struct as Scan return value to make it easier.

jlojosnegros force-pushed the node-signature branch from f720f60 to 7c39b10 Compare February 22, 2023 08:56

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

k8s-ci-robot requested review from ffromani and marquiz February 22, 2023 08:56

ffromani reviewed Feb 22, 2023

View reviewed changes

deployment/helm/node-feature-discovery/templates/topologyupdater.yaml Outdated Show resolved Hide resolved

jlojosnegros added 2 commits February 22, 2023 10:22

topology-updater:compute pod set fingerprint

b340d11

Add an option to compute the fingerprint of the current pod set on each node. Report this new fingerprint using an attribute in NRT object.

topology-updater: e2e test for podFingerprint

b650150

jlojosnegros force-pushed the node-signature branch from 7c39b10 to b650150 Compare February 22, 2023 09:23

ffromani reviewed Feb 22, 2023

View reviewed changes

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

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

k8s-ci-robot merged commit 163a6dc into kubernetes-sigs:master Feb 22, 2023

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

topology-updater:compute pod set fingerprint #1049

topology-updater:compute pod set fingerprint #1049

jlojosnegros commented Feb 3, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 3, 2023 •

edited

Loading

netlify bot commented Feb 3, 2023 •

edited

Loading

k8s-ci-robot commented Feb 3, 2023

k8s-ci-robot commented Feb 3, 2023

ffromani commented Feb 3, 2023

ffromani commented Feb 3, 2023

ffromani left a comment

ffromani Feb 6, 2023

jlojosnegros Feb 8, 2023

ffromani Feb 6, 2023

PiotrProkop commented Feb 9, 2023

ffromani commented Feb 9, 2023

PiotrProkop commented Feb 9, 2023

ffromani commented Feb 9, 2023

jlojosnegros commented Feb 9, 2023

marquiz Feb 20, 2023

ffromani Feb 20, 2023 •

edited

Loading

jlojosnegros Feb 21, 2023

ffromani Feb 21, 2023

ffromani left a comment

k8s-ci-robot commented Feb 21, 2023

marquiz left a comment

jlojosnegros commented Feb 21, 2023

ffromani left a comment

k8s-ci-robot commented Feb 22, 2023

marquiz left a comment

k8s-ci-robot commented Feb 22, 2023

topology-updater:compute pod set fingerprint #1049

topology-updater:compute pod set fingerprint #1049

Conversation

jlojosnegros commented Feb 3, 2023 • edited Loading

linux-foundation-easycla bot commented Feb 3, 2023 • edited Loading

netlify bot commented Feb 3, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

k8s-ci-robot commented Feb 3, 2023

k8s-ci-robot commented Feb 3, 2023

ffromani commented Feb 3, 2023

ffromani commented Feb 3, 2023

ffromani left a comment

Choose a reason for hiding this comment

ffromani Feb 6, 2023

Choose a reason for hiding this comment

jlojosnegros Feb 8, 2023

Choose a reason for hiding this comment

ffromani Feb 6, 2023

Choose a reason for hiding this comment

PiotrProkop commented Feb 9, 2023

ffromani commented Feb 9, 2023

PiotrProkop commented Feb 9, 2023

ffromani commented Feb 9, 2023

jlojosnegros commented Feb 9, 2023

marquiz Feb 20, 2023

Choose a reason for hiding this comment

ffromani Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

jlojosnegros Feb 21, 2023

Choose a reason for hiding this comment

ffromani Feb 21, 2023

Choose a reason for hiding this comment

ffromani left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 21, 2023

marquiz left a comment

Choose a reason for hiding this comment

jlojosnegros commented Feb 21, 2023

ffromani left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 22, 2023

marquiz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 22, 2023

jlojosnegros commented Feb 3, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 3, 2023 •

edited

Loading

netlify bot commented Feb 3, 2023 •

edited

Loading

ffromani Feb 20, 2023 •

edited

Loading