Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add priority flag to kube-proxy on Windows hosts #96935

Closed
ravisantoshgudimetla opened this issue Nov 29, 2020 · 23 comments
Closed

Add priority flag to kube-proxy on Windows hosts #96935

ravisantoshgudimetla opened this issue Nov 29, 2020 · 23 comments
Assignees
Labels
area/kube-proxy good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/windows Categorizes an issue or PR as relevant to SIG Windows. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@ravisantoshgudimetla
Copy link
Contributor

ravisantoshgudimetla commented Nov 29, 2020

What would you like to be added:

Add priority flag to kube-proxy so that end-user can specify priority

Why is this needed:

On Windows hosts, there is a chance that the kube-proxy process won't get enough CPU cycles because of the default priority set for the Windows process.

xref: #95735 (comment)

Additional context:

xref: #95735

cc @marosset @jsturtevant @JocelynBerrendonner @aravindhp

Let me know if you guys are fine with it. I can create a PR for the same.

@ravisantoshgudimetla ravisantoshgudimetla added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 29, 2020
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 29, 2020
@ravisantoshgudimetla
Copy link
Contributor Author

/sig network
/sig windows

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. sig/windows Categorizes an issue or PR as relevant to SIG Windows. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 29, 2020
@JocelynBerrendonner
Copy link

@sbangari, FYI

@thockin
Copy link
Member

thockin commented Dec 4, 2020

Why is this in kube-proxy and not whatever starts kube-proxy? I'd prefer NOT to add flags if we can avoid it...

@jayunit100
Copy link
Member

/assign

@jayunit100
Copy link
Member

jayunit100 commented Dec 6, 2020

re flag vs config -- The important thing is that we bump the priority, luckily its easy enough to do, via the golang windows library in the PR... , bc kube-proxy running at low-priority on windows causes alot of problems.... wether done by a flag or not is probably less of a big deal, right @ravisantoshgudimetla

Is there a reasonable default we can use, if its <= Normal priority? that might satisfy Tims suggestion

@marosset
Copy link
Contributor

marosset commented Dec 7, 2020

@thockin There are a few different ways to start processes with non-normal priority but all have some drawbacks (mostly due to how priority can only be set on running processes in Windows)

  • start { /abovenormal | /high | /realtime } creates the process in a new shell Windows and makes it hard to capture / redirect stdout/stderr
  • Using powershell to start the process and then set the priority has similar issues but not all environments/nodes may have powershell installed
  • PsExec by SysInternals requires introducing a new tool and accepting a EULA
  • Setting scheduled tasks to find specific processes and raise their priority is finicky

I think that adding an extra flag would be the solution that is the most re-usable across all of the different solutions already out there for setting up the nodes.

@jayunit100 / @ravisantoshgudimetla I'm asking some folks in the Windows container platform team if they think using AboveNormal or High would be the best priority class to recommend.

@marosset
Copy link
Contributor

marosset commented Dec 7, 2020

/milestone v1.21
/triage accepted
/priority important-soon

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Dec 7, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Dec 7, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 7, 2020
@jayunit100
Copy link
Member

jayunit100 commented Dec 7, 2020

  • @marosset impl detail, can we do this via the kube-proxy config map api ?
  • spoke with @ravisantoshgudimetla and i think hes ok with this as well.
  • @alejandrox1 and i can help on that front if minor refactors needed since we're digging cleaning up the no-op situation around kube proxy config flags anyways (see KEP below)

TLDR: kube-proxy flags, specifically, are not used at all anymore ... theyre in a zombie state and we're working to remove them, (with exception of master-override, config, and maybe a couple other inherited ones)

See kubernetes/enhancements#2158 if interested in the specifics, but some history -- it looks like the flags in kube-proxy are largely "appendages of the past"

@marosset
Copy link
Contributor

marosset commented Dec 7, 2020

@marosset impl detail, can we do this via the kube-proxy config map api ?

Setting this via the config map API would be fine - I think the important detail is to have the kube-proxy process set its own process priority. I don't have a preference on how kube-proxy is instructed to do so.

@ravisantoshgudimetla
Copy link
Contributor Author

@marosset - Thank you for elaborating on why the kube-proxy process has to set it owns priority. I just want to add one more point related to having external entity set the priority, as community we need to give specification on how to set priority of the kube-proxy and kubelet if we're relying on external entities to do so.

@jayunit100 / @ravisantoshgudimetla I'm asking some folks in the Windows container platform team if they think using AboveNormal or High would be the best priority class to recommend.

I believe you're referring to recommending in the docs.

spoke with @ravisantoshgudimetla and i think hes ok with this as well.

Setting this via the config map API would be fine - I think the important detail is to have the kube-proxy process set its own process priority. I don't have a preference on how kube-proxy is instructed to do so.

+1

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 8, 2021
@desponda
Copy link

desponda commented Mar 8, 2021

Hi 👋 Bug Triage here, code freeze is tomorrow! This issue hasn't been updated in a while and has been marked for 1.21 milestone, is there a plan to continue working on this @immuzz

@desponda
Copy link

/milestone clear
@immuzz will this be targeted for a future milestone?

@k8s-ci-robot k8s-ci-robot removed this from the v1.21 milestone Mar 10, 2021
@danwinship
Copy link
Contributor

/area kube-proxy

@LappleApple
Copy link

/help
/good-first-issue

@k8s-ci-robot
Copy link
Contributor

@LappleApple:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/help
/good-first-issue

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 k8s-ci-robot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Apr 18, 2021
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 18, 2021
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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.

@aravindhp
Copy link
Contributor

/reopen

This is still required.

@k8s-ci-robot
Copy link
Contributor

@aravindhp: Reopened this issue.

In response to this:

/reopen

This is still required.

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 k8s-ci-robot reopened this Jun 17, 2021
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kube-proxy good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/windows Categorizes an issue or PR as relevant to SIG Windows. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.