[kafka] Migrate from ZooKeeper to KRaft mode - #2352

packages/system/vm-adoption-controller/images/controller/main.go (110-127)

The health check server is running on port 8081, but this port is not defined in the Deployment spec. Ensure the port is exposed in the Deployment to allow probes to function correctly.

packages/system/vm-adoption-controller/images/controller/main.go (406-408)

The controller returns an error if spec.template is missing. Consider if this is too strict for all VM types, or if a more graceful handling (e.g., logging and skipping) is appropriate for partially defined VMs.

packages/system/vm-adoption-controller/images/controller/main.go (522-524)

The controller fails if the generated VMInstance name exceeds 63 characters. Consider truncating the name or adding a hash suffix to ensure the name remains valid while avoiding collisions.

packages/system/forklift-operator/Makefile (1)

15-15: Consider simplifying the namespace document stripping logic.

The Python one-liner is complex and hard to maintain. A more readable approach using yq (if available) or a simpler Python script would improve maintainability.

♻️ Suggested alternative using grep/awk

-	python3 -c "import sys; content=open('templates/forklift-operator.yaml').read(); docs=content.split('---'); result=[d for d in docs if 'kind: Namespace' not in d.strip().split(chr(10))[0:5] or not any(l.strip()=='kind: Namespace' for l in d.strip().split(chr(10)))]; open('templates/forklift-operator.yaml','w').write('---'.join([d for d in result if d.strip() and not any(l.strip()=='kind: Namespace' for l in d.strip().split(chr(10)))]))"
+	python3 -c "\
+import yaml; \
+docs = list(yaml.safe_load_all(open('templates/forklift-operator.yaml'))); \
+filtered = [d for d in docs if d and d.get('kind') != 'Namespace']; \
+open('templates/forklift-operator.yaml', 'w').write('---\n'.join(yaml.dump(d) for d in filtered))" || \
+	awk 'BEGIN{RS="---"; ORS="---"} !/kind: Namespace/' templates/forklift-operator.yaml > tmp && mv tmp templates/forklift-operator.yaml

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/forklift-operator/Makefile` at line 15, The current complex
python3 -c one-liner that edits 'templates/forklift-operator.yaml' should be
replaced with a simpler, maintainable step: either call a short standalone
script (e.g., scripts/remove_namespace_docs.py) or use yq to filter out
documents with "kind: Namespace", and update the Makefile to invoke that
script/command instead of the long inline python command; locate the existing
python3 -c "import sys; content=..." one-liner in the Makefile and swap it for a
readable command that clearly removes YAML documents where kind: Namespace from
templates/forklift-operator.yaml.

packages/apps/vm-import/docs/scripts/adopt-vm.sh (2)

69-69: Use bash parameter expansion instead of sed.

Per shellcheck SC2001, bash parameter expansion is more efficient and avoids spawning a subprocess.

♻️ Suggested fix

-            DISK_REF=$(echo "$disk" | sed "s/^vm-disk-//")
+            DISK_REF="${disk#vm-disk-}"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/vm-import/docs/scripts/adopt-vm.sh` at line 69, Replace the
sed-based strip with bash parameter expansion: in the assignment for DISK_REF
(currently using echo "$disk" | sed "s/^vm-disk-//"), use parameter expansion on
the variable disk (remove the "vm-disk-" prefix via ${disk#vm-disk-}) so you
avoid spawning a subprocess and satisfy SC2001; ensure this is used wherever
DISK_REF is computed from disk.

---

102-102: Use bash parameter expansion for consistency.

Same pattern as line 69 - prefer bash built-ins over external commands.

♻️ Suggested fix

-            SUBNET_NAME=$(echo "$net" | sed 's/^.*\///')
+            SUBNET_NAME="${net##*/}"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/vm-import/docs/scripts/adopt-vm.sh` at line 102, Replace the
external sed call used to set SUBNET_NAME from the net variable with bash
parameter expansion to extract the substring after the last '/' so it matches
the pattern used earlier (same approach as on line 69); update the assignment
that sets SUBNET_NAME to use parameter expansion on net to avoid spawning sed
and keep consistent shell-builtins usage.

packages/apps/kafka/templates/kafkanodepools.yaml (1)

70-84: Controller pool is missing resources specification.

The broker pool (line 50) and combined pool (line 23) include the resources specification via cozy-lib.resources.defaultingSanitize, but the controller pool does not. This may be intentional if controllers have minimal resource needs, but it creates an inconsistency.

💡 Suggested fix to add resources

 spec:
   replicas: {{ $controllerReplicas }}
   roles:
     - controller
+  resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.kafka.resourcesPreset .Values.kafka.resources $) | nindent 4 }}
   storage:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/kafka/templates/kafkanodepools.yaml` around lines 70 - 84, The
controller node pool spec is missing the resources field; add the same resources
stanza used in the broker and combined pools by invoking the cozy-lib helper
(cozy-lib.resources.defaultingSanitize) inside the controller pool spec (the
block under "spec:" for the controller role) so controllers include a resources
section consistent with the other pools; ensure you place the resources entry
alongside replicas/roles/storage in the controller pool definition and use the
same template invocation and parameters as the broker/combined pool.

packages/system/forklift/templates/forklift-cr.yaml (1)

3-5: Consider using templated namespace.

The namespace is hardcoded to cozy-forklift. If the Helm release is deployed to a different namespace, this CR will still target cozy-forklift, which may cause deployment issues or require cross-namespace permissions.

If this is intentional (e.g., the ForkliftController must always be in cozy-forklift), consider adding a comment explaining the constraint.

💡 Suggested change to use release namespace

 metadata:
   name: forklift-controller
-  namespace: cozy-forklift
+  namespace: {{ .Release.Namespace }}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/forklift/templates/forklift-cr.yaml` around lines 3 - 5, The
namespace in the custom resource is hardcoded to "cozy-forklift" which prevents
the CR from following the Helm release namespace; update metadata.namespace to
use the Helm release namespace (e.g., the release namespace template variable)
so the CR is deployed into the same namespace as the release, or if the
controller must always live in "cozy-forklift" add a clear comment explaining
that constraint; target the metadata.namespace field and validate behavior via
the ForkliftController CR (metadata.name: forklift-controller) after change.

packages/system/vm-adoption-controller/images/controller/Dockerfile (1)

13-13: Hardcoded GOARCH=amd64 limits multi-architecture builds.

Consider using build arguments to support multi-arch images (e.g., arm64), which is increasingly common for Kubernetes clusters.

♻️ Suggested improvement for multi-arch support

-RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o controller main.go
+ARG TARGETOS=linux
+ARG TARGETARCH=amd64
+RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build -a -o controller main.go

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/vm-adoption-controller/images/controller/Dockerfile` at line
13, The Dockerfile currently hardcodes GOARCH=amd64 in the build step (RUN
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o controller main.go); change
this to use a build ARG (e.g., ARG TARGETARCH) and substitute GOARCH with that
ARG (or default to amd64) so the build supports multi-arch (arm64, amd64) when
invoked with buildx/--platform; update the Dockerfile to declare the ARG and use
it in the RUN that builds main.go to produce the controller binary.

packages/system/vm-instance-rd/cozyrds/vm-instance.yaml (1)

11-11: Consider adding spec.fullnameOverride to keysOrder for stable UI ordering.

Since fullnameOverride is now in the OpenAPI schema, explicitly ordering it can keep form layout deterministic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/vm-instance-rd/cozyrds/vm-instance.yaml` at line 11, The
OpenAPI schema now includes fullnameOverride but the form ordering (keysOrder)
omits spec.fullnameOverride; add "spec.fullnameOverride" into the keysOrder
array where other spec fields (e.g., "spec.instanceProfile", "spec.resources",
"spec.runStrategy") are listed so the UI renders fullnameOverride in a stable,
deterministic position; touch the keysOrder entry in the vm-instance.yaml schema
and insert spec.fullnameOverride next to the other spec.* keys.

hack/e2e-apps/kafka.bats (1)

3-40: Prefer framework-level teardown over inline cleanup in this E2E test.

Please move the in-test kubectl delete cleanup to shared teardown so cleanup is consistently handled across failures and reruns.

Based on learnings: In E2E bats tests like hack/e2e-apps/harbor.bats, replace inline cleanup in test bodies with framework/global teardown cleanup.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@hack/e2e-apps/kafka.bats` around lines 3 - 40, The test "Create Kafka"
contains an inline cleanup line (kubectl -n tenant-test delete
kafka.apps.cozystack.io $name) which should be removed from the test body and
instead registered in the framework/global teardown used by other E2E tests
(e.g., the pattern used in harbor.bats); update the test to omit the delete, and
add the Kafka resource cleanup (deleting kafka $name in namespace tenant-test)
to the shared teardown hook so cleanup runs even on failures and reruns.

packages/apps/kafka/tests/metrics-configmap_test.yaml (1)

29-35: Add a migration-path test case for ZooKeeper metrics presence.

Current tests cover new-instance KRaft defaults well; consider adding one case that simulates existing ZooKeeper metadata state and asserts data["zookeeper-metrics-config.yml"] is rendered during migration stages.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/kafka/tests/metrics-configmap_test.yaml` around lines 29 - 35,
Add a new test case alongside the existing "it: does not include zookeeper
metrics for new instance" that simulates an existing ZooKeeper metadata state
and asserts the zookeeper metrics config is rendered; specifically, create an
"it" block describing migration with pre-existing ZK metadata and include an
asserts entry using "exists: path: data[\"zookeeper-metrics-config.yml\"]" so
the test expects that key to be present during migration rendering (mirror the
structure used by the existing test but flip the notExists to exists to validate
the migration path).

packages/apps/vm-import/docs/examples/advanced-import.yaml (1)

18-18: Thumbprint format appears truncated in example.

The SHA-1 thumbprint should be 40 hexadecimal characters (20 bytes). The example shows "11:22:33:44:55:66:77:88:99:AA:BB:CC:DD:EE:FF:00:11:22:33" which is only 19 bytes. Consider using a full 20-byte example for accuracy:

-  thumbprint: "11:22:33:44:55:66:77:88:99:AA:BB:CC:DD:EE:FF:00:11:22:33"
+  thumbprint: "11:22:33:44:55:66:77:88:99:AA:BB:CC:DD:EE:FF:00:11:22:33:44"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/vm-import/docs/examples/advanced-import.yaml` at line 18, The
thumbprint value in the advanced-import.yaml example is missing one byte (SHA-1
should be 20 bytes / 40 hex chars); update the "thumbprint" example to a full
20-byte SHA-1 formatted as 40 hexadecimal characters (optionally colon-separated
per the existing style) so the example is accurate—locate the thumbprint field
in the examples/advanced-import.yaml and replace the truncated string with a
complete 20-byte hex thumbprint.

packages/apps/vm-import/docs/examples/monitoring-import.md (1)

168-169: Consider using runStrategy instead of deprecated running field.

Modern KubeVirt versions deprecate spec.running in favor of spec.runStrategy. The README.md in this same package already uses runStrategy correctly (lines 147-149).

📝 Suggested fix for consistency

 # Start VMs
-kubectl patch vm nginx-frontend-01 -n production --type merge -p '{"spec":{"running":true}}'
+kubectl patch vm nginx-frontend-01 -n production --type merge -p '{"spec":{"runStrategy":"Always"}}'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/vm-import/docs/examples/monitoring-import.md` around lines 168
- 169, The patch command shown uses the deprecated VM field spec.running; update
the example to use spec.runStrategy instead by changing the patched payload to
set spec.runStrategy (e.g., "RunStrategyAlways" or the appropriate strategy)
rather than spec.running, ensuring consistency with the README example that
already uses runStrategy and preserving the same resource (vm nginx-frontend-01)
and namespace (production).

packages/system/vm-adoption-controller/images/controller/main.go (4)

119-127: Add timeout context for graceful server shutdown.

Using context.Background() for shutdown means it could hang indefinitely. Consider using a timeout context.

Proposed fix

 		go func() {
 			<-ctx.Done()
-			server.Shutdown(context.Background())
+			shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			defer cancel()
+			server.Shutdown(shutdownCtx)
 		}()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/vm-adoption-controller/images/controller/main.go` around
lines 119 - 127, The shutdown goroutine currently calls
server.Shutdown(context.Background()) which can hang; change it to create a
timeout context (e.g. ctxShutdown, cancel :=
context.WithTimeout(context.Background(), 5*time.Second)), call
server.Shutdown(ctxShutdown) and defer cancel() (or call cancel after Shutdown)
and log any Shutdown error; update the goroutine that references server and ctx
so it uses the new ctxShutdown and cancel to ensure the HTTP server shuts down
within the timeout.

---

111-118: Unchecked error from w.Write.

The return value from w.Write is ignored. While unlikely to fail for small writes, this is flagged by linters.

Proposed fix

 		mux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) {
 			w.WriteHeader(http.StatusOK)
-			w.Write([]byte("ok"))
+			_, _ = w.Write([]byte("ok"))
 		})
 		mux.HandleFunc("/readyz", func(w http.ResponseWriter, r *http.Request) {
 			w.WriteHeader(http.StatusOK)
-			w.Write([]byte("ok"))
+			_, _ = w.Write([]byte("ok"))
 		})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/vm-adoption-controller/images/controller/main.go` around
lines 111 - 118, The health/readiness handlers registered via mux.HandleFunc
(the "/healthz" and "/readyz" functions) ignore the error returned by w.Write;
update both handlers to capture the return value (err := w.Write(...)) and
handle it (e.g., log the error with log.Printf or klog.Errorf) if non-nil so
linters stop flagging the unchecked write error; do this in the anonymous
handler closures that call w.Write and keep the existing
w.WriteHeader(http.StatusOK) behavior.

---

145-163: Consider using controller-runtime for watch-based reconciliation.

The current polling approach with a ticker works but has higher API server load compared to watch-based reconciliation. For production workloads, controller-runtime with informers provides:

• Event-driven reconciliation (lower latency)
• Built-in leader election
• Automatic rate limiting and retries
• Standard metrics and logging

This is a larger architectural consideration for future iterations.

Based on learnings: "Follow controller-runtime patterns and kubebuilder style for Go code."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/vm-adoption-controller/images/controller/main.go` around
lines 145 - 163, The Run method on AdoptionController currently polls with a
time.NewTicker (watchInterval) and calls c.reconcile(ctx), which causes higher
API load; refactor to use controller-runtime patterns: create a
SetupWithManager(ctrl.Manager) for AdoptionController, register
watches/informers for VirtualMachine resources and any related types, implement
Reconcile as the controller-runtime reconcile.Reconciler (move logic from
reconcile into Reconcile(ctx, req)), and use mgr.Start(ctx) (with manager
configured for leader election, rate-limiting and metrics) instead of the
ticker-driven AdoptionController.Run; ensure you remove or deprecate the polling
Run and wire the controller via ctrl.NewControllerManagedBy(mgr).For(...) so
events drive reconciliation.

