-
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
[master] Fixed default DNS min replica for single node clusters #8112
[master] Fixed default DNS min replica for single node clusters #8112
Conversation
Hi @smasset. 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. |
/sig cluster-lifecycle |
@smasset-orange: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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 for your pull request. /ok-to-test |
@@ -3,7 +3,7 @@ | |||
dns_memory_limit: 170Mi | |||
dns_cpu_requests: 100m | |||
dns_memory_requests: 70Mi | |||
dns_min_replicas: 2 | |||
dns_min_replicas: "{{ [ 2, groups['all'] | length ] | min }}" |
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.
nit: all
group contains etcd, and if dedicated etcd node exists on all 2 nodes 1 could be better for the case.
For such case, kube_node
or k8s_cluster
is better instead of all
. That is a corner case, maybe not so important.
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.
Fixed using kube_control_plane
(used by role task anyway) with latest pushed commit
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.
It is still possible to have a cluster with one controller (groups['kube_control_pane']|length = 1
) and more workers in which case there would be room enough to deploy more coredns replicas. In this case I would suggest to stick with the recomandation of using either kube_node
or k8s_cluster
as the group reference.
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.
For respecting @cristicalin comment, it is better to get more feedback before merging.
/lgtm cancel
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.
Fixed using k8s_cluster
group
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.
confirmed the point has been applied on the latest pull request.
4c609c5
to
9872850
Compare
Thanks for updating. /lgtm |
/assign @Miouge1 |
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.
@smasset Thanks for the PR
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: floryut, smasset 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 |
oops sorry @cristicalin @oomichi ; didn't see your discussion. |
9872850
to
6ede98c
Compare
Thanks for updating. /hold cancel |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Single node cluster don't need 2 coreDNS pods.
This PR compares
2
(current absolute default value) and the length of thek8s_cluster
inventory group and uses the minimum as the default value fordns_min_replicas
Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
N/A
Does this PR introduce a user-facing change?: