-
Notifications
You must be signed in to change notification settings - Fork 77
Add support for broker demotion #191
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
base: main
Are you sure you want to change the base?
Conversation
bbb1dc1 to
7fc8acd
Compare
Signed-off-by: Kyle Liberti <[email protected]>
7fc8acd to
abb52f1
Compare
see-quick
left a comment
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 for the proposal, I think it looks good, just a few nits/questions...
Out of curiosity, I didn't find it in the proposal but what happens when demotion is in progress but target broker fails in the middle of operation?
Signed-off-by: Kyle Liberti <[email protected]>
|
Thanks for the review @see-quick
I just added this note to the Validation and constraints section of the proposal: "If a target broker fails while leadership is being transferred to it, all demotion operations involving that broker are aborted, and the source brokers remain the leaders for the affected partitions. |
d308e3a to
07cf465
Compare
Signed-off-by: Kyle Liberti <[email protected]>
7d66426 to
f2f77b9
Compare
fvaleri
left a comment
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 @kyguy. I left some comments.
I think it is also important to document that broker demotion does NOT prevent new partition leaders to be scheduled on the selected broker.
tinaselenge
left a comment
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 for the proposal. Overall, it looks good to me. I left a few comments to clarify.
128-broker-demotion-support.md
Outdated
| | brokers | integer array | List of ids of broker to be demoted in the cluster. | | ||
| | concurrentLeaderMovements | integer | Upper bound of ongoing leadership swaps. Default is 1000. | | ||
| | skipUrpDemotion | boolean | Whether to skip demoting leader replicas for under-replicated partitions. | | ||
| | excludeFollowerDemotion | boolean | Whether to skip demoting follower replicas on the broker to be demoted. | |
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.
what does demoting follower replicas mean?
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 moves the ids of the demoted brokers to the end of the replica lists.
128-broker-demotion-support.md
Outdated
| * When an impossible demotion operation is requested for example demoting all brokers or transferring leadership from the only in-sync replica when the KafkaRebalance `spec.skipUrpDemotion` configuration is set to `false`, the demotion request will be rejected and the error will be reported in the `KafkaRebalance` status. | ||
|
|
||
| * If a target broker fails while leadership is being transferred to it, all demotion operations involving that broker are aborted, and the source brokers remain the leaders for the affected partitions. | ||
| In this case, the overall demotion request continues on a best-effort basis with with the remaining proposed operations, transferring the leadership on brokers that are available. |
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.
Does the request completes successfully when only some of the leadership were transferred?
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
| # Broker demotion support via KafkaRebalance resource | ||
|
|
||
| This proposal extends the `KafkaRebalance` custom resource to support broker demotion by integrating with Cruise Control's `/demote_broker` endpoint. | ||
| This would allow users to demote brokers, removing them from partition leadership eligibility, in preparation for maintenance, decommissioning, or other operational needs. |
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.
How does this work? I get it that it removes them from partition leadership. But how does it ensure they are not eligible to become leaders again?
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.
Cruise Control has the broker ids marked as ineligible in memory and moves the broker ids to the end of the replica lists, then triggers a leadership election. Since leadership election prioritizes choosing the first broker id of the replica list (the preferred leader) as the leader, the demoted brokers are less likely to be elected as leaders.
But how does it ensure they are not eligible to become leaders again?
If a rebalance or demotion request is made the broker ids marked as ineligible in memory are excluded from having partitions moved to them or being listed first in the replica lists.
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.
What happens when CC pod is restarted (upgrade, migration to different node, etc).? User will have to trigger "rebalance" proposal and approval to refresh in-memory data?
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.
At this time yes, to refresh the in-memory demotion data after Cruise Control pod restart, a user would have to trigger another KafkaRebalance demotion request.
| **NOTE**: As part of this proposal, we will also add a `excludeRecentlyDemotedBrokers` field for the `full`, `add-brokers`, and `remove-brokers` KafkaRebalance modes to give users the ability to prevent to leader replicas to be moved to recently demoted brokers. | ||
| When `excludeRecentlyDemotedBrokers` is set to `true`, a broker is considered demoted for the duration specified by the Cruise Control `demotion.history.retention.time.ms` server configuration. | ||
| By default, this value is 1209600000 milleseconds (14 days) but is configurable in the `spec.cruiseControl.config` section of the `Kafka` custom resource. |
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.
How does this work? Where is the information about the demotion work?
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.
Cruise Control stores the demotion information in memory (yes this is far from ideal). Cruise Control will make sure the ids of the demoted brokers are in the end of the replica lists and will exclude the brokers from being considered for partition movement when generating optimization proposals.
|
|
||
| The implementation includes the following validation: | ||
|
|
||
| * When `demote-brokers` mode is specified, the `brokers` field must be provided. |
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.
Will it be validated only in the broker code or also with CEL?
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.
Was originally planning on validating the field in the operator code but this may change depending on the proposal surrounding the API changes discussed #191 (comment).
I'll link the separate proposal here when I have a draft ready and we can pick this up once that proposal and its implementation is sorted.
Signed-off-by: Kyle Liberti <[email protected]>
Signed-off-by: Kyle Liberti <[email protected]>
|
Thanks everyone for the reviews! I am putting this proposal on hold temporarily as we address some of the API concerns raised in the thread here: #191 (comment). We need devise a better path forward for the parameters that are used in conjunction with I am going to draft a separate proposal for those API changes and link it here. Once that proposal and its implementation is complete, we can continue this broker demotion proposal with a cleaner, more maintainable foundation. |
Signed-off-by: Kyle Liberti <[email protected]>
|
|
||
| Broker demotion is independent of other rebalance modes but can be used before or after them manually: | ||
|
|
||
| * **add-brokers**: After new brokers are added to the cluster, broker demotion could be used to explicitly transfer partition leadership away from existing brokers to accelerate leadership adoption on newly added brokers. |
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 am not sure about this use case, isn't the auto-rebalancing (we support) already doing something like this? It's moving partitions but not demoting brokers.
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.
From what I understand, auto-rebalancing and the add-brokers modes will move partition replicas onto the added brokers but not necessarily transfer partition leadership.
The use case I have in mind is one where a user wants to gradually transfer traffic to a new set of brokers and eventually remove the old brokers but is not ready to decommission them yet. The user can first rebalance the existing load onto the new brokers using add-brokers mode and then demote the old brokers using demote-brokers mode to shift the bulk of the traffic (the leadership) to the new brokers while still relying on the old brokers to host follower replicas.
Does that make sense?
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 It does, thanks.
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 It does, thanks.
Related problem
When preparing to remove or decommission brokers from a Kafka cluster there is currently no built-in way to ensure those brokers are first demoted (no longer eligible to act as partition leaders). This can result in partitions becoming temporarily unavailable or degraded when those brokers are taken offline, especially in clusters with uneven leadership distribution. Having a mechanism to programmatically demote brokers would allow for safer, more predictable operations during maintenance or scaling events.
Suggested solution
This feature would extend the KafkaRebalance resource to leverage Cruise Control’s
/kafkacruisecontrol/demote_brokerendpoint [1], allowing users to specify a list of brokers to be demoted. This would trigger the migration of partition leadership away from those brokers in preparation for decommissioning or maintenance.Addresses issue discussed here [2]
[1] https://github.com/linkedin/cruise-control/wiki/REST-APIs#demote-a-list-of-brokers-from-the-kafka-cluster
[2] strimzi/strimzi-kafka-operator#11907