Skip to content

fix(cruisecontrol): sequence CC rolls around scaling operations (fixes #301) - #304

Open
amuraru wants to merge 3 commits into
adobe:masterfrom
amuraru:cc-restart-race
Open

fix(cruisecontrol): sequence CC rolls around scaling operations (fixes #301)#304
amuraru wants to merge 3 commits into
adobe:masterfrom
amuraru:cc-restart-race

Conversation

@amuraru

@amuraru amuraru commented Aug 11, 2026

Copy link
Copy Markdown

Fixes #301.

Note: This replaces the earlier "flip capacity.json out of the pod-template hash" approach (also proposed in #303). That flip removes the CC roll that loads a newly-added broker's capacity into CruiseControl — CC reads capacity.json only at startup and estimates an unknown broker via a -1 default that koperator does not generate — so it silently breaks upscale. This PR keeps the roll and sequences it instead, with no dependency on capacity estimation or a -1 default.

The race

A broker add/remove regenerates capacity.json, which is hashed into the CruiseControl pod-template annotation and rolls the CC Deployment. The roll (KafkaClusterReconciler) and the add_broker/remove_broker CruiseControlOperation (CruiseControlOperationReconciler) are driven by two uncoordinated controllers, so a roll can land on an in-flight op — the fresh CC pod loses its in-memory task and resets its metric-sampling window, stalling scaling.

For removal it's a chicken-and-egg: marking a broker GracefulDownscaleRequired requires a successful cc.BrokersWithState() (CC REST-ready), but in the pre-downscale window capacity.json is regenerated with a fallback entry for the departing broker, which rolls CC and keeps it un-ready — so the downscale is never marked and no remove_broker op is ever created. This is what stalled the KRaft removal e2e on master.

Fix — sequence the roll (keep exact per-broker capacity)

  1. Reuse capacity.json for the whole removal window, including before the downscale is marked. GenerateCapacityConfig already reuses the deployed config once a broker is in a downscale state; broaden the guard with isBrokerRemovalPending (a broker present in status but dropped from spec) so CC is not rolled during removal → CC stays ready → the downscale is marked → the remove_broker op is created. (This is the load-bearing change.)
  2. Gate the operation controller on a settled CC Deployment (requeueIfCCDeploymentNotRolledOut / isDeploymentRolledOut): defer an add_broker/remove_broker while the CC Deployment is mid-rollout, so an add runs against a settled CC that has already loaded the new broker's exact capacity, and no op is submitted against a rolling CC. Stop-execution is never gated.

No deadlock (asymmetric): removal reuses capacity.json (no roll); an add still rolls CC (new broker in spec, no removal pending) and the op simply waits for that roll to settle. No estimation / -1 dependency, so it's correct for homogeneous and heterogeneous clusters.

Tests

  • Unit: TestIsBrokerRemovalPending, TestIsDeploymentRolledOut.
  • e2e testKRaftBrokerScaling: upscales a KRaft cluster 3→4 brokers (add broker 6) then downscales 4→3, asserting exactly one add_broker then one remove_broker operation, broker-pod count tracks the spec, and the 3 controller-only pods are never touched. It deliberately covers both directions — upscale is the path the flip would have broken; downscale is the path that stalled on master.

Verified locally: go build ./..., go vet -tags e2e ./..., golangci-lint (0 issues), unit tests, and the controllers/tests envtest suite pass.

🤖 Generated with Claude Code

amuraru and others added 3 commits August 12, 2026 11:23
…e#301)

A broker add/remove regenerates capacity.json, which is hashed into the CruiseControl
pod template and rolls the CC Deployment. Because the CC roll (KafkaClusterReconciler)
and the add_broker/remove_broker CruiseControlOperation (CruiseControlOperationReconciler)
are driven by two uncoordinated controllers, a roll can hit an in-flight op: the fresh CC
pod loses the in-memory task and resets its metric-sampling window, stalling scaling.

Worse, for removal it is a chicken-and-egg: marking a broker GracefulDownscaleRequired
requires a successful cc.BrokersWithState() (CC REST-ready), but in the pre-downscale
window capacity.json is regenerated with a fallback entry for the departing broker,
rolling CC and keeping it un-ready - so the downscale is never marked and no remove_broker
operation is ever created.

Sequence the roll instead of removing it (keeps exact per-broker capacity; no dependency on
capacity estimation or a "-1" default):

- Reuse the deployed capacity.json for the whole removal window, including before the
  downscale is marked: isBrokerRemovalPending (broker in status but dropped from spec) now
  also loads the existing ConfigMap so GenerateCapacityConfig short-circuits. CC is not
  rolled during removal -> stays ready -> the downscale is marked and the remove_broker op
  is created.
- Gate the operation controller so it defers an add_broker/remove_broker while the CC
  Deployment is mid-rollout (requeueIfCCDeploymentNotRolledOut / isDeploymentRolledOut),
  so an add runs against a settled CC that already loaded the new broker's capacity, and no
  op is submitted against a rolling CC.

No deadlock: reuse is scoped to removal; an add still rolls CC (new broker in spec, no
removal pending) and the op simply waits for the roll to settle.

Add unit tests: TestIsBrokerRemovalPending, TestIsDeploymentRolledOut.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…be#301)

Upscale a KRaft cluster from 3 broker-only nodes to 4 (add broker 6) and then downscale
back to 3, asserting Cruise Control drives each direction with exactly one add_broker /
remove_broker operation, the broker-only pod count tracks the spec, and the 3
controller-only pods are never touched.

Exercises BOTH directions on purpose: upscale is the path a "stop hashing capacity.json"
fix would have silently broken (the capacity roll loads the new broker's exact capacity),
and downscale is the path that stalled on master. Uses 4-to-3 and back so at least 3
brokers remain for RF=3 / min.insync.replicas=3 during the reassignment, reusing the
existing 3-broker KRaft install.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@amuraru amuraru changed the title fix(cruisecontrol): don't roll CruiseControl on capacity.json changes (fixes #301) fix(cruisecontrol): sequence CC rolls around scaling operations (fixes #301) Aug 12, 2026
@amuraru

amuraru commented Aug 12, 2026

Copy link
Copy Markdown
Author

Reworked from the earlier capacity.json pod-hash flip to a sequencing approach after validating against the CruiseControl source: the flip removes the CC roll that loads a newly-added broker's capacity, and CC resolves an unknown broker via a -1 default (koperator generates none) — so it silently breaks upscale. This version keeps the roll and (1) reuses capacity.json during a pending removal so CC isn't rolled before the downscale is marked, and (2) defers add/remove ops while the CC Deployment is actively re-rolling. Same reasoning applies to #303. Force-pushed; the branch is now 3 commits (fix + e2e + a fixup! that squashes into the fix at merge).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CruiseControl restart races in-flight add_broker/remove_broker operations (scaling stalls during CC roll)

1 participant