Skip automatic rolling updates for a Kafka node - #229
Conversation
37a3396 to
29f68d8
Compare
im-konge
left a comment
im-konge
left a comment
There was a problem hiding this comment.
Thanks for the proposal, I didn't go fully through it, but for now few points from me:
- please have one sentence per line, it's easier to do reviews and comment exact sentence than pick up from few senteces on one line.
- you are mentioning having API change in NodePools, annotation, then you are comparing what is better and not, then you have Feature Gate - you should pick one approach, say why you picked it and then you can have some rejected alternatives
- also, I'm not sure you need Feature Gate for something that will not change default behavior, but it will be opt-in based on some configuration or something, but that's maybe just my feeling
- speaking about
v1and API stuff is kind of confusing for me, as we already have v1 API, but here you mention some "v1 of the implementation"
And then something from my side. I respect our AI policy, but having a long text generated by AI is not good for the maintainers, as they have to go through a lot of text, that may not provide much or everything that is needed.
The points about long text, having one sentence per line etc. is actually something for those 2 other proposals you created.
scholzj
left a comment
scholzj
left a comment
There was a problem hiding this comment.
Thanks for the proposal.
I have some doubts about the usefulness. I think it would in most cases ultimately block the operator from rolling the other nodes and thus end up in the same state as the paused reconciliation.
However, I think if we trim down the implementation to something smaller and leaner, it would be a reasonable feature to add and I would be fine with it. It might not help that much in some situations, but it would add at least something.
I left bunch of comments, but I think the key would be to try to simplify it. Please think whether this really needs dedicated metrics or API in the .spec section. I think it does not and if we trim it down, I think this proposal would be fine. Let's start smaller, get the feature added and see how it works from there.
|
|
||
| The cluster-wide pause is the wrong tool for the most common operational emergencies, which are almost always single-node problems. An operator needs to take one node out of operator-driven rolling — while the rest of the cluster keeps being managed — for many reasons, for example: | ||
|
|
||
| - A broker doing a long on-disk log recovery (`RECOVERY`) that exceeds the operation timeout, where a restart throws the in-progress recovery away. |
There was a problem hiding this comment.
I think this should be covered and the Roller should not roll brokers in recovery. Is that not the case for you? That would be IMHO a bug.
There was a problem hiding this comment.
Good point — you're right that a clean RECOVERY is already handled by 048 and the roller shouldn't roll a recovering broker. The loop here was the agent hanging and returning BrokerState(-1) instead of RECOVERY (#12513), so it fell through to force-restart. I've reframed the motivation: #12513 is the right fix for recovery and should land on its own; this lever covers the cases the heuristic can't see.
| 1. The affected broker is in `RECOVERY`, so the Kafka Agent readiness endpoint fails and the pod is `0/1` NotReady. | ||
| 2. `KafkaRoller` waits up to `STRIMZI_OPERATION_TIMEOUT_MS` (default `300000` ms) for readiness, does not get it, and takes its not-ready-pod path. It rolls (deletes/recreates) the still-recovering pod. The empty reasons list seen in the incident logs (`due to []`) reflects that the `podNeedsRestart` predicate produced no reasons — the restart was driven by the roller's internal not-ready / force-restart path, not by the predicate. | ||
| 3. The reconcile times out again, surfaces as a `FatalProblem`, and the next periodic reconcile (`STRIMZI_FULL_RECONCILIATION_INTERVAL_MS`, default `120000` ms) repeats the whole thing. |
There was a problem hiding this comment.
As mentioned above, this sounds like a bug. Can you reproduce it? The KAfka Agent running inside the broker should be providing the information about the recovery and the operator should not touch such a node. So maybe something changed in Kafka and this does not work anymore? (I assume you use some reasonably recent Kafka version) CC @tinaselenge
There was a problem hiding this comment.
Agreed it's a bug. It reproduces when the data volume is stuck: the agent stays reachable on TCP but never returns a broker-state response, and getBrokerState() has no request timeout (#12513), so it resolves to BrokerState(-1, null) and 048's == 2 guard doesn't fire. Kafka version was recent; the timeout fix is #12675. Moved to a short note in the motivation.
There was a problem hiding this comment.
KafkaRoller generally decides to roll unresponsive nodes when operator cannot reach through Admin API bu if we cannot determine whether it's in log recovery or not via KafkaAgent, I wouldn't expect the force-restart to happen. Unless if the node was in one of the stuck states ( "CrashLoopBackOff", "ImagePullBackOff", "ContainerCreating", "InvalidImageName"), and was outsided which would mean it wasn't doing recovery. If it was simply in not ready state, it shouldn't have been restarted. We should see the following logs in this case:
LOGGER.warnCr(reconciliation, "Failed to wait for the readiness of the pod {}. We will proceed and check if it needs to be rolled.", nodeRef.podName(), e.getCause());
...
await(isReady(namespace, nodeRef.podName()), operationTimeoutMs, e -> new FatalProblem("Error while waiting for non-restarted pod " + nodeRef.podName() + " to become ready", e));
FatalProblem should cause the reconciliation to fail, not restart non ready pod.
There was a problem hiding this comment.
Thank you for taking the time to walk through the roller logic — your first sentence is exactly what happened: the restart came from the cannot-reach-through-Admin-API handling, not the stuck-state path. Tracing it through the code:
- The pod was NotReady but not in any of the stuck states, so
restartIfNecessarywaited for readiness, timed out, and calledgetBrokerState(). The stuck volume meant the agent request failed rather than reportingRECOVERY, andgetBrokerState()swallows the failure and returnsBrokerState(-1, null)(per its javadoc), soisBrokerInRecovery()was false and execution fell through with the "We will check if KafkaRoller can do anything about it" warning. - In
checkIfRestartOrReconfigureRequired, the bootstrap admin client initialized fine (the other brokers were healthy), but the per-broker probe —getConfig(nodeRef), which the code comments note is "sent to that specific broker" as proof it can respond — failed against the stuck broker with aForceableProblem. - Once the backoff was exhausted, that
ForceableProblembecomes a restart either way: insidecheckIfRestartOrReconfigureRequiredit setsneedsRestartwithCONFIG_CHANGE_REQUIRES_RESTART, and if it propagates instead, the outercatch (ForceableProblem)force-rolls withPOD_FORCE_RESTART_ON_ERROR(subject tocanRoll). (POD_UNRESPONSIVEis the sibling branch for when even the bootstrap client can't be created.) - The
FatalProblemwait you quote is theelsebranch, reached only when the roller can talk to the broker and there's nothing to restart — an unreachable broker never gets there.
So I agree with your conclusion that there's no roller bug here beyond the agent-client one: with strimzi/strimzi-kafka-operator#12675 giving the agent request a timeout, a clean RECOVERY answer stops the roller exactly as 048 intends. The annotation is aimed at the cases where nothing the operator can read distinguishes "restart me" from "leave me alone" — which the unresponsive handling, by design, resolves as "restart".
|
|
||
| This is an infinite restart loop. The intrinsic harm — independent of storage setup — is that every forced restart discards in-progress recovery: `LogLoader` starts over from scratch, so a long recovery can never finish and the cluster stays permanently under-replicated. In this particular incident it was made worse by storage topology: the restart rescheduled the pod onto a different Kubernetes node (host), and because the volumes were `ReadWriteOnce` the pod then hit `Multi-Attach` errors (the PersistentVolume was still attached to the prior host). That is an aggravating factor specific to RWO storage, not a precondition for the loop — the recovery-restart loop happens regardless. | ||
|
|
||
| Notably, the deployed operator already shipped proposal-048's recovery detection and it still did not prevent the loop. With the data volume stuck, the Kafka Agent stayed reachable on TCP but never returned a broker-state response, so `KafkaAgentClient.getBrokerState()` — which has no request timeout, [strimzi-kafka-operator#12513](https://github.com/strimzi/strimzi-kafka-operator/issues/12513) — hung and resolved to `BrokerState(-1, null)`. Because `-1 != 2` (`RECOVERY`), `KafkaRoller` fell through to force-restart as if 048 were absent. A timeout fix is tracked in [#12675](https://github.com/strimzi/strimzi-kafka-operator/pull/12675), but in the operator source reviewed here `KafkaAgentClient` still sets no request timeout, so the hang is reproducible — check its merge state before relying on it. |
There was a problem hiding this comment.
Ok, I guess that actually answers the previous comments. But why does the Agent timeout?
There was a problem hiding this comment.
The volume was stuck, so the agent never completed the broker-state read and getBrokerState() had no request timeout to bail out (#12513 / #12675). Folded into the reframed motivation.
There was a problem hiding this comment.
I think it's a problem we should investigate and fix with KafkaAgent.
There was a problem hiding this comment.
Fully agree, and that's been done independently of this proposal: strimzi/strimzi-kafka-operator#12513 tracked it and strimzi/strimzi-kafka-operator#12675 (merged — thank you for the review there) bounds the HTTP timeouts in KafkaAgentClient. The proposal text frames that fix as landing on its own; the annotation is only for what the recovery heuristic can't decide, not a workaround for that bug.
|
|
||
| Throughout this proposal, **"v1"** refers to the first implementation increment of this feature — not a Strimzi release version or a CRD API version (e.g. `v1beta2`). To give reviewers a smaller, clearly bounded first cut, v1 is intentionally narrow. Everything outside this list is either deferred (see Out of scope) or a later increment of the same design: | ||
|
|
||
| - **Brokers only; controllers refused in v1.** Broker skip is the motivating case and is quorum-safe by construction. In v1 a skip that targets 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 normally. The whole controller machinery (refuse-by-default quorum math, the `force` exception, per-reconcile quorum re-evaluation) is **future work**, not v1. This keeps the most contentious surface out of the first review. |
There was a problem hiding this comment.
It is just a language. But refused sounds confusing here as you cannot refuse it. I would stick with ignored.
There was a problem hiding this comment.
Agreed — switched to "ignored" throughout. A controller node ID in the list is ignored and the operator keeps managing it.
| - **Known limitation — combined-role deployments.** Because a combined broker+controller node *is* a controller, v1 refuses skips for it. In a fully **combined-mode** cluster (every node holds both roles, common on small KRaft deployments) this means v1 refuses **every** skip and the feature is effectively inert there; it is useful only where dedicated broker-only nodes exist. The motivating incident is covered only if the recovering broker is a broker-only node. This is an accepted v1 limitation; combined-node skips become available with the controller work. | ||
| - **Two surfaces, one meaning.** The ephemeral pod-annotation fast path (`strimzi.io/skip-rolling-update="true"`) and the durable `KafkaNodePool.spec` field. The pod annotation is what an operator reaches for mid-incident; the `spec` field is what survives pod recreation (and any declarative re-apply, GitOps included). | ||
| - **Remove the skipped node from the roller's working set *and* from the post-roll readiness stage.** This is the mechanism (see "KafkaRoller behavior"): the node is excluded from the set `KafkaRoller` iterates, so it is never a roll candidate. Excluding it from the roller is **necessary but not sufficient** — the reconcile pipeline runs a *separate* `KafkaReconciler.podsReady()` stage after rolling, which waits on the unfiltered `kafka.nodes()` set and would still time out on the skipped pod (see "Reconcile readiness stage and CR status"). v1 must exclude the skipped node from both. As a consequence *every* operator-driven roll of the node is suppressed at once — the not-ready/force-restart loop, config, cert-renewal, and version-upgrade rolls — and it keeps its current config/cert/version until unskipped. This is a deeper change than filtering restart reasons, and the part v1 strictly requires. | ||
| - **Behind a feature gate**, default off (see below). |
There was a problem hiding this comment.
Not sure this needs feature gate TBH. It is something the users would trigger by annotation. So maybe it is not needed.
There was a problem hiding this comment.
Agreed — dropped the feature gate. It's opt-in by annotation and doesn't change default behavior.
|
|
||
| ### Controller handling: refused in v1 | ||
|
|
||
| Skipping a controller is more dangerous than skipping a broker, because controllers form the KRaft metadata quorum. Rather than ship a half-built quorum-safety story, **v1 refuses controller skips outright** and defers the full machinery: |
There was a problem hiding this comment.
Is this really the case? Wouldn't it just essentially block the rolling update when rolling update is needed?
There was a problem hiding this comment.
You're right, I was overselling this. The rewrite now says plainly that when the skipped broker is down, rolls of other brokers sharing an at-risk partition are deferred by the existing min-ISR check — exactly as any unsafe roll is today. The honest win over pause is narrower: non-rolling reconciliation continues cluster-wide and unaffected nodes still roll. Availability-math change moved to future work.
| ### Drain Cleaner interaction | ||
|
|
||
| A skip does **not** make a node un-evictable. Strimzi Drain Cleaner intercepts pod eviction (e.g. from a node drain) and asks the operator to roll the pod gracefully; a skipped node is still a normal pod to the eviction path. So a drain can evict a skipped, recovering broker → the `StrimziPodSet` recreates it → recovery restarts from scratch — the exact harm the skip was meant to prevent, via a different door. | ||
|
|
||
| v1 guidance, at minimum: | ||
|
|
||
| - **Operationally exclude the skipped node from Drain Cleaner.** While a node is skipped, the operator documentation instructs disabling Drain Cleaner handling for that node (and cordoning the Kubernetes node so the cluster autoscaler / drain tooling does not evict it), so an eviction does not silently undo the skip. | ||
| - A first-class integration where Drain Cleaner itself honors the skip and refuses to evict a skipped node is noted as future work, not v1. |
There was a problem hiding this comment.
It is not really clear to me what exactly you suggest here. Also, keep in mind that the Drain Cleaner works through the manual rolling update. So it relates to the other part where I already raised an comment. That should be linked and thought through wjat we really want here.
There was a problem hiding this comment.
Thanks, this was the interaction I'd hand-waved. Since Drain Cleaner evicts through the manual-rolling-update path, and skip deliberately doesn't suppress manual rolls, a drain can still move a skipped node. Made that explicit and linked it to the manual-rolling-update semantics: for now handled operationally (exclude from Drain Cleaner + cordon the host), with a first-class integration as future work.
|
|
||
| New operator metrics (Prometheus, `strimzi_` prefix, labeled by cluster / node pool / node ID where applicable), to make the dangerous states (forgotten skip, quorum thinning, sole-replica-on-skipped-node) alertable rather than implicit. | ||
|
|
||
| v1: |
There was a problem hiding this comment.
TBH, this sounds like something to trim down and reduce to log warnings, maybe a status condition. Alternatively, maybe Kube State Metrics can be used to monitor this.
There was a problem hiding this comment.
Agreed — dropped all dedicated metrics. Just a .status condition plus log warnings, and Kube State Metrics can alert off that. Metrics noted as future work only.
| A skip suppresses operator-*initiated* rolls only. It does **not** stop: the `StrimziPodSet` controller recreating the pod if it is deleted/evicted (which is why the durable `spec` path exists and Drain Cleaner must be handled — a recreated pod is skipped again on the next reconcile); kubelet liveness/readiness probes; Kubernetes host-level eviction/drain/scheduler actions (see Drain Cleaner); or reconciliation of *other* nodes. | ||
|
|
||
| It also does **not** stop operator-driven **scale-down**. `KafkaReconciler.scaleDown()` rewrites the `StrimziPodSet` to the desired pod set derived from `kafka.nodes()`, entirely outside `KafkaRoller`. If the skipped node's ID is removed from the desired set — by lowering the pool's `replicas` or via `strimzi.io/remove-node-ids` — its pod is deleted regardless of the skip (and, mid-recovery, that discards in-progress recovery). v1 does **not** intercept this: the skip lever governs *rolling*, not membership. To avoid silently undoing a skip, v1 documents that a skipped node must not be scaled down (the existing scale-down safety check in [049](https://github.com/strimzi/proposals/blob/main/049-prevent-broker-scale-down-if-it-contains-partition-replicas.md) already refuses removing a broker that still holds replicas, which covers the common case); a first-class "refuse scale-down of a skipped node" guard is noted as future work. |
There was a problem hiding this comment.
What about changes to the Pod's support resources?
- Should services be updated?
- Should PVCs be added / removed?
- Should the Pod itself be updated in the StrimziPodSet (and recreated as updated next time) - e.g. when you change resources?
- The configuration files in ConfigMaps and Secrets, etc.
I assume these would go as before?
There was a problem hiding this comment.
Yes — those go as before. Skip only suppresses rolling restarts; non-rolling reconciliation (Services, PVCs, ConfigMaps/Secrets, the SPS pod template) continues, the change just isn't applied to the running pod until it's next restarted or unskipped. Added a section saying this, plus a note that membership stages (KRaft register/unregister, scale-down) keep deriving membership from kafka.nodes() so a skipped-but-down node isn't treated as removed.
|
|
||
| - **Whole-`Kafka`-CR pause (`strimzi.io/pause-reconciliation`).** This exists today and is exactly the pain this proposal addresses. It is too coarse: to protect one node you must freeze config, cert rotation, scaling, and rolling for the entire cluster, including all the healthy nodes you still want the operator to manage. It is an emergency sledgehammer, not a per-node control. | ||
|
|
||
| - **A durable node-pool *annotation* instead of a `spec` field.** Rejected on durability grounds (above): a pod annotation does not survive pod recreation, and any declarative re-apply (plain `kubectl apply`, or ArgoCD/Flux with self-heal) reverts an annotation not present in the manifest. The durable surface must be a `spec` field the user authors; the annotation survives only as the documented ephemeral fast path. |
There was a problem hiding this comment.
We treat annotations as durable. You can set it through GitOps, you just need to go through Git. So that is why I think this is the right way to go.
There was a problem hiding this comment.
You're right, and this collapses the proposal's biggest section. Removed the "spec field is required for durability" argument — the durable surface is the KafkaNodePool annotation, set/removed via Git like any other field. The rejected-alternatives entry now just notes a pod annotation isn't durable because pods are recreated from the SPS template.
Frawless
left a comment
Frawless
left a comment
There was a problem hiding this comment.
I agree with idea to having annotation on KafkaNodePools resources where user can specify broker IDs that will be excluded from the RU. It would be great to make a propsal smaller. In case there will be more maintainers that will agree on this approach, it will make the proposal much more easier to read.
Would be sufficient to have it effective on KNP level only in case it will make the implementation logic easier?
|
Thank you all for the thorough reviews — this was really helpful. The consistent message was that the proposal is too long and tries to do too much, and I completely agree. I've rewritten it around the minimal design you suggested and cut it down by roughly 3x (about 6500 to 2300 words). Here's what changed:
I'll apply the same length and one-sentence-per-line treatment to the other two proposals as well. Thanks again for taking the time — please let me know if the trimmed version is closer to what you had in mind. |
There was a problem hiding this comment.
Thank you for the proposal. To be honest, I'm not sure about the motivation. It sounds like the the main issue we are trying to fix here is rolling a node in recovery mode due to KafkaAgent being unresponsive. Then we probably should investigate and fix that so that it is handled automatically, rather than manually via annotation.
Do we know if there has been a demand from several users to skip a node from rolling rather than pausing reconciliations for other reasons? I only remember requests such as slowing down rolling or speeding up the rolling.
|
|
||
| ### Mechanism | ||
|
|
||
| A single predicate `isSkipped(nodeId)` is consulted at every gate that waits on node readiness; missing one would hang the reconcile, so the set is explicit: |
There was a problem hiding this comment.
Where would this predicate be set? is KafkaReconciler going to remove this from the set that is passed to the KafkaRoller?
There was a problem hiding this comment.
at every gate that waits on node readiness
So, where?
There was a problem hiding this comment.
Yes, exactly as you describe — the change lives in KafkaReconciler, and KafkaRoller itself needs no modification. Concretely, KafkaReconciler resolves the KafkaNodePool annotations into a set of skipped node IDs once per reconciliation (validating pool membership and roles at that point), and the set is applied in the reconcile() chain:
rollingUpdate()— today it passes the unfilteredkafka.nodes()tomaybeRollKafka(); the skipped IDs are removed from that set, so the roller never sees the node and none of its internal force-restart paths can fire.podsReady()— it waits on every pod fromkafka.nodes()withoperationTimeoutMs; without filtering here, a NotReady skipped pod would time this stage out on every reconcile even though the roller ignored it.manualRollingUpdate()— deliberately not filtered, so an explicitstrimzi.io/manual-rolling-updatestill rolls the node.
I also checked the later serviceEndpointsReady() / headlessServiceEndpointsReady() stages: they need no filtering, because Endpoints readiness needs only one ready address and the headless brokers service is created with publishNotReadyAddresses: true, so a single NotReady node doesn't block them.
Sorry this wasn't clear from the text — I'll replace the vague "every gate that waits on node readiness" wording with this concrete list in the next revision.
| A single predicate `isSkipped(nodeId)` is consulted at every gate that waits on node readiness; missing one would hang the reconcile, so the set is explicit: | ||
|
|
||
| - the `KafkaRoller` working set — the node is never a roll candidate, so it never reaches the force-restart paths (decided inside the roller after `POD_UNRESPONSIVE` or in the `catch (ForceableProblem)` branch, which produce no `RestartReasons` — a reason filter would never catch them); | ||
| - `KafkaReconciler.podsReady()` and the endpoint-readiness stages, which otherwise wait on the unfiltered `kafka.nodes()` and would time out every cycle. |
There was a problem hiding this comment.
Apologies — that sentence was too compressed to be useful. It's the second half of the answer above: KafkaReconciler.podsReady() runs right after rollingUpdate() and waits up to operationTimeoutMs for every pod in kafka.nodes() to be Ready. If the skip only filtered the roller's working set, a permanently NotReady skipped pod would still fail this stage every cycle and the reconciliation would never complete. So the same skipped set has to filter the pod list handed to ReconcilerUtils.podsReady(). I'll reword the sentence to say exactly that and to name the stages precisely instead of the vague "endpoint-readiness stages" (the endpoint stages themselves turned out to need no filtering — details in the thread above).
| Two things the skip must **not** do: | ||
|
|
||
| - It must not make the node look removed: membership stages (KRaft register/unregister, scale-down) keep deriving membership from `kafka.nodes()`, which still includes the skipped node. | ||
| - It must not advance cluster-wide state past the held node: a cluster-wide Kafka version / `metadata.version` change is deferred while any node is skipped, since the skipped node stays on its old version and finalizing the upgrade without it could leave it unable to rejoin. |
There was a problem hiding this comment.
so if kafka version is updated, it basically would pause the whole reconciliation?
There was a problem hiding this comment.
Good question — not the whole reconciliation, but the version rollout itself would be held, and that's deliberate. Rolling every other node to the new version and then finalizing metadata.version while one node is held on the old version could leave that node unable to rejoin, which seemed worse than waiting. So: if a version change is pending while any node is skipped, the operator defers the upgrade rollout, logs a warning, and surfaces the conflict in the status condition; everything else (scaling, PVCs, cert/config generation, rolls for other reasons on other nodes) continues. This is also why the doc says a skip must not be held across an upgrade — the condition makes the conflict visible rather than silent.
If you think the alternative is better — let the binary rollout proceed on the other nodes and hold only the metadata.version bump — I'm happy to explore that; deferring the whole upgrade just seemed both safer and simpler for the first version. I'll make this behavior explicit in the doc either way.
| ### Controller nodes | ||
|
|
||
| The first version handles broker nodes only. | ||
| A node ID that resolves to a controller (controller-only or combined broker+controller) is ignored and kept managed, surfaced in status and logs. |
There was a problem hiding this comment.
Where would this check happen? We currently check process role of the node in 2 different ways, NodeRef contains desired role and KafkaRoller checks pod label.
There was a problem hiding this comment.
Thanks for raising this — the two-sources subtlety is exactly the kind of thing the doc should pin down and didn't. My intent is to do the check at annotation-resolution time in KafkaReconciler, using the NodeRef desired roles, because that's the same level where the skip set is built and it works even when the pod doesn't exist. Since desired and actual roles can diverge during a role transition, I'd make it conservative: honor the skip only if the node is broker-only by desired role and, when the pod exists, its role labels don't claim controller; otherwise ignore the skip and log why. The reasoning: rolling one node too many during a transition is recoverable, while silently thinning the quorum is not. I'll spell this out in the proposal — and if you see a problem with keying off NodeRef at the reconciler level, I'd appreciate the correction.
|
|
||
| The first version handles broker nodes only. | ||
| A node ID that resolves to a controller (controller-only or combined broker+controller) is ignored and kept managed, surfaced in status and logs. | ||
| Quorum-safe controller skipping is future work, because a skipped controller can thin the KRaft metadata quorum; in a fully combined-mode cluster every node is a controller, so the skip is inert there until then. |
There was a problem hiding this comment.
I don't see how this would be done safely for controllers or how it is useful for controllers as recovery process is very quick for controllers. Normally users would not need more than 3 or 5 nodes for the quorum, skipping even just one decreases the majority and puts the quorum safety at risk. I understand it is safer with brokers as there can be much larger number of brokers that rolling can continue safely while one broker is skipped.
There was a problem hiding this comment.
Agreed on both counts — controller recovery is fast so the motivation barely applies, and the quorum-majority risk is real. The proposal is brokers-only and a controller-resolving ID is ignored and kept managed. Since you and Paolo both flagged the future-work entry as well, I'll drop controller skipping from Future work entirely rather than leave it as a half-promise (see the line 125 thread).
|
|
||
| ## Future work | ||
|
|
||
| - **Controller skip support**: a quorum-safe admission check (refuse a skip that would drop the KRaft quorum below majority), an explicit override flag, per-reconcile re-evaluation, and deciding other-controller rolls from real `DescribeQuorum` state. |
There was a problem hiding this comment.
As I said previously, I don't think this should be done for controllers.
There was a problem hiding this comment.
Understood, and agreed — I'll remove the controller-skip entry from Future work. The proposal will state that skipping applies to broker-only nodes, controller-resolving IDs are ignored and kept managed, and controller support is explicitly a non-goal rather than deferred work.
| - **Controller skip support**: a quorum-safe admission check (refuse a skip that would drop the KRaft quorum below majority), an explicit override flag, per-reconcile re-evaluation, and deciding other-controller rolls from real `DescribeQuorum` state. | ||
| - **First-class Drain Cleaner integration** so a skipped node is automatically excluded from drain-driven evictions. | ||
| - **Auto-expiry of a skip** after a configurable duration, so a forgotten skip does not block cert rotation, config, or upgrades indefinitely. | ||
| - **Availability-math refinement** to subtract skipped brokers from the effective ISR when judging whether other nodes can roll. |
There was a problem hiding this comment.
why would you subtract it?
There was a problem hiding this comment.
The thought was: a node is usually skipped because it's unhealthy or about to be worked on, so when deciding whether another broker can roll, counting the skipped node as a dependable ISR member is optimistic — it may drop out mid-roll. "Subtracting" it would treat it as unavailable in the safety math, which is conservative. But you're right to question it: the existing check already works from the live ISR reported by Kafka, which reflects reality well enough, and the extra pessimism has its own cost (blocking rolls that are fine when the skipped node is healthy). Since it's speculative and evidently more confusing than helpful, I'll drop this bullet.
| - **First-class Drain Cleaner integration** so a skipped node is automatically excluded from drain-driven evictions. | ||
| - **Auto-expiry of a skip** after a configurable duration, so a forgotten skip does not block cert rotation, config, or upgrades indefinitely. | ||
| - **Availability-math refinement** to subtract skipped brokers from the effective ISR when judging whether other nodes can roll. | ||
| - **Dedicated metrics** if the status condition proves insufficient for alerting. |
There was a problem hiding this comment.
It should be decided now whether it is sufficient or not.
There was a problem hiding this comment.
That's fair — a proposal shouldn't leave this open. Decision: the status condition plus log warnings is the observability surface for this feature; no dedicated metrics, unless the discussion on the line 96 thread lands the other way, in which case they get specced here rather than deferred. I'll remove the "if it proves insufficient" hedge either way.
| 1. The affected broker is in `RECOVERY`, so the Kafka Agent readiness endpoint fails and the pod is `0/1` NotReady. | ||
| 2. `KafkaRoller` waits up to `STRIMZI_OPERATION_TIMEOUT_MS` (default `300000` ms) for readiness, does not get it, and takes its not-ready-pod path. It rolls (deletes/recreates) the still-recovering pod. The empty reasons list seen in the incident logs (`due to []`) reflects that the `podNeedsRestart` predicate produced no reasons — the restart was driven by the roller's internal not-ready / force-restart path, not by the predicate. | ||
| 3. The reconcile times out again, surfaces as a `FatalProblem`, and the next periodic reconcile (`STRIMZI_FULL_RECONCILIATION_INTERVAL_MS`, default `120000` ms) repeats the whole thing. |
There was a problem hiding this comment.
KafkaRoller generally decides to roll unresponsive nodes when operator cannot reach through Admin API bu if we cannot determine whether it's in log recovery or not via KafkaAgent, I wouldn't expect the force-restart to happen. Unless if the node was in one of the stuck states ( "CrashLoopBackOff", "ImagePullBackOff", "ContainerCreating", "InvalidImageName"), and was outsided which would mean it wasn't doing recovery. If it was simply in not ready state, it shouldn't have been restarted. We should see the following logs in this case:
LOGGER.warnCr(reconciliation, "Failed to wait for the readiness of the pod {}. We will proceed and check if it needs to be rolled.", nodeRef.podName(), e.getCause());
...
await(isReady(namespace, nodeRef.podName()), operationTimeoutMs, e -> new FatalProblem("Error while waiting for non-restarted pod " + nodeRef.podName() + " to become ready", e));
FatalProblem should cause the reconciliation to fail, not restart non ready pod.
|
|
||
| This is an infinite restart loop. The intrinsic harm — independent of storage setup — is that every forced restart discards in-progress recovery: `LogLoader` starts over from scratch, so a long recovery can never finish and the cluster stays permanently under-replicated. In this particular incident it was made worse by storage topology: the restart rescheduled the pod onto a different Kubernetes node (host), and because the volumes were `ReadWriteOnce` the pod then hit `Multi-Attach` errors (the PersistentVolume was still attached to the prior host). That is an aggravating factor specific to RWO storage, not a precondition for the loop — the recovery-restart loop happens regardless. | ||
|
|
||
| Notably, the deployed operator already shipped proposal-048's recovery detection and it still did not prevent the loop. With the data volume stuck, the Kafka Agent stayed reachable on TCP but never returned a broker-state response, so `KafkaAgentClient.getBrokerState()` — which has no request timeout, [strimzi-kafka-operator#12513](https://github.com/strimzi/strimzi-kafka-operator/issues/12513) — hung and resolved to `BrokerState(-1, null)`. Because `-1 != 2` (`RECOVERY`), `KafkaRoller` fell through to force-restart as if 048 were absent. A timeout fix is tracked in [#12675](https://github.com/strimzi/strimzi-kafka-operator/pull/12675), but in the operator source reviewed here `KafkaAgentClient` still sets no request timeout, so the hang is reproducible — check its merge state before relying on it. |
There was a problem hiding this comment.
I think it's a problem we should investigate and fix with KafkaAgent.
| The feature began with a sharper incident: a broker stuck in a long on-disk log recovery, repeatedly force-restarted by the roller (`due to []` — no pending change), discarding in-progress recovery each time. | ||
| That recovery case should already be handled by proposal [048](https://github.com/strimzi/proposals/blob/main/048-avoid-broker-restarts-when-in-recovery.md): the roller reads the Kafka Agent broker state and, on `RECOVERY`, stops instead of force-restarting. | ||
| It looped only because the agent hung and returned `BrokerState(-1)` instead of `RECOVERY` ([#12513](https://github.com/strimzi/strimzi-kafka-operator/issues/12513), fix in [#12675](https://github.com/strimzi/strimzi-kafka-operator/pull/12675)), so 048's check fell through. | ||
| That part is a bug and the #12513 fix should land independently of this proposal. |
There was a problem hiding this comment.
Please provide a consistent link.
There was a problem hiding this comment.
Will do — I'll make #12513 a proper markdown link, consistent with the references on the previous line. Thanks for catching it.
|
|
||
| ### Mechanism | ||
|
|
||
| A single predicate `isSkipped(nodeId)` is consulted at every gate that waits on node readiness; missing one would hang the reconcile, so the set is explicit: |
There was a problem hiding this comment.
at every gate that waits on node readiness
So, where?
|
|
||
| ### Controller nodes | ||
|
|
||
| The first version handles broker nodes only. |
There was a problem hiding this comment.
Of this feature — the scope this proposal commits to implement. You're right that "first version" reads ambiguously next to API versioning; I'll reword it to "the initial implementation of this feature handles broker nodes only".
|
|
||
| A `.status` condition on the `Kafka` CR (paralleling `ReconciliationPaused`) lists skipped node IDs with their start time, plus ignored controller IDs and rejected invalid IDs; per-node detail is mirrored on the owning `KafkaNodePool`. | ||
| Because `Ready` excludes skipped nodes, the condition also flags when a skipped node holds the sole in-sync replica of any partition, so an intentionally-skipped node cannot make the cluster look healthy while a partition is actually offline. | ||
| Entering/leaving the skipped state is logged and emitted as an event. |
There was a problem hiding this comment.
A Kubernetes Event, published through the operator's existing event publisher the same way restart events are emitted today — with reasons named for entering/leaving the skip (e.g. RollingUpdateSkipEnabled / RollingUpdateSkipDisabled). The doc should have said that instead of the bare "an event"; I'll name the mechanism and the reasons explicitly.
| Because `Ready` excludes skipped nodes, the condition also flags when a skipped node holds the sole in-sync replica of any partition, so an intentionally-skipped node cannot make the cluster look healthy while a partition is actually offline. | ||
| Entering/leaving the skipped state is logged and emitted as an event. | ||
|
|
||
| Disabling a node's self-healing requires edit access to its `KafkaNodePool` — a tighter, more auditable surface than pod-edit. |
There was a problem hiding this comment.
It's contrasting with the rejected pod-annotation design, but I agree it doesn't say so — I'll rewrite it. The intended meaning: with a pod annotation, anyone with pod-edit permission could silently disable a node's self-healing. With the KafkaNodePool annotation, doing that requires edit rights on the KNP custom resource — which in most deployments is GitOps-managed and reviewed, so the action leaves an audit trail and is gated by the same access controls as any other cluster-shape change.
|
|
||
| ## Future work | ||
|
|
||
| - **Controller skip support**: a quorum-safe admission check (refuse a skip that would drop the KRaft quorum below majority), an explicit override flag, per-reconcile re-evaluation, and deciding other-controller rolls from real `DescribeQuorum` state. |
|
@tinaselenge replying to your review summary: Thank you for reading it critically — it's a fair challenge and I'd rather answer it honestly than oversell. You're right that the incident that started this was the agent bug, and the right fix for that is automatic: strimzi/strimzi-kafka-operator#12675 is merged, and the proposal explicitly says recovery handling should not depend on this annotation. The remaining motivation is the cases where no automatic heuristic can decide, because the deciding information isn't in anything the operator can read: a node held NotReady on purpose for live investigation, a known-bad host awaiting a hardware swap where a restart just reschedules the problem (or lands in a Multi-Attach crash loop on RWO storage), a degraded-but-alive disk where a restart makes things worse. In each of those, the operator's unresponsive-node handling is designed to restart — reasonably, for the common case — and the only counter-lever today is the cluster-wide pause, which we've had to use in production and which freezes cert rotation and scaling for every healthy node too. On demand: the concrete evidence I can personally vouch for is our own production experience operating these clusters, and I won't claim broader demand I can't substantiate. If the maintainers feel that isn't enough to justify even a small opt-in lever, I'm happy to park this and open a discussion issue to gauge interest instead. My case for proceeding is that the surface is now very small (one annotation, a filter in |
|
@ppatierno replying to your review summary: You're right, and I'm sorry for the extra review burden that caused. Several of the questions in this round ("which event?", "first version of what?", the readiness-gates bullet) were things I should have caught before pushing — text that gestures at a mechanism without actually naming it, which wastes reviewers' time. For this round I've answered every inline comment against the operator source itself (the |
|
Thank you @tinaselenge and @ppatierno for the careful second round — the questions were specific and caught real gaps, and I've tried to answer each one inline with the same specificity. I've pushed a revision (691abf5) with the changes that follow from them:
Two open questions where I kept the smaller option for now and will gladly change based on your preference: whether auto-expiry of a skip should be in the first version rather than future work, and whether the two metrics (gauge of skipped nodes, counter of suppressed rolls) should be specced in this proposal. |
acgtun
changed the title
Implements proposal 146 (strimzi/proposals#229): an opt-in, per-node control that excludes broker nodes from operator-driven rolling updates through the strimzi.io/skip-rolling-update annotation on a KafkaNodePool, while the operator keeps reconciling everything else. - Resolve the annotation in KafkaReconciler at the start of the reconciliation: per-entry validation (pool membership, format), controller-resolving IDs ignored based on desired roles and the role labels of existing Pods - Exclude skipped nodes from the working set passed to KafkaRoller in rollingUpdate() and from the Pods waited on in podsReady(); manual-rolling-update is deliberately not filtered - Defer a KRaft metadata version change while any node is skipped and surface it with a MetadataVersionChangeDeferred warning condition - Add a RollingUpdateSkipped condition on the Kafka status (mirrored on the owning KafkaNodePool) and emit RollingUpdateSkipEnabled/Disabled Kubernetes Events Signed-off-by: Haifeng Chen <haifengc@twitter.com>
4ef2f7d to
e7650e3
Compare
Add proposal 146: an opt-in, per-node control to exclude a single Kafka node from operator-driven rolling while the rest of the cluster keeps reconciling. Updates the README proposal index. Signed-off-by: Haifeng Chen <haifengc@twitter.com>
- podsReady()/endpoint-readiness must also exclude skipped nodes, else the reconcile times out on the skipped pod every cycle (working-set exclusion in KafkaRoller alone is insufficient) - Specify the Kafka CR Ready contract for a skipped+NotReady node - Call out combined-role/KRaft-combined limitation (all skips refused there) - Note operator scale-down can still delete a skipped node (not governed by skip) - Correct the FatalProblem/readiness-wait citations in KafkaRoller behavior - Cover CA replacement (not just expiry) for a skipped node - Filter skipped IDs out of the StrimziPodSet-level manual-rolling-update expansion Signed-off-by: Haifeng Chen <haifengc@twitter.com>
Trim the proposal to a single KafkaNodePool annotation (strimzi.io/skip-rolling-update), drop the pod annotation, the spec field, the feature gate, and dedicated metrics. Brokers only, with controller IDs ignored. Reformat to one sentence per line, make the automatic-vs-manual rolling semantics explicit, and set the availability expectations honestly. Signed-off-by: Haifeng Chen <haifengc@twitter.com>
Lead the motivation with up-node cases where the value is real and add the "cannot auto-distinguish stuck vs intentionally not-ready" argument. Defer cluster-wide version/metadata changes while a node is skipped, flag sole-ISR partitions in the status condition so Ready stays honest, frame Drain Cleaner suppression as a principled deferral, and add the "stop force-restarting not-ready pods" rejected alternative. Retitle to "automatic" and trim throughout. Signed-off-by: Haifeng Chen <haifengc@twitter.com>
Name the concrete KafkaReconciler integration points, make controller skipping an explicit non-goal, report two-level partition risk in the status condition, scope the upgrade deferral, replace the short-lived claim with the actual visibility mechanisms, and apply the wording and link fixes from the round-2 review. Signed-off-by: Haifeng Chen <haifengc@twitter.com>
Merge the duplicated pause-is-too-coarse argument, compress the incident story, drop two rejected alternatives nobody raised, and fold the Drain Cleaner and auto-expiry follow-ups inline where they are discussed. Signed-off-by: Haifeng Chen <haifengc@twitter.com>
4e7a702 to
df47ba5
Compare
There was a problem hiding this comment.
Thank you for addressing the comments. I've read through the updated proposal. The proposed implementation of how a node would be skipped from rolling kind of makes sense, but I'm still not convinced about the motivation.
The original motivating incident is now fixed by #12513. The remaining cases, degraded disk, hung mount, Multi-Attach on RWO storage are described only in general terms. It's not clear now that we have the fix, how this feature would still be useful in real scenarios.
I'd like to understand more concretely why pause-reconciliation is insufficient for these scenarios. The proposal says the skip annotation should be short-lived during manual intervention. But pause-reconciliation is also short-lived.
During that short window, rolling updates of other nodes may not even be scheduled since rolling doesn't happen all the time. So the value of per-node granularity over a cluster-wide pause seems to be small. And the skip doesn't protect against Kubernetes itself deleting the pod (eviction, liveness failure, node drain), so it's not a complete "don't touch this node" guarantee either.
Without stronger motivation, the complexity doesn't seem justified: annotation parsing with validation, role-checking logic, version upgrade deferral, a new status condition, the risk of a broker missing a CA renewal. I'm afraid this feels like a solution looking for a problem after the original problem was fixed.
|
|
||
| The feature began with a sharper incident: a broker in a long on-disk log recovery was repeatedly force-restarted by the roller, discarding the in-progress recovery each time. | ||
| That turned out to be a bug: proposal [048](https://github.com/strimzi/proposals/blob/main/048-avoid-broker-restarts-when-in-recovery.md) makes the roller stop on `RECOVERY`, but the agent request failed and returned `BrokerState(-1)` ([#12513](https://github.com/strimzi/strimzi-kafka-operator/issues/12513), fixed in [#12675](https://github.com/strimzi/strimzi-kafka-operator/pull/12675)), so 048's check fell through and the failing per-broker Admin API probe escalated to a force-restart once the backoff was exhausted. | ||
| That fix lands independently of this proposal. |
There was a problem hiding this comment.
| That fix lands independently of this proposal. | |
| That fix has already landed. |
| That turned out to be a bug: proposal [048](https://github.com/strimzi/proposals/blob/main/048-avoid-broker-restarts-when-in-recovery.md) makes the roller stop on `RECOVERY`, but the agent request failed and returned `BrokerState(-1)` ([#12513](https://github.com/strimzi/strimzi-kafka-operator/issues/12513), fixed in [#12675](https://github.com/strimzi/strimzi-kafka-operator/pull/12675)), so 048's check fell through and the failing per-broker Admin API probe escalated to a force-restart once the backoff was exhausted. | ||
| That fix lands independently of this proposal. | ||
|
|
||
| A human lever is still needed for the cases 048 cannot see: a node not-ready for a non-`RECOVERY` reason (a degraded-but-alive disk, a hung mount) where the agent reports nothing actionable and a force-restart only reschedules the pod into a crash loop (`Multi-Attach` on `ReadWriteOnce` storage). |
There was a problem hiding this comment.
I think either in here or in the proposal section, it would be useful to explain more about these scenarios and how much skipping a node from rolling would exactly help, where reconciliation pause cannot.
There was a problem hiding this comment.
Thank you for pushing on this — the motivation is much stronger for it. Expanded in dd4dbf3. The concrete case: a broker on a host with a failing-but-alive disk, and the replacement depends on datacenter ops — days, not hours. During that window every reconcile force-restarts the NotReady broker, and on ReadWriteOnce storage the recreated pod can land in a Multi-Attach crash loop. Holding pause-reconciliation for days stops cert rotation, config, and scaling for the whole cluster; a skip holds one node while everything else keeps reconciling — the difference is duration.
I also added two more scenario classes (on-host storage operations, live forensics, human-gated staged rollout) and the precedent: StatefulSets expose this per-node hold as spec.updateStrategy.rollingUpdate.partition; StrimziPodSets have no equivalent.
Thanks @tinaselenge for your comments and deep insight. |
Maybe making that clearer in motivation section could help because this sentence here seems to suggest that skip node is also short window, so I wasn't sure how long we are talking about. |
@tinaselenge You're right, and thank you for reading closely enough across rounds to catch this — the proposal text was wrong, not your reading. "Short-lived" should have been "bounded by the intervention", which can be days. The real constraint is events, not time: a skip must not be held across a version upgrade or CA renewal. Rewritten in dd4dbf3: no hard time limit, a version rollout is not initiated while a skip exists, and a standing warning covers the rest. The timescale is now in the motivation too. The same push takes your complexity concern seriously rather than arguing with it: partition-risk reporting is cut (Kafka's |
- Apply suggested wording: the #12675 fix has already landed - Expand motivation with a concrete days-long hardware-swap scenario and the duration contrast with pause-reconciliation - Add two further scenario classes (on-host storage operations, live forensics, human-gated staged rollout) and the StatefulSet updateStrategy partition precedent - Replace the contradictory 'short-lived' wording: skip duration is bounded by events (version upgrade, CA renewal), not wall-clock time - Trim the observability surface: drop operator-computed partition-risk reporting (Kafka metrics are authoritative), per-cause conflict detection, the KafkaNodePool status mirror, and per-node start times - Add the Affected/not affected projects section from the template - Tighten the bug backstory, controller rationale, and test wording Signed-off-by: Haifeng Chen <haifengc@twitter.com>
dd4dbf3 to
cce1105
Compare
| "Automatic" rolling means a roll the operator decides on its own: config change, certificate renewal, version upgrade, or the not-ready / force-restart path. | ||
| It is distinct from an *explicit* roll a human triggers with `strimzi.io/manual-rolling-update`, which is deliberately **not** suppressed (see "Interaction with manual rolling update"). |
There was a problem hiding this comment.
I think this should be rephrased. Currently, the certificate renewal is the only thing the operator decides on its own. The rest are all user decisions. You change the config, you do the upgrade, etc. I think the intention is good here and not rolling in these cases makes sense. Just the wording is wrong.
| ``` | ||
|
|
||
| The value reuses the node-ID format of the existing `strimzi.io/next-node-ids` / `strimzi.io/remove-node-ids` annotations (IDs and ranges, e.g. `[2,5]` or `[2,4-6]`). | ||
| It lives on the `KafkaNodePool` rather than the `Kafka` CR because node IDs are owned by the pool; it is set and removed by the user, so it survives pod recreation and GitOps re-apply. |
There was a problem hiding this comment.
Putting this on the NodePool rather then on the Kafka CR surely impacts the implementation (at this point I'm not sure if it makes the implementation easier or harder TBH). You should share some implementation details on how it will be read, stored, etc. by the KafkaRoller.
I also wonder if it is confusing to put it there. You are right that the NodePool owns the node IDs. But the setup where you put strimzi.io/skip-rolling-update="[5]" on a wrong node pool and it is ignored is strange. It also requires more complex validation (does this ID exist + does it belong to this node pool).
| Drain Cleaner is the awkward case: it is automated (so by that rule it *should* honor the skip) but it triggers rolls through the manual-rolling-update path, which the initial implementation of this feature cannot cleanly intercept. | ||
| So a drain can still move a skipped node; while a node is skipped, exclude it from Drain Cleaner and cordon its host. | ||
| A first-class Drain Cleaner integration that honors the skip can be added as a follow-up. |
There was a problem hiding this comment.
Why would you drain a node with the Pod where you do some manual maintenance? I do not think this needs any special handling. You should just not drain the node and if you do, it will be rolled 🤷
| - It must not advance cluster-wide state past the held node: a cluster-wide Kafka version / `metadata.version` change is deferred while any node is skipped, since the skipped node stays on its old version and finalizing the upgrade without it could leave it unable to rejoin. | ||
| Only the upgrade rollout is held: all other reconciliation, including rolls of other nodes for other reasons, continues, and the deferral is logged and surfaced in the status condition. |
There was a problem hiding this comment.
TBH, this is an expert option that you should use for some manual emergency handling. I do not think it should have any special treatment for upgrade or metadata version updates. If you do upgrade while the node is skipped, it either works or not (depending on the in-sync partitions). It should be your responsibility to not update metadata versions or upgrade Kafka when in some emergency state. So I think this should be (similar to the Drain Clener) a documentation thing.
| This kind of per-node hold is an established control for stateful workloads on Kubernetes: StatefulSets expose an ordinal-based version of it as `spec.updateStrategy.rollingUpdate.partition`, bounding which pods a rollout may touch. | ||
| StrimziPodSets have no equivalent; this annotation supplies one. | ||
|
|
||
| The skip does **not** promise that the rest of the cluster keeps rolling freely: when a skipped broker is actually down, a roll of another broker sharing an at-risk partition is deferred by the existing min-ISR check, exactly as any unsafe roll is today. |
There was a problem hiding this comment.
I think this is a bit misleading ... in most clusters, taking the node down would fail the rolling update the and cluster anyway because the partition replicas would be missing (with the typical min.insync.replicas 2 and replication factor 3, you cannot really take down a node easily).
This annotation really only ensures the cluster remains ready until the next rolling update where it fails. And if the node is unavailable for a long time, the state of the cluster might become more and more inconsistent.
This should be clearly documented here as well as in the docs if/once approved and implemented:
- You should keep this as short as possible
- You should avoid any changes to the cluster while the node rolling is skipped as they would not work well anyway
6 participants
Type of Change
Description
Adds proposal 146: an opt-in, per-node control that excludes a single Kafka node from operator-driven rolling, while the operator keeps reconciling every other node in the cluster.
Today the only pause control is
strimzi.io/pause-reconciliationon the wholeKafkaCR, which is cluster-wide and far too coarse: to protect one node you must freeze config, cert rotation, scaling, and rolling for the entire cluster. This proposal adds a scalpel — stop the operator from rolling one node, and only that node.Highlights:
strimzi.io/skip-rolling-update) fast path and a durableKafkaNodePool.spec.skipRollingUpdatefield.KafkaRoller(not aRestartReasonsfilter), so a skipped node is never a roll candidate and the reconcile never waits on its readiness.SkipNodeRollingUpdatefeature gate, default off.Checklist