Design proposal: Database Horizontal Autoscaler#32
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a design proposal for a Cozystack database horizontal autoscaler. It defines the CRD, topology-aware reconciliation flow, scaling safeguards, ownership and compatibility rules, security boundaries, rollout phases, testing, and alternatives. No runtime code changes are included. ChangesDesign Proposal Document
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a design proposal for a Database Horizontal Autoscaler (db-autoscaler) in Cozystack to automatically scale read replicas of managed databases based on load. The review feedback highlights several areas in the design that require clarification or adjustment: detailing the mechanism and RBAC permissions for draining read connections, clarifying whether replica limits refer to total instances or strictly read replicas, addressing false-positive replication lag freezes during write-idle periods, and adding the necessary RBAC permissions to read tenant quotas and resource presets.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 4. Query VictoriaMetrics for the driver metric and the replication lag. | ||
| 5. Compute `desired = ceil(currentReplicas * currentMetric / targetMetric)` with a tolerance dead-zone. | ||
| 6. Apply guardrails (see below): clamp to `[min,max]`, quorum floor, lag brake, stabilization windows, step limit, tenant quota. | ||
| 7. If `desired != current` and the decision passes → PATCH `spec.replicas` on the `Application` (with `RetryOnConflict`); on scale-down, drain read connections first. |
There was a problem hiding this comment.
The proposal mentions that on scale-down, the autoscaler will "drain read connections first". However, the exact mechanism for this is not described. Since the db-autoscaler only has read/patch permissions on applications.apps.cozystack.io and read-only permissions on pods (with no write/patch access to Pods, Services, or Endpoints), it is unclear how it can actively drain connections or de-register a pod from the read-only service before patching the replica count.
If the draining relies entirely on the underlying database operator's native graceful shutdown handling, the DHA does not need to perform any action "first". If the DHA is expected to actively manage connection draining (e.g., by labeling pods to exclude them from the service), the RBAC permissions must be updated to include write/patch access to Pods or Services, and the design should clarify how this avoids fighting the database operator's reconciliation.
| metadata: { name: db, namespace: tenant-acme } | ||
| spec: | ||
| targetRef: { kind: Postgres, name: db } # apiGroup defaults to apps.cozystack.io | ||
| minReplicas: 2 |
There was a problem hiding this comment.
There is a potential ambiguity between "total instances" and "read replicas". In database operators like CloudNativePG, the instances (or replicas in Helm values) controls the total number of instances (primary + replicas). If the DHA's minReplicas and maxReplicas map directly to this value, they represent total instances. However, the proposal describes the autoscaler as scaling "read replicas".
Please clarify in the spec description whether minReplicas and maxReplicas refer to the total instance count (including the primary) or strictly the number of read-only standby replicas. If it represents total instances, a warning or validation should prevent users from setting minReplicas to 1, which would leave them with only a primary and 0 read replicas.
| scaleDown: { stabilizationWindowSeconds: 1800, step: 1 } | ||
| constraints: | ||
| respectQuorum: true | ||
| maxReplicationLagSeconds: 30 |
There was a problem hiding this comment.
Using a time-based replication lag threshold (e.g., maxReplicationLagSeconds: 30) is highly susceptible to false positives during periods of write idle. In PostgreSQL, standard lag metrics like now() - pg_last_xact_replay_timestamp() will continuously increase when there are no new transactions on the primary, which would falsely trigger the lag brake and freeze scaling.
It is recommended to specify how the PromQL query will handle write-idle periods (e.g., by checking if the primary's LSN is advancing, or using byte-based lag like pg_wal_lsn_diff as a fallback/supplement) to prevent false-positive scaling freezes.
|
|
||
| ## Security | ||
|
|
||
| - **RBAC.** The operator needs: read DHA and read/patch `applications.apps.cozystack.io`, read `workloadmonitors.cozystack.io`, read `pods`, and read-only HTTP to vmselect. It has no direct access to engine operator CRs or Flux `HelmRelease` objects — only the aggregated apps API. |
There was a problem hiding this comment.
The proposed RBAC permissions do not include access to ResourceQuota or custom Tenant resources, nor do they include access to any platform configuration ConfigMaps defining resource presets. However, the "Tenant quota" guardrail states that the autoscaler must validate whether the new replica count × preset resources fits within the tenant quota.
To perform this validation, the operator will need RBAC permissions to read these quota and preset resources. Please update the RBAC section to include these permissions, or clarify how the quota validation will be performed without them.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-proposals/database-horizontal-autoscaling/README.md`:
- Around line 142-143: Clarify the enforced `replicas` ownership policy for the
DHA so it is not just advisory: specify which controller or writer is the
explicit owner when a DHA is active, and how conflicting declarative updates are
blocked or resolved. Update the `Ownership` section to describe the concrete
conflict path using the relevant DHA autoscaler behavior, and state whether this
is enforced via a lock/marker, admission rejection, or a single explicit winner.
Make it clear that `RetryOnConflict` alone is insufficient and should not be
presented as the ownership mechanism.
- Around line 40-43: The proposal is inconsistent about the owned write target
for autoscaling, switching between patching the Application values and PATCHing
spec.replicas. Pick one single owned path and use the same term everywhere in
the README so the controller write target is unambiguous; update the wording
around Application and the scaling flow to consistently refer to that chosen
field/path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fdc3ad6-15e6-4257-aa9d-9d14b06d30bb
📒 Files selected for processing (1)
design-proposals/database-horizontal-autoscaling/README.md
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
Solid, well-structured proposal that follows the template closely and gets the hard parts right (HPA-via-scale-subresource fights Flux; opt-in/off-by-default; single-flight + fail-safe-freeze). But three substantive issues should be resolved before this moves from Draft to Accepted: the core scaling math rests on a wrong meaning of replicas, the quorum-floor formula is unsafe against CNPG's own constraint, and the CustomQuery metric type contradicts the Security section and opens a cross-tenant read. Details below; all are fixable and none change the overall direction.
I verified the code-level claims against cozystack/cozystack@main rather than trusting the prose — findings 1, 2 and the WorkloadMonitor claims are grounded in the actual chart/types.
Findings
MAJOR
[MAJOR] replicas is total CNPG instances, not read replicas — the core formula conflates the two.
packages/apps/postgres/templates/db.yaml:8 maps the chart value directly: instances: {{ .Values.replicas }}. In CNPG instances is the total count (1 primary + N−1 standbys). Read traffic lands on the <release>-ro endpoint, which targets standbys only — i.e. replicas − 1. The proposal itself acknowledges this in the Rollout PoC ("reads route to <release>-ro"), yet:
desired = ceil(currentReplicas * currentMetric / targetMetric)divides load bycurrentReplicas(total instances, including the non-read-serving primary), so "per read replica" targets are computed against the wrong denominator.minReplicas: 2 / maxReplicas: 6in the CRD are ambiguous: total instances or read replicas?
Decide one interpretation and make it consistent: either the DHA counts total instances (then rename the metric target away from "per replica" and account for the primary), or it counts read replicas (then map instances = readReplicas + 1 on the write path and divide the driver metric by replicas − 1). This is the same ambiguity Gemini raised; it is central, not cosmetic.
[MAJOR] QuorumFloor uses minSyncReplicas + 1; CNPG's hard constraint is replicas > maxSyncReplicas.
The chart's own values.yaml documents maxSyncReplicas as "Maximum number of synchronous replicas allowed (must be less than total replicas)", and both minSyncReplicas/maxSyncReplicas are passed straight into the CNPG Cluster (db.yaml:219-220). A scale-down clamped to minSyncReplicas + 1 (as stated in §4 and §2 of Design) can leave replicas ≤ maxSyncReplicas, which CNPG rejects/auto-caps, and — more importantly — if fewer than minSyncReplicas synchronous standbys remain, synchronous commits block and writes stall. The safe floor is maxSyncReplicas + 1 (and never below what keeps minSyncReplicas standbys available). Please recompute QuorumFloor from maxSyncReplicas, not minSyncReplicas. (Worth pinning to the CNPG version cozystack ships, since the sync-replica API changed across versions.)
[MAJOR] CustomQuery metric type contradicts the Security section and risks cross-tenant metric reads.
metrics[].type: CustomQuery (line 119) and the Alternatives note ("a KEDA/PromQL trigger can be reused as a metric source inside the operator via CustomQuery") mean a tenant supplies arbitrary PromQL that the operator executes against shared vmselect using the operator's own credentials. The Security section claims "the only tenant-supplied inputs are the DHA fields, all bounded and schema-validated" — an arbitrary PromQL string is neither bounded nor schema-validatable, and vmselect in cozystack is a shared store with no per-tenant label enforcement at that layer. A crafted query can read other tenants' series. Either drop CustomQuery from the tenant-facing CRD (keep only the fixed, safe metric types), or require the operator to inject a mandatory tenant/namespace label matcher into every query and reject queries it cannot safely constrain. Please also reconcile the Security section's "all inputs bounded" claim with whatever you decide.
MINOR
[MINOR] "Drain read connections on scale-down" is under-specified and appears to need privileges the RBAC section denies.
The RBAC list is deliberately narrow (read DHA/WorkloadMonitor/pods, read+patch applications, read-only HTTP to vmselect; explicitly no engine-CR/exec access). Draining a CNPG replica's connections requires either removing the pod from the -ro endpoint or terminating backends (pg_terminate_backend) — neither is possible with the stated permissions. Specify the concrete drain mechanism and reconcile it with the RBAC surface (Gemini raised the same). If the real mechanism is "let CNPG cordon the instance on instance-count decrease", say so and drop the "drain" language.
[MINOR] RBAC omits what quota validation needs.
Scale-up is "validated against the tenant quota" and clamped with ScalingLimited=True, but the RBAC list does not include reading ResourceQuota or the resourcesPreset sizing definitions required to compute "new replica count × preset resources". Add them.
[MINOR] Lag brake false-positives on a write-idle primary.
A replication-lag-seconds signal can read high/stale when the primary is idle, freezing both directions during exactly the low-load windows scale-down targets. Consider gating the brake on write activity (or using bytes-behind + a freshness check) rather than raw seconds.
[MINOR] "Alternatives considered" mischaracterizes the Application write path.
pkg/registry/apps/application/rest.go shows the Application is a pure projection of the HelmRelease — Get/Create/Update convert both ways, there is no separate Application store. So patching Application.spec.replicas is writing HelmRelease.Spec.Values.replicas; there is no background "REST layer that regenerates the release" to clobber a direct values patch. The genuine reasons to prefer the apps API are validation, label management, and it being the supported tenant surface — not clobber-avoidance. Tighten that paragraph so the rationale is accurate.
NIT
[NIT] Terminology: "patch the Application values" vs "PATCH spec.replicas". These are the same write (Application spec mirrors chart values), but pick one phrasing and use it everywhere (CodeRabbit).
[NIT] Verify the metrics endpoint identifier. The data-flow diagram hardcodes vmselect-shortterm:8481; confirm the service name/port against packages/system/monitoring so implementers don't copy a stale name.
[NIT] Multi-metric semantics unspecified. metrics is a list, but the formula uses a single metric. State the aggregation when several are set (HPA takes the max of per-metric desired counts).
What's good
- WorkloadMonitor reuse is accurate:
api/v1alpha1/workloadmonitor_types.goconfirmsstatus.operational,availableReplicas,observedReplicasexist exactly as used. - Correct diagnosis that a stock HPA fights Flux (scale subresource vs. reconciled values) and is topology-unaware.
- Opt-in, off-by-default, package-gated, and fully reversible rollback — the upgrade/rollback story is clean.
- Single-flight +
operational && availableReplicas == spec.replicasgate and fail-safe freeze on missing metrics are the right guardrails for a stateful target. - Follows
design-proposals/template.mdsection-for-section; Scope/Non-goals are crisp.
Caveats
This is a docs-only design proposal (Status: Draft), so there is no upgrade/fresh-install runtime impact to assess (Phase 5b: N/A — no packages/ touched). The findings above are design-correctness gates for the Draft → Accepted transition, framed against the currently-shipped postgres chart and apps API; they do not block continued discussion on the PR.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
design-proposals/database-horizontal-autoscaling/README.md (1)
171-176: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd an apiserver-backed test for SSA ownership.
The current test plan only mentions mocked clients, which won't exercise managed-fields conflicts or marker updates. Please add an envtest/e2e case for the competing-writer path so the ownership guarantee is actually verified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@design-proposals/database-horizontal-autoscaling/README.md` around lines 171 - 176, The testing plan only covers mocked clients and misses the competing-writer/managed-fields path needed to verify SSA ownership. Add an envtest or e2e test for the concurrent GitOps write to replicas using the apiserver, and assert the SSA conflict condition and no thrash through the relevant reconcile flow in the controller. Update the Testing section to include this ownership-verification case alongside the existing reconcile and manual scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@design-proposals/database-horizontal-autoscaling/README.md`:
- Around line 171-176: The testing plan only covers mocked clients and misses
the competing-writer/managed-fields path needed to verify SSA ownership. Add an
envtest or e2e test for the concurrent GitOps write to replicas using the
apiserver, and assert the SSA conflict condition and no thrash through the
relevant reconcile flow in the controller. Update the Testing section to include
this ownership-verification case alongside the existing reconcile and manual
scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 260ff6d2-d3ce-47e8-92cc-1617b26d80b0
📒 Files selected for processing (1)
design-proposals/database-horizontal-autoscaling/README.md
VerdictLGTM with non-blocking notes The revision (
The direction is sound and the design is ready to move from Draft toward Accepted. The notes below are refinements, not gates. FindingsMINOR[MINOR] SSA field-manager ownership holds against well-behaved writers, but not against a force-applying competitor. [MINOR] SSA-conflict path cannot be verified with a mocked Application client (echoing CodeRabbit). NIT[NIT] What's good
CaveatsThis is a docs-only design proposal (Status: Draft), so there is no upgrade or fresh-install runtime impact to assess (Phase 5b: N/A, no |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
The revision cleanly addresses the round-1 review: the replicas-as-total-instances vs read-replica math is now explicit, the quorum floor is corrected to maxSyncReplicas + 1, tenant-supplied PromQL is gone in favor of a bounded metric enum, draining is replaced by operator-native gracefulScaleDown, the SSA/force-apply ownership limitation is now analyzed honestly (with the webhook promoted from optional to recommended), and the Alternatives section no longer misstates the Application↔HelmRelease relationship. The core approach — patch the Application's replicas on the supported apps API, own the field via SSA field-manager + admission webhook, guard with quorum/lag/single-flight/quota — is sound.
The notes below are design-completeness items for the Open questions section, not merge blockers for a Draft in community. Two of them (convergence timeout, quorum-vs-quota precedence) are worth resolving before the MVP implementation starts.
Findings
[MAJOR] Single-flight has no convergence deadline — a scale-up that never converges freezes the autoscaler permanently
Reconcile loop step 3 and the Single-flight guardrail: the next decision is gated on operational=true && availableReplicas == replicas. Failure and edge cases covers vmselect-down and lag, but not the case where a patch is applied and the cluster never converges: the new standby pod is rejected by ResourceQuota admission (see the quota-race note below), its PVC can't bind, or it's unschedulable. Then availableReplicas never reaches replicas, single-flight never releases, and the autoscaler can no longer scale down either — it is stuck waiting forever on a scale-up that will never complete, exactly when a scale-down might be what relieves the pressure. Please define a convergence deadline: after N × stabilization windows without convergence, surface AbleToScale=False(StuckScaling), alert, and either hold or roll the replicas value back to the last-converged count. Note this interacts with the tenant-quota guardrail: the pre-check is advisory (a concurrent allocation can consume quota between check and pod creation), so the stuck path is reachable in practice, not just theoretically.
[MAJOR] Quorum-floor-up vs tenant-quota-down precedence is unspecified when they conflict
Guardrails states two normative rules that can directly contradict: (a) "the quorum floor wins: desired is clamped up to the floor, even above maxReplicas", and (b) tenant quota clamps desired down with ScalingLimited=True. maxSyncReplicas is tenant-mutable at runtime, so QuorumFloor can legitimately exceed what the tenant quota permits (raised maxSyncReplicas + tight quota). In that state the two guardrails demand opposite clamps and the resolved behavior is undefined — does the autoscaler exceed quota to satisfy quorum (and then hit the stuck-scaling path above when the pod is quota-rejected), or does it violate the "floor wins" rule? Please state the precedence explicitly and the resulting condition, e.g. "quota cannot be exceeded; if the quorum floor does not fit the quota, freeze with ScalingLimited=True(QuorumExceedsQuota) and do not patch."
[MINOR] The recommended validating webhook needs a defined failurePolicy — both values have a sharp edge
Upgrade and rollback compatibility (Ownership) and Open questions promote a validating admission webhook that rejects conflicting replicas writes for DHA-managed applications. The webhook's failurePolicy is itself a design decision with no safe default: Fail means that while the webhook pod is down, all replicas writes to managed applications are blocked — including manual operator edits and Flux reconciliation of unrelated values on the same object — which is a self-inflicted availability risk on an optional add-on package. Ignore means the ownership enforcement silently evaporates exactly when the webhook is unavailable, i.e. the force-applier wins during the outage. Please call this out in the Open question and state the intended value + scoping (namespaceSelector / objectSelector to only DHA-managed apps) so the blast radius is bounded.
[MINOR] Static resourcesPreset → resources table compiled into the operator will drift from cozy-lib
Security (RBAC) compiles the preset ladder into the operator to avoid a cluster read. That trades a runtime dependency for a version-coupling hazard: when cozy-lib changes the preset ladder, the compiled table is silently stale and the tenant-quota guardrail computes wrong resource totals — over-permitting (quota bypass on scale-up) or under-permitting (spurious ScalingLimited). Please note that the table must be generated from the cozy-lib source at build time (not hand-maintained) with a test that fails on drift, and pinned to the shipped cozy-lib version — same discipline already applied to the CNPG sync-replica API pin.
[MINOR] availableReplicas == replicas single-flight assumes WorkloadMonitor's counter is unit-comparable to the total instance count — verify before relying on equality
Reconcile loop step 3 compares WorkloadMonitor.status.availableReplicas against the Application's replicas (total instances). This equality is load-bearing (it releases single-flight). If availableReplicas as reconciled by internal/controller/workloadmonitor_controller.go counts something not identically scoped to "total engine instances" — e.g. ready managed pods excluding the primary, or including a bootstrap/backup job pod — the two never become equal and the autoscaler freezes indefinitely. Please confirm the exact semantics of availableReplicas/observedReplicas against the controller source and state the invariant the design depends on, rather than assuming name-equality implies value-equality.
[MINOR] "operator-native graceful scale-down (no backend termination)" slightly overstates CNPG behavior
Goals / Design step 7: decreasing CNPG instances deletes the highest-ordinal standby pod. CNPG stops routing it in <release>-ro, but in-flight read connections to that removed standby are severed when the pod terminates — CNPG does not drain existing client sessions off it first. The claim that DHA "does not terminate backends" is true (it delegates), but the net tenant-visible effect is still dropped read connections on scale-down. Suggest softening the wording to "delegates removal to the engine operator (highest-ordinal standby, de-registered from <release>-ro before deletion)" so implementers don't assume connection-level graceful drain that CNPG doesn't provide.
[NIT] Rollback "leaving the application at its current replicas" holds only for non-GitOps-declared apps
Upgrade and rollback compatibility (Rollback): if the tenant declares replicas in their own GitOps repo, deleting the DHA hands the field back to Flux, which reconciles it to the declared value — not the current scaled value. The "current replicas" statement is only accurate for applications whose replicas is not otherwise declared. Worth a one-line clarification, since this is the same ownership boundary the SSA/webhook section is about.
[NIT] Blocking scale-up on high replication lag is a defensible but non-obvious tradeoff — state the rationale inline
Guardrails (Lag brake) forbids both directions on high lag. Blocking scale-down is obvious; blocking scale-up less so, since read saturation is precisely when you'd want more replicas. The justification (a new standby bootstraps from a backup and its initial sync adds replication load, transiently worsening lag) is sound but implicit. A sentence making it explicit would preempt the "why won't it scale up when I'm saturated?" question, and it also belongs in the Open questions bullet about scale-down defaults.
Caveats
- Upgrade / fresh-install impact (Phase 5b): proposal is opt-in, off by default, ships as an optional platform package; existing clusters/APIs/manifests are unaffected until a tenant creates a DHA, and deletion/package-disable is reversible with no data migration. No default flips, no RBAC contraction on existing tenants (the tenant ClusterRole tiers add the new namespaced resource). Fresh-install impact is limited to the new optional package. This is correctly handled — flagged here only to record that Phase 5b was considered; the residual risks are the webhook failurePolicy and RBAC additions noted above, not upgrade regressions.
- This is a docs-only design review: no code, chart, migration, or RBAC manifest exists yet to lint. The RBAC, webhook, quota-math, and WorkloadMonitor-coupling findings are design assumptions to verify against
cozystack/cozystacksource at implementation time, not defects confirmed in shipping code.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Fixes from @IvanHunters, Gemini and CodeRabbit review: - Clarify replicas = total CNPG instances; compute over read-serving (replicas-1) - QuorumFloor = maxSyncReplicas + 1 (was minSyncReplicas + 1) - Drop tenant-facing CustomQuery (cross-tenant PromQL risk); fixed metric enum - Operator-native gracefulScaleDown instead of active connection draining - RBAC: add resourcequotas read; presets are static compiled table - Write-activity-gated replication-lag brake (no idle-primary false freeze) - Enforced replicas ownership via SSA field manager + marker annotation - Correct Alternatives: Application is a pure HelmRelease projection Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Non-blocking notes from @IvanHunters (2026-07-13 LGTM): - Ownership: SSA guarantee holds only vs non-force writers; spec.force:true competitor wins the managed-fields entry. Recommend a validating admission webhook to close the force-apply case deterministically. - Testing: add apiserver-backed envtest for the ownership/competing-writer path (managed-fields conflicts cannot be verified with a mocked client). - Guardrails: state quorum-floor precedence over minReplicas/maxReplicas. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
…a/quorum precedence From @IvanHunters approval review (2026-07-15): - Single-flight: add convergenceDeadlineSeconds; on timeout surface AbleToScale=False(StuckScaling), alert, and roll replicas back to status.lastConvergedReplicas so a relieving scale-down is not blocked. - Guardrails: define precedence quota > quorum floor > min/max; when the quorum floor does not fit the tenant quota, freeze with ScalingLimited=True(QuorumExceedsQuota) and do not patch. - Add status.lastConvergedReplicas and the two failure-case scenarios. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
scooby87
force-pushed
the
design/database-horizontal-autoscaling
branch
from
b047725 to
6d99743
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Request changes
Two design issues sit underneath mechanisms the proposal itself calls "enforced, not advisory": the field-ownership guarantee via SSA is not confirmed for this codebase's hand-written aggregated API, and the HA / leader-election story is unaddressed while per-target reconcile state lives in annotations.
MAJOR: SSA field-manager ownership is not confirmed for the aggregated (non-CRD) resource, and the whole design rests on it (lines 155, 180)
The proposal states that ownership of replicas is "enforced, not advisory" via server-side apply with a dedicated field manager on applications.apps.cozystack.io. In cozystack/cozystack, Application is served by a hand-written rest.Patcher (pkg/registry/apps/application/rest.go), not a CRD. The existing tests (rest_conflict_test.go) only cover RetryOnConflict around a 409 on the backing HelmRelease resourceVersion, which is a different mechanism from per-field managed-fields SSA. There is no evidence the aggregated Patch handler is wired with a typed field manager / TypeConverter for per-field managed-fields tracking. The Testing section (line 180) acknowledges this is unverified (envtest does not exist yet). If SSA does not track .spec.replicas at field granularity, the "enforced ownership" story and the lastConvergedReplicas rollback path silently degrade to advisory. Please add a spike against a real apiserver; if it does not hold, the admission webhook listed under Open Questions becomes mandatory rather than optional.
MAJOR: No leader-election / HA story for an operator that stores per-target reconcile state in annotations (line 155)
The design writes autoscaling.cozystack.io/managed-by: <dha-name> onto the target Application and ties decisions to single-flight plus a convergence deadline with rollback of replicas to status.lastConvergedReplicas. This is exactly the pattern where two non-elected operator replicas reconciling the same DHA/Application race on the annotation write and on the rollback decision: a replica can observe a stale lastConvergedReplicas, revert another replica's decision, or double-apply inside the convergence window. Neither Design, Rollout, nor Security states a replica count or a leader-election requirement. Please make this explicit: either replicas: 1 in the shipped Deployment, or a declared leader-election requirement.
MINOR: The guardrail mixes a PostgreSQL SQL function with a PromQL expression and mis-names the column (line 110)
pg_wal_lsn_diff(pg_current_wal_lsn(), replayed_lsn) is presented as if evaluated inside PromQL. PromQL cannot call pg_wal_lsn_diff(); the function runs server-side inside a Postgres monitoring query, and CNPG already exposes the result as a Prometheus gauge (no custom query needed, so the "reuse existing telemetry" intent is correct). Please reference the actual metric name. Separately, the column is replay_lsn in pg_stat_replication, not replayed_lsn.
MINOR: The RBAC-wiring claim for the new CRD is imprecise about the mechanism and points toward the wrong implementation (line 161)
"granted in the tenant ClusterRole tiers exactly like apps.cozystack.io" is misleading. In packages/system/cozystack-basics/templates/clusterroles.yaml the SA tier and view tier grant apps.cozystack.io via resources: ["*"], but the write tiers (admin / super-admin) grant create/update/patch/delete through a hard-coded per-kind allowlist (postgreses, redises, ...), not a wildcard. databasehorizontalautoscalers.autoscaling.cozystack.io is a new, separate apiGroup; "exactly like" can be read as "add mine to that list", i.e. editing the shared core cozystack-basics file instead of shipping a self-contained package-owned ClusterRole labelled rbac.cozystack.io/aggregate-to-tenant[-view|-admin|-super-admin]: "true" (the actual extensibility mechanism of this RBAC model). Please state the mechanism explicitly.
MINOR: Author(s) metadata deviates from the convention of neighbouring proposals (line 4)
Every other proposal in the repo (compute-plane, external-database-exposure, unified-tls-pki, kubernetes-nodes-split, tenant-oidc-per-tenant-realm) uses a GitHub handle. This one uses a full name plus a personal email. Consider matching the handle-only convention.
MINOR: Rollout does not name the cold-install dependency on the monitoring stack (lines 184-189)
The MVP step is described as an "optional paas-bundle package", but the entire decision loop is driven by VictoriaMetrics / vmselect (packages/system/monitoring) plus WorkloadMonitor. Neither Rollout nor Design declares this as a hard PackageSource.dependsOn-class requirement. This is the same cold-install-failure risk class the codebase already handles for cert-manager-dependent charts. Please name it explicitly before the implementation PR.
…C, deps From @IvanHunters CHANGES_REQUESTED (2026-07-16): - Ownership/Testing/Open questions: flag that field-level SSA is unconfirmed for the hand-written aggregated apps API (rest.Patcher, not a CRD); require an apiserver spike; webhook becomes mandatory if SSA field-tracking absent. - Security/Rollout: operator is a single active reconciler (replicas: 1 + leader-election), no active/active race on marker/single-flight/rollback. - Guardrails/adapter: use CNPG gauge cnpg_pg_replication_lag; fix replay_lsn; drop PromQL-calls-SQL error. - Security: DHA ships package-owned ClusterRoles with rbac.cozystack.io/aggregate-to-tenant* labels (not editing cozystack-basics). - Rollout: declare hard PackageSource.dependsOn on the monitoring stack. - Metadata: Author(s) -> @scooby87 (repo handle convention). Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-proposals/database-horizontal-autoscaling/README.md`:
- Line 162: Update the “Single active reconciler (HA)” proposal to run at least
two operator replicas, with suitable anti-affinity and a PodDisruptionBudget,
while retaining controller-runtime leader election. Revise the availability
wording to accurately describe active/standby HA and remove the claim that
replicas: 1 provides HA.
- Around line 59-60: Update the autoscaling input contract to require every
metric target to be strictly greater than zero, adding this constraint to both
CRD schema validation and controller-side validation. Ensure invalid zero or
negative targets are rejected before desiredRead calculation or reconciliation
proceeds.
- Line 12: Update the proposal’s Flux/GitOps statement near the
horizontal-scaling scope to qualify the guarantee: explain that patching the
Application replicas field avoids direct engine-CR ownership conflicts, but does
not prevent concurrent non-force Flux write conflicts, and that spec.force: true
can take ownership from the autoscaler.
- Line 112: Update the convergence-timeout rollback flow to initialize
status.lastConvergedReplicas from the observed replica count before the first
scale, then validate that target against the current quorum floor,
maxSyncReplicas, and quota before every rollback. If the stored target is unset
or unsafe, freeze without patching replicas; otherwise roll back only to the
validated safe target.
- Around line 107-109: The autoscaling contract must explicitly prioritize the
quorum floor over the per-decision step limit. Update the precedence rules
around QuorumFloor, desired, and behavior.*.step to state that desired may
increase by more than step replicas when required to reach the quorum floor; do
not describe this case as freezing unless the quorum floor also exceeds the
tenant quota and triggers QuorumExceedsQuota.
- Around line 98-103: Update the convergence and rollback tracking around
lastConvergedReplicas so convergence is recorded only after the autoscaler’s own
replicas write has been observed, using its write generation or ownership
marker. Detect a force=true GitOps replacement during the in-flight operation
and do not record that competing target as converged; preserve the existing
freeze and requeue behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28e9bedc-beb2-4eaf-8d84-a826a0d1ef9c
📒 Files selected for processing (1)
design-proposals/database-horizontal-autoscaling/README.md
| - `desiredRead = ceil(Rcur × currentMetric / targetMetric)` (metric averaged over read-serving replicas only, i.e. divided by `replicas − 1`, never by the total) | ||
| - `desiredReplicas = desiredRead + PrimaryCount` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require strictly positive metric targets.
desiredRead divides by targetMetric, but the input contract only says targets are numeric. A zero target can cause division-by-zero behavior, while a negative target reverses scaling. Require target > 0 in CRD validation and controller validation.
Also applies to: 163-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/database-horizontal-autoscaling/README.md` around lines 59 -
60, Update the autoscaling input contract to require every metric target to be
strictly greater than zero, adding this constraint to both CRD schema validation
and controller-side validation. Ensure invalid zero or negative targets are
rejected before desiredRead calculation or reconciliation proceeds.
| - **Precedence — quota > quorum floor > min/max.** `maxSyncReplicas` is tenant-mutable after the DHA is created, so at runtime `QuorumFloor` (`maxSyncReplicas + 1`) may exceed `minReplicas`/`maxReplicas`, and may even exceed what the tenant quota permits. The resolution order is fixed and unambiguous: (1) the **tenant quota is a hard ceiling and is never exceeded**; (2) subject to that, the **quorum floor wins** over `minReplicas`/`maxReplicas` — `desired` is clamped *up* to the floor (even above `maxReplicas`), never letting `min`/`max` push the cluster below a safe quorum. When these two rules collide irreconcilably — the quorum floor does not fit the quota (raised `maxSyncReplicas` + tight quota) — the operator does **not** patch and freezes with `ScalingLimited=True` reason `QuorumExceedsQuota` (alert), rather than exceeding quota (which would only hit the StuckScaling path) or scaling below a safe quorum. | ||
| - **Lag brake:** replication lag above `maxReplicationLagSeconds` forbids both scale-down and scale-up (`AbleToScale=False`). The signal is the CNPG-exported gauge **`cnpg_pg_replication_lag`** (seconds), already scraped into VictoriaMetrics and used by cozystack's own CNPG dashboards and alerts (`dashboards/db/cloudnativepg.json`, `packages/system/postgres-operator/alerts/`), so no custom query is added. Because that seconds value keeps climbing on a write-idle primary, the brake is **write-activity gated**: it is honoured only while the primary's WAL position is advancing (from CNPG's exported current-vs-`replay_lsn` LSN metrics), so an idle primary does not produce a false freeze during the low-load windows scale-down targets. | ||
| - **Cooldown / stabilization:** separate windows for scale-up and (longer) scale-down; scale-down only when the signal held for the whole window. | ||
| - **Single-flight with convergence deadline:** one change at a time; the next decision only after `operational=true && availableReplicas == replicas`. Because that gate can never clear if a scale-up cannot converge — a new standby rejected by ResourceQuota admission, an unbindable PVC, or an unschedulable pod — a patched change must reach convergence within `behavior.convergenceDeadlineSeconds` (default a small multiple of the scale-up window). On timeout the operator surfaces `AbleToScale=False` with reason `StuckScaling`, alerts, and **rolls `replicas` back to `status.lastConvergedReplicas`**, releasing single-flight so a subsequent scale-down (which may itself relieve the pressure) is not blocked. `status.lastConvergedReplicas` records the last count that reached `availableReplicas == replicas`. Note the tenant-quota pre-check is advisory (a concurrent allocation can consume quota between check and pod creation), so this stuck path is reachable in practice, not just in theory. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Revalidate rollback targets against current guardrails.
lastConvergedReplicas can be unset or become unsafe after maxSyncReplicas or quota changes. Rolling back directly to that value could violate the current quorum floor or quota. Initialize it from the observed replica count before the first scale, and recheck quorum/quota before every rollback; freeze without patching if no safe target exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/database-horizontal-autoscaling/README.md` at line 112,
Update the convergence-timeout rollback flow to initialize
status.lastConvergedReplicas from the observed replica count before the first
scale, then validate that target against the current quorum floor,
maxSyncReplicas, and quota before every rollback. If the stored target is unset
or unsafe, freeze without patching replicas; otherwise roll back only to the
validated safe target.
…ck safety From CodeRabbit (2026-07-16): - HA: run >=2 replicas with leader-election + anti-affinity + PDB; drop the replicas:1-is-HA claim (Security + Rollout). - Require metrics[].target > 0 (CRD exclusiveMinimum:0 + controller check) to keep the desiredRead division safe. - Overview: qualify the Flux claim — patching replicas avoids engine-CR conflicts but not concurrent non-force Flux writes; spec.force:true can seize. - Rollback: initialize lastConvergedReplicas from observed count; re-validate target vs quorum floor/maxSyncReplicas/quota; freeze if unset/unsafe. - Precedence: quorum floor overrides the per-decision step limit (not a freeze unless it also exceeds quota). - Convergence: record only after observing the operator's own write; ignore a force=true in-flight replacement, keep freeze+requeue. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM
Re-reviewed at head 57f4ecc. All MAJOR points from the earlier round are addressed: the replica model is split explicitly (primary vs read replicas), the quorum floor is maxSyncReplicas + 1 with a fixed quota > quorum floor > min/max precedence, the tenant-facing CustomQuery is replaced by a fixed metric-type enum, ownership now documents SSA field-manager plus a force-apply caveat and a conditionally-mandatory admission webhook, and an HA story (>= 2 replicas + leader-election, single-flight convergence deadline with rollback to lastConvergedReplicas) plus a hard dependsOn on the monitoring stack are now present.
I re-verified the code-heavy claims against cozystack/cozystack@main by reading the sources directly, and they hold: Application is a projection of HelmRelease, packages/apps/postgres maps replicas to CNPG instances, WorkloadMonitor exposes the status fields the single-flight mechanism relies on, and rest_conflict_test.go indeed only covers RetryOnConflict on resourceVersion (so the proposal's caveat about field-level SSA being unverified is accurate).
One non-blocking correction and two nits below; none block the design discussion.
MINOR: The RBAC rationale is factually wrong, though the conclusion is right (line 161)
The text says cozystack-basics write tiers grant apps "via a hard-coded per-kind allowlist rather than a wildcard". In packages/system/cozystack-basics/templates/clusterroles.yaml the tenant tiers actually grant apiGroups: ["apps.cozystack.io"], resources: ["*"], verbs: ["*"], i.e. a wildcard, not a per-kind allowlist. The architectural conclusion (the DHA package must ship its own aggregated ClusterRoles) is still correct, but for a different reason: DatabaseHorizontalAutoscaler lives in a separate API group autoscaling.cozystack.io, which the apps.cozystack.io wildcard does not cover. Worth fixing so the RBAC model is not misrepresented to a future implementer.
NIT: The Date: line references only the first review round (line 5)
Date: 2026-07-08 (addressing review by @IvanHunters, Gemini, and CodeRabbit) predates the later commits at this head.
NIT: PR-description "never fights reconciliation" overstates the body
The README correctly caveats that a writer with spec.force: true wins ownership and the autoscaler backs off; the "never fights reconciliation" phrasing in the PR description is stronger than the body warrants.
2 participants
Summary
Adds a design proposal for a Database Horizontal Autoscaler (
db-autoscaler) — a dedicated operator that automatically scales the number of read replicas of managed databases (postgres,mariadb,redis,mongodb) based on load.Proposal:
design-proposals/database-horizontal-autoscaling/README.mdHighlights
DatabaseHorizontalAutoscaler(autoscaling.cozystack.io/v1alpha1).Application'sreplicasvalue — Flux/GitOps-safe, never fights reconciliation.WorkloadMonitor; no new exporters.maxSyncReplicas + 1), write-activity-gated replication-lag brake, single-flight, operator-native graceful scale-down (no backend termination), tenant-quota clamp, fail-safe freeze,dryRun.Scope
Horizontal (read replicas) only. Vertical and storage autoscaling are deferred to sibling proposals; write-path/sharding rebalance is out of scope.
Review status
Revised (commit
d3f9f98) to address the first-round review from IvanHunters, Gemini, and CodeRabbit:replicasclarified as total CNPG instances; scaling math computed over read-serving replicas (replicas − 1).maxSyncReplicas + 1.CustomQueryremoved (cross-tenant PromQL risk); fixed, bounded metric enum.gracefulScaleDown; RBAC kept narrow (+resourcequotasread).replicasownership via SSA field manager + marker annotation.Alternativescorrected: theApplicationis a pure projection of theHelmRelease.Status: Draft — feedback welcome, especially on the default driver metric, scale-down defaults, and whether an admission webhook is needed for ownership enforcement (see Open questions).
Summary by CodeRabbit
dryRunreporting.