|
| 1 | +## Context |
| 2 | + |
| 3 | +The koperator manages Kafka cluster lifecycle via a set of reconcilers. Downscale (broker removal) is handled in two phases: |
| 4 | + |
| 5 | +1. **KafkaCluster reconciler** (`pkg/resources/kafka/kafka.go`): Detects brokers removed from spec. Collects all such brokers in one pass and sets them all to `GracefulDownscaleRequired` via a single atomic `UpdateBrokerStatus` call. |
| 6 | + |
| 7 | +2. **CruiseControlTask reconciler** (`controllers/cruisecontroltask_controller.go`): Picks up brokers in `GracefulDownscaleRequired` state and submits CC operations. The `add_broker` path already batches all pending brokers into one CC operation. The `remove_broker` path does not — it picks only the first task and breaks. |
| 8 | + |
| 9 | +3. **External listener reconcilers** (`pkg/resources/envoy/`, `pkg/resources/istioingress/`): Gate broker inclusion in envoy/istio/contour config on `ShouldIncludeBroker()`. This function returns `false` when `brokerConfig == nil` (broker not in spec), causing draining brokers to vanish from external listener config immediately upon spec removal. |
| 10 | + |
| 11 | +**Key invariant:** `GetActiveTasksByOp(OperationRemoveBroker)` only returns brokers in `GracefulDownscaleRequired` state (via `IsRequired()` → `IsRequiredState()`). Brokers already `Scheduled` or `Running` are not returned. Because the KafkaCluster reconciler transitions all removed brokers atomically, all brokers from a single manifest apply are guaranteed to be in `Required` state simultaneously when the CC task reconciler fires. |
| 12 | + |
| 13 | +## Goals / Non-Goals |
| 14 | + |
| 15 | +**Goals:** |
| 16 | +- All broker IDs removed in a single manifest apply are submitted as one CC `remove_broker` operation. |
| 17 | +- Brokers removed from spec remain in external listener config until `GracefulDownscaleSucceeded`. |
| 18 | +- Brokers stuck in `GracefulDownscaleCompletedWithError` or `GracefulDownscalePaused` remain in external listener config (broker still holds data; manual investigation needed). |
| 19 | + |
| 20 | +**Non-Goals:** |
| 21 | +- Guaranteed single-operation batching across multiple sequential manifest applies (best-effort; second apply may produce a second CC operation if first batch is already `Scheduled`). |
| 22 | +- Changes to CRD schema or the CC REST API. |
| 23 | +- KRaft controller-only node handling (controller-only nodes skip CC graceful downscale entirely; unchanged). |
| 24 | + |
| 25 | +## Decisions |
| 26 | + |
| 27 | +### D1: Mirror `addBrokers` pattern for `removeBrokers` |
| 28 | + |
| 29 | +The `addBrokers` helper (lines 366-368) already accepts `[]string` and submits one CC operation. The `removeBroker` helper (lines 370-372) takes a single `string`. |
| 30 | + |
| 31 | +**Decision:** Rename `removeBroker` → `removeBrokers`, change signature to `[]string`, and replace the early-break loop with the collect-all pattern. |
| 32 | + |
| 33 | +**Why not a separate aggregation layer?** The batching boundary is already correct — `GetActiveTasksByOp` returns exactly the set to batch. No new abstraction needed. |
| 34 | + |
| 35 | +### D2: Fix `ShouldIncludeBroker` as the single gatekeeper |
| 36 | + |
| 37 | +`ShouldIncludeBroker` is called by all external listener reconcilers (envoy configmap, service, deployment; istio gateway, virtualservice, meshgateway; contour). When `brokerConfig == nil`, the function currently falls through to `return false`. |
| 38 | + |
| 39 | +**Decision:** Add a fallback block for `brokerConfig == nil`: check the broker's `CruiseControlState` in status. If `IsDownscale() && !IsSucceeded()` and the broker has the requested `ingressConfigName` in its `ExternalListenerConfigNames`, return `true`. |
| 40 | + |
| 41 | +**Why `ExternalListenerConfigNames` check?** A broker may have been associated with a specific ingress config. Re-using the persisted `ExternalListenerConfigNames` (set when the pod was created, never cleared until `GracefulDownscaleSucceeded`) ensures we only retain the broker for the listener configs it actually served. |
| 42 | + |
| 43 | +**Why `IsDownscale() && !IsSucceeded()` instead of an explicit state list?** |
| 44 | +`IsDownscale()` covers all 6 downscale states. Excluding `IsSucceeded()` retains the broker in all non-terminal states, including `CompletedWithError` and `Paused`, which is the desired behavior for manual investigation. If new downscale states are added to the enum in future, they're covered automatically. |
| 45 | + |
| 46 | +**Alternatives rejected:** |
| 47 | +- Fix each caller individually: more code, same logic duplicated. |
| 48 | +- Add a new function: unnecessary indirection; `ShouldIncludeBroker` is the right seam. |
| 49 | + |
| 50 | +### D3: Task order to satisfy CI (green at every commit) |
| 51 | + |
| 52 | +The original plan placed failing tests before implementation. With CI gating on green builds, the order must be: |
| 53 | + |
| 54 | +``` |
| 55 | +Commit 1: Unit test for createCCOperation (passes immediately — tests downstream, not the wrapper) |
| 56 | +Commit 2: Implement removeBrokers + integration test (both green together) |
| 57 | +Commit 3: Implement ShouldIncludeBroker fix + unit test (both green together) |
| 58 | +Commit 4: E2E test + sample manifest |
| 59 | +``` |
| 60 | + |
| 61 | +## Risks / Trade-offs |
| 62 | + |
| 63 | +**[Race: second manifest apply before first batch starts]** → If a user applies a second spec change (removing more brokers) before the first CC operation is created, those new brokers will be included in the same batch (still in `Required` state). If the first CC operation is already `Scheduled`, new brokers get a separate operation. Acceptable; same behavior as `add_broker`. |
| 64 | + |
| 65 | +**[CompletedWithError brokers stay in envoy indefinitely]** → A broker that fails CC draining stays in external listener config. This is intentional (data is still present, clients need connectivity), but operators must monitor and manually recover. No change from current behavior for the envoy side — previously, the broker would have been dropped from envoy even with data present, which was worse. |
| 66 | + |
| 67 | +**[ExternalListenerConfigNames populated assumption]** → The fix assumes `ExternalListenerConfigNames` is non-empty for any broker that was ever reconciled. This field is set on pod creation and never cleared. Clusters created before this field existed would not benefit from the fix for those brokers, but all newly created or recently reconciled brokers are covered. |
| 68 | + |
| 69 | +## Migration Plan |
| 70 | + |
| 71 | +No migration required. Both fixes are backwards-compatible: |
| 72 | +- `removeBrokers` produces the same CC API calls as `removeBroker` for single-broker downscales. |
| 73 | +- `ShouldIncludeBroker` only changes behavior for brokers with `brokerConfig == nil` (already removed from spec); existing behavior for in-spec brokers is unchanged. |
| 74 | + |
| 75 | +Rollback: revert the two commits. No persistent state is affected. |
| 76 | + |
| 77 | +## Open Questions |
| 78 | + |
| 79 | +None — all decisions resolved during exploration. |
0 commit comments