Design proposal: Database Horizontal Autoscaler#32
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
📝 WalkthroughWalkthroughThis PR adds a design proposal for a Cozystack database horizontal autoscaler. It defines the CRD, reconcile flow, guardrails, compatibility rules, rollout phases, testing, and alternatives. No runtime code changes are included. ChangesDesign Proposal Document
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
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 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
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
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
dryRunbehaves.