operator: manage Broker CRs from the V2 Redpanda reconciler - #1735
operator: manage Broker CRs from the V2 Redpanda reconciler#1735hidalgopl wants to merge 13 commits into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
b29f536 to
78b1f60
Compare
|
Could we also add Telemetry to see what deployments are starting to use the Broker CR? |
896ffb7 to
c676172
Compare
Panel review — four-reviewer pass (staff engineer, documentation engineer, security engineer, Codex adversarial)Verdict: fix-first. The core engineering is careful — the disruptive-op arbitration, observed-not-recorded terminal conditions, rollback ordering, and quiescence guarantees are real and mostly pinned by tests. Two verified defects should be fixed before merge, both in the rollback path. Reviewed: 32 files, +2531/−117 across 7 commits, at the merge-base diff ( Must fix
Should fix
🤖 Generated with Claude Code |
|
Thanks @RafalKorepta - all four are addressed on the branch.
|
Pods created by the Broker controller carried the chart's pod-template labels plus the operator's own, but not the identity labels the StatefulSet controller injects on the pods it creates. External tooling — cloud control planes in particular — selects on those labels: per-broker Service selectors and ordinal fieldRefs read statefulset.kubernetes.io/pod-name and apps.kubernetes.io/pod-index, so a broker-born pod (or a pod rotated in broker mode) was distinguishable from its StatefulSet-created equivalent and silently fell out of such selectors. RenderBrokers now stamps both labels into the Broker pod template, derived from the network index exactly as the StatefulSet controller derives them from the ordinal. Pod-template only — a Broker CR is not a pod, and its non-ordinal name is not a pod name. Template metadata is excluded from the rotation hash, so existing broker-mode pods pick the labels up through the in-place metadata sync without a rotation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extend broker mode — introduced for V1 Clusters by the parent branch — to V2 Redpanda resources. The operator.redpanda.com/use-broker-cr annotation on a Redpanda CR selects Broker CRs as the pod-management flavor for the cluster's lifetime: an annotated existing cluster is migrated in place (per-pool state machine, pods adopted without restarts), a cluster born with the annotation provisions Broker CRs directly, and removing the annotation rolls back to StatefulSets. - The lifecycle pool fetch learns broker mode: pools whose StatefulSet is gone are synthesized from Broker CRs into STS-shaped facades (replicas from decommission intent, readiness from pods, updatedness from Broker.PodOutdated), so the PoolTracker's status, readiness and scale accounting keep working unchanged after handover. Facades are excluded from the STS mutation planners and revision-based rolls. - reconcilePools dispatches per desired pool into the CR-agnostic brokerset engine; rollback runs when the annotation is absent but Broker CRs remain. Engine requeues are stashed, not returned, so every pool reconciles each pass. - Owner-specific behavior moves behind a brokerset.OwnerHooks interface (IsClusterHealthy / OnQuiesced / MigrationBlockedReason) with implementations for the V1 Cluster adapter and the V2 reconciler, replacing the grown-by-accretion function fields. - Brokers of NodePool-owned pools point spec.clusterRef at the NodePool per the RFC ownership table; the Broker controller resolves such Brokers to their cluster through the cluster-name label, and the ref kind is stamped explicitly for the printer column. - NodePools removed from the spec are drained broker-by-broker through the engine's decommission path instead of erroring forever on a missing desired render. - The BrokerMigration condition is aggregated cluster-scoped across pools, so one finished pool never reports Complete while another is blocked; migration progress is reported through a per-pool reporter. - V2 flag V2UseBrokerCR (same annotation key as V1), deliberately kept out of the V2 defaulting bundle so SetDefaults cannot re-add it — removal must stick, since it triggers rollback. - Coverage: envtest integration tests for migration, rollback, the NodePool Broker path and post-migration quiescence; acceptance features for broker-born V2 clusters and STS→Broker migration with scale and rollback, including grant-serialized config rolls and pod-UID stability checks across the migration boundary. - Disruptive operations are arbitrated across pools within a pass, as on the V1 side: one brokerset.Arbitration per reconciliation state, so the one-disruptive-operation-at-a-time gates see marks and grants written microseconds earlier by a sibling pool that the informer cache cannot yet show. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
After broker migration a NodePool has no StatefulSet, so the NodePool controller reported Deployed=False (NotDeployed), zeroed the embedded replica counts, and froze DeployedGeneration even though the pool's brokers were Running and Ready. When the operator runs with the broker controller enabled, pools without a StatefulSet now fall back to deriving their status from the pool's Broker CR statuses alone (no pod reads): - Replicas: brokers whose PodScheduled reason is not PodMissing - Ready/RunningReplicas: brokers with Ready=True (pod-liveness based, deliberately independent of the cluster-health-coupled readiness probe StatefulSet ReadyReplicas reflects) - UpToDateReplicas: brokers with ConfigSynced=True, which the Broker controller computes from PodOutdated across all rotation keys - CondemnedReplicas: brokers marked for decommission - Deployed condition: same desired-vs-existing comparison as the StatefulSet path DeployedGeneration comes from the chart's nodepool-generation label, now carried from the rendered StatefulSet onto the Broker CRs by brokerSetFor and kept current by brokerset's in-place metadata sync; the controller takes the minimum across the pool's brokers. A live StatefulSet stays authoritative (mid-migration shadow Brokers are inert), and without the broker-controller flag the Broker CRD may not be installed, so both the Broker watch and the fallback lookup are gated on the new NodePoolReconciler.BrokerCREnabled field. The two zz_generated files pick up import-order fixes from the generate pipeline. DiskLost tombstones (dead incarnations awaiting their replacement's registration) are excluded from every count on both surfaces — fetchBrokerBackedPools' facades and the NodePool status synthesis — since after index release a tombstone's pod name belongs to its replacement and counting both would double-count the pair (or read it as a scale-up). Tombstones still anchor their pool's facade: a drained pool whose last broker is a tombstone keeps its facade so the engine's DiskLost lifecycle can finish. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pod adoption — the moment of the STS→Broker handover where kube GC has stripped the StatefulSet ownerRef and the pod belongs to nobody — was invisible to the Broker controller's watches: Owns(Pod) maps an ownerless pod to no Broker, so nothing enqueued the Broker when its pod became adoptable. Whether adoption happened promptly depended on winning a race against the reconcile flurry from the Broker CR's own creation; when the handover landed a few seconds after the CRs (a re-migration, where shadow creation defers the handover by a pass), the flurry was already spent and every broker sat Pending for a full periodicRequeue — three minutes of a migration stalled with its pods adoptable the whole time. Add a Watches(Pod) with a per-cluster mapper that routes ownerless-pod events to the Broker whose deterministic PodName() matches (Broker names are generated, so the orphan is matched by listing the namespace's Brokers on the cluster's cached client). Owned pods return early and keep riding Owns(Pod). Red-first: TestOrphanedPodAdoptionIsEventDriven re-enters shadow mode by handing an adopted pod to a foreign controller, lets the event flurry drain, orphans the pod, and requires adoption within a minute — red before this change (adoption waited for the periodic requeue), green with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eady state TestV2MigrationAndRollback failed deterministically in CI: rollback finished (Broker CRs gone, StatefulSet restored and pods re-adopted) but the BrokerMigration condition stayed InProgress until the test timed out. The per-loop story: finalizeRollback deletes the migration backup ConfigMap — the resume marker — and only then reports RolledBack. In V2 that report rides the end-of-pass status write, and the finalize pass starts right after the previous pass's status write (enqueued by the flood of rollback watch events), so its cached Redpanda carries a stale resourceVersion and the update 409s — which ignoreConflict swallows by design. With the ConfigMap already gone, every later pass took the steady-state early return and never re-reported: the terminal latch was lost forever. Migration is immune to the same loss because NeedsCompletion re-promotes Complete every pass; rollback had no symmetric mechanism. (V1 is also immune — its reporter writes durably with RetryOnConflict.) Fix: add MigrationReporter.NeedsRollback and promote a lingering non-terminal condition to RolledBack in rollback's steady state until the write sticks — the terminal state is observed, not recorded, exactly like Complete. Quiescence-safe: once RolledBack persists, NeedsRollback is false and the pass writes nothing; clusters that never migrated have no condition and never get one. Red-first: TestRollbackRederivesLostTerminalReport models the lossy V2-shaped reporter and failed before the fix; the quiescence guards (never-migrated and already-rolled-back report nothing) pass throughout. TestV2MigrationAndRollback passes against a live testenv with the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code changes: - Export RedpandaKind/NodePoolKind/BrokerKind consts from the v1alpha2 package and use them for every ownerReference and clusterRef kind comparison and stamp instead of scattered string literals. - Rename extraConditionsChanged to brokerMigrationConditionChanged — it only ever carried the BrokerMigration condition's dirty signal. - Drop the error from OwnerHooks.MigrationBlockedReason: every implementation (V1 BrokerSetResource, v2OwnerHooks, NopHooks) reads in-memory state and returned nil unconditionally. - Flatten getV2Cluster: extract the NodePool clusterRef dereference into derefNodePool, whose NodePool-gone fallback now synthesizes a V2 ref from the controller-owning Redpanda and rides the common fetch tail instead of duplicating it; parse the owner's APIVersion with schema.ParseGroupVersion instead of strings.SplitN. - Flatten the pod-fetch error handling in fetchBrokerBackedPools and document why decommissioning brokers leave specReplicas but keep counting pods (spec < status is CheckScale's in-flight signal). - Extract clusterNameFromPoolSet and document the <cluster>-<pool> StatefulSet naming contract it inverts. - Comment the stashed-requeue returns in reconcilePools (the requeue is applied at end of Reconcile; returning it would abort the chain before the cluster-level recovery steps), reword the mid-migration StatefulSet mutation logs to say the StatefulSet is still live, drop RFC references from doc comments in favor of stating the ownership rule directly, and document brokerPodNameBase's empty-string contract and why soonestRequeue is not the min builtin (zero means unset). Acceptance additions: - New step 'pods for cluster X should have no container restarts', asserted alongside the pod-UID snapshot checks in the migration scenario: UID stability proves no pod was recreated but misses in-place container crash loops, while a recreated pod restarts the count at zero — neither check subsumes the other. - The broker-born cluster scenario now condemns the index-0 broker via spec.decommission and asserts it is replaced in place (same pod name, fresh node identity) — the operation a StatefulSet cannot express for anything but its highest ordinal. - The migration scenario now completes the migrate -> rollback -> re-migrate cycle: opting back in must adopt the restored pods promptly (event-driven, not on the periodic requeue) with the original pod UIDs and zero restarts held through all three transitions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s restarts in v2 broker mode Three review findings, each proven red-first and fixed: 1. Flag-off/downgrade guard. A broker-mode cluster reconciled by an operator without --enable-broker (downgrade, values regression) had its broker-backed pools invisible to the pool tracker; the plain path created a fresh-render StatefulSet that fought the Broker controller for the pods and began replacing them. reconcilePools now refuses all pool mutations when the use-broker-cr annotation is present but the flag is off — annotation-only detection, so it works even when the Broker CRD is not installed — sets a terminal ResourcesSynced condition telling the operator to restore the flag or roll back before downgrading, and aborts the chain (downstream steps can't do useful work blind to the broker pods, and their deferred status writes would double-set the condition — the integration test caught that as a status-system panic before it could ship). Test: TestV2BrokerModeFlagOffOperatorDoesNotCreateStatefulSet drives a hand-built flag-off reconciler against a converged broker-born cluster; suite plumbing exposes the shared manager for it. 2. NodePool.Status.DeployedGeneration froze at each Broker's creation-time generation: the NodePool controller reads the generation from Broker CR labels, and UpdateBroker never synced CR labels after creation, so consumers gating rollout completion on DeployedGeneration == Generation waited forever. UpdateBroker now merge-syncs the rendered CR labels — only keys the render sets, nothing removed, inside the existing no-op guard so converged CRs still produce zero writes. The suite now also runs the NodePoolReconciler with BrokerCREnabled, matching production wiring. Test: TestV2NodePoolDeployedGenerationAdvances (frozen at gen 1 for the full timeout before; advances in seconds now). 3. Spurious fleet restarts under restart-cluster-on-config-change. RenderBrokers never carried the cluster-config version, only MarkForRestart stamped it post-hoc (every pass, by design, for crash safety) — so every pod born or adopted from an unstamped template was marked PodOutdated by the first stamp: a serialized full-fleet restart after bootstrap, scale-up, and migration (deterministic for adopted pods, whose backfill skipped the version key outright). Fixes: RenderBrokers seeds pods born current from the owner's persisted Status.ConfigVersion (the same source and gate as STS mode's injected config-version label; V2 only — V1 has no persisted version and stamps only on genuine changes); backfillRotationKeys now covers all of RotationAnnotations; the pre-hash-pod backfill in reconcilePod generalizes to any missing rotation key (missing = born before the key existed, backfill; different = genuine pending rotation, roll). On live CRs MarkForRestart stays the sole writer of the version key: UpdateBroker carries the existing stamp forward unconditionally, since the persisted render source trails the stamp within a pass (and indefinitely on a lost status write) and letting it overwrite would regress fresh stamps and roll pods for no change. Tests: TestPodsBornFromRenderNotOutdatedByMarkForRestart, TestBackfillRotationKeysCoversAllRotationKeys, TestUpdateBrokerNeverRegressesRestartMarker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ithout restarts or loss Rolling back a cluster whose pods had been rotated or replaced IN broker mode (config roll, decommission-replacement) restarted — and could silently lose — exactly those pods. One review finding, four stacked defects underneath, each isolated by successive runs of the new UID-discriminating integration test: - Broker-created pods carry no controller-revision-hash (only the StatefulSet controller stamps it, at pod creation), so the revision-based roll planner treated every adopted pod as outdated. finalizeRollback now stamps the restored StatefulSet's revision onto label-less pods before deleting the backup ConfigMap, keeping the handover resumable. - PodsToRoll rolled on 'can't know': both the no-owned-revisions state (transient right after the restore, before the StatefulSet adopts its predecessor's orphaned revisions) and an EMPTY revision label (the permanent state of an adopted pod under OnDelete) enrolled pods for rolling. The empty-label case was the actual deleter in most failing runs — it fired from the pass-start tracker snapshot in the same pass that stamped the pod, at trace verbosity. Both cases now skip: unknowable outdatedness must not restart brokers. - reconcileDelete decided 'pod not owned -> remove finalizer' from the informer cache, which can miss the controller's own just-made adoption and a just-created replacement pod; any ownerRef left pointing at the removed CR is dangling and the garbage collector deletes the pod and its claims with no event and no log. The pod is now read uncached, so a re-adopted pod is released before the finalizer drops, and finalizeRollback re-strips Broker ownerRefs from pods and their claims as a belt. - The Broker controller's adoption paths (pod and PVC) re-adopted the very pods rollback had just released — the event-driven adoption made that a near-certainty — and an adoption written by a reconcile holding an already-deleted CR from a stale cache creates the dangling ref directly. Adoption is now refused when the owning cluster has left broker mode, checked on an UNCACHED read of the owner (rollback only deletes CRs after the annotation removal is observable, so the check is exact); Brokers owned by unrecognized kinds keep adopting. Rollback additionally acts only on Broker CRs controller-owned by the rolling-back cluster: it lists by label selector — name-based, mutable, copyable labels — and then strips ownerRefs and deletes every match, while every steady-state path already filters with IsControlledBy, and the V2 caller runs on every pass of every non-annotated cluster. Tests: TestV2RollbackAdoptsBrokerCreatedPodsWithoutRoll (integration; decommission-replaces a broker in broker mode, rolls back, and pins the pod's UID through the handover — three consecutive green runs against a ~60% pre-fix failure rate), TestRollbackTouchesOnlyOwnedBrokers (red-first: a label-matching foreign Broker survives), the adopted-pod-without-revision-label and no-revisions PodsToRoll cases, and a revision-stamp assertion in the V1 rollback fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The changelog entry taught users to opt in via the use-broker-cr annotation while the annotation's own doc comment claimed it was not publicly documented, and neither mentioned that a stock install cannot use the feature at all: the Broker CRD ships in the experimental bundle (operator chart value crds.experimental: true, or redpanda-operator crd --experimental). Both now say the same thing — experimental, both prerequisites named, and the BrokerMigration condition (InProgress, Blocked, Complete, RolledBack) called out as the observable handle for migration and rollback progress. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
00c0237 to
ca4716f
Compare
Panel review — PR #1735 (operator: manage Broker CRs from the V2 Redpanda reconciler), 9a7d35e..ca4716fVerdict: fix-first. All four findings from the 2026-08-25 panel review are genuinely fixed at this head — each was verified in source by at least two reviewers, and each carries a real regression test. But the fix for the rollback-restart finding introduced a new, worse defect on the one rollback flow it didn't cover: a broker-born cluster that rolls back ends up with pods that are permanently invisible to every future rolling restart — image bumps and restart-requiring config changes silently never apply. That is a must-fix before merge; the remaining findings are follow-up material. Reviewed: 34 files, +3352/−164 across 10 commits (0 merge commits). Merge-base diff Status of the four prior findings — all fixed, verified in source
Must fix
Should fix
Worth knowing
Reviewer notes
All must-fix/should-fix claims were re-verified against the source by the panel lead before inclusion. 🤖 Generated with Claude Code |
A cluster BORN in broker mode has no migration backup ConfigMap — only the migration state machine writes one — so removing its annotation hit finalizeRollback's no-backup branch, which reported RolledBack and returned before the adopt-verify-stamp loop. The freshly rendered StatefulSet then adopted the ordinal-named pods, but under OnDelete the StatefulSet controller labels pods only at creation, and the rollback stamping loop is the codebase's only other writer of controller-revision-hash. The revision-based roll planner deliberately skips unlabeled pods (they are otherwise spuriously restarted at handover), so every adopted pod became permanently invisible to rolling restarts: image bumps and restart-requiring config changes minted new revisions and rolled nothing, indefinitely and silently. Fix: RollbackConfig gains a DesiredStatefulSets render hook; before any destructive step, Rollback synthesizes the backup ConfigMap from the current render when none exists (broker-born pods were built from exactly that render, so restore-verbatim semantics hold), and the standard restore -> adopt -> stamp -> delete-backup flow then runs unchanged. Synthesizing before the CR deletions makes the ConfigMap the resume marker for the whole tail, mirroring the migration's backup-before-destructive-step ordering. The V2 reconciler feeds the hook from the pool tracker's desired renders; V1 deliberately leaves it nil — its roll signal is the per-pod config checksum, not ControllerRevisions, so unlabeled pods still roll there. Red-first at both levels: TestV2BrokerBornRollbackKeepsPodsRollable (broker-born -> rollback -> every pod must carry the StatefulSet's revision, then a config change must roll all pods; failed unfixed on the missing labels) and a new acceptance leg in broker-crd-v2-cluster.feature (rollback the broker-born cluster, then prove a config change still rolls pods one at a time; failed unfixed with 0/3 pods ever rolling until the step timeout). Both green with the fix; the roll assertions need >= 3 brokers — with 2, the pre-restart probe correctly refuses every roll. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… resources Two more places trusted the name-based, copyable labels alone (review follow-up to the Broker-list scoping fix): - finalizeRollback listed pods by the cluster selector and then stripped Broker ownerRefs and gated completion on StatefulSet adoption for every match. A label-matching pod from another cluster would be orphaned from ITS owner and, never becoming StatefulSet-owned, wedge this cluster's rollback at the adoption wait forever. Act only on the ordinal pod names derived from the backup's StatefulSets. - The NodePool reconciler listed Brokers by the pool-name label, which a V1 Cluster with a same-named broker-mode node pool in the same namespace also stamps on ITS Brokers. Counting those pollutes the replica counts and drags DeployedGeneration (a minimum) down indefinitely. Keep only Brokers whose clusterRef actually names this NodePool (extracted to brokersForNodePool for testability). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-ups, no behavior change: - errBrokerModeFlagOff's doc claimed the guard "keeps the recovery chain running" — it deliberately ABORTS the chain (downstream steps cannot do useful work against a pool view blind to the broker pods). - The changelog said removing the annotation rolls back "with no pod recreation or restarts"; only the handover itself is restart-free — config changes made while in broker mode then roll through the normal health-gated update flow after rollback. Say so. - FetchExistingAndDesiredPools' doc had two copies of its lead sentence interleaved with the parameter docs; merged into one block. - NopHooks had no references left; deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fd46a79 to
fe7b4db
Compare
chrisseto
left a comment
chrisseto
left a comment
There was a problem hiding this comment.
Didn't get through everything but I think there's plenty of follow up conversations to have!
| framework.RegisterStep(`^I set annotation "([^"]*)" to "([^"]*)" on Redpanda "([^"]*)"$`, setAnnotationOnRedpanda) | ||
| framework.RegisterStep(`^I remove annotation "([^"]*)" from Redpanda "([^"]*)"$`, removeAnnotationFromRedpanda) |
There was a problem hiding this comment.
nit: I set annotation <key> to <value> on <Kind>/<Name> would be more reuseable.
| // Broker-backed pools are fetched whenever the operator flag is on — not | ||
| // only in broker mode — so that a cluster mid-rollback (annotation | ||
| // removed, Broker CRs still owning pods) keeps accurate pool status and | ||
| // readiness instead of reporting "no pods" until the rollback completes. | ||
| pools, err := r.LifecycleClient.FetchExistingAndDesiredPools(ctx, rpcluster, injectedConfigVersion, nil, r.BrokerCREnabled) |
There was a problem hiding this comment.
Rather than having an additional parameter, why not pass r.BrokerCREnabled in at creation of the LifecycleClient? I was fully expecting for this to be the union of the CR flag and CLI flag. Good comment!
| defer func() { | ||
| if err != nil { | ||
| logger.Error(err, "error reconciling broker pools") | ||
| if internalclient.IsTerminalClientError(err) { | ||
| state.status.Status.SetResourcesSynced(statuses.ClusterResourcesSyncedReasonTerminalError, err.Error()) | ||
| } else { | ||
| state.status.Status.SetResourcesSynced(statuses.ClusterResourcesSyncedReasonError, err.Error()) | ||
| } | ||
| } | ||
| trace.EndSpan(span, err) | ||
| }() |
There was a problem hiding this comment.
This can trigger a panic. It's called on like 697, which has the same defer logic. Calling .SetResourcesSynced twice will result in a panic.
| for _, data := range cm.Data { | ||
| var backup appsv1.StatefulSet | ||
| if err := json.Unmarshal([]byte(data), &backup); err != nil { | ||
| continue // restoreStatefulSetsFromBackup already reported this |
There was a problem hiding this comment.
Seems like this should be panic("unreachable") rather than a continue?
| // The Broker controller can RE-adopt a pod between Rollback's | ||
| // ownerRef strip and its CR's deletion (a reconcile of the live CR | ||
| // is usually in flight). When that write lands after the GC's | ||
| // orphaning pass, the pod — and the claims the Broker controller | ||
| // created — are left referencing a CR that no longer exists, and | ||
| // the garbage collector deletes such dangling dependents silently: | ||
| // no event, no operator log, just a recreated pod. Keep re-stripping | ||
| // Broker ownerRefs until the StatefulSet holds the pod; the backup | ||
| // ConfigMap keeps this loop resumable. |
There was a problem hiding this comment.
Two separate thoughts here:
- This comment is a bit confusing. Is it stating that dangling controller references will be GC'd or that unowned Pods will be GC'd?
| func (r *RedpandaReconciler) ensureAdminClient(ctx context.Context, state *clusterReconciliationState) error { | ||
| if state.admin != nil { | ||
| return nil | ||
| } | ||
| admin, err := r.ClientFactory.RedpandaAdminClient(ctx, state.cluster.Redpanda) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| state.admin = admin | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Could we make initAdminClient idempotent rather than having two functions to create the admin client?
| @@ -336,6 +431,12 @@ func (r *RedpandaReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ | |||
| logger.V(log.DebugLevel).Info("cluster not settled; scheduling fast poll", "requeueAfter", requeueTimeout) | |||
| pollResult.RequeueAfter = requeueTimeout | |||
There was a problem hiding this comment.
nit: could this call state.stashRequeue for consistency?
| // soonestRequeue returns the earlier of two requeue delays, treating zero | ||
| // (and negatives) as unset. Not the min builtin: min would return the unset | ||
| // zero whenever only one delay is set. | ||
| func soonestRequeue(a, b time.Duration) time.Duration { | ||
| if a <= 0 { | ||
| return max(b, 0) | ||
| } | ||
| if b <= 0 { | ||
| return a | ||
| } | ||
| return min(a, b) | ||
| } |
There was a problem hiding this comment.
Shouldn't this method just not be called with a zero value? I you want the extra protection you could panic if a || b == 0.
| // stashRequeue records the soonest broker-machinery requeue for this pass. | ||
| func (s *clusterReconciliationState) stashRequeue(d time.Duration) { | ||
| s.brokerRequeue = soonestRequeue(s.brokerRequeue, d) | ||
| } |
There was a problem hiding this comment.
nit: how about requestRequeue and remove the notation of this being broker specific. The mechanic seems broadly useful.
As an aside to the below comment, you could panic here if d == 0 and ignore .brokerRequeue if it's zero.
| return s.IsClusterHealthy(ctx) | ||
| } | ||
| return nil | ||
| return s.Hooks.IsClusterHealthy(ctx) |
There was a problem hiding this comment.
Is there a reason block isn't called here?
|
after discussing with @chrisseto I'm closing this one in favor of 5 (most likely) smaller PRs that will be much easier to review. First one: #1792 next ones will be stacked on top |
5 participants
What
The V2 half of broker mode (#1671 shipped V1): with
--enable-broker, theoperator.redpanda.com/use-broker-crannotation on aRedpandaselectsBroker CRs as the pod-management flavor for the cluster's lifetime. An
annotated existing cluster migrates in place (pods adopted, zero restarts),
a cluster born with the annotation provisions Broker CRs directly, and
removing the annotation rolls back to StatefulSets - also restart-free.
How it works
Pools whose StatefulSet is gone are synthesized from Broker CRs into
STS-shaped facades (replicas from decommission intent, readiness from pods,
updatedness from
Broker.PodOutdated), so thePoolTracker's status,readiness and scale accounting work unchanged after handover; facades are
excluded from STS mutation planners and revision-based rolls. A live
StatefulSet stays authoritative mid-migration.
reconcilePoolsdispatcheseach desired pool into the shared
brokersetengine - engine requeues arestashed rather than returned so every pool reconciles each pass, and one
brokerset.Arbitrationper pass keeps the one-disruptive-operation-at-a-timegates aware of sibling pools' writes the informer cache can't show yet.
Owner-specific behavior (health gate, quiescence, migration blockers) moves
behind a
brokerset.OwnerHooksinterface with V1 and V2 implementations.Per the RFC ownership table, Brokers of NodePool-owned pools point
spec.clusterRefat the NodePool; NodePools removed from the spec drainbroker-by-broker through the engine's decommission path. Migration progress
aggregates into one cluster-scoped
BrokerMigrationcondition so a finishedpool never reports Complete while another is blocked.
Also in this PR
Deployed, replica counts andDeployedGenerationfrom Broker CRstatuses alone (no pod reads). DiskLost tombstones are excluded from all
counts - a tombstone+replacement pair shares a pod name. Both the Broker
watch and the fallback are gated on
NodePoolReconciler.BrokerCREnabled,since without the flag the Broker CRD may not be installed.
statefulset.kubernetes.io/pod-nameandapps.kubernetes.io/pod-index(cloud control planes select on them). Synced in place, no rotation.
ownerless pod maps to no Broker under
Owns(Pod), so the post-handoveradoption used to race the CR-creation reconcile flurry and could stall a
re-migration for a full 3-minute periodic requeue. A
Watches(Pod)mappernow routes ownerless-pod events to the Broker whose
PodName()matches.Notable behavior
V2UseBrokerCRis deliberately excluded from the V2 defaulting bundle:annotation removal triggers rollback, so
SetDefaultsmust never re-add it.Tests
Envtest integration coverage for migration, rollback, the NodePool Broker
path and post-migration quiescence; acceptance features for broker-born V2
clusters and STS→Broker migration with scale and rollback (grant-serialized
config rolls, pod-UID stability across the migration boundary); red-first
integration test for event-driven adoption.