---

691-696: Silently skipping missing DataVolumes could mask issues.

If a DataVolume is expected but missing, logging a warning and continuing may hide problems. Consider whether this should be an error that triggers rollback, or at minimum log at a higher level.

 		dv, err := c.dynamicClient.Resource(dvGVR).Namespace(namespace).Get(ctx, dvName, metav1.GetOptions{})
 		if err != nil {
-			klog.Warningf("DataVolume %s/%s not found, skipping Helm labeling: %v", namespace, dvName, err)
+			klog.Errorf("DataVolume %s/%s not found, skipping Helm labeling: %v", namespace, dvName, err)
 			continue
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/vm-adoption-controller/images/controller/main.go` around
lines 691 - 696, The loop that fetches DataVolumes (for _, dvName := range
dvNames) currently logs a warning and continues when
c.dynamicClient.Resource(dvGVR).Namespace(namespace).Get(...) returns an error,
which can hide missing expected resources; change this to fail-fast or at least
escalate the handling: either return/propagate an error from the enclosing
function when a DataVolume is missing (stop the Helm labeling and trigger
rollback) or collect missing dvNames and after the loop return a consolidated
error; if you must continue, change klog.Warningf to klog.Errorf and include
namespace, dvName and the error to make the condition visible. Ensure you update
callers to handle the returned error if you choose to propagate.

hack/e2e-apps/vm-import.bats (2)

46-47: Replace fixed sleep with retry-based waiting.

The sleep 5 before kubectl wait is a fragile pattern. Consider using a retry loop or removing the sleep since kubectl wait already handles polling.

-  sleep 5
-  kubectl -n tenant-test wait hr vm-import-${name} --timeout=100s --for=condition=ready
+  # kubectl wait handles polling internally; increase timeout if needed
+  kubectl -n tenant-test wait hr vm-import-${name} --timeout=120s --for=condition=ready

Also applies to: 126-127, 179-180

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@hack/e2e-apps/vm-import.bats` around lines 46 - 47, Replace the fragile fixed
"sleep 5" before the kubectl wait with a retry-based check: instead of sleeping,
loop until the resource exists and is in a state where "kubectl -n tenant-test
wait hr vm-import-${name} --timeout=100s --for=condition=ready" will succeed (or
run kubectl wait in a retry loop with short backoff and a max attempts); update
the three occurrences (the "sleep 5" before the kubectl wait at the shown line
and the two other similar pairs around lines 126-127 and 179-180) to use this
retry logic so the script polls reliably rather than relying on a fixed delay.

