fix topology-updater cpu report #1979

AllenXu93 · 2024-12-13T06:46:42Z

Fix #1978

linux-foundation-easycla · 2024-12-13T06:46:47Z

The committers listed above are authorized under a signed CLA.

✅ login: AllenXu93 (f740e1d, a694d91, 17d374d, 5d40f79, d9d6ac5)

k8s-ci-robot · 2024-12-13T06:46:51Z

Welcome @AllenXu93!

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 · 2024-12-13T06:46:52Z

Hi @AllenXu93. 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-sigs/prow repository.

netlify · 2024-12-13T06:47:01Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`d9d6ac5`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/67e4bc6f6a7ad1000825faa5
😎 Deploy Preview	https://deploy-preview-1979--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 configuration.

ffromani · 2024-12-16T11:00:54Z

/cc

Need to review the code before to comment :)
While I get again up to speed, do we have test coverage for this change? can we use more unit/e2e tests?

ffromani · 2024-12-16T11:03:23Z

/ok-to-test

marquiz · 2024-12-17T13:39:37Z

While I get again up to speed, do we have test coverage for this change? can we use more unit/e2e tests?

We seem to have some as the tests failed. But yes, we should have specific test case for this scenario-

@AllenXu93 could you fix and update the tests?

AllenXu93 · 2024-12-17T13:41:42Z

While I get again up to speed, do we have test coverage for this change? can we use more unit/e2e tests?

We seem to have some as the tests failed. But yes, we should have specific test case for this scenario-

@AllenXu93 could you fix and update the tests?

ok I will fix in this week

AllenXu93 · 2024-12-23T02:53:16Z

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

pkg/resourcemonitor/podresourcesscanner_test.go

k8s-ci-robot · 2025-01-08T03:05:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AllenXu93
Once this PR has been reviewed and has the lgtm label, please assign marquiz for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

pkg/resourcemonitor/podresourcesscanner.go

marquiz · 2025-01-08T09:44:50Z

Now the unit tests fail

pkg/resourcemonitor/podresourcesscanner.go

AllenXu93 · 2025-03-25T12:50:50Z

@marquiz @ffromani Sorry to bother you, can you help to review this PR ?

marquiz · 2025-03-25T17:28:34Z

Sorry to bother you, can you help to review this PR ?

@AllenXu93 any thoughts on these comments?
#1979 (comment)

k8s-ci-robot · 2025-03-27T02:55:37Z

@AllenXu93: The following tests 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-e2e-test-master	`d9d6ac5`	link	true	`/test pull-node-feature-discovery-e2e-test-master`
pull-node-feature-discovery-verify-master	`d9d6ac5`	link	true	`/test pull-node-feature-discovery-verify-master`
pull-node-feature-discovery-build-master	`d9d6ac5`	link	true	`/test pull-node-feature-discovery-build-master`
pull-node-feature-discovery-build-image-cross-generic	`d9d6ac5`	link	true	`/test pull-node-feature-discovery-build-image-cross-generic`

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

AllenXu93 · 2025-03-27T02:57:24Z

Sorry to bother you, can you help to review this PR ?

@AllenXu93 any thoughts on these comments? #1979 (comment)

The latest code is updated as you suggestion. I rename suggestion

Sorry to bother you, can you help to review this PR ?

@AllenXu93 any thoughts on these comments? #1979 (comment)

Updated the code just now

isIntegralGuaranteed := hasExclusiveCPUs(pod)

rename to :

isPodHasIntegralCPUs := podHasIntegralCPUs(pod)

Do you think these changes ok ?

marquiz · 2025-03-27T11:56:56Z

pkg/resourcemonitor/podresourcesscanner.go

 	if err != nil {
 		return false, false, err
 	}

-	isIntegralGuaranteed := hasExclusiveCPUs(pod)
+	isPodHasIntegralCPUs := podHasIntegralCPUs(pod)


nit: not a blocker by any means but the name isPodHas... is just a funny. Some suggestions for renaming, as the PR needs to be udpated in any case because of build failures

Suggested change

isPodHasIntegralCPUs := podHasIntegralCPUs(pod)

podHasExclusiveCPUs := checkPodExclusiveCPUs(pod)

OR

Suggested change

isPodHasIntegralCPUs := podHasIntegralCPUs(pod)

podHasExclusiveCPUs := hasExclusiveCPUs(pod)

ffromani

sorry it took so long to me. I have mostly minorish comments, once addressed I think LGTM

ffromani · 2025-03-27T12:04:14Z

pkg/resourcemonitor/podresourcesscanner.go

