fix: exclude capacity.json from Cruise Control pod-template hash by default - #303
fix: exclude capacity.json from Cruise Control pod-template hash by default#303gomitrah wants to merge 1 commit into
Conversation
amuraru
left a comment
There was a problem hiding this comment.
The problem analysis here is correct — the CC-roll ↔ in-flight-operation race in #301 is real, and the two-controller root cause is accurately described. But the chosen fix (stop hashing capacity.json into the CC pod template, so a capacity change no longer rolls CC) fixes broker removal while breaking broker add (upscale). Requesting changes.
Why the flip breaks upscale
The CC roll that this PR removes is also what loads a newly-added broker's capacity into a running CruiseControl. CC does not pick it up any other way:
BrokerCapacityConfigFileResolverreadscapacity.jsononce at startup (configure()); it does not watch/reload the file.- When a broker is missing from the loaded map,
capacityForBrokerdoes not estimate from peers — it returns the broker-id-1default entry, and if there is no-1entry it NPEs (capacitiesForBrokers.get(-1).capacity()onnull); withallowCapacityEstimation=falseit throwsBrokerCapacityResolutionException:
// BrokerCapacityConfigFileResolver.capacityForBroker (~L169-190)
BrokerCapacityInfo capacity = capacitiesForBrokers.get(brokerId);
if (capacity != null) return capacity;
if (allowCapacityEstimation) {
return new BrokerCapacityInfo(capacitiesForBrokers.get(DEFAULT_CAPACITY_BROKER_ID).capacity(), ...); // -1 default; NPE if absent
} else {
throw new BrokerCapacityResolutionException(...);
}- koperator's
GenerateCapacityConfig(pkg/resources/cruisecontrol/configmap.go) emits a-1default only if the user suppliesspec.cruiseControlConfig.capacityConfigwith one; by default it generates per-broker entries with no-1. None of the shipped samples set a-1except the kitchen-sinkconfig/samples/banzaicloud_v1beta1_kafkacluster.yaml(and that one is a non-JBOD scalarDISK, not the per-log-dir map JBOD/KRaft brokers use). The KRaft samples and every e2e sample have no-1.
Consequence: with this change, adding a broker no longer rolls CC, so the running CC (started before the new broker) never learns the new broker's capacity — not in its startup capacity.json, no live reload, no -1 default → capacityForBroker(newBroker) NPEs / throws, CC can't build a model that includes the broker, and the upscale stalls with no roll to recover. That's a regression versus master, where the capacity-driven roll makes a fresh CC read the new broker's exact per-broker entry. Removal is unaffected (the departing broker leaves cluster metadata, and the remaining brokers are already in CC's startup capacity.json) — which is why a remove-only test passes and hides this.
Suggested direction
Keep the capacity-driven roll (so CC always gets exact per-broker capacity, with no dependency on estimation or a -1 default), and instead sequence it against operations:
- gate the
CruiseControlOperationReconcilerso it does not submit/track anadd_broker/remove_brokerwhile the CC Deployment is mid-rollout (not settled to a single Ready replica — theisCruiseControlDeploymentRolledOutcheck already used in the e2e), and - avoid rolling CC while a scaling operation is in flight,
so the ordering becomes: capacity change → roll CC → CC settles/re-warms → submit op → op runs against a stable CC. This preserves exact capacity for homogeneous and heterogeneous clusters and removes the estimation/-1 dependency entirely.
For transparency: the other #301 PR (#304) took this same flip approach and is being reworked to the sequencing approach above for the same reason. Whichever lands should include an add_broker e2e, since a remove-only scenario does not exercise the broken path.
|
dupe of #304 |
Description
Fixes #301
Two independent controllers race during broker scaling:
capacity.jsonis regenerated on every broker add/remove and, by default, its content ishashed into the Cruise Control Deployment's pod-template annotations
(
GeneratePodAnnotationsinpkg/resources/cruisecontrol/deployment.go). Any hash changecauses Kubernetes to roll the Cruise Control pod.
CruiseControlOperationReconcilersubmitsadd_broker/remove_brokeroperations basedsolely on Cruise Control's own REST API readiness (
CruiseControlStatus.IsReady()), with noawareness of the Deployment's rollout state.
When these overlap, the fresh Cruise Control pod loses the in-memory task for an in-flight scale
operation. The operation is marked errored and retried after a fixed delay plus however long CC
takes to re-warm its metrics window — stalling scaling for anywhere from seconds to minutes.
Verified this is reachable on current
master, independent of #300 (a KRaft-hardening PR whoseonly CC-adjacent change is unrelated node filtering in
remove_broker) — confirmed via a Go testdriving the real
GenerateCapacityConfig+GeneratePodAnnotationsfunctions for a 3→4 brokerscale-up, showing the pod-template hash changes purely from the broker-count change.
capacity.jsonis only read by Cruise Control at process startup, and the operator alreadyfalls back to CC's own capacity estimation for scale operations, so a capacity.json change does
not require restarting a running CC pod.
GeneratePodAnnotationsnow excludescapacity.jsonfrom the pod-template hash by default, inverting the polarity of a pre-existing (undocumented,
never-set-by-default) escape hatch:
Operators who need exact (non-estimated) capacity for newly added brokers immediately, and are
willing to accept the restart-race trade-off, can opt back in by setting
cruise-control.banzaicloud.com/broker-capacity-config: "static".Upgrade note: every existing cluster has this annotation unset today, so the old code always
hashed
capacity.json. The first reconcile after upgrading will drop thecruiseControlCapacity.jsonpod-template annotation, which Kubernetes will apply as oneDeployment update — i.e. every existing cluster gets exactly one Cruise Control pod restart on
upgrade. This is expected and one-time, not a bug (see the upgrade-transition unit test).
Scope: this closes the specific
capacity.jsontrigger described in #301. The other threeconfig sources hashed here (
cruisecontrol.properties,clusterConfigs.json,log4j.properties) still unconditionally roll the Deployment on change and can still race anin-flight operation via the same mechanism — that's a separate, larger fix (giving the operation
controller Deployment-rollout awareness) and is intentionally out of scope here.
Type of Change
Checklist
Testing
pkg/resources/cruisecontrol/deployment_test.go): default/unset, nil map, exact"static"opt-in, near-miss values (empty string, wrong case, unrelated value), passthrough ofunrelated annotations, the exact scale-up scenario from CruiseControl restart races in-flight add_broker/remove_broker operations (scaling stalls during CC roll) #301, the static opt-in still
restarting on change, and the one-time upgrade transition.
pkg/resources/cruisecontrol/cruisecontrol_reconcile_test.go): drivesthe real
Reconciler.Reconcileagainst a fake Kubernetes API to confirm the static opt-insurvives a full reconcile pass.
controllers/tests/kafkacluster_controller_test.go): new scenario exercising thestatic opt-in through the real controller + real API server, alongside updated assertions in
kafkacluster_controller_cruisecontrol_test.gofor the new default.go build ./...,go vet ./...,gofmt, andgolangci-lintall clean (pre-existinggoconstfindings inconfigmap.gounrelated to this diff, confirmed present onmastertoo).pkg/resources/cruisecontrol,controllers,controllers/tests,controllers/tests/clusterregistry,controllers/tests/contourwatch,pkg/scale) run beforeand after the change — pass/fail status identical except for the intentional assertion updates
above; no other workflow regressed.
Before fix: scale-up → capacity.json changes → CC pod restarts → in-flight operation lost →
stalls for 30s+ retry delay plus CC re-warm time.
After fix: scale-up → capacity.json changes → CC pod template unchanged → no restart →
operation proceeds uninterrupted.