---

86-91: Consider moving cleanup to framework-level teardown.

Inline cleanup at the end of tests won't execute if the test fails mid-execution, potentially leaving resources behind. Consider using Bats teardown or teardown_file functions to ensure cleanup runs regardless of test outcome.

# Example teardown approach
teardown() {
  # Clean up any VMImport resources created by tests
  kubectl -n tenant-test delete vmimport.apps.cozystack.io --all --ignore-not-found
  kubectl -n tenant-test delete secret -l app=vm-import-test --ignore-not-found
}

Based on learnings: "In E2E bats tests, replace inline cleanup with framework-level teardown/cleanup. Do not rely on traps or per-test cleanup."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@hack/e2e-apps/vm-import.bats` around lines 86 - 91, Move the inline cleanup
commands at the end of the test into a framework-level teardown (teardown or
teardown_file) so they always run even if the test fails; remove the inline
kubectl -n tenant-test delete vmimport.apps.cozystack.io ${name} and kubectl -n
tenant-test delete secret vmware-credentials-${name} lines from the test body
and instead implement teardown() that deletes VMImport resources and secrets
reliably (use kubectl -n tenant-test delete vmimport.apps.cozystack.io --all
--ignore-not-found or delete by label, and kubectl -n tenant-test delete secret
-l app=vm-import-test --ignore-not-found) and keep the wait/timeout logic in
teardown if needed to ensure resources are cleaned before exiting.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/apps/kafka/templates/kafka.yaml (1)

1-8: ⚠️ Potential issue | 🔴 Critical

Split the KRaft cutover flag from ZooKeeper cleanup.

strimzi.io/kraft must flip to enabled once Strimzi reaches KRaftPostMigration, while spec.zookeeper is only removed after ZooKeeper-related resources are gone. Reusing one $isKRaft flag for both behaviors means this code stalls the migration at migration, and simply broadening the condition would prune ZooKeeper too early. Also, defaulting an existing CR with missing status back to "ZooKeeper" can regress an already-enabled rollout, so the cutover decision needs a fallback to the current resource state when status.kafkaMetadataState is still unset. Please compute separate booleans here and reuse the same helper in the sibling templates. (strimzi.io)

Also applies to: 16-22, 69-88

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/kafka/templates/kafka.yaml` around lines 1 - 8, The template
currently uses a single $isKRaft flag causing premature ZooKeeper removal and
regressions when status is unset; instead compute two booleans (e.g.
$shouldEnableKRaftFlag and $shouldRemoveZookeeperSpec) using the existing
lookup/dig results: use dig "status" "kafkaMetadataState" $existingKafka but
treat an empty status as “unknown” and fall back to the current resource
spec/state (inspect $existingKafka.spec or the existing label/annotation that
indicates KRaft) to decide the cutover flag, while only setting
$shouldRemoveZookeeperSpec when the status explicitly indicates ZooKeeper
resources are gone (e.g. "KRaftPostMigration" or equivalent); replace uses of
$isKRaft with these two booleans and extract this logic into a helper so sibling
templates can call the same decision logic.