@@ -19,13 +19,12 @@ package resourcemonitor
 import (
 	"context"
 	"fmt"
+	"k8s.io/kubernetes/pkg/apis/core/v1/helper/qos"


misplaced import

ffromani · 2025-03-27T12:11:06Z

pkg/resourcemonitor/podresourcesscanner.go

+// In Scan(), if watchable is false, this pods scan will skip
+// so we can return directly if pod's namespace is not watchable
+func (resMon *PodResourcesScanner) isWatchable(podResource *podresourcesapi.PodResources) (bool, bool, error) {
+	if resMon.namespace != "*" && resMon.namespace != podResource.Namespace {


podResource.GetNamespace() hanbdles gracefully nil pointer, but I guess it's minor(ish)?

func (m *PodResources) GetNamespace() string { if m != nil { return m.Namespace } return "" }

(my preference is to always use GetNamespace everything else being equal)

ffromani · 2025-03-27T12:20:19Z

pkg/resourcemonitor/podresourcesscanner.go

-						},
-					}
+			cpuIDs := container.GetCpuIds()
+			if len(cpuIDs) > 0 && isExclusiveCPUs {


minor: isExclusiveCPUs should be true only if the pod does request some CPUs. IOW, I'd handle the corner case on which the container requests 0 CPUs (!!!!) inside the functions, not outside.
Arguably, a container which requests 0 CPUs does not have any exclusive CPUs assigned

ffromani · 2025-03-27T12:20:33Z

pkg/resourcemonitor/podresourcesscanner.go

+			cpuIDs := container.GetCpuIds()
+			if len(cpuIDs) > 0 && isExclusiveCPUs {
+				var resCPUs []string
+				for _, cpuID := range container.GetCpuIds() {


(re)use cpuIDs computed on line 157 above?

(Yes, the original code called it twice unnecessarily)

k8s-ci-robot requested review from fmuyassarov and zvonkok December 13, 2024 06:46

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 13, 2024

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 Dec 13, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 13, 2024

AllenXu93 force-pushed the bugfix/fix-topology-updater-cpu branch from 15fda0b to 109cb7c Compare December 13, 2024 06:51

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 13, 2024

fix topology-updater cpu report

a694d91

AllenXu93 force-pushed the bugfix/fix-topology-updater-cpu branch from 109cb7c to a694d91 Compare December 13, 2024 07:15

k8s-ci-robot requested a review from ffromani December 16, 2024 11:00

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 Dec 16, 2024

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 Dec 20, 2024

fix for UT

17d374d

AllenXu93 force-pushed the bugfix/fix-topology-updater-cpu branch from 281bc7e to 17d374d Compare December 23, 2024 08:08

marquiz reviewed Jan 2, 2025

View reviewed changes

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

update isIntegralGuaranteed

f740e1d

ffromani reviewed Jan 8, 2025

View reviewed changes

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

rewrite isWatchable in pod scan

5d40f79

AllenXu93 force-pushed the bugfix/fix-topology-updater-cpu branch from b41f3ad to 5d40f79 Compare January 8, 2025 13:55

marquiz reviewed Jan 22, 2025

View reviewed changes

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

rename podHasIntegralCPUs

d9d6ac5

marquiz reviewed Mar 27, 2025

View reviewed changes

ffromani reviewed Mar 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix topology-updater cpu report #1979

fix topology-updater cpu report #1979

AllenXu93 commented Dec 13, 2024

linux-foundation-easycla bot commented Dec 13, 2024 •

edited

Loading

k8s-ci-robot commented Dec 13, 2024

k8s-ci-robot commented Dec 13, 2024

netlify bot commented Dec 13, 2024 •

edited

Loading

ffromani commented Dec 16, 2024

ffromani commented Dec 16, 2024

marquiz commented Dec 17, 2024

AllenXu93 commented Dec 17, 2024

AllenXu93 commented Dec 23, 2024

k8s-ci-robot commented Jan 8, 2025

marquiz commented Jan 8, 2025

AllenXu93 commented Mar 25, 2025

marquiz commented Mar 25, 2025

k8s-ci-robot commented Mar 27, 2025

AllenXu93 commented Mar 27, 2025

marquiz Mar 27, 2025

ffromani left a comment

ffromani Mar 27, 2025

ffromani Mar 27, 2025

ffromani Mar 27, 2025

ffromani Mar 27, 2025

ffromani Mar 27, 2025

	isPodHasIntegralCPUs := podHasIntegralCPUs(pod)
	podHasExclusiveCPUs := checkPodExclusiveCPUs(pod)

	isPodHasIntegralCPUs := podHasIntegralCPUs(pod)
	podHasExclusiveCPUs := hasExclusiveCPUs(pod)

fix topology-updater cpu report #1979

Are you sure you want to change the base?

fix topology-updater cpu report #1979

Conversation

AllenXu93 commented Dec 13, 2024

linux-foundation-easycla bot commented Dec 13, 2024 • edited Loading

k8s-ci-robot commented Dec 13, 2024

k8s-ci-robot commented Dec 13, 2024

netlify bot commented Dec 13, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

ffromani commented Dec 16, 2024

ffromani commented Dec 16, 2024

marquiz commented Dec 17, 2024

AllenXu93 commented Dec 17, 2024

AllenXu93 commented Dec 23, 2024

k8s-ci-robot commented Jan 8, 2025

marquiz commented Jan 8, 2025

AllenXu93 commented Mar 25, 2025

marquiz commented Mar 25, 2025

k8s-ci-robot commented Mar 27, 2025

AllenXu93 commented Mar 27, 2025

marquiz Mar 27, 2025

Choose a reason for hiding this comment

ffromani left a comment

Choose a reason for hiding this comment

ffromani Mar 27, 2025

Choose a reason for hiding this comment

ffromani Mar 27, 2025

Choose a reason for hiding this comment

ffromani Mar 27, 2025

Choose a reason for hiding this comment

ffromani Mar 27, 2025

Choose a reason for hiding this comment

ffromani Mar 27, 2025

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Dec 13, 2024 •

edited

Loading

netlify bot commented Dec 13, 2024 •

edited

Loading