fix wrong NumCPU in kube-proxy under static CPU policy #99613

xh4n3 · 2021-03-02T02:48:21Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fixes wrong NumCPU used in conntrack-max calculation in kube-proxy on nodes with static CPU policy turned on.

Which issue(s) this PR fixes:

Special notes for your reviewer:

kube-proxy will try look up CPU core number in Node resource info to calculate conntrack-max, only if failed, it will then lookup through golang's runtime.NumCPU().

Does this PR introduce a user-facing change?

Fixed a bug that causes smaller number of conntrack-max being used under CPU static policy. (#99225, @xh4n3)

k8s-ci-robot · 2021-03-02T02:48:29Z

Welcome @xh4n3!

It looks like this is your first PR to kubernetes/kubernetes 🎉. 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/kubernetes 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 · 2021-03-02T02:48:30Z

@xh4n3: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2021-03-02T02:48:30Z

Hi @xh4n3. Thanks for your PR.

I'm waiting for a kubernetes 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.

aoxn · 2021-03-02T02:56:58Z

/ok-to-test

xh4n3 · 2021-03-02T04:29:54Z

/test pull-kubernetes-e2e-kind-ipv6

k8s-ci-robot · 2021-03-05T03:02:36Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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

Signed-off-by: xh4n3 <xyn1016@gmail.com>

thockin · 2021-03-05T07:08:39Z

cmd/kube-proxy/app/server_windows.go

@@ -185,6 +186,10 @@ func getProxyMode(proxyMode string, kcompat winkernel.KernelCompatTester) string
 	return proxyModeUserspace
 }

+func detectNumCPU() int {
+	return goruntime.NumCPU()


Is this OK? Does it need followup?

Should I put a comment in this line or skip calling getConntrackMax() by judging runtime.GOOS at

kubernetes/cmd/kube-proxy/app/server.go

Lines 671 to 677 in d6bc98f

// Tune conntrack, if requested

// Conntracker is always nil for windows

if s.Conntracker != nil {

max, err := getConntrackMax(s.ConntrackConfiguration)

if err != nil {

return err

}

In fact no windows will come to this point.

thockin · 2021-03-05T07:12:48Z

Thanks!

/lgtm
/approve
/retest

k8s-ci-robot · 2021-03-05T07:12:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin, xh4n3

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:

~~cmd/kube-proxy/OWNERS~~ [thockin]

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

xh4n3 · 2021-03-05T09:24:30Z

/retest

xh4n3 · 2021-03-05T11:23:59Z

/retest

fejta-bot · 2021-03-05T14:46:28Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-03-05T19:19:13Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

thockin · 2021-03-05T20:27:50Z

/retest

xh4n3 · 2021-03-08T02:02:17Z

@thockin can I have this commit backport to 1.20 or earlier version?

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Mar 2, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 2, 2021

k8s-ci-robot requested review from MrHohn and thockin March 2, 2021 02:49

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 2, 2021

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 Mar 2, 2021

xh4n3 changed the title ~~fix wrong NumCPU in kube-proxy under static CPU policy, fixes #99225~~ fix wrong NumCPU in kube-proxy under static CPU policy Mar 2, 2021

xh4n3 force-pushed the master branch from 8fcc2c1 to 40d84c4 Compare March 2, 2021 03:00

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Mar 2, 2021

xh4n3 force-pushed the master branch from 40d84c4 to 3a8b098 Compare March 2, 2021 03:37

xh4n3 force-pushed the master branch from 3a8b098 to f87e8fb Compare March 5, 2021 03:02

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 5, 2021

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 5, 2021

xh4n3 force-pushed the master branch from f87e8fb to 15b6a6f Compare March 5, 2021 03:03

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 Mar 5, 2021

xh4n3 force-pushed the master branch from 15b6a6f to ad01997 Compare March 5, 2021 03:47

fix wrong NumCPU in kube-proxy under static CPU policy

d6bc98f

Signed-off-by: xh4n3 <xyn1016@gmail.com>

xh4n3 force-pushed the master branch from ad01997 to d6bc98f Compare March 5, 2021 04:44

thockin reviewed Mar 5, 2021

View reviewed changes

k8s-ci-robot assigned thockin Mar 5, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 5, 2021

k8s-ci-robot merged commit 0849d1b into kubernetes:master Mar 6, 2021

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

xh4n3 mentioned this pull request Mar 10, 2021

Automated cherry pick of #99613: fix wrong NumCPU in kube-proxy under static CPU policy #100079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix wrong NumCPU in kube-proxy under static CPU policy #99613

fix wrong NumCPU in kube-proxy under static CPU policy #99613

xh4n3 commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

aoxn commented Mar 2, 2021

xh4n3 commented Mar 2, 2021

k8s-ci-robot commented Mar 5, 2021

thockin Mar 5, 2021

xh4n3 Mar 5, 2021

thockin commented Mar 5, 2021

k8s-ci-robot commented Mar 5, 2021

xh4n3 commented Mar 5, 2021

xh4n3 commented Mar 5, 2021

fejta-bot commented Mar 5, 2021

fejta-bot commented Mar 5, 2021

thockin commented Mar 5, 2021

xh4n3 commented Mar 8, 2021

	// Tune conntrack, if requested
	// Conntracker is always nil for windows
	if s.Conntracker != nil {
	max, err := getConntrackMax(s.ConntrackConfiguration)
	if err != nil {
	return err
	}

fix wrong NumCPU in kube-proxy under static CPU policy #99613

fix wrong NumCPU in kube-proxy under static CPU policy #99613

Conversation

xh4n3 commented Mar 2, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

aoxn commented Mar 2, 2021

xh4n3 commented Mar 2, 2021

k8s-ci-robot commented Mar 5, 2021

thockin Mar 5, 2021

Choose a reason for hiding this comment

xh4n3 Mar 5, 2021

Choose a reason for hiding this comment

thockin commented Mar 5, 2021

k8s-ci-robot commented Mar 5, 2021

xh4n3 commented Mar 5, 2021

xh4n3 commented Mar 5, 2021

fejta-bot commented Mar 5, 2021

fejta-bot commented Mar 5, 2021

thockin commented Mar 5, 2021

xh4n3 commented Mar 8, 2021