api/apps/v1alpha1/kafka/types.go (1)

97-98: Consider adding a drift guard between Go enum values and Helm version map.

To avoid future mismatch between type Version enum values and packages/apps/kafka/files/versions.yaml keys, add a small CI/unit check that compares both sets.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@api/apps/v1alpha1/kafka/types.go` around lines 97 - 98, Add a CI/unit test
that ensures the Go enum type Version in api/apps/v1alpha1/kafka/types.go stays
in sync with the YAML map of supported versions by parsing the YAML file that
lists version keys and comparing it to the set of defined Version constants;
implement a test named TestVersionsEnumMatchesYAML that programmatically
collects all constants of type Version (via reflection or by importing the
package and enumerating known consts) and loads the YAML version keys, then fail
the test if any key is missing from the enum or vice versa, with clear error
messages indicating which values are out of sync.

packages/apps/kafka/templates/kafka.yaml (1)

1-8: ⚠️ Potential issue | 🔴 Critical

The KRaft cutover gate is still too narrow.

Still unresolved: Line 6 falls back to "ZooKeeper", and Line 8 only treats "KRaft" as the KRaft side of the migration. In Strimzi’s migration state machine that can keep strimzi.io/kraft: migration and the ZooKeeper blocks rendered past the cutover point, which risks stalling the final transition. Please treat KRaftPostMigration, PreKRaft, and KRaft as KRaft, and fall back to the current annotation when status is empty; the same duplicated gate exists in metrics-configmap.yaml, workloadmonitor.yaml, and dashboard-resourcemap.yaml.

Strimzi 0.45.x ZooKeeper to KRaft migration: when should the Kafka CR annotation `strimzi.io/kraft` switch from `migration` to `enabled`, and which `status.kafkaMetadataState` values should be treated as the KRaft side of the cutover?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/kafka/templates/kafka.yaml` around lines 1 - 8, The current
KRaft gate in kafka.yaml treats only "KRaft" as KRaft and falls back to
"ZooKeeper" when status is empty; update the logic that sets $metadataState and
computes $isKRaft so that: when $metadataState is empty you read the CR
annotation strimzi.io/kraft as the fallback, and treat any of "KRaft",
"KRaftPostMigration", or "PreKRaft" as KRaft; locate the lookup/dig usage that
assigns $metadataState and the $isKRaft definition and change the comparison to
include those three values (and use the annotation when status is empty); apply
the same fix to metrics-configmap.yaml, workloadmonitor.yaml, and
dashboard-resourcemap.yaml where the identical gate is duplicated.

