kubelet: Fix the bug of getting the number of windows cpu #97378

hwdef · 2020-12-18T03:56:38Z

fix the bug that the number of cpu cannot be obtained normally under Windows

What type of PR is this?

/kind bug

What this PR does / why we need it:

In some cases, kubelet will return an error value when obtaining the number of CPUs, only half of the correct value will be returned.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

I found the way to fix in https://github.com/microsoft/hcsshim/blob/master/internal/processorinfo/processor_count.go

same issue in moby: moby/moby#38935 (comment)

Does this PR introduce a user-facing change?:

kubelet: Fixed the bug of getting the number of cpu when the number of cpu logical processors is more than 64 in windows

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

hwdef · 2020-12-18T04:43:27Z

/sig windows

hwdef · 2020-12-18T06:09:46Z

/assign @liggitt @thockin @mrunalp

jayunit100 · 2020-12-19T14:54:02Z

Is there a scheduling e2e or other test that we can have for this as a follow on PR? Seems like verifying this on windows nodes which already can have scheduling issues would be good for the windows conformance suites

hwdef · 2020-12-19T17:55:45Z

@jayunit100 My colleagues are testing it, but there are no results yet. I will continue to add if I get the results

hwdef · 2020-12-22T06:36:28Z

@jayunit100 we tested it on a server with 72 cores and 144 logical processors(There are a total of 4 cpus, each cpu has 18 cores).

ehashman · 2021-01-25T19:26:23Z

/triage accepted

ehashman

This looks reasonable to me but I'm not a Windows reviewer, leaving for SIG Windows :)

ehashman · 2021-02-11T23:12:14Z

pkg/kubelet/winstats/perfcounter_nodestats.go

-	procGlobalMemoryStatusEx = modkernel32.NewProc("GlobalMemoryStatusEx")
+	modkernel32                 = windows.NewLazySystemDLL("kernel32.dll")
+	procGlobalMemoryStatusEx    = modkernel32.NewProc("GlobalMemoryStatusEx")
+	procGetActiveProcessorCount = modkernel32.NewProc("GetActiveProcessorCount")


https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getactiveprocessorcount

hwdef · 2021-02-18T09:10:06Z

@marosset
Could you give me some help?

jsturtevant · 2021-03-11T18:15:08Z

pkg/kubelet/winstats/perfcounter_nodestats.go

+// many of the Windows processor affinity functions will only return the information for a
+// single Processor Group. Since a single group can only hold 64 logical processors, this
+// means when there are more they will be divided into multiple groups.
+// for the above reason, the following function is used to get the cpu number of the windows node.


Suggested change

// for the above reason, the following function is used to get the cpu number of the windows node.

// For the above reason, procGetActiveProcessorCount is used to get the cpu count for all processor groups of the windows node.

Already modified, thanks for your review.

Thanks for your contribution! if you need a reviewer from sig-windows in the future you can find us in the sig-windows slack

…ly under Windows

jsturtevant · 2021-03-12T03:28:52Z

/milestone v1.21
/priority important-soon
/lgtm

going to run our sig-windows tests for sanity:
/test pull-kubernetes-e2e-aks-engine-windows-containerd

marosset · 2021-03-12T04:40:38Z

/lgtm

marosset · 2021-03-12T04:42:19Z

I agree with the comments above about not being able to easily test this in e2e tests if it requires very large amounts of cores.

marosset · 2021-03-12T04:42:44Z

@hwdef can you add a release note since this does fix a bug other users might run into at some point?

marosset · 2021-03-12T04:44:08Z

/retest

hwdef · 2021-03-12T05:48:51Z

@marosset
Thanks for your reminder, I have added the release note.

marosset · 2021-03-12T06:03:09Z

/assign @dchen1107 @mrunalp
For pkg\kubelet approval.

mrunalp · 2021-03-16T19:09:15Z

/approve

k8s-ci-robot · 2021-03-16T19:09:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hwdef, mrunalp

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:

~~pkg/kubelet/OWNERS~~ [mrunalp]

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

singholt · 2021-08-17T19:12:15Z

Is this going to be back-ported to 1.20 and below? Or is it available just on 1.21?

hwdef · 2021-08-18T04:34:38Z

@singholt

just support on 1.21. Did not cherrypick to 1.20.

