-
Notifications
You must be signed in to change notification settings - Fork 117
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
Topology spread constraints on zones and anti-affinity for receivers and dispatchers #2092
Topology spread constraints on zones and anti-affinity for receivers and dispatchers #2092
Conversation
…tchers Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Codecov Report
@@ Coverage Diff @@
## main #2092 +/- ##
=============================================
- Coverage 82.69% 66.60% -16.10%
Complexity 676 676
=============================================
Files 72 141 +69
Lines 2289 9033 +6744
Branches 195 195
=============================================
+ Hits 1893 6016 +4123
- Misses 291 2623 +2332
- Partials 105 394 +289
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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!!
One more file needs similar changes...sourcev2
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Done |
# To avoid node becoming SPOF, spread our replicas to different nodes and zones. | ||
topologySpreadConstraints: | ||
- maxSkew: 2 | ||
topologyKey: topology.kubernetes.io/zone | ||
whenUnsatisfiable: ScheduleAnyway | ||
labelSelector: | ||
matchLabels: | ||
app: kafka-broker-dispatcher | ||
affinity: | ||
podAntiAffinity: | ||
preferredDuringSchedulingIgnoredDuringExecution: | ||
- podAffinityTerm: | ||
labelSelector: | ||
matchLabels: | ||
app: kafka-broker-dispatcher | ||
topologyKey: kubernetes.io/hostname | ||
weight: 100 |
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.
Do you know if this combination is functionally better than having two pod topology spread constraints - one for zone and one for node? (Was trying to read about this...)
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.
Since we don't have any specific anti-affinity rules for nodes, we could instead have multiple pod topology spread constraints?
# To avoid node becoming SPOF, spread our replicas to different nodes and zones.
topologySpreadConstraints:
- maxSkew: 2
topologyKey: topology.kubernetes.io/zone
whenUnsatisfiable: ScheduleAnyway
labelSelector:
matchLabels:
app: kafka-broker-dispatcher
- maxSkew: 2
topologyKey: kubernetes.io/hostname
whenUnsatisfiable: ScheduleAnyway
labelSelector:
matchLabels:
app: kafka-broker-dispatcher
This should also satisfy the existing anti-affinity rule for preferredDuringSchedulingIgnoredDuringExecution
.
On the other hand, our existing dispatchers already had podAntiAffinity
for nodes, so if we want to make minimal changes, that is understandable
cc: @pierDipi was this your thinking too?
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.
Yes, afaik, topologySpreadConstraints
+ maxSkew = 1
+ ScheduleAnyway
should be equivalent to our existing antiAffinity rule, I'm ok to migrate antiAffinity rules to use
topologySpreadConstraints
+ maxSkew = 1
+ ScheduleAnyway
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.
Ok, i think making this equivalent change separately another time/another PR is fine too.
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aavarghese, pierDipi 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 |
…and dispatchers (knative-extensions#2092) * Topology spread constraints and anti-affinity for receivers and dispatchers Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Include sourcev2 dispatcher Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
…and dispatchers (knative-extensions#2092) * Topology spread constraints and anti-affinity for receivers and dispatchers Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Include sourcev2 dispatcher Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
To spread receivers and dispatchers across zones for HA.
Signed-off-by: Pierangelo Di Pilato pierdipi@redhat.com
Part of #1537 and Broker HA