packages/apps/kafka/tests/kafkanodepools_test.yaml (1)

6-123: Cover the existing-broker branch before merge.

Every case here renders the empty-lookup path, so the split-pool migration branch in packages/apps/kafka/templates/kafkanodepools.yaml:38-85 still has no assertions around document count, controller replica quorum, or controller storage. This PR’s upgrade path depends on that branch; please add direct coverage, or extract the layout decision behind something the suite can drive.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/kafka/tests/kafkanodepools_test.yaml` around lines 6 - 123, The
test suite only exercises the empty-lookup path and misses the existing-broker
(split-pool migration) branch in
packages/apps/kafka/templates/kafkanodepools.yaml (lines ~38-85); add explicit
test coverage in packages/apps/kafka/tests/kafkanodepools_test.yaml that drives
the template into the existing-broker branch (e.g., set a value like
kafka.existingBroker: true or kafka.splitPool: true) and assert the resulting
documents include the split-pool outputs: verify document count, presence/Kind
of the controller KafkaNodePool, controller replica quorum (spec.replicas for
controller), and controller storage settings (spec.storage.* for the controller
pool); alternatively, refactor the template decision into a tunable input (e.g.,
values.kafkanodepools.layout or values.kafka.splitPool) and add tests that set
that input to exercise and assert the split-pool branch.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/apps/kafka/templates/kafkanodepools.yaml (1)

70-74: ⚠️ Potential issue | 🟠 Major

Controller pool still misses resource requests/limits.

The controller-only KafkaNodePool defines replicas and roles but no spec.resources, unlike the other pools. This can cause quota/LimitRange failures and unstable scheduling for the control quorum.

Minimal fix

 spec:
   replicas: {{ $controllerReplicas }}
   roles:
     - controller
+  resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.kafka.resourcesPreset .Values.kafka.resources $) | nindent 4 }}
   storage:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/kafka/templates/kafkanodepools.yaml` around lines 70 - 74, The
controller-only KafkaNodePool is missing a spec.resources block; add a
spec.resources stanza to the controller KafkaNodePool (the same block placement
as in the other node pools) containing requests and limits for CPU and memory
(and any other cluster-required resource fields) so the controller replicas have
guaranteed scheduling and respect quota/LimitRange; copy the resources values
used by the other pools or set controller-appropriate values and ensure units
and keys match (spec.resources.requests and spec.resources.limits).

packages/apps/kafka/Makefile (1)

4-6: LGTM! The test target integrates correctly with the CI discovery script.

The target name matches what hack/helm-unit-tests.sh expects, and since package.mk doesn't define a test target, there's no override conflict.

Minor optional suggestion: consider declaring test as .PHONY to ensure it always runs even if a file or directory named test is ever created:

.PHONY: test
test:
	helm unittest .

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps/kafka/Makefile` around lines 4 - 6, Add a .PHONY declaration
for the Makefile target named test so the target always runs even if a file or
directory called test exists; modify the Makefile to include a line declaring
.PHONY: test followed by the existing test target (the target named "test" that
calls "helm unittest .") so CI discovery behavior is unchanged but the target
cannot be shadowed by a filesystem entry.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes