docs(dha): revise proposal to HPA-on-scale-subresource after spike - #44
docs(dha): revise proposal to HPA-on-scale-subresource after spike#44scooby87 wants to merge 5 commits into
Conversation
The implementation spike disproved the enforced-ownership premise the first revision rested on: SSA field ownership does not hold on the aggregated apps API, admission webhooks cannot fire there, and the fallback HelmRelease webhook is advisory, bypassable, and platform-wide. Revise the mechanism to reuse what Kubernetes already ships: a stock HorizontalPodAutoscaler on the engine operator's scale subresource, a chart conditional that omits the replica field under autoscaling (so Flux and the autoscaler no longer contend), and a thin engine-aware guard for the two brakes HPA lacks (synchronous-quorum floor and replication-lag gate). Record the spike findings and the four alternatives the first Alternatives section skipped. Scope the MVP to PostgreSQL (CNPG); MariaDB follows once its chart supports scale-out; Redis and MongoDB, which expose no scale subresource, are deferred to a thin-shim follow-up. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe proposal replaces bespoke database autoscaling components with stock KEDA, a chart-rendered ChangesDatabase horizontal autoscaling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant HelmRelease
participant KEDA
participant VictoriaMetrics
participant HorizontalPodAutoscaler
participant EngineScaleSubresource
HelmRelease->>KEDA: render ScaledObject
KEDA->>VictoriaMetrics: query aggregate standby load
VictoriaMetrics-->>KEDA: return autoscaling metric
KEDA->>HorizontalPodAutoscaler: provide external metric
HorizontalPodAutoscaler->>EngineScaleSubresource: apply desired replica count
Possibly related issues
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 130: Update the metric selector in the autoscaling example to use the
documented apps.cozystack.io application lineage labels for the intended
workload instead of the unsupported app: postgres-db label. Preserve the
selector’s existing metric and workload-targeting behavior while aligning it
with the guaranteed chart contract.
- Line 155: Update the “Dependent objects” guidance to explicitly distinguish
the engine CR’s status.instances from WorkloadMonitor status. Refer to
WorkloadMonitor’s documented availableReplicas or observedReplicas fields, and
clarify that dashboards and consumers must not read
WorkloadMonitor.status.instances.
- Around line 99-103: Define the HPA bound reconciliation model for the
controller described in this section before reusing minReplicas/maxReplicas as
guard state: specify how tenant-configured bounds are preserved, how
quorum-floor and lag-brake constraints are combined into effective bounds, and
how restart or policy changes restore and recompute them without retaining stale
brake values or losing tenant limits.
- Around line 153-154: Update the autoscaling enablement and rollback migration
guidance to preserve the live instance count in both directions: before the
chart stops owning spec.instances, observe and write the current count; before
removing HPA/guard ownership during rollback, recompute and stage replicas from
the live autoscaled count so Flux cannot reconcile a lower value. Clarify that
these staging steps occur before ownership changes and retain the existing CNPG
default and rollout constraints.
- Line 166: Update the HPA failure-behavior statements around the single-metric
scaling guidance to scope the “no metric and holds the current count” outcome to
single-metric HPAs, and describe the multi-metric behavior accurately:
unavailable metrics may still allow scale-up from available metrics but can
block scale-down. Keep the guard alert behavior and avoid implying a universal
no-scaling freeze.
- Line 25: Update the MVP engine-scope section to pin the supported
CloudNativePG version and require validating its Cluster /scale contract in the
POC, including exposure of spec.subresources.scale for Cluster.spec.instances.
If the selected version does not guarantee this contract, change the MVP target
to a compatible engine or CNPG image instead of leaving PostgreSQL as an
unverified target.
- Around line 56-60: Update the HPA metric contract and replica calculation
described in the autoscaling proposal: do not use a standby pre-averaged metric
with AverageValue. Either emit a scale-compatible total metric that includes the
primary baseline, or replace the stock HPA path with a controller applying
desiredRead + primaryCount directly, while preserving the documented target and
replica semantics.
🪄 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 Plus
Run ID: dbe9bc59-a6c8-48c2-a496-38d8a6188d55
📒 Files selected for processing (1)
design-proposals/database-horizontal-autoscaling/README.md
Design reviewStrong proposal overall. The second revision honestly dissects the failure of Direction is right — this is "needs work before implementation", not a rework. BlockersB1. The HPA metric arithmetic is internally inconsistent (§1 / §4 / example). B2. The ownership problem is not removed, only moved onto the HPA's Should be addressed before implementation
Nits
Verdict: needs work. As a Draft proposal it can be merged with open |
| There is no automated way to add or remove read replicas under load. A stock `HorizontalPodAutoscaler` does not fit: it only writes a replica count (for CloudNativePG, `Cluster.spec.instances`) and is blind to database topology. It has nothing to encode the synchronous-commit quorum floor, so it can drive the count below `maxSyncReplicas + 1` — where the operator either rejects the change or loses its write quorum — and it has no gate on replication lag, scaling on the load metric alone while standbys are arbitrarily behind. Which instance to add or remove, and in what order, is the engine operator's decision (CloudNativePG removes the highest-ordinal standby and never the primary); an autoscaler for stateful databases must own the count and the safety guardrails while leaving instance lifecycle to the operator. | ||
|
|
||
| ## Goals | ||
| There is no automated way to add or remove read replicas under load. A stock HPA is the natural fit for the *decision* — it computes a desired replica count from a metric with stabilization, min/max, and multi-metric semantics — but on its own it is missing two database-specific safety properties: it has no synchronous-commit quorum floor (it can drive the count below `maxSyncReplicas + 1`, where CNPG rejects the change or starves commits), and no replication-lag gate (it would scale on the load metric alone while standbys are arbitrarily behind). This proposal keeps HPA as the decision engine and adds exactly those two brakes — nothing more. |
There was a problem hiding this comment.
minReplicas are a thing on HPA. You can also synthesize any metric you wish for the HPA, including one that will account for read/non-read replicas. Please have a look at the difference between Custom Metrics and External Metrics in the context of HPA.
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Review: right direction — changes requested on the seams, not the premise
This revision is what I asked for on cozystack/cozystack#3381: the ownership machinery is gone, the decision loop is HPA's, the chart conditional removes the Flux conflict at its root, and the spike findings are on the record. Direction endorsed. The remaining requests are concentrated in the seams of the new mechanism — and in each case the fix is to lean further into stock HPA, not less.
Editorial, first: the introduction is drawn out. A new reader has to wade through the first revision's history — the Overview retelling, then the full spike-findings section — before reaching what the design actually is. Lead with the design: state "stock HPA on the engine CR's scale subresource + a chart conditional that stops declaring the replica field + database-specific brakes" in the first paragraph, and compress the rev1 retrospective into a short "Why this changed" note with the spike findings as an appendix. The findings are valuable as a record; they should not be the opening act.
1. The replica-model math in §1 is wrong as written — and the fix is metric synthesis, not a controller
§1 claims the metric is "pre-averaged over standbys so HPA's own averaging is a no-op". It is not. For an External metric with an AverageValue target, HPA computes desired = ceil(metricValue / target) — there is no + primaryCount term in any HPA formula. Feed it a pre-averaged per-standby value and it concludes desired ≈ ceil(avg/target) total instances (a 3-instance cluster at avg 210 vs target 150 "wants" 2 — catastrophically wrong); feed it the raw sum and every answer is off by the primary.
But this needs no new controller math — synthesize the metric so stock HPA arithmetic produces the model exactly. One clean encoding: serve a per-pod Custom Metric (custom.metrics.k8s.io, type: Pods, averaged by HPA over the scale target's pods) where each standby reports its read load and the primary reports exactly the target value. Then HPA's own formula gives desired = ceil(N · avg / target) = ceil(Σstandbys/target + 1) = desiredRead + primaryCount — the §1 model, falling out of unmodified HPA. The current draft conflates Custom Metrics (object/pod-scoped, divisor = the scale target's pods) with External Metrics (a free-standing value, divisor semantics entirely different); the choice is the formula, and §1/§4 need to be rewritten around a single, worked-out encoding with the arithmetic shown. This is the first thing the PoC must validate.
2. Decide who owns the HPA object — the guard writing tenant HPA spec recreates the ownership fight one level up
The lag brake works by the guard pinning maxReplicas = currentInstances on an HPA the tenant created and will, realistically, declare in Git. Flux reverts the pin; the guard re-pins; that is the same war rev1 just lost, on a smaller battlefield. Two consistent resolutions, pick one:
- The policy renders and owns the HPA. The tenant declares only the
DatabaseScalingPolicy; the HPA becomes an implementation detail the guard creates. This also fixes the RBAC gap (see below) and answers the CRD-vs-annotations open question in favor of the CRD. - Move the brakes into the metric layer and write nothing.
minReplicasalready exists on the HPA — the quorum floor is a defaulting/validation rule (minReplicas ≥ maxSyncReplicas + 1), not a reconcile loop, and CNPG rejects unsafe counts as the backstop anyway. The lag gate can live in the served metric (e.g. the adapter clamps the series to the target while lag exceeds the threshold with an actively-writing primary, which freezes the desired count in both directions — incidentally closer to rev1's brake semantics than max-pinning, which only blocks scale-up and silently allows scale-down under lag; if that weakening is intentional, argue it).
Either way, "a controller that edits a user-declared object's spec" should not survive to the next revision. It is the one piece of rev1's DNA still in this design.
3. Mandate scale-down policies — HPA's default is 100% of pods per 15 seconds
The example sets only stabilizationWindowSeconds. With default policies, once the 1800s window passes a database HPA may drop 6→2 in a single action. The first revision's step-of-1 conservatism was one of the properties its design review fought for, and it silently vanished here. Whatever renders or validates the HPA must pin behavior.scaleDown.policies: [{type: Pods, value: 1, periodSeconds: …}] (and pick a deliberate periodSeconds — it is the pacing between removals).
4. Smaller items
- Tenant RBAC does not exist. cozystack-basics grants tenants nothing on
autoscaling/v2, so "the tenant creates a standard HPA" cannot happen today. Also worth one honest sentence: an HPA lets a tenant drive.spec.instancesof a CNPGClusterthey have no direct write access to — acceptable and bounded, but say it. (Guard-owned HPAs make both points moot.) - The metrics adapter is a cluster singleton and deserves a decided section, not an open-question bullet. Nothing in the platform claims the custom/external metrics APIService today; whatever ships (prometheus-adapter, KEDA, purpose-built) becomes shared infrastructure other features will lean on, and the Custom-vs-External choice from §1 constrains it.
- Migration option (a) does not survive Helm's three-way merge as written. Helm deletes a key that was in the old manifest and is absent from the new one regardless of who last wrote it — so pre-setting
.spec.instancesvia the scale subresource does not prevent the flip upgrade from deleting the field, CNPG defaulting to 1, and the HPA re-raising it only after CNPG has begun removing standbys. The must-test-on-dev-cluster requirement is right; the listed mechanics need a real two-phase design (or the chart templating a floor during the transition window). - Write down why the conditional keys off
autoscaling.enabledand not field presence. The apps API re-materializesreplicas: 2from the values-schema default on every round-trip; that is harmless only because the chart ignores the value under autoscaling. One sentence in §3 prevents someone later "simplifying" the conditional to ahasKeycheck and reopening the conflict. - Quota: dropping the pre-check is correct (over-quota scale-up fails ResourceQuota admission like any workload) — keep an alert on a persistently unmet desired count so it is not silent.
Bottom line
Rev1 needed a premise change; this revision made it, and made it honestly. What is left is finishing the thought: get the HPA arithmetic actually right via metric synthesis (§1/§4 rewrite), remove the last write-contention by deciding HPA ownership (or eliminating the guard's writes entirely), and pin scale-down pacing. With those, I am happy for this to merge and for cozystack/cozystack#3381 to be reworked against it.
…le-down pacing Rework the seams flagged in PR #44 review (lllamnyp, IvanHunters): - Metric arithmetic (§1/§4): drop the incorrect 'pre-averaged / External AverageValue' framing. Use a Custom (Pods) metric where each standby reports its read load and the primary reports exactly the target, so stock HPA computes desired = ceil(N*avg/target) = desiredRead + primaryCount. Worked example included; Custom-vs-External made explicit. - HPA ownership (§5): the DatabaseScalingPolicy guard now renders and owns the HPA; the tenant declares only the policy. Removes the min/maxReplicas revert war, the tenant-HPA-RBAC gap, and settles CRD-vs-annotations. - Lag brake moved to the metric layer (clamp series to target while lag high + primary writing) so it freezes both directions, with hysteresis. - Pin scale-down policies (Pods/1 per ~600s) to restore step-of-1 pacing. - Custom-metrics adapter promoted to a decided section (new shared infra; no custom.metrics.k8s.io served today). Two-phase migration mechanism (Helm three-way merge deletes the omitted field regardless of writer). - Add provisioning-latency + feedback-loop, minReplicas>=2 footprint, read-disruption, and quota-alert notes. Lead with the design; move the spike findings to an appendix. - Nits: lineage-label selector, WorkloadMonitor vs engine .status.instances, drop redis/mongo from the overview framing. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
Pushed a revision ( Blockers
Should-address
Nits
Editorial — leads with the design; the rev1 retrospective is compressed to a "Why this changed" note with the spike findings moved to an Appendix. Two items are intentionally left as open questions rather than pinned in the doc: the concrete metrics-adapter implementation (prometheus-adapter vs KEDA vs purpose-built, constrained by the Custom(Pods) + lag-clamp requirement) and the exact two-phase migration mechanic — both to be settled on a dev cluster at PoC. Happy to pin either now if you'd prefer. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
design-proposals/database-horizontal-autoscaling/README.md (1)
169-169: 🎯 Functional Correctness | 🟡 MinorScope the missing-metric behavior to single-metric HPAs.
The policy supports multiple metrics, but an unavailable metric does not universally hold the replica count: another available metric may still scale up, while scale-down is conservatively blocked. Scope this statement or document the guard’s explicit multi-metric freeze behavior.
🤖 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 169, Update the missing-metric behavior statement in the autoscaling policy documentation to apply only to single-metric HPAs. For multi-metric HPAs, document that other available metrics may still scale up while scale-down is conservatively blocked, unless the guard explicitly freezes all scaling; describe that behavior accurately.
🤖 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 12: Update the MVP scope statement and related Scope section to pin a
specific CloudNativePG (CNPG) version and require pre-MVP verification that its
/scale subresource supports Cluster.spec.instances via spec.subresources.scale.
Make this compatibility validation an explicit release-blocking requirement
rather than referring to CNPG generically.
- Around line 95-96: Define the tenant-configured HPA bounds as the source of
truth, separate from the quorum-adjusted values. In the quorum-floor guard and
its reconciliation path, recompute effective minReplicas and maxReplicas from
those policy bounds on every reconciliation, including after restart or policy
changes; validate and handle cases where the quorum floor exceeds the configured
maxReplicas rather than producing invalid bounds.
- Line 152: Update
design-proposals/database-horizontal-autoscaling/README.md:152-152 to define the
exact two-phase enablement order and staging invariant that preserves the live
instance count; update README.md:157-157 to observe and stage that count before
removing HPA ownership and restoring chart templating; update README.md:197-197
to make validating this migration mechanism with a PoC a prerequisite for MVP
rather than leaving it unresolved.
- Around line 42-46: The autoscaling design must bind each custom-metrics
adapter query to the policy’s targetPerStandby value. Update the proposed
primary-baseline metric encoding and PoC plan so shared adapters use per-policy
selectors or series names, and validate behavior with multiple targetPerStandby
values.
- Around line 44-48: The metric adapter must guarantee exactly one sample for
every target pod, including zero-load standbys. Update the active-connection
metric query/processing described in the MVP metrics section to join against the
complete standby pod set and zero-fill missing cnpg_backends_total series, while
excluding or safely handling pods in creation and terminal states so HPA
arithmetic remains based on current eligible pods.
---
Duplicate comments:
In `@design-proposals/database-horizontal-autoscaling/README.md`:
- Line 169: Update the missing-metric behavior statement in the autoscaling
policy documentation to apply only to single-metric HPAs. For multi-metric HPAs,
document that other available metrics may still scale up while scale-down is
conservatively blocked, unless the guard explicitly freezes all scaling;
describe that behavior accurately.
🪄 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 Plus
Run ID: 0400789f-191d-44ee-9d8e-b58fa7b61b02
📒 Files selected for processing (1)
design-proposals/database-horizontal-autoscaling/README.md
| This proposal adds automatic horizontal scaling of a managed database's **read replicas** in response to load, using **the stock Kubernetes `HorizontalPodAutoscaler` (HPA) acting on the engine operator's `scale` subresource**, plus a one-line chart change so the replica field is no longer declared in Git, plus a thin engine-aware controller — the **`DatabaseScalingPolicy` guard** — that renders the HPA, encodes the two database-specific brakes HPA lacks (synchronous-quorum floor and replication-lag gate), and drives a custom metric that makes stock HPA arithmetic compute the correct read-replica count. | ||
|
|
||
| The proposal is deliberately scoped to **horizontal scaling of read replicas**, because a stateful database primary cannot be scaled horizontally the way a stateless Deployment can. The autoscaler is topology-aware per engine, respects the synchronous-replica quorum, brakes on replication lag, and applies its decisions by patching the application's `replicas` value (the `Application` `spec`) — the same field a human would edit. Patching that field avoids the engine-CR ownership conflict a stock HPA causes; it does **not** by itself stop a concurrent Flux writer that also declares `replicas` (a non-force writer surfaces an SSA conflict, and a `spec.force: true` writer can seize ownership — see Ownership). | ||
| The proposal is deliberately scoped to **horizontal scaling of read replicas**: a stateful primary cannot be scaled horizontally the way a stateless Deployment can. The MVP targets **PostgreSQL (CloudNativePG)**; see [Scope](#scope-and-related-proposals) for the engine ladder. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
Pin the CNPG version and verify its /scale contract before MVP.
The MVP still names CloudNativePG generically and does not require validating that the selected version exposes spec.subresources.scale for Cluster.spec.instances. This remains a release-blocking compatibility assumption.
Also applies to: 22-22
🤖 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 12,
Update the MVP scope statement and related Scope section to pin a specific
CloudNativePG (CNPG) version and require pre-MVP verification that its /scale
subresource supports Cluster.spec.instances via spec.subresources.scale. Make
this compatibility validation an explicit release-blocking requirement rather
than referring to CNPG generically.
…, effective bounds - §4: bind the primary baseline to the policy's targetPerStandby and key series per policy (a shared adapter must not cross targets); require one sample per current pod, zero-filling standby series CNPG omits at zero active connections, and handling Pending/Terminating pods - §5: make the DatabaseScalingPolicy the source of truth for min/maxReplicas and derive effective bounds statelessly each reconcile; when the quorum floor exceeds policy.maxReplicas, quorum wins (raise effectiveMax) and surface a condition, never clamp below quorum Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Requesting changes once more — §1 already contains the answer, and it deletes §4's machinery and all of §5
This revision addressed my previous review faithfully, and §1 now derives exactly the right series: a metric whose per-pod sum is Σ(standby read load) + target, so that stock HPA arithmetic yields desired = 1 + ceil(Σ/target). The step the proposal misses is that nothing needs to emit that series — it can be queried into existence. HPA arithmetic only ever sees the aggregate: for an External (or Object) metric with an AverageValue target, desired = ceil(value / target), so serving the single value Σ + target is exactly equivalent to the per-pod encoding. The primary-reports-target scheme, the per-policy baseline keying, and the zero-fill-per-pod requirements in §4 exist only to make per-pod samples add up to a number we can compute directly with one query — the chart knows target at render time and folds it in as a constant:
sum(cnpg_backends_total{namespace="tenant-acme",state="active"}
* on(namespace,pod) group_left() kube_pod_labels{namespace="tenant-acme",
label_apps_cozystack_io_application_name="db",label_cnpg_io_instance_role="replica"})
+ 150
(This also corrects the "External metric — rejected: off-by-primary" entry in Alternatives, which came from my own earlier review: it only applied while the served value was Σ rather than Σ + target.)
Where the query lives
An HPA object cannot carry a query — its metric spec holds only a name and a label selector — so the query must live where the metrics-API backend reads it, and the options differ sharply:
-
prometheus-adapter: ruled out. Queries live in one global ConfigMap, so a per-app query means per-app adapter config plus a reload — registration for every database deployment. It also speaks to a single upstream URL, while every tenant's metrics live behind a different vmselect.
-
KEDA: recommended. The query lives inline in a namespaced
ScaledObject, and the chart renders that object exactly where it would have rendered an HPA — no global config, no registration step, and KEDA generates the HPA itself. Everything this design needs passes through:scaleTargetRefaccepts any CR with ascalesubresource (CNPGClusterqualifies),minReplicaCount/maxReplicaCounttake the template-computed bounds,advanced.horizontalPodAutoscalerConfig.behaviorcarries the scale-down policies verbatim, andserverAddressis per-object — so each tenant's ScaledObject points at its own vmselect, which no global-config adapter can do:triggers: - type: prometheus metadata: serverAddress: http://vmselect-shortterm.tenant-root.svc:8481/select/0/prometheus query: <the expression above> threshold: "150" # AverageValue ⇒ desired = ceil(value/150) = 1 + ceil(Σ/150)
-
kube-metrics-adapter (Zalando) is the lighter alternative: the query lives in annotations on the HPA itself. Workable, but a much smaller project than KEDA and its per-tenant-server story is weaker.
The query is authored by the chart template — the tenant supplies only numbers through values — so the mandatory-scoping rule (no raw tenant PromQL against shared vmselect) is satisfied by construction.
With the query in hand, §5 dissolves
Nothing left in the guard needs a controller, and the DatabaseScalingPolicy — which in this revision has grown back most of the old DHA's fields — is not needed:
- Quorum floor — template arithmetic:
minReplicaCount: max(.Values.autoscaling.minReplicas, .Values.quorum.maxSyncReplicas + 1, 2). Both values live in the same chart, so a tenant raisingmaxSyncReplicasre-renders the floor atomically in the same values write — strictly better than a reconcile loop converging on it. - Scale-down pacing — a literal
behavior.scaleDown.policies: [{type: Pods, value: 1, periodSeconds: …}]in the rendered object. - Lag brake — a clamp in the same query: while lag exceeds the threshold and the primary is actively writing, serve
currentInstances × target(current from a pod count or KSM's HPA status series), which pinsdesired = currentand freezes both directions. Hysteresis is expressible query-side (max_over_time(lag[<cooldown>])against the recovery threshold is a hysteresis band), and HPA tolerance plus stabilization windows damp the residue. - Dry-run — render the dashboard and alerts but not the ScaledObject (or use KEDA's pause annotation).
- The tenant API —
autoscaling: {enabled, minReplicas, maxReplicas, target, maxReplicationLagSeconds, dryRun}in the app's own values, validated by values.schema.json — how every other knob in cozystack works.
That also closes the ownership question more completely than guard-owned rendering: the autoscaling object is rendered inside the HelmRelease, Flux owns it declaratively, and no runtime writer of its spec exists at all.
What remains, honestly
KEDA (or whichever backend) becomes the one net-new platform singleton claiming external.metrics.k8s.io; the policy CR's status/conditions surface goes away in favor of HPA/ScaledObject events plus the alert rules — an acceptable trade for deleting a controller and an API group; at Σ = 0 the formula yields desired = 1, so the minReplicas ≥ 2 floor is load-bearing (the template already enforces it — worth one sentence in the doc); and the Redis/MongoDB shim follow-up is unchanged. The PoC gate stays what it is, with the encoding swapped: validate that the Σ + target single-value query drives a real HPA to 1 + ceil(Σ/target) across the ceil boundaries.
The ask: state in §1 that the series need not be emitted per-pod (aggregate value Σ + target with an AverageValue target is equivalent), collapse §4 to the backend choice above, and replace §5 and the DatabaseScalingPolicy with the autoscaling values block plus a cozy-lib helper that renders the ScaledObject. The net-new surface of this proposal then rounds down to a helm helper, one values block, and one query — which is, I think, where this design was always trying to go.
…nd CRD Address @lllamnyp's review: the read-replica metric need not be emitted per-pod — an aggregate value Σ+target with an AverageValue target makes stock HPA compute 1+ceil(Σ/target), so one PromQL query the chart authors replaces the per-pod encoding entirely. With the query in hand the guard controller and DatabaseScalingPolicy CRD dissolve: the chart renders a KEDA ScaledObject; KEDA queries vmselect and manages the HPA on the engine scale subresource. Brakes become static — quorum floor via minReplicaCount template arithmetic, scale-down pacing via a behavior literal, lag brake via a query-side clamp with hysteresis, dry-run by not rendering the object. Flux owns the ScaledObject declaratively, so no runtime writer of its spec exists. - §1: single-value Σ+target metric with worked arithmetic and Σ=0→1 note - §4: metric backend collapsed to the KEDA choice (prometheus-adapter and kube-metrics-adapter weighed) - §5: autoscaling values block + cozy-lib helper replace the controller/CRD - rollback made count-preserving (disable must not shrink a live cluster) - honest note: no active rollback of a stuck scale-up (pending+alert instead) - Alternatives: correct the External-metric entry; add rev3 guard+CRD as rejected; net-new surface rounds down to a helper, a values block, a query Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
Pushed §1 — metric queried, not emitted. Dropped the per-pod Custom(Pods) encoding. The metric is now a single value §4 — backend collapsed to KEDA. prometheus-adapter ruled out (global-ConfigMap registration, single upstream vs per-tenant vmselect); KEDA recommended (inline query in a namespaced §5 + CRD dissolved. No controller, no Also addressed: the External-metric entry in Alternatives corrected (the off-by-primary hazard only applied while the served value was Net-new surface now rounds down to a Helm helper, one values block, one query, plus KEDA as a platform component. Open items left explicit rather than pinned: the lag-clamp query shape + hysteresis band, the two-phase enable/disable mechanic, and KEDA packaging/ownership (version, HA, the |
…tency nits - §Upgrade: commit the enable AND disable migration to an explicit two-phase order in prose (observe live count -> stage -> stand up under a floor -> hand the field off via SSA field-manager release), leaving only the SSA-vs-helm-merge handoff detail as the single PoC item; the disable mirror stages .Values.replicas to the live count so it cannot shrink a running cluster - fix the rendered ScaledObject name (postgres-db, not db) to match its scaleTargetRef and the release name - make the minReplicaCount=3 example reproducible (note maxSyncReplicas=2) - §4: reconcile the per-object serverAddress argument with the shared-root vmselect example (MVP reads the root vmselect scoped by label; per-object serverAddress enables per-tenant stacks later) - PoC checklist: verify spec.subresources.scale on the pinned CNPG version Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
Follow-up polish in
|
3 participants
What this PR does
Revises the Database Horizontal Autoscaler design proposal in response to Timofei Larkin (@lllamnyp)'s review on cozystack/cozystack#3381, which requested changes on the design premise rather than the code. The implementation spike disproved the first revision's load-bearing claim — that the autoscaler can be the enforced single owner of the application's
replicasvalue — so this revision changes the mechanism to the cheapest shape that Kubernetes already ships: a stockHorizontalPodAutoscaleracting on the engine operator'sscalesubresource, plus a one-line chart change so the autoscaled field is no longer declared in Git, plus a thin engine-aware guard for the two brakes HPA genuinely lacks (synchronous-quorum floor and replication-lag gate).This is the design half of the response. The code in #3381 will follow this proposal — see Follow-up below.
How each review point is addressed
autoscaling/v2)scalesubresourceCluster.spec.instancesand MariaDB.spec.replicasscale subresources confirmedreplicasunder autoscalingConfirmed code findings (for the record)
Timofei Larkin (@lllamnyp)'s three confirmed findings in #3381 are real. They are not fixed by this doc PR; instead the new design removes the code they live in, so they become moot rather than patched:
Follow-up
replicasunder autoscaling + stock HPA on the CNPG scale subresource + the thin guard, deleting the ownership operator (SSA scheme, marker, ValidatingWebhookConfiguration, terminal-freeze). As an interim step it may be pared todryRun-only recommendation mode.scalesubresource.release-note
Summary by CodeRabbit