-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 pdb to coredns #10557
add pdb to coredns #10557
Conversation
Hi @lobiyedKarim1. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
Thanks to the PR. The default config should not be changed :-) And the /ok-to-test |
Does that really belong in Kubespray ? From #6400 I had the impression that one of the long-term goals was to somehow reduce the scope, and this looks like it could easily enough be in an standalone manifest applied after kubespray. |
1c45a3f
to
2156de1
Compare
29baaf8
to
2337a78
Compare
d315dfd
to
dcc719f
Compare
664002a
to
5656a52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, you're only applying your pdb to the second instance of coredns (when using coredns_dual
). It should be applied on both.
roles/kubernetes-apps/ansible/templates/coredns-poddisruptionbudget.yml.j2
Outdated
Show resolved
Hide resolved
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it's weird but it's already used in kubespray https://github.com/kubernetes-sigs/kubespray/blob/master/roles/network_plugin/cilium/tasks/install.yml#L63
Mmmh. I guess. Don't like that, but looks like there isn't a simpler way.
c85f0d8
to
bc34ba4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last thing and we're good.
…ol disruption behavior and set maximum unavailable pods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lobiyedKarim1
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lobiyedKarim1, mzaian 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 |
…ol disruption behavior and set maximum unavailable pods (kubernetes-sigs#10557)
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces changes to enhance CoreDNS. The following modifications have been made:
coredns_pod_disruption_budget
variable to enable/disable PodDisruptionBudget for CoreDNS.coredns_pod_disruption_budget
tofalse
.coredns_pod_disruption_budget_max_unavailable
variable to specify the maximum number of unavailable pods during disruptions. The default value is set to1
.coredns-poddisruptionbudget.yml.j2
to generate the corresponding PodDisruptionBudget YAML file with configurable options.These changes provide users with more control over CoreDNS's behavior during disruptions and enable fine-grained tuning of the PodDisruptionBudget settings.
Which issue(s) this PR fixes:
NONE
Special notes for your reviewer:
NONE
Does this PR introduce a user-facing change?: