/ok-to-test

This was brought up before, but the anti-affinity scheduler logic is not scalable to large # of nodes at the moment which means we cannot apply the feature to kube-dns by default. When that is fixed, we can add this.

Ref the previous attempt: #52193.

Seems like the issue to track anti-affinity fast path is closed: #54189. @bsalamat does it mean we are ready to give it another shot?

Seems like the issue to track anti-affinity fast path is closed: #54189. @bsalamat does it mean we are ready to give it another shot?

@MrHohn Yes. The PR is merged and now the head has the optimization for affinity/anti-affinity whose toplogyKey is kubernetes.io/hostname. So, in the next release we should be able to use the rule with reasonable performance for kube-dns.

Awesome! Thanks @bsalamat!
@bowei @MrHohn: Any thoughts on preferredDuringScheduling vs requiredDuringScheduling? (per the conversation above)

Any thoughts on preferredDuringScheduling vs requiredDuringScheduling? (per the conversation above)

@cblecker From previous discussion I think folks agreed on using preferredDuringScheduling. Ref #52193 (comment).

I would agree with @vainu-arto that requiredDuringScheduling would be always preferable in our case as having all kube-dns pods located on the same node creates a single point of failure and kind of defeats the preventSinglePointFailure setting of the kube-dns-autoscaler.

We use autoscaling quite heavily and from time to time end up with all two kube-dns pods on the same node.

What would be the advantage of using preferredDuringScheduling, except that less kube-dns pods would be pending? Shouldn't any performance concerns be covered by the autoscaler https://github.com/kubernetes/kubernetes/blob/578881/cluster/addons/dns-horizontal-autoscaler/dns-horizontal-autoscaler.yaml#L93 and better be circumvented by scaling up earlier instead of relying on always being schedulable?

IMO, requiredDuringScheduling is more appropriate for kube-dns. Note that preferredDuringScheduling is a "priority" function in scheduler. "Priority" functions are essentially scoring functions. Each assign a score to a node for running a given pod and then the scores are aggregated to find the best node. Scheduler has some default scoring functions, such as spreading, etc., which are combined with the user requested preferences. This causes the "priority" functions to be much less reliable.
I think running multiple instances of kube-dns on a single node has very limited value anyway. So, I would say even if some instances remain pending due to anti-affinity, it shouldn't be much worse than running multiple of them on a single node.

BTW, credit for implementing the anti-affinity optimization goes to @misterikkit.

Using "required" will result in a behavior change: if you have less than # nodes < requested replicas, you will have DNS pods that cannot be scheduled. There may be reasons to scale replicas for performance as well as fault tolerance reasons. It would be more prudent to first go to preferred first and see what the user experience is than to jump to required immediately.

I agree, @bowei.

@vainu-arto Can you make this change to the PR? Thanks!

The choice of preferred vs required basically is a choice on which users to prioritize. Do you prioritize the users who do frequent autoscaling / rolling updates (and may end up with DNS stacked on a single node), or do you prioritize users that need more than one kube-dns per node for performance reasons? Is there any sense of which of these groups of users is actually more prominent? My gut tells me group 1 (autoscaling) is far more common than group 2 (performance), and in the use cases where performance is the bottleneck some fine tuning would be needed anyway.

From my perspective there's little value in merging this unless it keeps requiredDuringScheduling. This PR was opened because of an outage that likely wouldn't have been fixed by using preferredDuringScheduling. That said, I get that from a "principle of least surprise" viewpoint it's less of a change to go to preferredDuringScheduling.

How about this, a revert of the revert of the previous attempt by @StevenACoffman to add preferred anti-affinity to kube-dns pods?

I still think preferred isn't strong enough for this case, but I'll try to start here in the interest of not letting the perfect be the enemy of the good (or at least better than without any anti-affinity).

Since autoscaling is a common argument in this thread. Pod affinity/antiaffinity is absolutely horrible for Cluster Autoscaler performance and we recommend our users to never ever use it in large clusters. MatchInterPodAffinity predicate is ~100 times slower than all other predicates combined even if no pods actually use pod affinity / antiaffinity (kubernetes/autoscaler#257)*. Because of this we completely disable the predicate if there are no pods using pod affinity / antiaffinity in cluster. Even a single pod (kube-dns) using it will force us to actually check it and take an associated massive performance hit.

This is only a case for requiredDuringScheduling version of antiaffinity. CA only runs scheduler predicates and not priorities, so it doesn't care about preferredDuringScheduling. So I'm very much in favor of using preferredDuringScheduling, as we will end up telling everyone who uses autoscaling to change it to preferredDuringScheduling anyway.

We need to come up with some long term solution for pod affinity performance, but that's a separate discussion. I'll create a separate issue for that.

====

*) This was measured before performance improvements done in #54189, but I doubt they will help us all that much. The fundamental issue is that contrary to scheduler CA simulates different clusters all the time, so it cannot precompute and maintain information about antiaffinity of already running pods (more precisely - either satisfiesExistingPodsAntiAffinity method kills us, because we don't have predicateMeta, or we have to calculate predicateMeta for different simulated clusters all the time, with the same result). After a quick glance at recent changes I don't see anything that would address this problem.

/retest

/retest

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, vainu-arto

The full list of commands accepted by this bot can be found here.

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@vainu-arto: The following tests failed, say /retest to rerun them all:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Automatic merge from submit-queue (batch tested with PRs 57683, 59116, 58728, 59140, 58976). If you want to cherry-pick this change to another branch, please follow the instructions here.

Sadly, this struck our large-cluster scalability again - #54164 (comment)