Repository navigation
Conversation
Add an optional, opt-in operator that automatically scales the number of read replicas of a managed database in response to load, driven by a new DatabaseHorizontalAutoscaler (DHA) custom resource in the autoscaling.cozystack.io/v1alpha1 group. Implements the database horizontal-autoscaling design proposal. The operator reads the driver metric from VictoriaMetrics and the linked WorkloadMonitor, computes the desired total instance count under the design's guardrails (replica model with PrimaryCount, quorum floor, replication-lag brake with write-activity gating, scale-up/scale-down stabilization windows, step limit, single-flight with a convergence deadline and rollback, tenant-quota ceiling, dry-run), and applies its decision by JSON-merge-patching the target Application's replicas value with a dedicated field manager plus a managed-by marker annotation. Because server-side-apply field ownership does not hold on the aggregated apps.cozystack.io API (opaque spec, managed-fields not round-tripped), a validating admission webhook enforces ownership on the backing Flux HelmRelease -- where kube-apiserver actually runs admission and where a force-applying GitOps writer targets the object directly -- rejecting a replicas change from any identity other than the autoscaler or the apps API extension server. Ships the Postgres/CloudNativePG topology adapter, a Helm package (2-replica leader-elected deployment, PDB, anti-affinity, RBAC with tenant aggregation, cert-manager, VMServiceScrape, PrometheusRule alerts), a PackageSource depending on the monitoring stack, and a bundle opt-in. Covered by table-driven unit tests for the decision math and every guardrail, httptest-backed VictoriaMetrics tests, webhook decision tests, and an apiserver-backed envtest suite that validates the CRD schema and exercises a full reconcile. Signed-off-by: Alexey Artamonov <[email protected]>
github-actions
Bot
added
area/uncategorized
📝 WalkthroughWalkthroughIntroduces an optional ChangesDatabase autoscaler
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DatabaseHorizontalAutoscaler
participant db-autoscaler
participant VictoriaMetrics
participant DatabaseApplication
User->>DatabaseHorizontalAutoscaler: create scaling configuration
db-autoscaler->>VictoriaMetrics: query database load and safety metrics
VictoriaMetrics-->>db-autoscaler: return metric observations
db-autoscaler->>DatabaseApplication: patch desired replica count
DatabaseApplication-->>db-autoscaler: report replica convergence
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Validated the operator end-to-end on a real cozystack cluster (CNPG postgres): dry-run, scale-up 2->3->4 with graceful CNPG standby addition and no Flux revert, replicas-ownership webhook denying a competing direct HelmRelease write, and graceful scale-down to the min without dropping below the quorum floor. The spike surfaced six issues that unit and envtest could not, fixed here: - Register the webhook via mgr.GetWebhookServer().Register; registering on a locally constructed server left it started but not wired, so the webhook silently never fired. - Mount the webhook cert with fsGroup 65532 and mode 0440 (was 0400): the nonroot process could not read tls.crt, so the webhook server failed to start with "permission denied". - Calibrate the CNPG PromQL to real metric labels: CNPG series carry no role or cnpg_io_cluster label, so read-serving replicas are selected by joining cnpg_backends_total / container_cpu_usage / cnpg_pg_replication_lag against kube_pod_labels on the lineage labels and label_cnpg_io_instance_role; write-activity gating uses cnpg_pg_stat_replication_sent_diff_bytes. - Add a pod/container securityContext (runAsNonRoot, seccomp RuntimeDefault, drop ALL, no privilege escalation) for restricted PodSecurity. - Bind the ServiceAccount to system:auth-delegator so the secure metrics endpoint can run TokenReview/SubjectAccessReview. - Guard the cert-permission and securityContext contract in the chart unittest. Signed-off-by: Alexey Artamonov <[email protected]>
ebebbfb to
9973784
Compare
Extend horizontal autoscaling beyond the Postgres MVP to the remaining primary-replica engines from the design proposal's adapter-expansion phase: - MariaDB (mariadb-operator async replication): PrimaryCount 1, no sync-replica quorum (floor 1); read replicas identified by the slave-only exporter series. - Redis (spotahome RedisFailover): 1 master + read-serving slaves selected by the redisfailovers-role label; Sentinel count is not the read-replica axis. - MongoDB (Percona replica set): scalable only when sharding is false — a sharded cluster reports ScalingActive=False (shard addition needs data rebalancing). Read members selected by the exporter member-state series; note the Percona PMM/mongodb_exporter ships disabled, so without metrics the loop fail-safe freezes rather than scaling blind. All driver/lag/write-activity queries are namespace-scoped for tenant isolation (guarded by a unit test). The exact PromQL for the three new engines is best-effort and calibrated on real workloads in follow-ups, as the design's Open questions note; the Postgres expressions remain the live-validated ones. Signed-off-by: Alexey Artamonov <[email protected]>
Validated the mariadb/redis/mongodb adapters against a live cozystack cluster. Redis scales up and down end-to-end. MongoDB correctly reports Scalable for a replica set and fail-safe freezes when the (disabled) exporter yields no metrics. MariaDB metrics come from a single mysqld_exporter Deployment that scrapes every instance under one job, distinguishing them by the per-instance `target` label (not the pod), so the initial per-pod / kube_pod_labels join matched nothing. Rescope the queries to job=<release>-metrics and select read replicas by the `target`s exposing mysql_slave_status_* (primary is the target without it). Also record, in the README, that the cozystack mariadb chart does not yet support on-the-fly scale-out (bootstrapFrom unset) and that MongoDB needs its exporter enabled. Signed-off-by: Alexey Artamonov <[email protected]>
Two blocking defects found in review, each now covered by a regression test that fails without the fix: - Redis lag brake compared a BYTE offset (redis_master_repl_offset minus the replica offset) against the seconds-based maxReplicationLagSeconds, so a replica a few bytes behind under write load falsely tripped the brake and froze scaling in both directions. Redis has no seconds-based replication-lag gauge, so ReplicationLagQuery now returns "" and the reconciler treats an empty query as "no lag signal" (brake disabled for Redis). Other engines keep their real seconds gauge. - A transient metric outage recorded RawDesired=0 into the stabilization window (fail-safe path leaves RawDesired unset), dragging the scale-up window minimum to 0 and suppressing scale-up for the whole window after recovery. History is now only appended when a metric was actually read. Also from review recommendations: - Add a CEL rule maxReplicas >= minReplicas (rejected at admission; envtest). - Reject a non-positive metric target with a distinct AbleToScale=False reason InvalidMetricTarget instead of the misleading MetricUnavailable, and fix the type comment that wrongly claimed the controller rejects it. - Document in the README that a manual replicas edit terminally backs the autoscaler off for that target until the DHA is recreated. Signed-off-by: Alexey Artamonov <[email protected]>
… document CPU units Second review pass findings: - respectQuorum was a settable, defaulted, documented constraint that the reconciler never read — the quorum floor was always enforced. Wire it into the decision: RespectQuorum=false now lets the count fall to minReplicas below the sync floor and disables the QuorumExceedsQuota freeze; the default (true) still clamps up to the floor. Covered by decision tests both ways. - gracefulScaleDown was an inert settable bool (the DHA only ever patches replicas and never terminates backends, so scale-down is graceful by construction). Removed the field rather than ship an ignored knob; the README now states scale-down is always graceful. - ReadCPUUtilization target unit was undocumented: the driver query yields millicores, so a target of "250m" (parsed as 0.25) would jump to maxReplicas. Documented that the target is millicores as a plain number (e.g. "250"), with a test pinning the "250" vs "250m" interpretation. Also clarified in the README that the manual-edit ownership back-off holds within an operator lifetime (it relies on in-memory state). Signed-off-by: Alexey Artamonov <[email protected]>
…arget in millicores Third review pass findings: - The ownership back-off (detecting a competing writer via the last count the autoscaler wrote) was in-memory only, so a controller restart or leader failover lost it: a new leader started with empty state, saw no conflict, and would re-adopt the target and silently clobber a tenant's manual `replicas` edit. Persist the written count in status.lastAppliedReplicas and reconstruct the back-off from it on the next reconcile. Regression test drives a fresh (empty) state against an Application whose replicas differ from the persisted value and asserts OwnershipConflict + no clobber. - ReadCPUUtilization target is now read as a Kubernetes CPU quantity in millicores via MilliValue(), so the idiomatic "250m" (250 millicores) and "1" (1000 millicores) both work — removing the earlier footgun where "250m" parsed as 0.25 and jumped to maxReplicas. Test pins both idioms. Signed-off-by: Alexey Artamonov <[email protected]>
… webhook latency Address the LGTM review's non-blocking recommendations: - Add an end-to-end reconciler test for the stuck-scaling path: scale-up patch -> availableReplicas never catches up -> single-flight freeze -> past the convergence deadline the operator rolls replicas back to lastConverged and reports StuckScaling. Previously this flow was only covered at the Decide unit level and the patch path separately; now it is one test (injectable clock added to the test harness). - Document in the README that the replicas-ownership webhook matches every HelmRelease UPDATE cluster-wide and so becomes a soft latency dependency for Flux reconciliation while the package is enabled (mitigated by 2 replicas + PDB, failurePolicy Ignore). Signed-off-by: Alexey Artamonov <[email protected]>
…tep-limit scale-down
Second independent review pass found two more defects, each now tested:
- CPU currentMetrics were reported 1000x too high: the CPU average is already
in millicores, and status wrapped it as NewMilliQuantity(avg*1000), rendering
whole cores ("250" instead of "250m"). The status value is now unit-aware —
CPU is stored as millicores so it renders in the same unit as the target;
plain-count metrics keep the *1000 scaling.
- The PackageSource depended on cert-manager only transitively (via the
victoria-metrics-operator edge), yet the chart provisions its webhook TLS with
cert-manager Issuer/Certificate + inject-ca-from. Declare cozystack.cert-manager
in dependsOn explicitly and assert it in the dependsOn unittest; update README.
Also from the review's non-blocking note:
- The quorum-floor step-bypass now applies only when climbing UP to a safe
quorum. Scaling DOWN toward the floor respects scaleDownStep instead of
shedding many standbys in one cycle. applyStep no longer takes the floor;
Decide owns the upward exemption. Covered by decision tests.
Signed-off-by: Alexey Artamonov <[email protected]>
…ntil viable Third independent review pass findings, verified on dev9: - Stuck-scaling now backs off exponentially. Previously a target that could not converge (unschedulable pod, quota race, unbindable PVC) oscillated patch->stuck->rollback every convergenceDeadlineSeconds forever. The reconciler records the failed size and defers re-attempting it (deadline, doubled per repeat, capped 8x), clearing once load no longer wants that size. Regression test drives scale-up -> non-convergence -> rollback -> and asserts the next high-load cycle does NOT re-patch (backoff holds). - MariaDB reports Scalable=false: the cozystack mariadb chart does not set replication.replica.bootstrapFrom, so the operator rejects on-the-fly scale-out (MariaDBScaleOutError, confirmed on dev9). An active adapter would thrash; gate it off until the chart supports scale-out. - MongoDB reports Scalable=false: the Percona PMM/mongodb_exporter ships disabled (no metric source) and the queries are uncalibrated, so scaling could act on wrong data. Gate it off until the exporter ships on and the PromQL is calibrated. Postgres and Redis remain enabled and are both live-validated (scale up/down). dev9 verification: redis scaled 2->4->2; mariadb and mongodb DHAs correctly report ScalingActive=False with a clear reason and never patch (no thrash). Also report adapter-not-scalable with reason NotScalable (was Sharded) so the message is accurate for non-sharded engines. Signed-off-by: Alexey Artamonov <[email protected]>
…e; preset drift guard Fourth independent review pass: - The Redis adapter source comment still said its PromQL was "calibrated in a follow-up", contradicting the README's "validated on a live cluster" claim. Redis was in fact exercised end-to-end on dev9 (scale 2->4->2) and the redis_exporter sidecar ships enabled by default (a real metric source, unlike MongoDB whose exporter is disabled). Corrected the comment to state it is calibrated/validated and to spell out the Redis-vs-MongoDB difference, so the enabled-ness is consistent with the safety bar the PR applies to MongoDB. - Tenant-quota ceiling now also honors limits-scoped quotas (limits.cpu / limits.memory), not only requests.* and the bare compute names; a quota that bounds only limits previously skipped the ceiling silently. Tested. - Added a guard test that the compiled presetLadder stays in sync with cozy-lib's _resourcepresets.tpl, so upstream preset changes fail loudly instead of drifting the quota math. Signed-off-by: Alexey Artamonov <[email protected]>
…rker via finalizer Fifth independent review pass — two blockers, each covered by a new test: - The quorum-floor step exemption only fired when the clamped target landed exactly ON the floor. When load demanded MORE than the floor (natural > qfloor) while current was below it, applyStep capped movement to current+scaleUpStep, which could be BELOW the floor — patching e.g. 2->3 with a floor of 5 and stepping through the unsafe state. Now any upward move whose step lands below the floor is raised to the floor, so the count is never patched below a safe quorum on the way up (downward moves still respect scaleDownStep). New test drives current<floor + high load and asserts desired >= floor. - The managed-by marker was never removed when a DHA was deleted (no finalizer), so the ownership webhook kept guarding the target's HelmRelease forever and a GitOps writer stayed locked out — contradicting the README. Add a finalizer that strips the marker annotation from the target Application on deletion (tolerating an already-deleted target), with the RBAC to update the DHA. envtest asserts the marker is present after reconcile and gone after the DHA is deleted and reconciled. Non-blocking: template --apps-api-username from the chart (appsApiNamespace, default cozy-system) instead of relying on the binary default, so an extension -server namespace change cannot silently start rejecting legitimate writes. Signed-off-by: Alexey Artamonov <[email protected]>
…efresh status on invalid target Non-blocking polish from the LGTM review pass: - ReadConnections currentMetrics now render as a whole integer (NewQuantity) instead of a milli-quantity, so 75.5 shows as "76" rather than "75500m". - The ownership-conflict warning event is emitted only on the transition into conflict, not on every 60s reconcile (a latch in targetState). - The InvalidMetricTarget early return now refreshes status.currentReplicas so it does not read stale until the spec is fixed. Each covered by a test assertion. Signed-off-by: Alexey Artamonov <[email protected]>
A controller restart or leader failover starts with an empty in-memory state map. lastWritten was already reconstructed from status to keep the ownership back-off durable, but lastConverged and the in-flight anchor were not. So a restart that landed mid-stuck-scale initialized lastConverged to the stuck (larger) current count, making safeRollbackTarget a no-op (target == current), and lost inFlightSince so the convergence deadline never elapsed. The auto-rollback degraded to a permanent StuckScaling freeze at the stuck count instead of rolling back. Restore lastConverged from status.lastConvergedReplicas and, when a scale is still in flight, reconstruct inFlightSince from status.lastScaleTime, so the deadline and rollback are measured across the failover. Covered by a new reconciler test that fails without either restore. Signed-off-by: Alexey Artamonov <[email protected]>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
dosubot
Bot
added
area/database
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
internal/dbautoscaler/webhook.go (1)
82-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
allowedmethod.
evaluateOwnershipiteratesallowedUsersinline (Lines 121-125) andHandlenever callsv.allowed, so this method is dead code. golangci-lint reports it as anunusederror, which will fail CI.♻️ Remove dead method
-func (v *ReplicasOwnershipValidator) allowed(user string) bool { - for _, u := range v.AllowedUsers { - if u == user { - return true - } - } - return false -} -🤖 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 `@internal/dbautoscaler/webhook.go` around lines 82 - 89, Remove the unused ReplicasOwnershipValidator.allowed method, leaving evaluateOwnership’s inline allowedUsers check and Handle unchanged.Source: Linters/SAST tools
packages/system/db-autoscaler/tests/deployment_test.yaml (1)
58-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the controller RBAC manifests in this suite.
Although the suite claims RBAC coverage, it only checks tenant aggregation roles. Add
templates/rbac.yamlandtemplates/rbac-bind.yamlwith assertions for exact resources, verbs, and ServiceAccount subjects so permission broadening or a missing auth-delegator binding is detected.🤖 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 `@packages/system/db-autoscaler/tests/deployment_test.yaml` around lines 58 - 73, Extend the RBAC test in the deployment suite to render templates/rbac.yaml and templates/rbac-bind.yaml, asserting the expected resources and verbs in the controller roles and the exact ServiceAccount subjects in their bindings, including the auth-delegator binding. Preserve the existing tenant aggregation assertions while ensuring permission changes or missing bindings fail the test.internal/dbautoscaler/quota.go (1)
52-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAll-or-nothing fallback can silently mismatch the quota-ceiling calculation.
If
resources.cpu/resources.memoryare set but not both parseable as Go strings (e.g. one field is missing, or a chart passes an unquoted numeric YAML value that decodes tofloat64instead ofstring), the function silently discards the explicit values and substitutes thet1.micro(or configured) preset. Since this feedsMaxReplicasWithinQuota, a real footprint that differs from the preset produces a wrong quota ceiling for the guardrail this file is meant to protect.Consider resolving cpu/memory independently (falling back per-field rather than all-or-nothing) and accepting numeric YAML values in addition to strings.
♻️ Sketch of a per-field fallback
func PerPodResources(appValues map[string]any) (cpu, mem resource.Quantity, ok bool) { - if res, isMap := appValues["resources"].(map[string]any); isMap { - cpuStr, cok := res["cpu"].(string) - memStr, mok := res["memory"].(string) - if cok && mok && cpuStr != "" && memStr != "" { - cq, cerr := resource.ParseQuantity(cpuStr) - mq, merr := resource.ParseQuantity(memStr) - if cerr == nil && merr == nil { - return cq, mq, true - } - } - } + preset, _ := appValues["resourcesPreset"].(string) + if preset == "" { + preset = "t1.micro" + } + pair, found := presetLadder[preset] + defCPU, defMem := resource.Quantity{}, resource.Quantity{} + if found { + defCPU, defMem = resource.MustParse(pair[0]), resource.MustParse(pair[1]) + } + cpu, cpuOK := resolveQuantity(appValues, "cpu", defCPU) + mem, memOK := resolveQuantity(appValues, "memory", defMem) + if !found && (!cpuOK || !memOK) { + return resource.Quantity{}, resource.Quantity{}, false + } + return cpu, mem, true }🤖 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 `@internal/dbautoscaler/quota.go` around lines 52 - 73, Update PerPodResources to resolve CPU and memory independently instead of discarding both when either value is absent or invalid. Accept numeric YAML values such as float64 alongside strings, parse each valid field separately, and use the configured preset (or t1.micro default) only for the field that cannot be resolved.
🤖 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 `@internal/dbautoscaler/adapter.go`:
- Around line 95-103: Update nestedInt to validate numeric inputs before
conversion: reject fractional, negative, and out-of-range values rather than
narrowing them to int32. Propagate that validation failure through
PostgresAdapter.QuorumFloor and the scaling decision so invalid maxSyncReplicas
configuration is reported as non-scalable/AbleToScale=False instead of silently
using a default.
In `@internal/dbautoscaler/status.go`:
- Around line 64-72: The backoff input is not bounded before applying the
maximum 8x multiplier, allowing overflow of time.Duration. In
internal/dbautoscaler/status.go, update backoffDuration to safely cap the
positive count at the maximum supported value before calculating the multiplier,
while preserving the existing minimum-count behavior. In
api/autoscaling/v1alpha1/databasehorizontalautoscaler_types.go, enforce the same
safe upper bound for ConvergenceDeadlineSeconds at API validation so invalid
values are rejected before reaching backoffDuration.
In `@packages/system/db-autoscaler/README.md`:
- Around line 13-20: Update the Postgres and Redis entries in the README to
qualify their live-cluster validation and PromQL calibration status, explicitly
documenting the remaining validation and calibration work instead of claiming it
is complete. Preserve the existing adapter behavior and the MariaDB/MongoDB
descriptions.
In `@packages/system/db-autoscaler/templates/metrics.yaml`:
- Around line 29-32: Update the metrics scrape tlsConfig to remove
insecureSkipVerify and configure CA validation with caFile or caSecretKeyRef
plus serverName, preserving bearer-token authentication while validating the
intended HTTPS endpoint.
In `@packages/system/db-autoscaler/templates/rbac.yaml`:
- Around line 14-16: Restrict the aggregated-apps RBAC rule by replacing the
wildcard in the resources field with the exact resource plural(s) managed by the
adapters, such as applications. Keep the existing apiGroups and verbs unchanged.
---
Nitpick comments:
In `@internal/dbautoscaler/quota.go`:
- Around line 52-73: Update PerPodResources to resolve CPU and memory
independently instead of discarding both when either value is absent or invalid.
Accept numeric YAML values such as float64 alongside strings, parse each valid
field separately, and use the configured preset (or t1.micro default) only for
the field that cannot be resolved.
In `@internal/dbautoscaler/webhook.go`:
- Around line 82-89: Remove the unused ReplicasOwnershipValidator.allowed
method, leaving evaluateOwnership’s inline allowedUsers check and Handle
unchanged.
In `@packages/system/db-autoscaler/tests/deployment_test.yaml`:
- Around line 58-73: Extend the RBAC test in the deployment suite to render
templates/rbac.yaml and templates/rbac-bind.yaml, asserting the expected
resources and verbs in the controller roles and the exact ServiceAccount
subjects in their bindings, including the auth-delegator binding. Preserve the
existing tenant aggregation assertions while ensuring permission changes or
missing bindings fail the test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 546ef1a9-b8be-42ec-83fd-465f6998bb66
📒 Files selected for processing (49)
.github/workflows/pr-labeler.yamlapi/autoscaling/v1alpha1/databasehorizontalautoscaler_types.goapi/autoscaling/v1alpha1/groupversion_info.goapi/autoscaling/v1alpha1/zz_generated.deepcopy.gocmd/db-autoscaler/main.gohack/update-codegen.shinternal/dbautoscaler/adapter.gointernal/dbautoscaler/adapter_engines_test.gointernal/dbautoscaler/adapter_mariadb.gointernal/dbautoscaler/adapter_metrics_quota_test.gointernal/dbautoscaler/adapter_mongodb.gointernal/dbautoscaler/adapter_postgres.gointernal/dbautoscaler/adapter_redis.gointernal/dbautoscaler/decide.gointernal/dbautoscaler/decision.gointernal/dbautoscaler/decision_test.gointernal/dbautoscaler/metrics.gointernal/dbautoscaler/metrics_exporter.gointernal/dbautoscaler/quota.gointernal/dbautoscaler/reconciler.gointernal/dbautoscaler/reconciler_test.gointernal/dbautoscaler/status.gointernal/dbautoscaler/suite_envtest_test.gointernal/dbautoscaler/testdata/crds/postgres.apps.cozystack.io.yamlinternal/dbautoscaler/webhook.gointernal/dbautoscaler/webhook_test.gopackages/core/platform/sources/db-autoscaler.yamlpackages/core/platform/templates/bundles/system.yamlpackages/core/platform/tests/sources_db_autoscaler_dependson_test.yamlpackages/system/db-autoscaler/Chart.yamlpackages/system/db-autoscaler/Makefilepackages/system/db-autoscaler/README.mdpackages/system/db-autoscaler/alerts/db-autoscaler-alerts.yamlpackages/system/db-autoscaler/definitions/autoscaling.cozystack.io_databasehorizontalautoscalers.yamlpackages/system/db-autoscaler/images/db-autoscaler/Dockerfilepackages/system/db-autoscaler/templates/alerts.yamlpackages/system/db-autoscaler/templates/certmanager.yamlpackages/system/db-autoscaler/templates/crds.yamlpackages/system/db-autoscaler/templates/deployment.yamlpackages/system/db-autoscaler/templates/metrics.yamlpackages/system/db-autoscaler/templates/pdb.yamlpackages/system/db-autoscaler/templates/rbac-bind.yamlpackages/system/db-autoscaler/templates/rbac.yamlpackages/system/db-autoscaler/templates/sa.yamlpackages/system/db-autoscaler/templates/service.yamlpackages/system/db-autoscaler/templates/tenant-clusterroles.yamlpackages/system/db-autoscaler/templates/validatingwebhookconfiguration.yamlpackages/system/db-autoscaler/tests/deployment_test.yamlpackages/system/db-autoscaler/values.yaml
| switch v := cur.(type) { | ||
| case float64: | ||
| return int32(v) | ||
| case int64: | ||
| return int32(v) | ||
| case int: | ||
| return int32(v) | ||
| case int32: | ||
| return v |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect every declared maxSyncReplicas schema/validation and its consumers.
rg -n -C3 'maxSyncReplicas' api internal packages -g '*.go' -g '*.yaml' -g '*.yml'
# Confirm all narrowing paths are covered by a checked conversion.
rg -n -C3 'func nestedInt|int32\(v\)|QuorumFloor' internal/dbautoscalerRepository: cozystack/cozystack
Length of output: 2055
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "PWD:" "$(pwd)"
echo "Files matching adapter.go:"
fd -a 'adapter\.go$' . | sed 's#^\./##' | head -50
echo "Git status:"
git status --short || trueRepository: cozystack/cozystack
Length of output: 2026
Reject invalid quorum values instead of narrowing them.
nestedInt accepts arbitrary unstructured numeric input and narrows it to int32; oversized, fractional, or negative maxSyncReplicas values can produce a truncated/invalid quorum floor. PostgresAdapter.QuorumFloor then adds one, so invalid synchronous-replica configuration can allow scale-down below the configured requirement. Propagate invalid config as non-scalable/AbleToScale=False instead of silently defaulting it.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 96-96: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(v)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 98-98: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(v)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 100-100: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(v)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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 `@internal/dbautoscaler/adapter.go` around lines 95 - 103, Update nestedInt to
validate numeric inputs before conversion: reject fractional, negative, and
out-of-range values rather than narrowing them to int32. Propagate that
validation failure through PostgresAdapter.QuorumFloor and the scaling decision
so invalid maxSyncReplicas configuration is reported as
non-scalable/AbleToScale=False instead of silently using a default.
Source: Linters/SAST tools
The AtLimit, QuorumFloor and QuotaExceeded branches set LimitedReason but
left Decision.Message empty, so the rendered ScalingLimited condition
carried a reason with no explanation (kubectl describe showed e.g.
"QuorumFloor:" with a blank message). The freeze branches already carried
messages; make the boundary clamps consistent by stating the effective
bound ("held at quorum floor 3", "held at maxReplicas 6", "clamped to
tenant quota ceiling 4"). Asserted by the existing clamp tests.
Found while live-testing the quorum floor on a cluster.
Signed-off-by: Alexey Artamonov <[email protected]>
…rement Live testing surfaced an operational gotcha: the Postgres/MariaDB adapters recover the read-replica role by joining engine metrics against kube_pod_labels, which is cluster-scoped (scraped only by the root monitoring). A target whose namespace resolves to a tenant-local monitoring stack (monitoring: true) has the engine metrics but not kube_pod_labels, so the role join is empty and the DHA fail-safe-freezes with MetricUnavailable instead of scaling. Document the requirement and the common working layout (monitoring: false, pointing at root) in the package README. Signed-off-by: Alexey Artamonov <[email protected]>
…& backoff Resolves the CodeRabbit review threads on #3381: - RBAC: the apps.cozystack.io rule granted patch on "*"; scope it to the four database kinds the adapters can target (postgreses, mariadbs, redises, mongodbs) so the controller cannot touch unrelated aggregated-API resources. - Metrics TLS: the VMServiceScrape used insecureSkipVerify. Serve metrics with the cert-manager serving cert (the one already mounted for the webhook) and verify it in the scrape against its CA, with serverName matching a cert SAN (no metrics-service DNS name needed, since the pod is reached by IP but TLS verifies serverName). - Quorum: the postgres values schema does not lower-bound maxSyncReplicas, so a negative value produced a quorum floor below 1 and silently disabled the floor guard. Report it as non-scalable (AbleToScale=False) and clamp QuorumFloor to a minimum of 1 defensively. - Backoff overflow: cap ConvergenceDeadlineSeconds at 24h in the API and clamp the base in backoffDuration so the 8x re-attempt ceiling cannot overflow a time.Duration. - README: qualify the Postgres/Redis validation claims (functionally validated on a live dev cluster, not load-tested at production scale). Regression tests: negative-maxSyncReplicas (non-scalable + clamped floor), backoffDuration no-overflow, and helm-unittest assertions for the least- privilege RBAC and the verified metrics TLS. CRD regenerated for the new maximum. Signed-off-by: Alexey Artamonov <[email protected]>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/system/db-autoscaler/definitions/autoscaling.cozystack.io_databasehorizontalautoscalers.yaml (1)
183-200: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject empty target references at admission.
kind,name, andapiGroupare required fields, but empty strings are still accepted. This allows an invalid DHA to be created and only fail during reconciliation. AddminLength: 1; validatenamewith Kubernetes object-name constraints.Proposed schema tightening
targetRef: properties: apiGroup: + minLength: 1 type: string kind: + minLength: 1 type: string name: + minLength: 1 + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string🤖 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 `@packages/system/db-autoscaler/definitions/autoscaling.cozystack.io_databasehorizontalautoscalers.yaml` around lines 183 - 200, Tighten the targetRef schema in the DatabaseHorizontalAutoscaler definition so apiGroup, kind, and name reject empty strings by adding minLength: 1. Apply Kubernetes object-name validation to targetRef.name while preserving the existing required fields and default apiGroup behavior.packages/system/db-autoscaler/tests/deployment_test.yaml (1)
45-47: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not lock in fail-open ownership enforcement.
failurePolicy: Ignoreallows replica updates whenever the webhook is unavailable or errors, bypassing the ownership guard during exactly the failure cases it should cover. UseFail, or obtain explicit approval for a documented break-glass design and update the ownership-safeguard objective accordingly.🤖 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 `@packages/system/db-autoscaler/tests/deployment_test.yaml` around lines 45 - 47, Update the deployment test assertion for webhooks[0].failurePolicy to require Fail instead of Ignore, preserving fail-closed ownership enforcement when the webhook is unavailable or errors.
🧹 Nitpick comments (2)
packages/system/db-autoscaler/tests/deployment_test.yaml (2)
111-131: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover the CA key in the TLS regression test.
The test verifies the secret name but not
tlsConfig.ca.secret.key. A wrong or missing key would pass while the scrape fails to load the CA. Add an assertion that the key isca.crt.🤖 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 `@packages/system/db-autoscaler/tests/deployment_test.yaml` around lines 111 - 131, Add an equal assertion to the metrics TLS regression test for spec.endpoints[0].tlsConfig.ca.secret.key, expecting ca.crt and targeting the VMServiceScrape document, alongside the existing CA secret name assertion.
85-110: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the complete RBAC allowlist.
The current assertions still pass if an unrelated resource is appended to
rules[2].resources. Assert the full permitted set, or its exact size and members, so this test enforces the scopedapps.cozystack.iocontract rather than only checking required entries.🤖 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 `@packages/system/db-autoscaler/tests/deployment_test.yaml` around lines 85 - 110, Strengthen the RBAC assertions in the “apps.cozystack.io access is least-privilege” test to enforce the complete resources allowlist for rules[2].resources. Assert that it contains exactly postgreses, mariadbs, redises, and mongodbs, using an exact list or matching size plus members so extra resources fail the test.
🤖 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.
Outside diff comments:
In
`@packages/system/db-autoscaler/definitions/autoscaling.cozystack.io_databasehorizontalautoscalers.yaml`:
- Around line 183-200: Tighten the targetRef schema in the
DatabaseHorizontalAutoscaler definition so apiGroup, kind, and name reject empty
strings by adding minLength: 1. Apply Kubernetes object-name validation to
targetRef.name while preserving the existing required fields and default
apiGroup behavior.
In `@packages/system/db-autoscaler/tests/deployment_test.yaml`:
- Around line 45-47: Update the deployment test assertion for
webhooks[0].failurePolicy to require Fail instead of Ignore, preserving
fail-closed ownership enforcement when the webhook is unavailable or errors.
---
Nitpick comments:
In `@packages/system/db-autoscaler/tests/deployment_test.yaml`:
- Around line 111-131: Add an equal assertion to the metrics TLS regression test
for spec.endpoints[0].tlsConfig.ca.secret.key, expecting ca.crt and targeting
the VMServiceScrape document, alongside the existing CA secret name assertion.
- Around line 85-110: Strengthen the RBAC assertions in the “apps.cozystack.io
access is least-privilege” test to enforce the complete resources allowlist for
rules[2].resources. Assert that it contains exactly postgreses, mariadbs,
redises, and mongodbs, using an exact list or matching size plus members so
extra resources fail the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 40dcf3d6-1d7d-4af4-a62e-574b6569b53e
📒 Files selected for processing (11)
api/autoscaling/v1alpha1/databasehorizontalautoscaler_types.gocmd/db-autoscaler/main.gointernal/dbautoscaler/adapter_metrics_quota_test.gointernal/dbautoscaler/adapter_postgres.gointernal/dbautoscaler/decision_test.gointernal/dbautoscaler/status.gopackages/system/db-autoscaler/README.mdpackages/system/db-autoscaler/definitions/autoscaling.cozystack.io_databasehorizontalautoscalers.yamlpackages/system/db-autoscaler/templates/metrics.yamlpackages/system/db-autoscaler/templates/rbac.yamlpackages/system/db-autoscaler/tests/deployment_test.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/system/db-autoscaler/templates/rbac.yaml
- api/autoscaling/v1alpha1/databasehorizontalautoscaler_types.go
- packages/system/db-autoscaler/README.md
- internal/dbautoscaler/adapter_postgres.go
- internal/dbautoscaler/status.go
- internal/dbautoscaler/adapter_metrics_quota_test.go
- cmd/db-autoscaler/main.go
VerdictLGTM with non-blocking notes Static review found no blocking defect: the change is additive and opt-in (off unless listed in Findings[MINOR] [MINOR] Claim mismatches[PARTIAL] "MVP ships the Postgres/CloudNativePG adapter" / "adapters redis/mariadb/mongodb out of scope" — the Redis adapter is shipped ACTIVE, not deferred. Caveats
Recommended follow-ups
|
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Review: request changes — on the premise, not the code
This objection belongs on cozystack/community#32 and I am sorry it arrives only now; but the implementation has surfaced facts that invalidate the merged design's central premise, so this PR is where it has to be said. The code itself is careful and unusually honestly documented — that is not the issue. The issue is that the design under it did not get a real design effort — the alternatives that matter were never examined — and merging 5.8k lines, a new permanent API group, and a cluster-wide webhook would entrench the outcome.
The premise is broken
The merged proposal rests on one load-bearing claim: the autoscaler can be the enforced single owner of the application's replicas value. This PR's own spike disproved it, step by step: SSA field ownership does not hold on the aggregated apps API (internal/dbautoscaler/reconciler.go:508-510); admission webhooks cannot fire there either, so enforcement moved to the backing Flux HelmRelease (webhook.go:36-43); there it must allowlist the apps-API ServiceAccount, so every tenant edit bypasses it; and it must be failurePolicy: Ignore, so the force-applying GitOps writer — the design's named adversary — wins during any outage. What remains is advisory ownership, a terminal freeze on any competing write (recoverable only by deleting and recreating the DHA), and a ValidatingWebhookConfiguration intercepting every HelmRelease UPDATE on the platform to guard a handful of databases. The design promised "enforced, not advisory"; what is buildable is neither, at a platform-wide cost the proposal never priced.
What a design effort would have surfaced
The proposal's Alternatives section dismissed "stock HPA + KEDA" in three clauses — "fights Flux, is topology-unaware, and offers no safe scale-down" — and elaborated nothing else in that direction. Here is the ladder of alternatives that dismissal skipped, each cheaper than what this PR builds:
1. Don't create a new API. The new DatabaseHorizontalAutoscaler CRD is a field-for-field re-spec of autoscaling/v2: metrics[].target.averageValue, behavior.scaleUp/scaleDown.stabilizationWindowSeconds, min/max, even the condition names ScalingActive/AbleToScale/ScalingLimited are HPA's own. The proposal never asks the question this equivalence begs: if we are re-drawing HPA's API surface one field at a time, what justifies a permanent parallel API group plus a from-scratch reimplementation of its decision loop instead of reusing the original, its tooling, and a decade of hardening?
2. Let HPA act on the engine CR — the operator that manages the database already knows how to scale it. The "topology-unaware" clause does not survive contact with the vendored CRDs: CNPG Cluster exposes a scale subresource at .spec.instances (packages/system/postgres-operator/charts/cloudnative-pg/templates/crds/crds.yaml:7252) and MariaDB at .spec.replicas (packages/system/mariadb-operator/charts/mariadb-operator/charts/mariadb-operator-crds/templates/crds.yaml:9529), and scaling through the CR delegates the removal decision to the engine operator — the one component that actually is topology-aware. CNPG removes the highest-ordinal standby gracefully and caps/rejects counts that violate maxSyncReplicas — behavior the proposal itself cites, as justification for duplicating it as a guardrail. The quorum concern reduces to minReplicas ≥ maxSyncReplicas + 1, a validation rule, not a controller.
3. When an app is autoscaled, stop declaring replicas at all — then there is nothing for Flux and the autoscaler to fight over. This is the decisive point, and it is completely absent from the proposal. The "fights Flux" clause is true only because our own charts unconditionally template the field (packages/apps/postgres/templates/db.yaml: instances: {{ .Values.replicas }}). That is a contract this repo owns, one chart conditional away from not existing. Omitting the replica field under autoscaling is the standard upstream answer to autoscaler-vs-GitOps — the Kubernetes, Flux, and Argo docs all prescribe exactly this for HPA. Every gram of the ownership machinery — the SSA scheme, the marker annotation, the cluster-wide webhook, the terminal conflict freeze — exists downstream of not asking whether the field needs to be declared. The real costs of this route are chart-sized, not operator-sized: a careful migration (Helm's three-way merge deletes a previously-set field on omission, and CNPG would default to 1 instance), a cold-start gap until HPA raises to minReplicas, and the WorkloadMonitor/dashboard currently deriving from .Values.replicas.
4. Worst case, keep HPA as the decision engine and write a thin DB shim as the actuator. The closest the proposal gets is a note that a KEDA trigger "could be reused as a metric source inside the operator" — the inversion of the right shape. Let a stock HPA (fed from VictoriaMetrics via a metrics adapter, with platform-injected namespace scoping — a constraint the proposal already worked out) compute the desired count with battle-tested stabilization and multi-metric semantics; a shim watches it and propagates to the application values behind the genuinely database-specific gates. Those gates — the write-gated lag brake, single-flight with a convergence deadline, quota-freeze semantics, dry-run — are the only novel content of this PR, and they are shim-sized. This shape also covers Redis and MongoDB, whose CRDs lack scale subresources and which are therefore the honest argument for a bespoke actuation layer — though note this PR ships Redis with its convergence guardrails accidentally disabled and MongoDB gated off, so that argument is not being cashed in here either.
The synthesis: the proposal treated a self-imposed chart contract as a law of nature, and every hard problem in this PR — the unbuildable ownership model, the cluster-wide webhook, the reimplemented HPA loop — descends from that unexamined premise. A design review that stepped outside the frame would have caught it; the rounds that sharpened HA and rollback within the frame could not.
The reimplementation is already failing in the ways reimplementations do
I did a full pass and will file detailed findings if the mechanism survives, but three confirmed examples make the point: the Redis adapter looks up a WorkloadMonitor that does not exist (adapter_redis.go:47 vs packages/apps/redis/templates/redisfailover.yaml:76) and the miss fails open, silently disabling single-flight, the convergence deadline, and rollback for one of the two engines shipped as enabled; the scale-down stabilization window is void after every restart or on a fresh DHA because recommendation history is neither persisted nor required to span the window (decision.go:180-208); and the one test the design itself called "the only layer that actually verifies the ownership guarantee" (the force-writer envtest) was never written. These are not nitpicks — they are the exact scale-down-safety and restart-safety properties the design review fought over, failing inside freshly written machinery that stock HPA has had a decade to harden.
The ask
Not "fix the bugs." Pick one:
- (a) Pare this PR to
dryRun-only: ship recommendation mode (status, events, metrics, alerts — all already here and good), defer actuation, and settle the mechanism with real recommendation data in hand. This preserves most of the work and ships value now. - (b) Spike alternative 3+2 for postgres: chart conditional omitting
replicaswhen autoscaled + stock HPA on the CNPG scale subresource + a thin guard for the DB-specific brakes, and compare honestly. - (c) Amend the community proposal with the SSA/webhook/terminal-freeze findings and the four alternatives above on the record, and make the written case for why the bespoke actuator is still right. The merged proposal cannot serve as that record — it promised an enforcement model this PR demonstrates is unbuildable, and its Alternatives section never engaged the shapes that avoid the problem entirely.
Until one of those happens, I do not think this should merge, independent of any code-level fixes.
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Request changes: close this PR — the design it implements no longer exists, and the actual blocker is a CNPG version bump
GitHub has no "reject" state, so formally this is request-changes; in substance it is a recommendation to close.
The design this PR implemented (the bespoke db-autoscaler operator, DatabaseHorizontalAutoscaler CRD, ownership webhook) was superseded in the design proposal's later revisions — the approved mechanism in cozystack/community (design-proposals/database-horizontal-autoscaling, as revised through community#44) is a chart conditional that stops templating the replica field under autoscaling plus a chart-rendered KEDA ScaledObject driving the CNPG Cluster scale subresource, with no bespoke controller, no new CRD, and no webhook. There is nothing left for this PR to be reworked toward: every major component it contains is absent from the approved design.
The live PoC run against this PR's branch did surface a real blocker — the KEDA-managed HPA fails with InvalidSelector because the CNPG Cluster scale subresource serves no selector — but that blocker is version-bound, not structural: upstream shipped exactly the missing piece in cloudnative-pg#8996 ("feat: add label selector to the Cluster scale subresource", closing cloudnative-pg#2574), which populates Cluster.status.selector and wires labelSelectorPath: .status.selector into the CRD. It shipped in CNPG v1.28.4 / v1.29.2 / v1.30.0 (2026-06-29) and was never backported to the 1.27 line the PoC ran on (cozystack vendors chart 0.26.1 / appVersion 1.27.1).
Suggested path instead of this PR:
- A small separate PR bumping the vendored cloudnative-pg chart in
packages/system/postgres-operatorto a version shipping CNPG ≥ 1.28.4 — routine maintenance, with the normal upgrade validation for the operator under every tenant database. - Re-run the dev9 PoC on the bumped operator: the KEDA +
Σ + targetmechanism should then work unchanged on stock CNPG; this is the verification gate the design's testing section already calls for. - Implement the approved design fresh (chart conditional, cozy-lib helper rendering the
ScaledObject, theautoscalingvalues block + schema, KEDA as a platform package, alerts/dashboards) — it shares no code with this branch, so a fresh PR will be smaller than a rework.
None of this diminishes what this PR contributed: its implementation spike disproved the original ownership premise and reshaped the design twice, its metric encoding validated on real data, and its PoC surfaced the selector gap that the version floor now guards. Those results are captured in the design document; the branch remains available for reference. Closing this PR is the bookkeeping that matches where the design ended up.
|
Closing per Timofei Larkin (@lllamnyp)'s review (2026-08-18). This PR implements the superseded bespoke design (the db-autoscaler operator, the DatabaseHorizontalAutoscaler CRD, and the ownership webhook); the approved design in community#44 is a chart conditional plus a chart-rendered KEDA ScaledObject on the CNPG Cluster scale subresource, which shares no code with this branch. The live PoC's blocker (the KEDA-managed HPA failing with InvalidSelector because CNPG's Cluster scale subresource served no status.selector) is version-bound, not structural: upstream added the selector in cloudnative-pg#8996, shipped in CNPG 1.28.4 / 1.29.2 / 1.30.0 and not backported to the 1.27 line the PoC ran on. Path forward: record the finding and the CNPG version floor in community#53, bump the vendored CNPG to >= 1.28.4, then open a fresh PR implementing the approved design. The spike, metric validation, and PoC results from this branch are captured in the design document; the branch stays available for reference. |
3 participants
What this PR does
Closes #3394.
Implements the Database Horizontal Autoscaler design proposal: an optional, opt-in operator (
db-autoscaler) that automatically scales the number of read replicas of a managed database in response to load, driven by a newDatabaseHorizontalAutoscaler(DHA) custom resource in theautoscaling.cozystack.io/v1alpha1group. It is off by default and enabled viabundles.enabledPackages; nothing changes for existing clusters until a tenant creates a DHA.The operator reads the driver metric from VictoriaMetrics and the linked
WorkloadMonitor, computes the desired total instance count under the design's guardrails (the primary-vs-read replica model, quorum floormaxSyncReplicas+1, replication-lag brake with write-activity gating, separate scale-up/scale-down stabilization windows, per-decision step limit, single-flight with a convergence deadline and rollback to the last converged count, tenant-quota ceiling with precedence quota > quorum > min/max, fail-safe freeze, and dry-run), and applies its decision by JSON-merge-patching the targetApplication'sreplicasvalue with a dedicated field manager plus amanaged-bymarker annotation. Scale-down is handed to the engine operator's graceful instance removal.Ownership note: server-side-apply field-level ownership does not hold on the aggregated
apps.cozystack.ioAPI (itsspecis an opaque JSON blob and managed-fields are not round-tripped). The validating admission webhook is therefore registered against the backing Flux HelmRelease, not the aggregated API — kube-apiserver does not run admission webhooks for aggregated APIServices, whereas the HelmRelease is a CRD it serves and the object a force-applying GitOps writer targets directly. The webhook rejects areplicaschange from any identity other than the autoscaler or the apps-API extension server, closing the design's named competing-writer case; itsfailurePolicyisIgnoreso an outage never blocks unrelated writes, degrading to the controller's advisory marker plus convergence-based conflict detection.This PR ships the Postgres/CloudNativePG topology adapter, the Helm package (2-replica leader-elected deployment, PDB, pod anti-affinity, RBAC with tenant aggregation, cert-manager, VMServiceScrape, PrometheusRule alerts), a
PackageSourcedepending on the monitoring stack, and the bundle opt-in. It is covered by table-driven unit tests for the decision math and every guardrail, httptest-backed VictoriaMetrics tests, webhook decision tests, and an apiserver-backed envtest suite that validates the CRD schema and exercises a full reconcile (patch + marker).This is opened as a draft: the design proposal's Testing section requires a live-cluster spike to confirm the ownership/webhook behaviour against the real aggregated API before relying on the deterministic guarantee, and the driver PromQL expressions need calibration on a real workload. Follow-up adapters (
mariadb,redis,mongodb) are out of scope here.Screenshots
Not applicable (no UI changes).
Downstream repositories
I walked the trigger map against the diff. This PR adds a new optional platform component (a
packages/system/package plus aPackageSourceand a bundle opt-in), which is not covered by the automated website docs generator (that generator only trackspackages/appsandpackages/extra). The platform-stack / variants documentation incozystack/websiteneeds a follow-up describing the new optionaldb-autoscalercomponent and theDatabaseHorizontalAutoscalerCR — filed as cozystack/website#627. No other downstream repository is reached: the new group isautoscaling.cozystack.io(the Terraform provider treats non-modelledcozystack.io/backups.cozystack.ioCRDs as opaque JSON and is unaffected), and nohack/, layout, namespace, or installer contract changed.Release note
Summary by CodeRabbit