Skip to content

Follow-up: non-blocking review remarks from #3954 (postgres KEDA autoscaling) #4187

Closed

Description

@scooby87

Non-blocking review remarks carried over from the approved review of #3954 (postgres read-replica autoscaling via KEDA). #3954 is LGTM and merge-ready; these are craft-level follow-ups tracked here so they land in a separate PR rather than blocking the feature. File:line anchors are against the #3954 head.

To fix (small, safe, testable)

  • [MINOR] packages/apps/postgres/templates/_autoscaling.tpl — the read-load trigger query joins on kube_pod_labels{...} without pinning job="kube-state-metrics", while the sibling alert rules in packages/system/keda/templates/alerts.yaml do pin it. A second exporter emitting kube_pod_labels (or a duplicate series) would multiply the join. Pin the job on every kube_pod_labels selector in the query for parity with the alerts.
  • [MINOR] packages/apps/postgres/templates/scaledobject.yaml — the KEDA-missing fail-closed guard's rationale comment still describes the replaced "db.yaml drops the static .spec.instances" design; the shipped design SEEDS spec.instances at max(replicas, effectiveMin). Update the comment to the seed rationale so it matches the code.
  • [MINOR] packages/library/cozy-lib/templates/_keda.tpl — cozy-lib.keda.scaledObject accepts negative minReplicaCount/maxReplicaCount (only the min <= max ordering is checked). Unreachable from the postgres caller today, but the helper is engine-agnostic and will gain callers; add a lower-bound guard (< 0 fails) plus a unit case.
  • [MINOR] packages/library/cozy-lib/templates/_keda.tpl — the annotations helper parameter has no caller and no test. Add a cozy-lib-tests case exercising it (extra annotations merged under the pause pair) so the documented parameter is covered.
  • [docs] release note / field docs — the "every Postgres release now applies client-side" phrasing overstates the rollout. The HelmReconciler propagates only chartRef/valuesFrom/labels onto existing HelmReleases, so a release flips to client-side apply only when its Application is next written through the aggregated API, i.e. per-app over time, not fleet-wide at upgrade. Correct the wording where it appears.

Deferred (by design / needs upstream change — tracked, not scheduled here)

  • [MINOR] api/apps/v1alpha1/postgresql/types.go — the four autoscaling ints carry no +kubebuilder:validation:Minimum. Render-time guards in scaledobject.yaml reject the dangerous cases today; CRD-level bounds are not emittable because types.go/values.schema.json are generated by cozyvalues-gen from values.yaml, which has no minimum/CEL directive. Closing this needs a cozyvalues-gen enhancement, out of scope for the follow-up PR.
  • [by-design] packages/system/keda/templates/alerts.yaml — the two DatabaseAutoscaler* alerts match every KEDA-managed HPA/ScaledObject, not only database ones, because the underlying series carry no owning-app label. Correct today since database autoscaling is KEDA's sole consumer in cozystack (acknowledged in the alert file header). Scope the rules by a label the DB objects carry if a non-database ScaledObject is ever added.

Follow-up PR stacks on #3954 and will be opened against main once #3954 merges.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/databaseIssues or PRs related to managed databases (postgres, mariadb, redis, etcd, kafka, clickhouse)kind/featureCategorizes issue or PR as related to a new featuretriage/needs-triageIndicates an issue needs triage by a maintainer

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions