fix: KRaft controller pods are now dynamically added/removed from the voter list - #1007
Closed
razvan wants to merge 30 commits into
Closed
fix: KRaft controller pods are now dynamically added/removed from the voter list#1007razvan wants to merge 30 commits into
razvan wants to merge 30 commits into
Conversation
Covers admitting/removing controllers from the KRaft dynamic quorum's voter set on scale-up/scale-down, via a self-managed sidecar container rather than operator-side kube-exec. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- clarify kraft_mode_cluster() fixture is owned by Task 5, not Task 3/4 - add missing test coverage for the Kerberos-disables-sidecar global constraint Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Compute a comma-joined host:port list of KRaft controller voters in build_controller_rolegroup_statefulset, reusing the existing pod_descriptors(...) call instead of calling it twice. Stored as a local variable (currently _-prefixed, unused) for Task 4 to consume when building the controller sidecar container's env vars. Test-feasibility note: the brief assumed KafkaPodDescriptor's pub(crate) fields require the test to live inside crd/mod.rs. That's incorrect: pub(crate) is crate-wide visibility, not module-scoped, so KafkaPodDescriptor can be constructed directly from any module in this crate. Added a real unit test for kraft_controllers (the existing pure join helper, previously untested) in controller/build/properties/mod.rs, constructing KafkaPodDescriptor values via NamespaceName/StatefulSetName/ServiceName's FromStr and DomainName::try_from, and asserting the comma-joined host:port output while confirming non-controller roles are filtered out. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address review findings on the quorum-manager sidecar (Task 4): Critical: - Mount the internal TLS volume (STACKABLE_TLS_KAFKA_INTERNAL_VOLUME_NAME) on the sidecar in addition to the config volume. controller_admin_client_ properties always sets security.protocol=SSL and points its keystore/ truststore at that directory, so without this mount every add-controller/ remove-controller call failed SSL init and the feature was a no-op. - Wrap every kafka-metadata-quorum.sh invocation in the preStop script with timeout 15, so a hung admin-client call can no longer burn into terminationGracePeriodSeconds regardless of the script's own 25s budget. - Fix the preStop majority guard to only count describe --replication rows whose Status is a recognized voter value (Leader/Follower), excluding Observer rows, instead of treating every non-header row as a voter. If no recognized voter rows are found (e.g. a real column-layout mismatch), the check now fails closed (retries, never removes) instead of failing open. Important: - Log an explicit diagnostic when the main-loop's metrics scrape yields an empty/unrecognized Raft state, instead of looping silently forever. - Fix the preStop loop so the "would break quorum majority" branch actually retries within the 25s deadline instead of breaking out after one attempt. - Add a unit test asserting the sidecar mounts every directory referenced by admin-client.properties (config + internal TLS), and a unit test guarding that the add_common_kafka_env signature refactor (passing a pre-computed node_id_offset instead of computing it internally) left the broker's own NODE_ID_OFFSET env var value unchanged. Minor: - Removed the now-stale allow(dead_code) on supports_dynamic_quorum. - Fixed the quorum_manager_pre_stop_command doc comment, which documented a non-existent node_id parameter. - Moved the "must be confirmed against a live cluster" caveat about describe --replication's column layout out of the shipped bash script and into a Rust doc comment; dropped the internal-planning-doc reference. - Named the sidecar's container name via a const instead of an allocated String. - Guard against an empty POD_INDEX in the preStop script before the arithmetic that derives REPLICA_ID. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Serializes scale-down so each controller's preStop hook (self-removal from the KRaft voter set) completes before the next pod terminates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…uster sidecar bugs found while verifying
- tests/templates/kuttl/operations-kraft/{60,70}-assert.yaml.j2: insert the
missing '---' document separator between the broker and controller
StatefulSet assertions (the second document was silently dropped), and add
a voter-count assertion using kafka-metadata-quorum.sh describe
--replication, filtered to Leader/Follower rows (Observer rows include
broker nodes, which also replicate the metadata log but aren't voters).
- rust/operator-binary/src/controller/build/command.rs: two real bugs found
and fixed by running the operator against a live minikube cluster
(closing out Task 4's deferred describe --replication verification):
1. quorum_manager_container_command ran as the container's PID 1 with no
signal trap, so it never noticed SIGTERM (the kernel suppresses the
default action of unhandled signals for PID 1). Confirmed live: the
kafka container in the same pod shut down promptly while this sidecar
kept looping every ~15s until Kubernetes gave up and force-killed it
after the full 1800s terminationGracePeriodSeconds, holding the whole
pod (and the controller StatefulSet's scale-down) well past kuttl's
step timeout. Fixed with a TERM trap plus an interruptible
'sleep 10 &' / 'wait ' pair.
2. add-controller was always failing with 'node.id not found in
configuration file' because it was pointed at the plain
admin-client.properties, which has no node.id. add-controller reads
node.id/listeners from the same --command-config file it connects
with, to build the voter registration payload — but the rendered
controller.properties has no bare security.protocol/ssl.* (only
listener.name.<name>.ssl.*-prefixed ones), so using it alone would
leave the AdminClient unable to reach the TLS-only bootstrap
controller. Fixed by rendering controller.properties (same
REPLICA_ID derivation as the kafka container's own entrypoint) and
concatenating it with admin-client.properties into a merged config
used only for add-controller. Confirmed live: 'Added controller
<id> with directory id ... and endpoints: ...' after the fix, versus
'Timed out waiting for a node assignment' / 'node.id not found'
before it.
Both fixes are backed by real describe --replication output and pod/sidecar
logs observed on a live minikube cluster; see the task report for the full
transcript. A full kuttl run of operations-kraft_kafka-kraft-4.2.1 was
in progress validating steps 20/25/30/50/60 (60's new voter-count assertion
included) when the run was stopped before reaching a final PASS/FAIL for the
whole suite — see report for exact status.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
razvan
changed the title
… assertions Fix round for review findings on commit 58d2743 (Task 7). All 9 findings addressed: Critical: - The quorum-manager sidecar was only given POD_NAME/NODE_ID_OFFSET, but its own controller.properties render needs ROLEGROUP_HEADLESS_SERVICE_NAME, NAMESPACE, CLUSTER_DOMAIN and KAFKA_CLIENT_PORT too, so its `listeners` value was most likely broken. Extracted controller_pod_shared_env_vars() (statefulset.rs) shared by the kafka container and the sidecar (including envOverrides), so they can't drift apart again. Added a regression test that renders the real controller.properties and asserts every ${env:...} placeholder it references has a matching env var on the sidecar. - The sidecar's render/merge preamble (cp/config-utils template/cat) had no error handling, so a failure would silently start the retry loop with a stale config. Scoped 'set -e' to just the preamble (with 'set +e' after) so it crash-loops loudly instead. Also added the same '[ -n "$POD_INDEX" ] || exit 0' guard the preStop script already had to the main loop's REPLICA_ID derivation. Important: - Reversed the add-controller config concatenation order (controller.properties first, admin-client.properties last) so the client TLS config always wins on a key collision by construction, not by accident of there being no overlap today. Updated the doc comment to explain why the order matters. - Added --max-time 5 --connect-timeout 2 to the metrics curl call in the main loop, so an unresponsive (not refused) connection can't block the SIGTERM-trap fix's prompt shutdown indefinitely. Minor: - Extracted DERIVE_POD_INDEX/EXPORT_REPLICA_ID shared constants for the REPLICA_ID derivation duplicated 4x across command.rs. - Renamed two tests that only check literal command-string contents but were named as if they verified runtime behavior. - Added comments noting the kuttl assert 'timeout: 30' field is known-inert for TestAssert commands, and that the hardcoded :9093 port couples to the test fixture's default TLS config. - Fixed a doc-comment typo ('kept report as' -> 'kept reporting as'). Verified with cargo build/test/clippy/fmt --check (no live kuttl run, per instruction - a live run already validated the underlying bugs this fix round hardens against). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r membership Fixes 5 issues found in the final review of the 9-task KRaft dynamic voter membership plan: 1. Drop Kafka 3.7.x support entirely (scope change authorized by human partner): delete supports_dynamic_quorum and its tests, remove the 3.7.x match/branch from initial_controllers_command (dropping the now-unused product_version parameter, which cascaded through broker_start_command/broker_kafka_container_commands/ controller_kafka_container_command since it was threaded through solely for this), ungate build_quorum_manager_container on Kerberos alone, delete the obsolete 3.7 sidecar test, and update kraft-controller.adoc with a clear 3.9.0+ minimum-version statement and undefined-behavior warning for unsupported versions. 2. Fix the preStop majority guard, which was backwards: it computed a majority threshold from the *pre-removal* voter count and blocked the last safe removal of a 2-voter quorum (2 -> 1), leaving a dead quorum with only 1 live member. Removing a departing voter can only ever lower the majority threshold for the remaining set, and remove-controller itself can't corrupt anything if peers are unreachable (the call just fails). The only real invariant is "never remove the last voter" - replaced the condition accordingly and updated the doc comment, log message and tests. 3. Raise the quorum-manager sidecar's resource limits: kafka-run-class.sh defaults KAFKA_HEAP_OPTS to -Xmx256M when unset, which could already exceed the old 128Mi memory limit before JVM/metaspace/SSL overhead. Set an explicit -Xmx128M, raise memory to 256Mi request / 512Mi limit, and raise CPU to 500m limit so a JVM cold start, SSL handshake and admin round-trip fit inside the sidecar's existing 15s/25s timeouts. 4. Stop the sidecar's render/merge preamble from crash-looping the container: with OrderedReady pod management now applying to every non-Kerberos controller StatefulSet, a crash-looping sidecar makes its pod NotReady and blocks scale/update progress for every sibling pod in the role. The preamble's inputs are static, operator-rendered config, so retrying won't help a genuine misconfiguration. On failure it now falls into a degraded loop that logs a clear error every 30s and never attempts add-controller, keeping the container Running while staying visible via kubectl logs. 5. Document the --initial-controllers scale-up procedure as a known, tracked risk in kraft-controller.adoc's Known Issues: new controllers are formatted with the full desired voter set rather than Kafka's documented join-existing-quorum procedure. Confirmed working in manual live testing but not yet verified end-to-end through the fully automated sidecar path. Also (optional fixes from the review): - config_map.rs: added a dedicated ClientProperties error variant, used by both client.properties and admin-client.properties serialization, instead of reusing the misleading JvmSecurityPropertiesSnafu context. - Added a targeted assertion that the quorum-manager sidecar carries NODE_ID_OFFSET, consumed directly by its EXPORT_REPLICA_ID bash logic under set -u. Verified: cargo build/test/clippy/fmt all clean, make regenerate-charts produces no diff (no CRD schema changes in this fix wave). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ecar's job
Per explicit request: adding a new controller to the KRaft voter set is
now solely the concern of the quorum-manager sidecar.
- Remove KafkaPodDescriptor::as_voter(), to_initial_controllers(), and
initial_controllers_command() entirely.
- Exactly one controller (lowest node_id among all controller pod
descriptors, stable across replica-count changes of an existing role
group) formats with kafka-storage.sh format --standalone, bootstrapping
a single-node quorum by itself. Every other controller -- whether part
of the initial desired replica count or added later on scale-up --
formats with --no-initial-controllers and joins purely through the
sidecar's existing add-controller loop. Brokers always format with
--no-initial-controllers too (never voters, never the bootstrap
candidate).
- Found and fixed, via live testing, a second instance of the same
pod-template-churn problem: the sidecar's own command also baked in the
full peer list as a Rust-level literal, so it changed with replica
count too. Fixed by reading controller.quorum.bootstrap.servers at
runtime from the already-mounted, already-fully-resolved (no
${env:...} placeholders) controller.properties ConfigMap file instead.
- Found and fixed, via live testing, a genuine previously-undiscovered
blocking bug: the quorum-manager sidecar never mounted the data volume,
so add-controller could never read its own meta.properties to identify
itself -- every attempt failed with "Unable to read meta.properties
from /stackable/data/kraft". The feature could not have worked at all
without this. Confirmed live after the fix: both joiners log "Added
controller ... with directory id ..." and reach follower/leader state.
- Confirmed live: the standalone bootstrap node self-heals correctly
after an unplanned restart (removes itself via preStop, rejoins via
add-controller once back up) -- validates the design's resilience
under a real, non-scale-down termination.
- Confirmed live (and locked in with a regression test): the container
command for both the kafka container and the sidecar is now identical
regardless of replica count. A platform-level ConfigMap-restarter
mechanism (unrelated to this change, ubiquitous across Stackable
operators) still restarts every controller pod on scale today, because
controller.quorum.bootstrap.servers still legitimately changes content
with replica count -- documented as a known issue rather than chased
further, per explicit instruction.
- Clean up now-genuinely-dead 3.7.x jinja guards in the operations-kraft
kuttl test templates and README (3.7 support was fully removed in the
prior commit; these were stale leftovers).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Root cause, confirmed both in isolation and live: GNU `timeout N cmd`
without `--kill-after` only *sends* SIGTERM once N seconds elapse -- it
does not force-kill `cmd`, so if the wrapped process doesn't honor the
signal promptly, the call can run far longer than N seconds.
Confirmed directly, independent of Kafka:
$ time timeout 3 bash -c 'trap "" TERM; sleep 30'
real 0m30.004s # not 3s
$ time timeout --kill-after=2 3 bash -c 'trap "" TERM; sleep 30'
Killed
real 0m5.001s # correctly bounded
Confirmed live: during a full namespace deletion (every controller
terminating concurrently, so a peer's describe/add-controller/
remove-controller call can hit a blackholed rather than actively-refused
connection), a controller's quorum-manager sidecar kept running well past
its own preStop script's ~25-40s design budget, because none of its
`timeout 15 kafka-metadata-quorum.sh ...` calls were actually bounded to
15 seconds.
Fix: add `--kill-after=5` (CLI_CALL_KILL_AFTER_SECONDS) to every
kafka-metadata-quorum.sh invocation in both the sidecar's main loop
(add-controller) and its preStop hook (describe/remove-controller), so a
hung call is now genuinely force-killed shortly after its nominal
timeout, regardless of what the JVM does internally.
Verified live after the fix: a controller pod's own SIGTERM-to-fully-gone
transition is now a few seconds, not 100+. The residual multi-minute
total for a full 3-controller namespace teardown is now dominated by a
separate, distinct cause (OrderedReady serializing pod-by-pod deletion
even during full teardown, compounded by apparent slowness in this
long-running test environment) -- reported separately, not fixed here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ervice controller.quorum.bootstrap.servers is required by the real Kafka server process (both kafka and broker containers) whenever using KRaft's dynamic quorum mode -- it's not removable, per kafka-storage.sh format --help, which requires it in place of controller.quorum.voters for all three dynamic-quorum modes. But its *value* didn't need to enumerate every current controller pod. kraft_controllers() now emits one address per distinct controller role group, pointing at that role group's own headless Service DNS name (no pod prefix) instead of individual pod FQDNs. A headless Service's own DNS name resolves to every backing pod's IP -- exactly what Kafka's own client.dns.lookup=use_all_dns_ips default already expects -- and the operator's headless Service already sets publishNotReadyAddresses: true, so this also resolves correctly during initial cluster formation before any pod is Ready. This also benefits the quorum-manager sidecar, which reads this same ConfigMap value at runtime for its own --bootstrap-controller argument. Removed KafkaPodDescriptor::fqdn()/pod_name(), now genuinely unused (their only remaining caller was the per-pod address construction this replaces). Verified live, both initial formation and scale-up: - A fresh 3-controller cluster bootstrapped correctly end-to-end using only the service-level bootstrap address (no per-pod list at all); kafka-metadata-quorum.sh describe --replication against just the service name correctly reached the leader and showed all 3 as real voters. - Scaling 3->5 left every existing controller pod completely untouched: identical UIDs, zero restarts, and the StatefulSet's currentRevision/ updateRevision hash did not change at all (previously this scale operation restarted every existing controller pod, both via the container-command churn fixed earlier and via the platform's ConfigMap-restarter reacting to this exact property's changing content -- both causes are now eliminated). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Stackable platform's admission control rejects any container whose memory limit-to-request ratio isn't exactly 1, which the operator's own builder had already been warning about (visible throughout this branch's live testing: "memory max limit to request ratio for Container quorum-manager is 1.0, but ratio was exceeded"). The smoke-kraft test cluster enforces this, so the earlier fix that raised the sidecar's memory limit to 512Mi while leaving the request at 256Mi (a 2:1 ratio) made the controller StatefulSet's pods entirely uncreatable there: Warning FailedCreate: pods "test-kafka-controller-automatic-log-config-0" is forbidden: memory max limit to request ratio per Container is 1, but provided ratio is 2.000000 Fixed by setting the request to 512Mi too (matching the limit), rather than lowering the limit -- the original reasoning for 512Mi still holds (a JVM cold start plus SSL handshake plus admin round-trip inside the sidecar's existing timeout/preStop budgets). Added a regression test asserting every container in both the broker and controller pods has memory request == memory limit, so a future container addition can't reintroduce this for either role. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed live on a 3-controller cluster: a controller pod that is the last remaining voter at termination time (scaling controllers down to 1, or the last survivor of a full namespace teardown) hit the "would leave zero voters" branch of the preStop guard, which had no early exit -- it kept retrying every 2s until the full 25s DEADLINE elapsed, for no benefit, since nothing else adds a voter for this pod while it is terminating and the outcome can never change. Compared against a normal removal (2+ remaining voters), which completes in 5-8s: the last-voter case always cost the full 25s before even reaching SIGTERM. Fixed by breaking out of the retry loop immediately once "would leave zero voters" is detected, instead of falling through to sleep/retry. Also confirmed but NOT root-caused or fixed here: in the same live test, the pod continued sitting in Terminating for several more minutes after the preStop hook itself finished. The sidecar's own `trap 'exit 0' TERM` fires instantly once SIGTERM actually reaches it (confirmed by sending it manually via `kubectl exec`), so the extra delay is not caused by this sidecar's signal handling -- something external delayed the SIGTERM being delivered at all in that run. That test bypassed the operator (StatefulSet was patched directly with the operator paused, to capture preStop's own output, which Kubernetes does not surface via `kubectl logs`), so it isn't yet confirmed this reproduces in normal operation; flagging as a known open question rather than guessing at a fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merging in the better KRaft controller readiness/liveness probes from PR #1006 since they're a better fit as part of this dynamic voter membership branch than as a standalone PR. The author will close #1006 separately. # Conflicts: # CHANGELOG.md # docs/modules/kafka/pages/usage-guide/kraft-controller.adoc # rust/operator-binary/src/controller/build/resource/statefulset.rs
Confirmed from a live report: scaling a KRaft cluster's only controller role group down to 0 replicas while brokers kept running passed validation and failed much later, and far more confusingly, while building the *broker* role group's ConfigMap: failed to build ConfigMap for role group default, no Kraft controllers found to build That happened because the broker ConfigMap's render also reads the controller quorum's pod descriptors (for controller.quorum.bootstrap.servers), which come back empty once there are zero controller replicas -- an unrelated role group's build failing as a side effect of the real problem, with a message that doesn't name it. Brokers with zero controllers have no metadata quorum to talk to at all, so this combination is now rejected up front in the validate step, summed across all controller role groups (so e.g. two role groups at 0+0 replicas each are also caught). Deliberately narrower than "controllers can never be zero": controllers *and* brokers at zero together is allowed, since that's exactly what `clusterOperation.stopped` already does today, unconditionally, for every Stackable operator (scaling every managed StatefulSet's replicas to 0 at apply time, bypassing this validate-time check entirely) -- a coordinated whole-cluster stop is already a supported shape, not one this check should forbid. `replicas: unset` (left for a HorizontalPodAutoscaler to own) is not treated as zero for either role. ZooKeeper-mode clusters are unaffected -- their controllers aren't a metadata quorum. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…longer panics/errors The zero-controller-replicas validate check (342e503) allowed controllers and brokers scaled to 0 together, but two downstream build steps still assumed at least one controller pod descriptor existed whenever KRaft mode was active, regardless of that role group's own replica count: - build_rolegroup_config_map hard-errored (NoKraftControllersFound) on any empty pod descriptor list in KRaft mode. - controller_quorum_format_flag panicked via .expect() when computing the lowest controller node id from an empty descriptor list. Both StatefulSets and ConfigMaps are still built for 0-replica role groups (just never scheduled), so a whole-cluster-at-zero build must succeed even with an empty descriptor list. Confirmed via the operations-kraft kuttl test's controller-shutdown step, which scales both roles to 0 in the same apply and previously failed reconciliation with 'no Kraft controllers found to build'. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
Author
|
closing in favor of a new, cleaner and hopefully easier to review pr |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Do not review yet. This is very unpolished and unverified.
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker