-
Notifications
You must be signed in to change notification settings - Fork 294
adding in priority value in podSpec for controller, scheduler and apiserver #1631
adding in priority value in podSpec for controller, scheduler and apiserver #1631
Conversation
Welcome @erleene! |
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.
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. |
Codecov Report
@@ Coverage Diff @@
## v0.13.x #1631 +/- ##
========================================
Coverage 25.49% 25.49%
========================================
Files 98 98
Lines 5049 5049
========================================
Hits 1287 1287
Misses 3619 3619
Partials 143 143 Continue to review full report at Codecov.
|
7880864
to
fd364ab
Compare
Please test before we merge |
fd364ab
to
4581ead
Compare
… scheduler and apiserver
This looks to have been fixed for Kubernetes 1.13.5+, the default for kube-aws is now 1.13.7 so this shouldn't impact newly created clusters. Kubernetes upstream has fixed this for versions as far back as v1.11.x, do we want to do the same in kube-aws? |
Yes, that has got us confused somewhat as we observed the behavior on a 1.13.7 cluster without this work-around! It shouldn’t happen. |
The problem still occurs in 1.13.7, where no controllers are running and getting evicted. We have the api server and the scheduler up, and kubelet shows the following:
some more interesting output:
|
Does this only happen in clusters created with kube-aws, or is it a problem in upstream k8s? |
Thanks, it was also confirmed that the controller-manager pods had been evicted - this is an upstream kubernetes issue which may also affect other releases, e.g. 1.14.x, 1.15.x (although it is supposed to have been patched). It affects kube-aws customers deploying smaller controllers that are pushed near or over capacity and won't affect well sized nodes. What it has shown us though is that when resources are under pressure some of the most critical components were chosen for eviction. I suggest we merge this as a work-around on 1.13.x and test the behaviour on 1.14.x and apply similar work-around if observed on that branch. Good work Erleen! |
Cool, i'm behind that. Code looks good. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dominicgunn 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:
Approvers can indicate their approval by writing |
/lgtm |
Thanks for your first contribution! 🙏 |
We have found a bug in 1.13.x where mirror pods get evicted when the node is under pressure. This PR sets the priority directly so that those pods won't get evicted. We don't believe that this is required with 1.14+.
Relates to this issue kubernetes/kubernetes#73572