k8s-ci-robot requested review from dchen1107 and krmayankk December 18, 2020 03:57

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 18, 2020

k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Dec 18, 2020

hwdef force-pushed the master branch from eac22b3 to 22b4962 Compare December 18, 2020 05:07

k8s-ci-robot assigned liggitt, mrunalp and thockin Dec 18, 2020

ehashman added this to Needs Reviewer in SIG Node PR Triage Jan 6, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 25, 2021

marosset added this to In Review (v1.21) in SIG-Windows Feb 11, 2021

ehashman reviewed Feb 11, 2021

View reviewed changes

thockin unassigned liggitt Mar 5, 2021

jsturtevant reviewed Mar 11, 2021

View reviewed changes

mrunalp moved this from Needs Approver to Waiting on Author in SIG Node PR Triage Mar 11, 2021

kubelet: fix the bug that the number of cpu cannot be obtained normal…

ae8c4c2

…ly under Windows

hwdef force-pushed the master branch from e157ec3 to ae8c4c2 Compare March 12, 2021 02:22

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 12, 2021

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 12, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 12, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 12, 2021

k8s-ci-robot assigned marosset Mar 12, 2021

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 12, 2021

k8s-ci-robot assigned dchen1107 Mar 12, 2021

ehashman moved this from Waiting on Author to Needs Approver in SIG Node PR Triage Mar 12, 2021

mrunalp moved this from Needs Approver to Done in SIG Node PR Triage Mar 16, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2021

k8s-ci-robot merged commit c2ad998 into kubernetes:master Mar 16, 2021

SIG-Windows automation moved this from Reviewed - Needs Approval From Other SIGs to Done (v1.21) Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: Fix the bug of getting the number of windows cpu #97378

kubelet: Fix the bug of getting the number of windows cpu #97378

hwdef commented Dec 18, 2020 •

edited

hwdef commented Dec 18, 2020

hwdef commented Dec 18, 2020

jayunit100 commented Dec 19, 2020

hwdef commented Dec 19, 2020

hwdef commented Dec 22, 2020 •

edited

ehashman commented Jan 25, 2021

ehashman left a comment

ehashman Feb 11, 2021

hwdef commented Feb 18, 2021

jsturtevant Mar 11, 2021

hwdef Mar 12, 2021

jsturtevant Mar 12, 2021

jsturtevant commented Mar 12, 2021

marosset commented Mar 12, 2021

marosset commented Mar 12, 2021

marosset commented Mar 12, 2021

marosset commented Mar 12, 2021

hwdef commented Mar 12, 2021

marosset commented Mar 12, 2021

mrunalp commented Mar 16, 2021

k8s-ci-robot commented Mar 16, 2021

singholt commented Aug 17, 2021

hwdef commented Aug 18, 2021

	// for the above reason, the following function is used to get the cpu number of the windows node.
	// For the above reason, procGetActiveProcessorCount is used to get the cpu count for all processor groups of the windows node.

kubelet: Fix the bug of getting the number of windows cpu #97378

kubelet: Fix the bug of getting the number of windows cpu #97378

Conversation

hwdef commented Dec 18, 2020 • edited

hwdef commented Dec 18, 2020

hwdef commented Dec 18, 2020

jayunit100 commented Dec 19, 2020

hwdef commented Dec 19, 2020

hwdef commented Dec 22, 2020 • edited

ehashman commented Jan 25, 2021

ehashman left a comment

Choose a reason for hiding this comment

ehashman Feb 11, 2021

Choose a reason for hiding this comment

hwdef commented Feb 18, 2021

jsturtevant Mar 11, 2021

Choose a reason for hiding this comment

hwdef Mar 12, 2021

Choose a reason for hiding this comment

jsturtevant Mar 12, 2021

Choose a reason for hiding this comment

jsturtevant commented Mar 12, 2021

marosset commented Mar 12, 2021

marosset commented Mar 12, 2021

marosset commented Mar 12, 2021

marosset commented Mar 12, 2021

hwdef commented Mar 12, 2021

marosset commented Mar 12, 2021

mrunalp commented Mar 16, 2021

k8s-ci-robot commented Mar 16, 2021

singholt commented Aug 17, 2021

hwdef commented Aug 18, 2021

hwdef commented Dec 18, 2020 •

edited

hwdef commented Dec 22, 2020 •

edited