-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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 ability to provide custom CoreDNS tolerations and affinity #12234
Add ability to provide custom CoreDNS tolerations and affinity #12234
Conversation
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. |
Welcome @hierynomus! |
Hi @hierynomus. 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 Once the patch is verified, the new status will be reflected by the 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. |
CLA has been signed. |
@mikesplain @olemarkus Is there something more needed for this PR? Or you just haven't gotten around to it yet? |
Hi, sorry it is on my list. Have you tested the PR locally? I'd be surprised if this actually works. Further, offering this kind of customization on a core addon is unlikely to be added. We'd at least have to understand the usecase. Kops will not really work on a cluster where there aren't worker nodes workloads cannot be scheduled on by default. |
Hi Ole, No problem! The usecase for us is that our cluster is configured in such a way that by default we want everything in our k8s clusters to run on AWS spot nodes. But CoreDNS needs to be scheduled on the master and on-demand nodes only. With this change the default behavior is unchanged, but it is now allowed to specify custom tolerations and affinity such as this (taken from our kops cluster definition):
|
@olemarkus Is the usecase that I described clear? |
So this is essentially to avoid coredns from running on spot instances? CoreDNS should work fine on spot instances. Even more so if the NTH addon is enabled. I would specifically avoid running anything on master nodes that absolutely does not have to run there. |
I think it's more generic, that in the case of setting up a kubernetes cluster with multiple instance-groups, that you would want to be able to control where certain pods land. We see the same at our customers who want to target specific workloads to specific instance-groups. |
Brought this up in office hours today and we decided this feature looks good. /ok-to-test |
/retest |
@olemarkus What is still needed to proceed with this PR? I see that there are a few CI jobs which require an approval before they're run ( Other than that, I think it needs a code-review/approval? |
Thanks a lot! That would've indeed cost me considerably more time. I've added your commit and squashed them all into one. |
/retest |
You need to run |
That's indeed what I tried locally before doing the |
@olemarkus If I just urn |
6a4dee1
to
416ffde
Compare
Not sure if you noticed, but
|
Missed that in the build logs, weird thing is that doesn't happen locally. Does that behave differently when run on the CI env? |
It should happen to you locally as well. Do you see this command exiting successfully? |
4e37f0b
to
56ac813
Compare
Ok, after merging in master, I had the same error locally. The template now uses a newly added |
Signed-off-by: Jeroen van Erp <jeroen@hierynomus.com>
56ac813
to
c30ec8e
Compare
@olemarkus Any idea about why that test fails? Is it something I changed, or is it flaky somehow? |
Looks like a flake /retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus 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 |
Thanks @olemarkus! Two tests now failing on that cni-kuberouter test, looks like flakes indeed. /retest |
/test pull-kops-e2e-cni-kuberouter |
Thanks for the guidance @olemarkus! |
…234-origin-release-1.22 Automated cherry pick of #12234: Add ability to provide custom CoreDNS Tolerations and
No description provided.