Stop and retain a single Kafka broker - #230
Conversation
Signed-off-by: Haifeng Chen <haifengc@twitter.com>
- Enforce "at least one broker running"; the bootstrap/Admin pipeline steps (serviceEndpointsReady, clusterId, metadataVersion, nodeUnregistration) wedge if the last running broker is stopped, independent of runningNodes() - Correct the roller rationale: restartIfNecessary no-ops on a missing pod; the real hazard is the async-deletion race vs inline rollingUpdate() - Controller refusal is a new parse-time gate (KafkaPool.fromCrd), not "no change" - Forbid scale-down that would remove a currently-stopped node (049/108/PVC) - PDB must size from runningNodes(); gate the ISR scan and reuse 049's scan - Precise listener-status claim (nodePort only); note status.replicas skew - Call out combined broker+controller exclusion prominently Signed-off-by: Haifeng Chen <haifengc@twitter.com>
|
What does this block in the description mean? You never know what ID of the proposal you will have - that depends when it will be approved and merged. So you don't need to add any number based on your other PRs, it will be anyway something that might change. Also, as I mentioned in the previous proposal, please have one sentence per line. You are mentioning scope |
Thanks @im-konge |
scholzj
left a comment
scholzj
left a comment
There was a problem hiding this comment.
Thanks for the proposal. I think my comments are actually pretty similar to #229. I think the idea makes sense. But to be honest, the implementation seems to be a bit overcomplicated. That makes it hard to accept and maintain.
I was thinking about the following:
- The #229 proposes a mechanism for skipping a broker from rolling updates. I suggested to use an KafkaNodePool or Kafka annotation for it. It would make sure the pod is skipped by the roller and by the readiness checks.
- One of the comments I raised there was about how the skipped Pod would be updated (its Pod configuration, Kafka configuration, services, ...) and whether it would be recreated when shutdown. Let's assume based on this proposal that in #229 the Pod will be kept up to date in StrimziPodSet and recreated when deleted, but just not rolled.
So why don't we add here a second annotation to stop the Pod from being respawned? When you set such annotation, StrimziPodSetController would never recreate the Pod. That means that you can stop it and it would not be started.
And you can combine it with the annotation from #229 to handle the rest:
- Make sure the KafkaRoller ignores this Pod
- Make sure the readiness checks skip the Pod
- Etc.
That sounds like something that would match the use-case and be pretty straight forward to implement and maintain.
| - **Data is retained, not lost.** PVCs are kept; on start the broker recovers its logs and rejoins the ISR. This is the key difference from scale-down. | ||
| - **Scale-down of a pool with a stopped member is forbidden.** `stoppedNodes` governs whether a pod runs, *not* pool membership — so the two must not conflict. If a user lowers `replicas` (or uses `remove-node-ids`) such that a stopped node's ID would leave the desired set, that ID enters `removedNodes()` and is treated as a real removal: the 049 check, 108 unregistration, and PVC deletion all fire, destroying exactly what "retain" promised. v1 refuses a scale-down that would remove a currently-stopped node (warning + `.status`); the operator must start the node first, then scale down. (The existing 049 replica-holding check already blocks the common case, but membership removal of a stopped node must be refused explicitly, not left to 049.) | ||
| - **Graceful shutdown.** Stopping deletes the pod via the normal background propagation, so the broker receives SIGTERM (forwarded by `tini`) and runs a controlled shutdown (leadership handoff, fence, flush). For partition-heavy brokers the default `terminationGracePeriodSeconds` (30s) may be too short for a clean handoff; operators should set `template.pod.terminationGracePeriodSeconds` accordingly (a longer maintenance grace may be applied on the stop path). | ||
| - **PodDisruptionBudget interaction.** A stopped broker has no pod, so it counts against the pool/cluster PDB's available replicas: with `maxUnavailable: 1` a single stop consumes the whole budget and blocks *voluntary* disruptions (host drains, manual rolls) of other brokers — directly relevant since the host-maintenance flow drains nodes. The PDB is therefore recomputed excluding stopped nodes, so the budget reflects only running brokers. |
There was a problem hiding this comment.
Strictly speaking, the PDB is more or less virtual because the expectation is that you use Drain Cleaner that delegates rolling updates to the Cluster Operator that rolls things as part of the manual rolling update according to the regular availability checks. So I'm not sure how much the PDB matters here.
I'm also not really sure what you suggest. I do not think we want to have some special PDB logic based on the stopped and retained nodes. At the end, the PDBs are fully configurable (although as I said, their role is normally smaller than people might be used to)
There was a problem hiding this comment.
Understood — I hadn't considered that the PDB is mostly nominal once Drain Cleaner delegates the actual rolling decisions to the operator. The PDB recomputation is dropped entirely in the rewrite; PDBs stay exactly as they are today.
|
|
||
| Stopping a broker takes its replicas offline (they are retained on disk but not served), so partitions it leads/follows lose a replica until it returns. | ||
|
|
||
| - **Availability check — stance and cost (open question for review).** If stopping a broker would push any partition below `min.insync.replicas`, the operator raises a loud warning event and a `.status` condition naming the affected partitions; partitions whose only in-sync replica is on the stopped node become unavailable for `acks=all` writes (surfaced via metrics). The proposal's default is **warn-and-allow** (honor the explicit operator action), deliberately laxer than [049](https://github.com/strimzi/proposals/blob/main/049-prevent-broker-scale-down-if-it-contains-partition-replicas.md)'s refuse-by-default — because the impacts differ in kind: scale-down **permanently removes** the broker and risks data loss, whereas a stop **retains data** and the unavailability is temporary and self-healing on restart. Two caveats reviewers should weigh: (1) given the maintenance motivation, **refuse-by-default with an explicit `force` override** may be the safer v1 stance — this is genuinely a judgement call, noted as the alternative. (2) The per-partition ISR check has **no existing home** in a stop path; the only comparable machinery is 049's conditional `describeTopics` scan in `KafkaClusterCreator`. Running a full ISR scan **every reconcile** is Admin-heavy and costly on large clusters, so it must be gated (only when `stoppedNodes` is non-empty) and ideally reuse 049's scan rather than add a second one. The `..._sole_isr_on_stopped_node` / `..._under_min_isr` metrics are produced from that same scan, not an independent one. |
There was a problem hiding this comment.
TBH, I do not follow what is this trying to suggest. Are you suggesting that the operator should check the availability of partition replicas and report issues as warnings during every reconciliation? For me personally, this might be reasonable. But I'm not sure everyone would agree (+ there are also other tools how to monitor that with metrics etc.). But in any case, that sounds more like a separate proposal to me.
There was a problem hiding this comment.
Agreed — the per-reconcile ISR scan and the partition-level metrics are gone. The rewrite just states the consequence plainly (a stopped broker's partitions lose a replica; below min-ISR, acks=all producers fail until it returns) and treats the stop as an explicit administrative action, without the operator trying to measure or prevent the impact. If cluster-wide availability reporting is worth having, it deserves its own proposal, as you say.
|
|
||
| - **Availability check — stance and cost (open question for review).** If stopping a broker would push any partition below `min.insync.replicas`, the operator raises a loud warning event and a `.status` condition naming the affected partitions; partitions whose only in-sync replica is on the stopped node become unavailable for `acks=all` writes (surfaced via metrics). The proposal's default is **warn-and-allow** (honor the explicit operator action), deliberately laxer than [049](https://github.com/strimzi/proposals/blob/main/049-prevent-broker-scale-down-if-it-contains-partition-replicas.md)'s refuse-by-default — because the impacts differ in kind: scale-down **permanently removes** the broker and risks data loss, whereas a stop **retains data** and the unavailability is temporary and self-healing on restart. Two caveats reviewers should weigh: (1) given the maintenance motivation, **refuse-by-default with an explicit `force` override** may be the safer v1 stance — this is genuinely a judgement call, noted as the alternative. (2) The per-partition ISR check has **no existing home** in a stop path; the only comparable machinery is 049's conditional `describeTopics` scan in `KafkaClusterCreator`. Running a full ISR scan **every reconcile** is Admin-heavy and costly on large clusters, so it must be gated (only when `stoppedNodes` is non-empty) and ideally reuse 049's scan rather than add a second one. The `..._sole_isr_on_stopped_node` / `..._under_min_isr` metrics are produced from that same scan, not an independent one. | ||
| - **Data is retained, not lost.** PVCs are kept; on start the broker recovers its logs and rejoins the ISR. This is the key difference from scale-down. | ||
| - **Scale-down of a pool with a stopped member is forbidden.** `stoppedNodes` governs whether a pod runs, *not* pool membership — so the two must not conflict. If a user lowers `replicas` (or uses `remove-node-ids`) such that a stopped node's ID would leave the desired set, that ID enters `removedNodes()` and is treated as a real removal: the 049 check, 108 unregistration, and PVC deletion all fire, destroying exactly what "retain" promised. v1 refuses a scale-down that would remove a currently-stopped node (warning + `.status`); the operator must start the node first, then scale down. (The existing 049 replica-holding check already blocks the common case, but membership removal of a stopped node must be refused explicitly, not left to 049.) |
There was a problem hiding this comment.
This sounds like unnecessary complication to me:
- You control things, so you can just decide to not scale down (or for example disable HPAs if you have auto-scaling)
- Even if you scale down, it is pretty likely that the scaledown would not succeed because the data cannot be moved from the broker being scaled down?
- If the scale down can actually safely happen, why not scale down?
Dropping this would make this change easier to implement and maintain and more likely to be approved. So I think you should carefully consider if it is really needed.
There was a problem hiding this comment.
You're right, and your second point settles it: a stopped broker cannot move its replicas away, so the existing scale-down check blocks the removal anyway — the dedicated refusal logic was redundant. Dropped; the rewrite states that no new scale-down logic is needed and links the existing check.
|
|
||
| ### Feature gate | ||
|
|
||
| A `StopKafkaNode` feature gate, default **disabled** in the introducing release, graduating alpha → beta → GA. The CRD always accepts `spec.stoppedNodes` (Kubernetes admission does not know about feature gates), but when the gate is disabled the operator ignores it entirely — no pod omission and no `StopRefused` conditions (so users do not see refusals for entries that would be valid). The gate is warranted because this lets a human take a broker offline indefinitely — a new, availability-relevant behavior. |
There was a problem hiding this comment.
Similarly to the other proposal. It is not clear to me why the feature gate is needed if this feature requires some special steps to activate anyway.
Should there be a feature gate, it would be food to suggest the graduation schedule at least theoretically ... e.g. 3 releases in alpha, 2 in beta, GA afterwards etc.
There was a problem hiding this comment.
Agreed — the feature is opt-in by annotation and an absent annotation leaves behavior exactly as today, so a gate adds ceremony without protection. The rewrite drops it and records the reasoning under rejected alternatives.
|
|
||
| ### Status and metrics | ||
|
|
||
| - A `Kafka` CR `.status` condition (`type: NodeStopped`) listing stopped node IDs and, per node, the time it was stopped ("stopped since `<timestamp>`" via `lastTransitionTime`). Mirror per-node detail in the owning `KafkaNodePool.status`; a node that resolves to a controller shows `StopRefused` instead. This condition is informational and is **not** folded into the `Ready` condition. |
There was a problem hiding this comment.
I think the condition is not unreasonable. But the timestamps might not make sense. Especially since in most situations when this would make sense it would not be the operator stopping the brokers.
There was a problem hiding this comment.
Good point — in the new design the operator only observes that the pod is gone and keeps it gone, so a "stopped since" timestamp owned by the operator would be misleading. The condition in the rewrite just lists the stopped node IDs (plus any ignored entries), with no timestamps.
Frawless
left a comment
Frawless
left a comment
There was a problem hiding this comment.
Not a SME, but having the logic simple with annotation sounds to me like much a better idea than changing CRs API.
And as for previous proposal - making it less verbose will be much better to make it more readable for everyone which will also make the reviews easier and quickier.
|
|
||
| ## Motivation | ||
|
|
||
| The driving use case is **planned host maintenance** where the same broker must return with its data intact: take the broker offline so the underlying Kubernetes host (or its disks) can be serviced, then bring the identical broker back. Because the broker's storage and identity are retained, on restart it rejoins, runs log recovery, and catches up — no partition reassignment, no re-replication of the full dataset, no node ID churn. |
There was a problem hiding this comment.
Isn't usage of DrainClainer suitable solution for that? Move the broker to different node gracefully?
There was a problem hiding this comment.
For networked storage, yes — draining moves the pod and the volume follows, and the rewrite says so explicitly. As you spotted in the next line, the feature is for the cases where moving is impossible: local or host-attached storage pins the pod to one host, or the host itself is the maintenance target. Thanks for confirming the text answered it; I've kept that framing prominent in the motivation.
|
|
||
| This earns its keep specifically when the broker's pod **cannot** simply be rescheduled elsewhere: | ||
|
|
||
| - **Local / host-attached storage** (`PersistentVolume`s bound `WaitForFirstConsumer` to a specific host). Draining the host cannot move the broker — its pod just goes `Pending` on the gone node — so there is no graceful way to take it down and bring it back on the same disks today. |
There was a problem hiding this comment.
Guess this answers my previous question
|
|
||
| - **Brokers only.** A stop targeting a controller node (controller-only or combined broker+controller) is **refused** — the entry is ignored, a `.status` condition and warning event explain why, and the operator keeps managing that node. Stopping a controller removes it from the KRaft metadata quorum, which needs dedicated quorum-safety machinery; that is future work. | ||
| - **Known limitation — combined-role deployments.** Because a combined broker+controller node *is* a controller, every stop targeting it is refused. In a fully **combined-mode** cluster (every node holds both roles, common on small KRaft deployments — and exactly the clusters most likely to run the host-attached storage this feature targets) v1 can stop *nothing*. The feature is only useful where dedicated broker-only nodes exist; combined-node stops arrive with the controller work. | ||
| - **At least one broker must remain running (enforced).** Despite the title's "a single broker," `stoppedNodes` is a per-pool `array<integer>` and nothing structurally bounds it. The operator must refuse a stop that would leave **zero** running brokers cluster-wide (ignored entry + warning + `.status`), because the bootstrap-dependent reconcile steps below would otherwise wedge regardless of `runningNodes()`. v1's safe stance is "one broker at a time"; stopping enough brokers to break bootstrap/min-ISR is the operator's responsibility, surfaced loudly (see Safety). |
There was a problem hiding this comment.
Should we really enforce this on KafkaRoller/StrimziPodSetController? Like I would expect the current rolling logic will handle it and in case user will stop 3 pods from 3 it is his fault.
There was a problem hiding this comment.
Agreed — the enforcement is dropped. The rewrite says the operator does not limit how many brokers can be stopped, and that the consequences are the same as taking those brokers down manually and are the user's responsibility.
ppatierno
left a comment
ppatierno
left a comment
There was a problem hiding this comment.
I am so sorry but I had to stop reading half way through after leaving some comments.
I see this proposal very poor written by AI and also not well checked by a human before being submitted.
| - `StrimziPodSetController` treats `spec.pods` as absolute: it creates any listed pod that is missing (`maybeCreateOrPatchPod`) and **deletes any matching pod not listed** (`removeDeletedPods`). A terminal-state pod is deleted and recreated. So `kubectl delete pod` always bounces back, and hand-editing the `StrimziPodSet` is overwritten on the next `KafkaReconciler` run. | ||
| - The Kafka container command is hardcoded to `/opt/kafka/kafka_run.sh`, which unconditionally execs `kafka-server-start.sh`. There is no idle/sleep mode and no per-node command override. | ||
| - `KafkaRoller` assumes every input node should be running. (It does not throw on a *missing* pod — `restartIfNecessary` no-ops when `podOperations.get(...)` returns `null`; the `FatalProblem("Pod is unschedulable or is not starting")` fires only for a pod that *exists* and is stuck/Pending. The hazard for a stopped node is therefore not that path but the transient window described under "Readiness and reconciliation.") | ||
| - No annotation or CRD field expresses stop/park/offline; no `.status` field tracks per-node running state. |
There was a problem hiding this comment.
imho the above is a very poor AI generated paragraph with a lot of code reference which doesn't make clear what's the current situation to anyone who isn't a maintainer (and knows the code).
There was a problem hiding this comment.
You're right — that paragraph assumed the reader knows the operator internals. The current-situation section is rewritten as four plain sentences (pod deletion bounces back, scale-down is permanent removal, whole-CR pause freezes everything else) with no class or method names. Code specifics now appear only in the short mechanism section, where they're the actual subject.
| - `KafkaRoller` assumes every input node should be running. (It does not throw on a *missing* pod — `restartIfNecessary` no-ops when `podOperations.get(...)` returns `null`; the `FatalProblem("Pod is unschedulable or is not starting")` fires only for a pod that *exists* and is stuck/Pending. The hazard for a stopped node is therefore not that path but the transient window described under "Readiness and reconciliation.") | ||
| - No annotation or CRD field expresses stop/park/offline; no `.status` field tracks per-node running state. | ||
|
|
||
| The adjacent controls all do something else: `strimzi.io/pause-reconciliation` freezes the whole CR but does not stop Kafka; `strimzi.io/manual-rolling-update` restarts a pod; scale-down + `strimzi.io/skip-broker-scaledown-check` permanently removes a node; `strimzi.io/delete-pod-and-pvc` deletes then recreates. |
There was a problem hiding this comment.
why strimzi.io/skip-broker-scaledown-check annotation was mentioned here? It serves a different purpose.
There was a problem hiding this comment.
Agreed, it served a different purpose and only added noise — removed in the rewrite.
|
|
||
| Doing this today forces a bad choice: | ||
|
|
||
| - **Scale down** to drop the broker, then scale back up — but that permanently removes the node, reassigns/loses its replicas, unregisters it (108), and the broker that comes back is a *different* node ID with empty storage that must re-replicate everything. |
There was a problem hiding this comment.
unregisters it (108)
What's 108? Actually I know what it is but it's another example of a badly AI generated text which was not reviewed properly by the author to make it clear to a human reader.
There was a problem hiding this comment.
Fair — a bare number is meaningless to anyone who doesn't already know the proposal index. The rewrite drops the numeric references and describes the behavior in words ("the node is unregistered from the KRaft metadata"); the one remaining cross-reference is a named link to the scale-down check proposal.
| ### Scope (v1) | ||
|
|
||
| - **Brokers only.** A stop targeting a controller node (controller-only or combined broker+controller) is **refused** — the entry is ignored, a `.status` condition and warning event explain why, and the operator keeps managing that node. Stopping a controller removes it from the KRaft metadata quorum, which needs dedicated quorum-safety machinery; that is future work. | ||
| - **Known limitation — combined-role deployments.** Because a combined broker+controller node *is* a controller, every stop targeting it is refused. In a fully **combined-mode** cluster (every node holds both roles, common on small KRaft deployments — and exactly the clusters most likely to run the host-attached storage this feature targets) v1 can stop *nothing*. The feature is only useful where dedicated broker-only nodes exist; combined-node stops arrive with the controller work. |
There was a problem hiding this comment.
this is actually very similar to the previous point where "combined" is mentioned.
There was a problem hiding this comment.
Reworded — the scope section now spells it out: "a node with the controller role (whether controller-only or with both the controller and broker roles)".
|
|
||
| ### How a node is taken offline | ||
|
|
||
| The key design point is the distinction between two node sets. `KafkaCluster.nodes()` stays the **desired-and-retained** set (it still includes a stopped node), so everything that should keep working for a stopped node keeps working unchanged: PVC generation, certificate Secrets, per-broker `ConfigMap`s, listener/Service generation, `KafkaNodePool.status.nodeIds`, the 049 scale-down check (sees no removal), and 108 unregistration suppression (sees the node as still present). A new accessor, **`runningNodes()` = `nodes()` minus stopped nodes**, is introduced for every place that requires a node to be *live right now*. |
There was a problem hiding this comment.
I found reading this sentence pretty hard. Clearly not written or checked by a human.
There was a problem hiding this comment.
That sentence didn't survive the rewrite — the whole section was redone in plain prose, one sentence per line. I hope the new version reads like it was written for a human this time.
There was a problem hiding this comment.
Was this answer written by a human? 🤔
@scholzj Thank you for the concrete direction — I've rewritten the proposal around it. The trigger is now a One adaptation I'd like your view on: rather than requiring the #229 annotation to be set alongside this one, the proposal makes |
@Frawless Agreed on both — the rewrite uses a |
@ppatierno That's fair, and I'm sorry for the review burden it caused — the sections you flagged leaned on internal code references and bare proposal numbers that only make sense to someone who already knows the codebase, which defeats the point of a proposal. I've rewritten it from scratch around Jakub's suggested design: plain prose, one sentence per line, no bare code symbols in the motivation or current-situation sections, and every cross-reference is now a named link. It's also about a third of the original length. I do use AI as a drafting tool, but the checking is my responsibility, and I'd appreciate you giving the rewrite a fresh look when you have time. |
|
@im-konge Following up properly on your points now that the rewrite is pushed: the proposal-ID note is gone from the PR description (you're right, the number is whatever it is at merge time); the text is one sentence per line throughout; the "v1" scope language is gone (broker-only is stated as the scope, controller support as future work); and the feature gate is dropped — as an opt-in annotation with unchanged default behavior it didn't warrant one. |
|
Thank you @scholzj, @Frawless, @ppatierno, and @im-konge for the review. I've rewritten the proposal from scratch around Jakub's suggested design and it's now about a third of the original length. The main changes:
One open point flagged inline for Jakub: the rewrite makes the annotation self-contained (it implies the roller and readiness exclusions) instead of requiring the #229 annotation to be set alongside it, because forgetting the second annotation would wedge the whole |
|
@acgtun I think you should reduce the usage of AI tbh. That's my personal view. I think most of the answers are AI generated and also you keep writing (even on other proposals) a sort of summary for each reviewer + a global summary for all of them. Is it really worth the time for the reviewers to read all this content? |
Thanks @ppatierno for the feedback. Sorry for this. I will balance this. |
|
thanks all for the discussion. i will close this for now. And work on it later if still needed. thank again. |
5 participants
Summary
Proposal for stopping a single Kafka broker — no
Pod, no Kafka process — while retaining its node ID, storage, and configuration, until it is explicitly started again. The rest of the cluster keeps reconciling normally. This targets planned host maintenance and investigation where the same broker must come back with its data intact and its pod cannot simply be rescheduled (local/host-attached storage, or the host itself is the maintenance target).Design (rewritten after the first review round)
KafkaNodePoolannotation,strimzi.io/stopped-nodes, using the same node-ID list format as the existing node-ID annotations, instead of aspecfield.StrimziPodSetControllerdoes not (re)create a pod for a listed node. The broker is stopped by deleting its pod (or letting a host drain evict it) and stays down; removing the ID from the annotation brings it back with the same identity, storage, and current configuration.