fix: KRaft controller pods are now dynamically added/removed from the voter list - #1007
Draft
razvan wants to merge 16 commits into
Draft
fix: KRaft controller pods are now dynamically added/removed from the voter list#1007razvan wants to merge 16 commits into
razvan wants to merge 16 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>
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