Closes #1902 Migrate Kafka from 3.2.1 + ZooKeeper to Strimzi Operator + Kafka 3.9 KRaft - #1907
abhishek-1809 wants to merge 2 commits into
Conversation
…rator + Kafka 3.9 KRaft Signed-off-by: abhishek-1809 <abhisahu1920@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR removes the legacy Kafka deployment assets and adds a Strimzi-based Kafka setup with broker and controller node pools, a Kafka custom resource, and scripts to install and delete the deployment. ChangesStrimzi Kafka Migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
deployment/v3/external/kafka/install-kafka.sh (1)
30-33: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo
kubectl waitfor node pools before waiting on the Kafka resource.The script proceeds straight to
kubectl wait kafka/kafka --for=condition=Readyafter applying the node pools; if node pool CRDs are slow to be picked up by the operator right after install, this could occasionally race. In practicekubectl waitonReadyshould still succeed since it polls, so this is a minor robustness nit rather than a real defect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deployment/v3/external/kafka/install-kafka.sh` around lines 30 - 33, Add a readiness wait for the node pool resources before the existing kafka/kafka kubectl wait in install-kafka.sh so the script does not race the operator right after applying them. Use the same kubectl wait pattern already used for the Kafka resource, and place it after the node pool apply step and before waiting on Kafka, keeping the existing kafka/kafka wait in place.deployment/v3/external/kafka/kafka.yaml (1)
1-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing
entityOperatorblock.Strimzi's reference examples for KRaft/node-pool deployments consistently include an
entityOperatorsection (topicOperator: {},userOperator: {}) to enable declarativeKafkaTopic/KafkaUsermanagement. Without it, topics/users must be managed manually via Kafka CLI/API. If MOSIP components rely onKafkaTopic/KafkaUserCRs elsewhere in the stack, this will silently not work.♻️ Proposed addition
min.insync.replicas: 2 + + entityOperator: + topicOperator: {} + userOperator: {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deployment/v3/external/kafka/kafka.yaml` around lines 1 - 35, The Kafka Strimzi manifest is missing the entityOperator configuration needed for declarative topic/user management. Update the Kafka spec in kafka.yaml by adding an entityOperator block alongside the existing kafka section, with both topicOperator and userOperator enabled as empty objects, matching the standard Strimzi KRaft/node-pool pattern. Use the Kafka resource definition as the anchor so the new block is placed within spec and not inside kafka.config.deployment/v3/external/kafka/delete-kafka.sh (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo
set -eunlike the install script.
install-kafka.shusesset -ebut this script doesn't. Given most commands here already use--ignore-not-found=trueor|| true, failures are mostly tolerated by design, so this is a minor stylistic inconsistency rather than a functional bug.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deployment/v3/external/kafka/delete-kafka.sh` around lines 1 - 3, This script is missing the same error-handling convention used by install-kafka.sh, so update delete-kafka.sh to include set -e near the top for consistency with the other Kafka deployment script. Keep the existing ignore-not-found/|| true safeguards in place, and ensure the change is applied alongside the existing NAMESPACE assignment in the script entrypoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@deployment/v3/external/kafka/delete-kafka.sh`:
- Around line 1-3: This script is missing the same error-handling convention
used by install-kafka.sh, so update delete-kafka.sh to include set -e near the
top for consistency with the other Kafka deployment script. Keep the existing
ignore-not-found/|| true safeguards in place, and ensure the change is applied
alongside the existing NAMESPACE assignment in the script entrypoint.
In `@deployment/v3/external/kafka/install-kafka.sh`:
- Around line 30-33: Add a readiness wait for the node pool resources before the
existing kafka/kafka kubectl wait in install-kafka.sh so the script does not
race the operator right after applying them. Use the same kubectl wait pattern
already used for the Kafka resource, and place it after the node pool apply step
and before waiting on Kafka, keeping the existing kafka/kafka wait in place.
In `@deployment/v3/external/kafka/kafka.yaml`:
- Around line 1-35: The Kafka Strimzi manifest is missing the entityOperator
configuration needed for declarative topic/user management. Update the Kafka
spec in kafka.yaml by adding an entityOperator block alongside the existing
kafka section, with both topicOperator and userOperator enabled as empty
objects, matching the standard Strimzi KRaft/node-pool pattern. Use the Kafka
resource definition as the anchor so the new block is placed within spec and not
inside kafka.config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7f5bd37e-f8df-4bd4-9288-9b2b341e5e39
📒 Files selected for processing (14)
deployment/v3/external/kafka/README.mddeployment/v3/external/kafka/backup.shdeployment/v3/external/kafka/broker-nodepool.yamldeployment/v3/external/kafka/controller-nodepool.yamldeployment/v3/external/kafka/delete-kafka.shdeployment/v3/external/kafka/delete.shdeployment/v3/external/kafka/install-kafka.shdeployment/v3/external/kafka/install.shdeployment/v3/external/kafka/istio-addons-values.yamldeployment/v3/external/kafka/kafka.yamldeployment/v3/external/kafka/restart.shdeployment/v3/external/kafka/restore.shdeployment/v3/external/kafka/ui-values.yamldeployment/v3/external/kafka/values.yaml
💤 Files with no reviewable changes (9)
- deployment/v3/external/kafka/istio-addons-values.yaml
- deployment/v3/external/kafka/README.md
- deployment/v3/external/kafka/restart.sh
- deployment/v3/external/kafka/ui-values.yaml
- deployment/v3/external/kafka/delete.sh
- deployment/v3/external/kafka/values.yaml
- deployment/v3/external/kafka/restore.sh
- deployment/v3/external/kafka/install.sh
- deployment/v3/external/kafka/backup.sh
…rator + Kafka 3.9 KRaft Signed-off-by: abhishek-1809 <abhisahu1920@gmail.com>
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Maintenance