mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-27 15:17:13 +09:00
main
216
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
1d813c9445 |
fix(backups): refuse concurrent restores and skip the CNPG purge only for this restore's Cluster (#4754)
## What this PR does Fixes #3322. An in-place Postgres (CNPG) restore decided whether to skip deleting the live `Cluster` by comparing its `creationTimestamp` with the RestoreJob's `StartedAt`. The timestamp says the Cluster is new, not which restore rendered it, so two cases let a restore skip its purge and report `Succeeded` on data from another restore: 1. Two RestoreJobs into the same application at once. The second sees the Cluster the first produced, created after its own `StartedAt`, and skips its purge. 2. The live Cluster is deleted while the restore waits for the WAL archive. The still-active HelmRelease re-renders it from the previous restore's values, again created after `StartedAt`. ### Refuse a restore into an application another restore holds The RestoreJob controller now refuses, for every driver, a RestoreJob that has not started yet when another unfinished RestoreJob in the namespace writes into the same application and goes first. The refused job fails with reason `ConcurrentRestore`, and its message names the job that goes first. It fails instead of waiting, so the application is never restored a second time later, after the first restore, by a job its author may no longer expect to run. A started restore goes before one that has not started. Between two that have not, the older goes first, by creation time and then by name; creation times have one-second precision, so two jobs created in the same second are ordered by name. A running restore is never failed by a newer one. The target application is resolved by `restoreJobTarget`, the same helper the BackupJob hold uses, so a Velero restore into another namespace does not collide with an in-place one. The gate lists RestoreJobs through the manager's API reader, not the informer cache. A job reconciled before the cache holds its peer's `StartedAt` would otherwise see that peer as not started and, on a same-second name tie, proceed alongside it. The controller runs a single worker, so a peer's `StartedAt` written by an earlier reconcile is already on the API server when the next job reads it; `SetupWithManager` now sets `MaxConcurrentReconciles: 1` explicitly, since the guarantee depends on it. ### Skip the CNPG purge only for this restore's Cluster The restore patch already sets `bootstrap.newServerName` to `restoredServerName(cluster, RestoreJob UID)`, a value unique to each RestoreJob. The postgres chart now annotates the `Cluster` with it as `postgres.cozystack.io/restored-server-name`. The annotation is rendered even when the barman plugin is not; the plugin's `serverName` also carries the value, but the plugin renders only with chart-level S3 coordinates. The driver skips the purge only when the annotation names this RestoreJob. A Cluster produced by a concurrent restore, or re-rendered from an earlier restore's values, carries another value and is purged. The mark lives on the rendered Cluster rather than in RestoreJob status. That keeps it valid in the case the skip exists for: the purge succeeded, but the `TargetPurged` status write failed. A UID recorded in status was rejected in the code for exactly that reason. A Cluster rendered by a chart that predates the annotation has none and keeps the creation-time check, so a restore in flight across an upgrade behaves as before. ### Tests - `TestRestoreAhead` covers order, a started job going first, the name tie-break, finished and terminating jobs, other applications, retargeted restores and a Velero restore into another namespace. - `TestReconcileRestore_ConcurrentRestoreIsRefused` checks that the newer job fails with `ConcurrentRestore` and the running one is left alone. - `TestReconcileRestore_ConcurrentRestoreReadsPastTheCache` gives the cache a stale, not-started peer and the API reader the started one, and requires the job to fail with `ConcurrentRestore`. - `TestCNPGClusterFreshlyRecovered` adds both cases from the issue: a Cluster marked by another restore and created after `StartedAt` must be purged. The timestamp-only cases are kept for the unmarked fallback. - `TestReconcileCNPGRestore_RepeatInPlacePurgesStaleRecoveryCluster` gains two reconcile-level cases. One purges a Cluster that another restore marked, even though it was created after `StartedAt`. The other runs a full purge, takes the mark from the `bootstrap.newServerName` the restore patch wrote, drops `TargetPurged` as a lost status write would, and requires a Cluster carrying that mark to survive, although it predates `StartedAt`. That pins the purge decision to the value the chart actually renders. - `packages/apps/postgres/tests/restore_mark_test.yaml` checks that the annotation is rendered with and without the barman plugin, and is absent on a Cluster that was never restored. Each new test was checked against the code it guards: disabling the gate call, listing through the cache, flipping the started-job precedence, ignoring the annotation, or deriving the expected mark from the app name instead of the cluster name each turns one red. `go test ./internal/backupcontroller/...` and `helm unittest packages/apps/postgres` (152 tests) pass. The restore flow was not run against a live cluster. `docs/operations/backup-classes.md` documents the one-restore-at-a-time rule and the annotation. ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff: Go changes in `internal/backupcontroller` (no CRD or API type change), one annotation added to the postgres chart's Cluster (no `values.yaml` or `values.schema.json` change), and a docs paragraph. No package is added or renamed, no platform values, ApplicationDefinition, RestoreJob CRD, metric, label or namespace changes, so no trigger is reached. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(backups): a RestoreJob now fails with reason ConcurrentRestore when another unfinished RestoreJob is already restoring the same application, instead of running alongside it; create it again once the first restore has finished. An in-place Postgres restore now skips deleting the live Cluster only when that Cluster was rendered for this RestoreJob, identified by the new postgres.cozystack.io/restored-server-name annotation, so it no longer reports Succeeded on data from a concurrent or earlier restore. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Restores targeting the same application are now handled one at a time. A competing restore is refused rather than queued, with started restores taking priority. * PostgreSQL now calculates a default `shared_buffers` value when the memory limit is below 512 MiB, while respecting explicitly configured values. * **Bug Fixes** * Recovery clusters are more reliably distinguished from manual recoveries and clusters created by other restores, reducing the risk of purging the wrong cluster. * **Documentation** * Clarified restore ordering and how recovery-cluster cleanup is determined. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
e4e991e837 |
fix(backups): skip the CNPG restore purge only for this restore's Cluster
The in-place CNPG restore skips deleting the live Cluster when it carries bootstrap.recovery and was created after the RestoreJob started, taking that as "this restore's own purge and re-render produced it". The timestamp does not say which restore rendered the Cluster. A Cluster deleted while the restore waits for the WAL archive is re-rendered by the still-active HelmRelease from the previous restore's values and is created after StartedAt too; the restore then skipped its purge and reported Succeeded on the previous restore's data. The restore patch already sets bootstrap.newServerName to restoredServerName(cluster, RestoreJob UID). The chart now annotates the Cluster with it as postgres.cozystack.io/restored-server-name, whether or not it renders the barman plugin that also carries the value, and the purge is skipped only when the annotation names this RestoreJob. The mark lives on the rendered Cluster rather than in RestoreJob status, so it still covers the failed status write the skip exists for. A Cluster rendered by a chart that predates the annotation keeps the creation-time check. Fixes #3322 Assisted-by: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Yasar Rahman
|
f6f879cf38 |
fix(postgres)!: refuse plaintext sessions on external instances
CloudNativePG offers TLS but its built-in last pg_hba rule, host all all all, also accepts plaintext, so a client outside the cluster that connects with sslmode=disable gets an unencrypted session. tls.enabled only decides whether the external hostname goes into the certificate and cannot prevent this. When external is true the chart now renders spec.postgresql.pg_hba with hostssl all all all scram-sha-256 followed by host all all all reject. CloudNativePG places user rules after its fixed rules and before its default rule, and the first match wins, so only TLS sessions are accepted. The pod network cannot be told apart from an external client, so in-cluster clients are held to the same rule; libpq's default sslmode=prefer already negotiates TLS. The new tri-state tls.required follows external when omitted. Setting it to false keeps accepting plaintext on an external instance and true requires TLS on an in-cluster one. The hostssl rule uses the same password method as CloudNativePG's own default rule (md5 before PostgreSQL 14, scram-sha-256 from 14), so existing md5 verifiers keep working on PostgreSQL 13. Rendering fails when postgresql.parameters.ssl is off while TLS is required, since hostssl would never match and every remote session would be refused. BREAKING CHANGE: external clients that connect without TLS are refused after the upgrade. Set tls.required to false to keep accepting them. Assisted-by: LLM Signed-off-by: Yasar Rahman <yasar.rehman@gmail.com> |
||
|
Yan Bondarenko
|
9c0085c358 |
fix(postgres): cap shared_buffers at the memory request
CloudNativePG rejects clusters whose shared_buffers exceeds the memory request. Memory overcommit can put that request below both a quarter of the limit and the 16MB floor. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
6dbd76ae50 |
fix(postgres): cap the default shared_buffers at a quarter of the memory limit
The chart renders no shared_buffers, so PostgreSQL starts with its 128MB default whatever the container gets. A full pool is the whole memory limit on t1.nano and half of it on t1.micro, the default preset. The pool is shared memory that the kernel cannot reclaim without swap, so when a dump, index scans over a large table or a replica catching up after a failover fill it, the instance is OOMKilled. Instances below a 512Mi memory limit now default to a quarter of the limit, at least 16MB. From 512Mi up a quarter is at least 128MB, so those instances render exactly as before. shared_buffers needs a restart, so on upgrade CloudNativePG restarts every instance below 512Mi that does not set it explicitly. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
IvanHunters
|
3cce629535 |
feat(redis,postgres): certify the per-engine external hostname (#4627)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Redis and postgres server certificates now carry the per-engine external name `<release>.<engine>.<apex>` (for example `redis-foo.redis.example.org`) next to the flat `<release>.<apex>` they already had. Both names appear under the same condition as before: TLS on and `external: true`, with a tenant apex present. A TLS-passthrough listener on the tenant Gateway forwards the handshake untouched, so the certificate the client checks is the engine's own. The design in #2815 routes databases by SNI under `*.redis.<apex>` and `*.postgres.<apex>`, and without the per-engine name in the SAN list a verifying client would fail the hostname check. Issuing both names now means a later move between the two hostname schemes reissues no certificate and keeps clients on the same trust anchor. This is the certificate step of #2815. Nothing renders a route yet. The SAN list changes, so an existing external TLS release gets its server certificate reissued once on upgrade, from the same CA. For redis the operator then rolls the Redis and Sentinel pods one at a time, the same way it handles a renewal. The READMEs say who can reach the new name through a passthrough listener. For postgres that is only clients using direct TLS negotiation (server 17+, `sslnegotiation=direct`), because by default a plaintext SSLRequest comes before the handshake. `redis-cli` needs `--sni`. MongoDB is not in this PR. PSMDB issues the certificate itself and its CRD has no field for extra SANs, so adding the mongos name means the chart taking over the certificate. That needs its own change. Covered by helm-unittest: the name is present with TLS and external on, and absent with external off, with TLS off, and in a tenant with no apex. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note feat(redis,postgres): server certificates of external redis and postgres releases also carry the `<release>.<engine>.<apex>` name that the tenant Gateway's TLS-passthrough listeners are designed to route by. Existing external TLS releases get their server certificate reissued once on upgrade, from the same CA. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * PostgreSQL TLS certificates now cover both the tenant hostname and the per-engine `postgres` hostname for external connections. * Redis TLS certificates now also cover the per-engine `redis` hostname when external services are enabled. * **Documentation** * Clarified connection requirements for per-engine hostnames: PostgreSQL routing requires direct TLS negotiation, PostgreSQL 17 or later, and a compatible client; Redis clients must specify SNI. * Clarified that the hostnames may need to be mapped manually to the LoadBalancer address. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
48e3d1d611 |
feat(redis,postgres): certify the per-engine external hostname
The tenant Gateway's TLS-passthrough listeners are designed to route databases by SNI under a per-engine subdomain (*.redis.<apex>, *.postgres.<apex>), and a passthrough listener forwards the handshake untouched, so the engine's own server certificate must carry <release>.<engine>.<apex> or a verifying client fails the hostname check. The name is issued next to the existing flat <release>.<apex>, under the same TLS-and-external condition, so that a later move between the two hostname schemes reissues no certificate and leaves clients on the same trust anchor. Nothing renders a route yet; the certificate is prepared ahead of it. Adding the name changes the SAN list, so existing external TLS releases get their server certificate reissued once on upgrade, from the same CA. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
520af57905 |
chore: drop private cluster, host and personal names from the tree
Private infrastructure names, a real hostname, a commented-out internal mirror block, an internal product name, and a reviewer's name in a comment do not belong in a public repository; drop them and replace what needs a stand-in with neutral placeholders. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Mattia Eleuteri
|
21fa1fcdb1 |
feat(postgres): add per-user createRole and bypassRls attributes
Without these, a tenant that needs to manage its own roles has no option but the postgres superuser, which also hands over filesystem access and command execution on the instance. Role attributes are not inherited through role membership, so the generated <db>_admin group role cannot carry them: they have to be set on the login user itself. users.<name>.createRole and users.<name>.bypassRls set CREATEROLE and BYPASSRLS on a managed login user. Both are reconciled on every init-script run, so setting one back to false emits the NO... form and revokes the attribute. createRole is refused on PostgreSQL 15 and older. Before PostgreSQL 16, CREATEROLE allowed granting membership in any non-superuser role, so a user holding it could grant itself pg_execute_server_program and reach command execution on the instance, which is exactly what not exposing the superuser prevents. PostgreSQL 16 restricted CREATEROLE to roles the grantee administers. bypassRls carries no such gate: BYPASSRLS only waives row-level security inside the tenant's own databases, whose owner already reads every row, so it adds no reach. The two attributes are needed together, not separately. PostgreSQL only lets a role create other roles carrying BYPASSRLS if it holds the attribute itself, so provisioning a privileged service role fails with "Only roles with the BYPASSRLS attribute may create roles with the BYPASSRLS attribute" when only createRole is granted. createRole also sets createrole_self_grant as a role-level default. PostgreSQL 16 grants a CREATEROLE holder only ADMIN OPTION on the roles it creates, governed by that parameter, whose default is empty. ADMIN OPTION allows granting the role onward but not SET ROLE to it, and creating an object owned by another role requires SET. The attribute therefore looks functional -- CREATE ROLE succeeds -- while the next statement a tenant writes, CREATE SCHEMA ... AUTHORIZATION <new role>, fails with "must be able to SET ROLE". The default is reset when the attribute is withdrawn. The init job runs as the superuser, and before this change it trusted two things a CREATEROLE holder controls for the roles it creates: role comments and role names. It picked the users and roles to drop by their comment, and a PostgreSQL 16 CREATEROLE holder may comment every role it created, then hand it to the chart for deletion. It adopted any role that already existed under a declared name, so a role created under a name an administrator declares later stayed under its creator's control. Neither can be caught from the catalog: creating a role through an intermediate role and then dropping that role leaves the squatted role with no member at all, while the objects, settings and default privileges planted in it stay. The chart therefore records the roles it manages as members of "cozystack:managed", a role only the superuser can grant, and the delete passes act only on those. Its first run marks every role the chart had commented before, when no role could yet hold CREATEROLE through it. Once any user has held createRole, recorded as a setting of the postgres database, the chart refuses to adopt a role it did not mark and fails with the role's name, rather than hand a squatted role REPLICATION or the ownership of a database. Verified against live PostgreSQL instances. On 17 a migration checklist replayed as the login user, with no superuser: role creation with attributes, CREATE SCHEMA ... AUTHORIZATION with the requested owner, extensions in a named schema, per-role search_path and timeouts, RLS including FORCE ROW LEVEL SECURITY, and SECURITY DEFINER functions all succeed. On 17 the forged-comment and squatting paths are refused or ignored, the upgrade from the previous chart marks the roles it already managed, and a fresh install still adopts the role CNPG bootstraps for users.app. On 13, 15 and 16 the init script runs and re-runs cleanly. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Aleksei Sviridkin
|
9734a16692 |
feat(postgres): add a postgis image flavor (#4587)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Adds a `flavor: postgresql | postgis` enum to the `postgres` app. `postgis` runs the cluster on the CloudNativePG PostGIS operand image, so tenants can enable `postgis`, `postgis_raster`, `postgis_topology`, `postgis_sfcgal`, `postgis_tiger_geocoder` and `address_standardizer` through the existing `databases.<name>.extensions`. The default stays `postgresql` and renders exactly the image it rendered before this change. This picks up #2672 by @matthieu-robin, which the stale bot closed after a rebase request, with these changes: - The postgis tags in #2672 (`17.7-3.5`, `16.11-3.5`, ...) do not exist in `ghcr.io/cloudnative-pg/postgis`; the registry answers 404, so the flavor would have ended in `ImagePullBackOff`. The map now uses tags that exist, `<minor>-<postgis>-standard-trixie`, on the same PostgreSQL minor as `files/versions.yaml`. - v18 is supported: PostGIS images for 18 now exist upstream. - `standard` rather than `system`: the chart archives through the barman-cloud plugin since the backup rework, so the operand no longer needs the barman binaries of the deprecated `system` variant. - `hack/update-versions.sh` derives the postgis map from the minors it picks, and `hack/check-postgres-postgis-versions.bats` fails if the two maps drift to different minors or away from `standard-trixie`. - The chart refuses a flavor change on a live `Cluster`. The default images are built on Debian bullseye (glibc 2.31) and the postgis ones on trixie (glibc 2.41): CNPG would roll the swap out as a plain image update, under an initialised data directory, which silently invalidates every index on collatable text, and the postgis-to-postgresql direction also drops the libraries behind every PostGIS object. Only the image name is compared, so minor bumps and registry mirrors keep passing. - `pgrouting` is not claimed: it is absent from the `18.1-3.6.1` build the chart pins and only appears in later postgis builds. A general mechanism (CNPG ImageVolume extensions) was considered and left out: it needs PostgreSQL 18 and Kubernetes 1.35 or the `ImageVolume` gate, so it cannot serve v13-v17 nor most management clusters today, and PostGIS is the only extension CloudNativePG ships as a full operand image. No other extension request exists in this repository. Testing: - `helm unittest packages/apps/postgres`: 145/145, including 6 new cases in `tests/flavor_test.yaml` (default image, postgis on the Cluster and on the init Job, the guard in both directions with a mocked live Cluster, and a mirrored digest-pinned image taking a new minor). Removing the guard makes exactly the two guard cases fail. - `hack/cozytest.sh hack/check-postgres-postgis-versions.bats` passes, and fails on a shifted minor or a `system` tag. - `make -C packages/apps/postgres generate` leaves no diff. - The rendered `ghcr.io/cloudnative-pg/postgis:18.1-3.6.1-standard-trixie` was started locally: `CREATE EXTENSION` for citext, postgis, postgis_raster and postgis_topology succeeds and `ST_Buffer` answers. A three-instance cluster on the `system` sibling of that image has been running PostGIS in production since 2026-09-29. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: cozystack/terraform-provider-cozystack#63 (issue: the provider models every field by hand, and the default handling needs a decision there) - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note feat(postgres): add a `flavor` field accepting `postgresql` (default) or `postgis`. `postgis` bootstraps the cluster from the CloudNativePG PostGIS image, so PostGIS extensions can be enabled per database through `databases.<name>.extensions`. The flavor is fixed at creation: changing it on an existing cluster is refused, and so is restoring a backup into a cluster of the other flavor, whether through a RestoreJob or `bootstrap.enabled`; move data across flavors with a logical dump. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a `flavor` option for PostgreSQL clusters. The default `postgresql` flavor remains available, while `postgis` selects an image with PostGIS extensions. - PostGIS images are available for PostgreSQL versions 13–18. - Cluster flavor is fixed at creation. To switch flavors, create a new cluster and migrate data with a logical dump. - **Bug Fixes** - Restores are refused when the backup and target cluster use different flavors. - **Documentation** - Updated PostgreSQL configuration guidance to describe flavor options and limitations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
fb7e780517 |
fix(postgres): reassign dropped roles' objects to an unprivileged role (#4574)
## What this PR does Objects owned by a role the Postgres init-job drops no longer end up owned by the `postgres` superuser. This covers a user removed from `users` and an orphaned managed `<db>_admin` or `<db>_readonly` role. Before dropping such a role the job reassigned everything it owned to `postgres`, and objects a tenant role created should not become superuser-owned. They now go to `cozystack:orphaned`, a NOLOGIN role that is granted nothing. The init-job creates it and resets its attributes on every run. The `:` in the name is outside the charset allowed for user and database names, so values cannot declare it. There is a trade-off. Grants and memberships of the dropped role do not carry over, so a view or function it owned that ran with its owner's privileges can lose access it had through them. To keep such objects working, keep the user in values or move the objects to another owner before removing it. `docs/operations/backup-classes.md` is updated accordingly. Role names read from the catalog now reach SQL as psql variables instead of being spliced in by the shell. The queries are echoed after substitution, so the job log shows which role is dropped. Tested with helm-unittest and by running the rendered init script against PostgreSQL 13 and 18. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(postgres): objects owned by a user removed from `users`, or by an orphaned managed database role, are now reassigned to the unprivileged `cozystack:orphaned` role instead of the `postgres` superuser. A view or function of the removed role that ran with its owner's privileges can lose access it had through that role's grants or memberships. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified that after restore, the first initialization releases undeclared managed databases without deleting their data and removes users not declared in the chart. * Removed users’ objects are reassigned to a restricted, non-login role. Grants and memberships are not preserved; functions use invoker rights, and PostgreSQL 15+ views are set to use invoker security. * Added guidance for finding eligible objects still owned by the PostgreSQL superuser for manual reassignment. * Declare a user in `users` to keep it; the chart will generate a new password. * **Bug Fixes** * Removed users’ objects are reassigned across connectable, non-template databases before their users are dropped. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Mattia Eleuteri
|
f714ada567 |
fix(postgres): refuse a physical restore across image flavors
A recovery replays the source's data directory, so it carries the same risk as switching the image of a live cluster: the postgresql and postgis images are built on different Debian releases, and the glibc change invalidates collatable indexes, while a postgis-to-postgresql move also loses the libraries behind every PostGIS object. The chart only guarded the live cluster, so a fresh release bootstrapped from a backup of the other flavor went through, and the guard's own advice was to restore a backup, which is that very move. The backup-controller now records the source flavor in the snapshot it stores on each Backup and fails a RestoreJob whose target runs another flavor, before the purge deletes the target's Cluster and PVCs. A snapshot without a flavor predates flavors and so comes from a postgresql cluster. For the chart-managed bootstrap, the chart compares against the recovery source's Cluster while it still exists. Both paths, and the live-cluster guard, point a flavor change to a logical dump. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
e4810d3268 |
fix(postgres): require a postgis image for every postgres major
The updater used to warn and leave a major out of postgis-versions.yaml
when the registry had no postgis build for its minor yet, while
versions.yaml still advertised it. `flavor: postgis` on that major then
failed to render. Postgis images trail the PostgreSQL ones, so this is a
routine state rather than an edge case: stop before writing either map
and let the next run pick up both once the image is published.
The version check now also compares the majors of the two maps, so a
map edited by hand cannot drop one silently. It is written in POSIX sh:
cozytest.sh sources it into /bin/sh, which is dash on the CI runners and
rejected the ${var//} expansions the first version used.
Assisted-by: LLM
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
|
||
|
Mattia EleuteriandMatthieu
|
dc2971264d |
feat(postgres): add a postgis image flavor
Tenants migrating spatial data need PostGIS, but the chart always runs the CloudNativePG PostgreSQL image, which does not ship it, and exposes no way to pick another one. CloudNativePG publishes a PostGIS operand image; a closed `flavor` enum selects it without handing tenants a free image field. The flavor pins the same PostgreSQL minor as the default one, on the standard-trixie variant: backups go through the barman-cloud plugin, so the deprecated system variant is not needed. update-versions.sh now derives the postgis map from the minors it picks, and a bats check fails if the two maps drift apart. The default images are built on bullseye and the postgis ones on trixie. Switching a live cluster between them would swap glibc under an initialised data directory and silently break every collatable index, and CNPG would roll it out as a plain image update. The chart therefore refuses a flavor change on an existing Cluster. Co-authored-by: Matthieu <matthieu@hidora.com> Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Aleksei Sviridkin
|
810d293421 |
fix(postgres): reassign dropped roles' objects to an unprivileged role
When a user leaves values, or a managed database role is orphaned, the init-job reassigned everything the role owned to the postgres superuser before dropping it. Objects a tenant role created should not become superuser-owned. They now go to "cozystack:orphaned", a NOLOGIN role whose attributes and memberships the init-job resets on every run; its name is outside the charset allowed for users and databases, so values cannot declare it. Because that one role receives the objects of every dropped role, functions it owns are switched to SECURITY INVOKER and, on PostgreSQL 15 and later, its views to security_invoker. The cost is that grants and memberships of the dropped role do not carry over, so a view or function it owned can lose access it had through them; keep the user in values, or move such objects to another owner first, to preserve it. The change is not retroactive: objects earlier releases reassigned to postgres stay superuser-owned, and the backup docs describe how to find and hand them over. The orphaned-roles pass now reassigns and drops in every connectable database, as the user pass already did, so a role owning an object outside the default database no longer fails DROP ROLE on every run. Role names read from the catalog now reach SQL as psql variables instead of being spliced in by the shell, and the queries are echoed after substitution so the log names the role being dropped. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Mattia Eleuteri
|
25cdd51881 |
feat(postgres): allow preloading an allowlist of shipped modules
shared_preload_libraries is blocked in postgresql.parameters, and rightly so: the parameter is free text and accepts a path, so it can point at a writable location such as the PGDATA volume. What the block leaves is an indirect route. CloudNativePG preloads pg_stat_statements, auto_explain and pgaudit by itself as soon as a parameter under their prefix is set, such as pg_stat_statements.track, so enabling one of them today means setting one of its tuning parameters, even to its default value. pg_prewarm has no route at all, and its autoprewarm worker only runs when the module is preloaded. postgresql.sharedPreloadLibraries is the explicit switch. It is a closed enum of four module names that already ship in the CloudNativePG PostgreSQL image, rendered onto the CNPG Cluster field of the same meaning, which the operator merges with the libraries it preloads on its own. A bare name can only resolve inside $libdir, which sits on the read-only image filesystem, and dynamic_library_path stays blocked, so no path can be introduced. The blocklist on postgresql.parameters is left untouched. The template repeats the enum as a backstop for renders that skip schema validation. helm-unittest always validates the schema and never reaches it, so a bats suite renders with --skip-schema-validation to keep the backstop, and its agreement with the enum, under test. Verified on a live CloudNativePG 1.27.3 cluster: with the field set, the operator writes shared_preload_libraries into postgresql.conf, SHOW reports the modules, and pg_stat_statements returns rows. Changing the list restarts PostgreSQL on every instance, the primary included, through the instance manager and without restarting the pod. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
Mattia Eleuteri
|
497dd6fc4f |
fix(backups): start a Postgres restore from the backup it names
Without recoveryTarget.backupID the barman-cloud plugin chooses the base backup itself: the newest one in the catalog when no recoveryTime is set, and the newest one ending by recoveryTime otherwise, on any timeline since no targetTLI is passed. A RestoreJob naming an older Backup therefore started from another one, and on an archive holding sibling timelines - left by in-place restores that archived back under the source serverName - from another branch, bringing back that branch's data. The restore reads barman's backupId and the backup's stoppedAt from the live cnpg.io/Backup behind the Backup, and passes the id through the chart as bootstrap.recovery.recoveryTarget.backupID. The plugin gives backupID precedence over time-based selection, and CNPG does not count it among the mutually exclusive targets, so it combines with targetTime. Two cases keep the plugin's choice. A recoveryTime earlier than one second past stoppedAt: the named backup cannot serve an instant before it is consistent, and stoppedAt is truncated to the second while barman compares its end time to the microsecond. And a cnpg.io/Backup that is gone: plugin-barman-cloud's retention deletes it together with the base backup it describes, and the driver deletes one only with its artifact, so pinning its id would purge the target and then fail the recovery on every retry. That restore records a BaseBackupGone warning event instead. A re-restore into the same target clears a backupID left by a previous one. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <mattia@hidora.io> |
||
|
IvanHunters
|
2fa51bf587 |
test(charts): isolate guard coverage for every guarded template
Add per-template guard suites so each guarded hostname site is exercised on its own: harbor harbor.yaml and httproute.yaml, bootbox machines.yaml, seaweedfs cosi-deployment.yaml, bucket deployment.yaml and httproute.yaml, monitoring alerta.yaml, grafana httproute.yaml and the grafanaHost helper via oidc-keycloak.yaml. Each suite renders only its target template, so removing that file's guard reddens it. Also add a postgres positive case that renders the Certificate with an apex set (external=true). Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
0f4d39f1ef |
test(charts): add empty-apex guard tests for the remaining guarded charts
Add host_guard_test.yaml for postgres, vpn, bootbox, seaweedfs, monitoring and bucket. Each renders its guarded template with a non-empty injected _namespace whose host is empty and no per-app host override, and asserts the ns-host-required failure, plus a positive render case with the apex set. Completes guard coverage alongside the existing harbor, qdrant and kubernetes tests. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
37f59369d3 |
fix(charts): fail fast on empty tenant apex in host composition
Charts that publish a service compose their hostname as <sub>.<_namespace.host>. When the injected tenant apex is empty and no per-app host override is set, they used to emit a malformed trailing-dot hostname (e.g. "harbor.") into Ingresses, HTTPRoutes and Certificate dnsNames silently. Source the apex through cozy-lib.ns-host-required at each hostname-emitting site so the render fails with a clear message instead, matching the gateway path. Covered: harbor, postgres (external), qdrant (external), kubernetes (control-plane endpoint), vpn, seaweedfs, bootbox, monitoring (grafana, alerta, oidc client), and system/bucket (adds the cozy-lib subchart symlink it lacked). The guard is placed at the point each resource is actually emitted, so standalone renders, no-exposure tenants and the kubernetes no-etcd beacon path are unaffected. The post-delete cleanup hooks are left unguarded so an empty apex cannot block uninstall. Behavior change: a tenant with an empty injected apex and no host override that enables one of these components now fails its render (HelmRelease NotReady) instead of deploying a broken hostname. In practice the apex is always injected (root has root-host, children inherit it), so this only trips a genuinely misconfigured tenant. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Aleksei Sviridkin
|
09d02fdc6c |
docs(apps): correct the shared resources and preset paragraphs (#4394)
## What this PR does Corrects two sentences of the boilerplate shared by the app READMEs, both raised in the review of #3936. - "When left empty, the preset defined in `resourcesPreset` is applied" describes an all-or-nothing rule, but `cozy-lib.resources.defaultingSanitize` merges per key: every resource `resources` leaves unset is taken from the preset. Twelve READMEs carried the sentence. - The legacy flat names were called aliases of their "1:1 instance-type equivalents", which reads as the `c1` series listed just before it. `nano` through `small` equal the `t1` sizes, and `medium` equals `c1.small` rather than `c1.medium`, the trap `docs/operations/resource-presets.md` already warns about. Ten READMEs carried the sentence. Only the hand-written section changes. `make generate` for all twelve packages leaves the diff at exactly those 22 lines. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff: README prose only, no values, schema, default or package change. The website regenerates the app reference pages from these READMEs on release, so nothing to open there. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified how resource settings work across several app READMEs: any unset resource field now inherits its value from the preset, rather than requiring the entire resource block to be empty. * Updated legacy preset name guidance to explain that deprecated flat names keep their original sizes, including `nano`–`small` matching `t1` sizes and `medium` matching `c1.small`. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
237f2e2bbd |
fix(postgres): say how to get past a refused extension clause
Before the charset guard, an extension value was spliced into CREATE EXTENSION whole, so a release can carry a clause after the name, such as earthdistance CASCADE, and that is the usual way to pull in a dependency. The guard refuses such a value on upgrade too, and keeping it that way is right, but the tenant needs to know what to write instead. The reject message now says: drop the clause, which changes nothing for an extension that is already installed because the script never drops one and CREATE EXTENSION IF NOT EXISTS leaves it alone, and list the dependencies first, since extensions are created in list order. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
70dcd9daed |
fix(backups): let a restored Postgres app converge its credentials
A restore sets bootstrap.enabled on the target app, which skips the init-job, so the recovered roles keep the source's password hashes while the target's credentials Secret holds generated ones and every application login fails. Once recovery converges, the driver records RecoveryConverged and then clears bootstrap.enabled, so the next HelmRelease reconcile runs the init-job and applies the passwords. Ready carries RestoreCompletedCredentialsPending and an Event says the convergence is still pending. A clear that keeps failing is retried under BootstrapDisablePending within a fixed grace window after convergence and then fails the restore. bootstrap.enabled is serialized without omitempty so the patch sends false instead of dropping the key. Running the init-job on a restored cluster exposes its delete pass to a catalog the spec does not describe: the driver sets spec.databases to the Backup's snapshot, but recovery replays WAL past it, so a database created after the Backup comes back still marked as managed and would be dropped with its data. The first run after a restore, told apart by the per-RestoreJob bootstrap.newServerName that the cluster has not yet recorded, releases such a database and its roles from management instead, and the run records the value once every pass has succeeded. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
2b5877eff9 |
feat(postgres)!: generate user passwords and read them at runtime
users[].password is gone from the chart, its schema and the API type. Every application-user password is generated into <release>-credentials and kept across reconciles by lookup, so an upgraded release keeps the password it already has and a password left in values is ignored. The init-job mounts that Secret and applies each password with ALTER ROLE and a psql variable, so no password is rendered into the init-script Secret that Helm keeps in release history. While bootstrap is enabled the Secret is marked credentials-pending, and NOTES say the generated passwords apply once the init-job runs. Names reach superuser SQL and the init-script shell, so render-time guards reject a username, database or extension name outside [-._a-zA-Z0-9] or past the length PostgreSQL silently truncates: 63 bytes for a role or extension, 54 for a database, whose <db>_readonly role would otherwise truncate and fail the next run. A username over 63 bytes that the credentials Secret already carries still upgrades. users.postgres, the CNPG superuser, is rejected in every bootstrap state. Changing a database's key in values drops the database, so the reject message and the README give the rename that keeps the data: a platform administrator renames it and its roles as the CNPG superuser before the key changes. The init-job cannot hang its Helm hook: the readiness wait ends on elapsed time below activeDeadlineSeconds and the Job never retries, so a purged restore target fails with a diagnostic. The delete-users pass reads managed users from pg_shdescription instead of parsing the version-dependent \du+ layout, which on PostgreSQL 16 and later matched nothing and left a removed user able to log in; it compares names as CREATE ROLE stores them and reassigns a removed user's objects in every database before dropping it. A database without roles renders, and extension names are quoted for hyphenated names and folded to lowercase so they resolve as they did unquoted. A value that carries its own surrounding quotes, the only way a hyphenated extension rendered before, has that pair removed and keeps its case, so such a release still upgrades. The README reads a password with jq, since jsonpath splits a dotted key. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Yan Bondarenko
|
cdcb2710cd |
docs(apps): correct the shared resources and preset paragraphs
Two sentences of the boilerplate shared by the app READMEs were wrong. "When left empty, the preset is applied" described an all-or-nothing rule, but cozy-lib.resources.defaultingSanitize merges per key: every resource that resources leaves unset comes from the preset. And the legacy flat names were called aliases of their "1:1 instance-type equivalents", which reads as the c1 series; nano through small match t1, and medium matches c1.small rather than c1.medium, the trap docs/operations/resource-presets.md already warns about. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
4dc6065ec9 |
fix(backups): give the sidecar sizing its reason per caller
The Keycloak comment and the helper doc claimed the tenant LimitRange as the reason for the resources, but cozy-keycloak has no LimitRange and neither does a tenant that leaves resourceQuotas empty: there the sidecar had no requests and no limit at all, and the change gives it a 1Gi ceiling where there was none. The comments and both doc blocks now say what applies to each caller. No rendered object changes. Assisted-by: LLM Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Yan Bondarenko
|
9214430bd8 |
fix(backups): give the barman-cloud sidecar its own resources
The barman-cloud plugin injects its sidecar without resource requests or limits. Tenant namespaces ship a LimitRange that defaults containers to 128Mi, so the sidecar inherits that default and is OOMKilled mid-backup: the ObjectStore stays healthy, the Cluster stays Ready, and only the backup fails. A cgroup high-water mark of 254 MiB on a 38 MB database shows the default cannot hold it. The shared cozy-lib helper and the Go path that builds the platform's own ObjectStore now request 100m/256Mi and cap memory at 1Gi. No CPU limit is set, matching the entityOperator precedent in the kafka chart: a throttled sidecar stalls WAL archiving instead of failing it. The helper is renamed from checksumSidecarConfiguration to sidecarConfiguration, since it no longer carries only the checksum pin. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com> |
||
|
Alexey Artamonov
|
9033d41bc0 |
test(postgres): assert the job pin on each autoscaler selector, not once for the query
The pin check matched job="kube-state-metrics" anywhere in the trigger query, so one pinned selector satisfied it no matter what the other three did: unpinning $joinReplica, $idleFloor or $joinCluster individually left the suite green while the join went back to matching any exporter's series. Anchor one assertion per selector on the label that identifies its site so a single dropped pin fails. Assisted-by: LLM Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
2d1e425339 |
docs(postgres): correct the KEDA-guard rationale and note the gradual client-side rollout
The KEDA-missing fail-closed guard's comment still described the replaced
drop-instances design ("db.yaml drops spec.instances ... CNPG collapses to 1");
under the shipped seed design db.yaml seeds instances at max(replicas,
effectiveMin), so a skipped ScaledObject leaves a fixed-size cluster with no HPA
- autoscaling silently absent, not a collapse. Correct the rationale. Also note
on the cozyrds apply-strategy annotation that the client-side switch rolls out
per-app as each Application is next written through the aggregated API, not
fleet-wide at upgrade (the ApplicationDefinition reconciler syncs only
chartRef/valuesFrom/labels onto a live HelmRelease). Follow-up from #3954 (#4187).
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
1f2ff31470 |
fix(postgres): pin job=kube-state-metrics on the autoscaler read-load query
The read-load trigger query joined on kube_pod_labels without pinning job="kube-state-metrics", while the sibling DatabaseAutoscaler* alert rules already pin it. A second exporter emitting a kube_pod_labels series (or a duplicate scrape) would multiply the join and distort the desired-count arithmetic. Pin the job on every kube_pod_labels selector in the query for parity with the alerts. Follow-up from #3954 (#4187). Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
scooby87
|
afc003c464 |
feat(postgres): database read-replica autoscaling via KEDA (#3954)
## What this PR does Adds automatic horizontal autoscaling of a PostgreSQL database's **read replicas** in response to load, implementing the merged design proposal [Database Horizontal Autoscaler](https://github.com/cozystack/community/tree/main/design-proposals/database-horizontal-autoscaling) (community#44). The mechanism is entirely stock — no bespoke operator, no new CRD: - **KEDA as an optional platform component** (`packages/system/keda`, wired into `core/platform` as a source + system-bundle entry). It claims the `external.metrics.k8s.io` APIService that nothing served before. - **`cozy-lib.keda.scaledObject` helper** (`packages/library/cozy-lib/templates/_keda.tpl`) renders a KEDA `ScaledObject`, fail-closed on missing/invalid arguments. - **`packages/apps/postgres` autoscaling** — an `autoscaling` values block (`enabled`, `minReplicas`, `maxReplicas`, `metric`, `target`, `maxReplicationLagSeconds`, `dryRun`, `transition`) validated by `values.schema.json`; under active autoscaling the chart renders a `ScaledObject` and seeds `spec.instances` to a constant floor (`max(replicas, effectiveMin)`), while the postgres release is forced to client-side (two-way) Helm apply so the seed is a merge no-op on upgrade and the KEDA-managed HPA — the runtime writer via the CNPG `Cluster` scale subresource — is not reverted. - **Single-value metric (§1)** — one PromQL query returns `Σ(read load over standbys) + target`, so a stock HPA computes `desired = ceil((Σ+target)/target) = 1 + ceil(Σ/target) = primary + desiredRead`, with no per-pod emission. - **The four static brakes (§5)** — quorum floor `max(minReplicas, maxSyncReplicas+1, 2)` (quorum wins over `maxReplicas`), scale-down pacing (`Pods/1/600s`), a replication-lag clamp folded into the query (freeze both directions while lag exceeds the threshold and the primary is writing, with `max_over_time` hysteresis), and dry-run (render paused). - **Two-phase enable/disable migration** via a `transition` sub-flag: phase 1 stages `spec.instances` to the live count and brings KEDA up with its HPA paused in both directions; flipping to phase 2 hands the runtime count to KEDA, and because the seed re-renders the same count the flip is a merge no-op with no dip. - **Observability** — a database-autoscaling Grafana dashboard and PrometheusRule alerts. **Live validation (dev cluster, CNPG 1.30.0).** The full path works end-to-end: the CNPG `Cluster` scale subresource serves `status.selector`, KEDA renders the HPA, the metric reaches it (`ScalingActive=ValidMetricFound`, `TARGETS 151/150`, `desiredReplicas=2 = 1 primary + 1 replica` — the §1 arithmetic), and the lag-clamp query returns the base value on live VictoriaMetrics. Two packaging fixes were needed and are included: KEDA must run on the cozystack `cozy.local` cluster domain (the upstream `cluster.local` default makes the metrics gRPC target NXDOMAIN and every HPA hangs on `FailedGetExternalMetric`), and the operator's client-go throttle is raised so a single-replica operator does not lose its leader lease under scale-subresource polling. Implements #3953. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. Two repositories are affected; both are follow-ups to open once this lands (and after its dependency #3951 merges and the values surface is final), left unticked here for a maintainer to confirm rather than claimed with a speculative link: - **cozystack/terraform-provider-cozystack** — this adds the nested `autoscaling` block to `packages/apps/postgres/values.schema.json`. The provider hand-models each app's fields (schema + model + expand/flatten), so it needs the new block to expose it in Terraform. - **cozystack/website** — KEDA is a new platform component (`guides/platform-stack`), and the postgres reference page gains the `autoscaling` params. The managed-app reference is regenerated from `README.md` by the release bot; the platform-stack component list is hand-maintained. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: to open (new platform component + postgres autoscaling params) - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: to open (new `autoscaling` block in postgres values.schema.json) ### Depends on - #3951 — bump CloudNativePG to 1.30.0. The HPA can only drive the CNPG `Cluster` scale subresource on CNPG ≥ 1.28.4, where it serves `status.selector`; without it the HPA fails with `InvalidSelector`. This PR should not merge before #3951. ### Release note ```release-note feat(postgres): add optional horizontal autoscaling of read replicas via KEDA, configured through the new `autoscaling` values block; KEDA ships as a new optional platform component. Requires CloudNativePG >= 1.28.4 for the Cluster scale subresource selector. Note: every Postgres release now applies client-side (`release.cozystack.io/helm-server-side-apply: "false"`) so the autoscaler's live `/scale` count is not reverted by Helm; a kind-wide side effect is that chart-rendered fields on Postgres releases are no longer re-asserted by server-side-apply drift correction (driftDetection is unset), a documented trade-off with a per-instance apply strategy as follow-up. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable PostgreSQL read-replica autoscaling with KEDA. * Added CPU and read-connection metrics, replica bounds, replication-lag safeguards, dry-run mode, and transition handling. * Added optional KEDA platform support, Grafana autoscaling dashboard, and Prometheus alerts. * **Documentation** * Clarified integer thresholds, prerequisites, configuration, and transition guidance. * **Bug Fixes** * Improved validation and actionable errors for unavailable KEDA or invalid settings. * **Tests** * Expanded coverage for autoscaling, observability, package selection, and validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
9511f56307 |
fix(postgres): require service.namespace.svc shape in the autoscaling SSRF host guard
The serverAddress host regex accepted any dotted tail ending in .svc, so a single-label host like foo.svc passed the in-cluster-DNS check. It then reached the namespace-pin, whose extraction regex left the unresolvable host unchanged and rejected it as a namespace outside the tenant subtree - the right outcome by the wrong path, with a misleading message. Anchor the host to two DNS labels before .svc so it is refused at the host check with the actionable message, and drop the stray dot the old label class allowed. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
678d22f209 |
docs(postgres): move the serverAddress namespace-pin note out of a template comment
Closes IvanHunters' MINOR on scaledobject.yaml:97. The API-owner-gate marker was a Go template comment, stripped at render, so the ownership question and the pin's residual surface were recorded where nobody reads them again. State the constraint on the serverAddress field doc (propagated into values.schema.json, README and the cozyrds openAPISchema) including the accepted residual: tenant-root is always in the allow list because the derived default points there, so a tenant can still aim the operator at a root Service as a blind in-cluster probe (no response body reaches the tenant), which the egress NetworkPolicy does not narrow. The API-owner sign-off is tracked on the PR, not in a stripped comment. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
07f1daa22d |
docs(postgres): correct the replicas "restored on disable" claim
Closes IvanHunters' MINOR on values.yaml:58. The replicas description (propagated into values.schema.json, README and the cozyrds openAPISchema) said the count is "restored on disable". Under client-side apply it is not: the two-way merge writes instances only when the rendered value changes, so a cluster KEDA grew past replicas stays there after autoscaling.enabled: false with no autoscaler left to reduce it — the sibling dryRun doc already states the true rule, so the two contradicted each other. Reword both the @param and the @typedef to say the live count is not auto-restored and that the operator lowers replicas to reclaim the grown capacity; regenerated the derived artifacts. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
3a09c3e9dc |
fix(postgres): guard dry-run/transition against the paused HPA min-clamp
Closes IvanHunters' MAJOR on scaledobject.yaml:39. The render guard rejected replicas > maxReplicas but not the symmetric low side. Under dry-run/transition the chart renders the static instances=replicas while the ScaledObject carries minReplicaCount=effectiveMin with both scale directions paused. The pause disables the HPA's scaling policies, not its min-replica clamp, so when replicas < effectiveMin the HPA raises the live count to the floor on its first reconcile — a scale-up in a mode documented as "actuates neither way" — and because the rendered instances value never changes, the client-side two-way patch never pulls it back, leaving permanent extra standbys and PVCs. Fail closed and tell the operator to stage replicas to the floor first. Under dry-run/transition this floor guard is strictly broader than the db.yaml quorum wedge (any replicas <= maxSyncReplicas is also < effectiveMin), so it supersedes it there; the wedge stays exercised on the disabled path. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
3516bbf314 |
docs(postgres): correct lag-brake floor and floor-raise shed comments
The lag-brake frozen summand's `or vector(0)` comment claimed it prevents a silent pin-to-floor when the pod-label count is momentarily empty under load. That state is unreachable: the frozen count selects the same kube_pod_labels series the base load term joins on, so base and frozen are empty or non-empty together and the floor never changes the result. State that it is a no-op marker. The enablement note framed raising the quorum floor / minReplicas as purely upward, but the merge rebases to the newly-rendered seed vs the previous seed, never to KEDA's live count — so raising the floor (or lowering replicas) to a seed still below a load-scaled live count sheds the surplus replicas and PVCs in one step before the HPA re-clones. Add the HAZARD note. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
187f18a167 |
fix(postgres): harden autoscaler serverAddress guard and cap maxReplicas
The serverAddress SSRF guard had two holes beyond the external-domain suffix already closed. It extracted the host by truncating at the first `:` or `/`, which a `http://<svc>.<ns>.svc:port@evil.example.com/` userinfo payload defeats: the truncated text passes the `.svc` check while net/url dials the host after the `@`. And it validated only the host shape, never the namespace, so a tenant could point the cluster-privileged KEDA operator at a sibling tenant's or a system namespace's Service (the egress NetworkPolicy's `cluster` entity does not stop in-cluster egress). Parse the URL authority properly (scheme, path/query/fragment, userinfo to the last `@`, then port) and pin the host's namespace to the tenant's own subtree via cozy-lib.rbac.allParentTenantsAndThis. The namespace pin narrows what a tenant may set and needs API-owner sign-off (noted at the guard). Separately, nothing bounded maxReplicas: the schema carries no maximum and the aggregated apiserver does not range-validate the CR, so a tenant could set maxReplicas: 100000 and drive KEDA to scale CNPG toward that count. Add a render guard rejecting a maxReplicas above a sane read-replica ceiling. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
d06354993e |
docs(postgres): correct the WorkloadMonitor replicas rationale
The comment claimed setting replicas to effectiveMin prevents a false under-provisioning alarm when KEDA idles the cluster to its floor. But WorkloadMonitorReconciler gates Status.Operational solely on availableReplicas < minReplicas and never reads spec.replicas, so with minReplicas: 1 the field is inert for health today. Keep the value (it is the honest desired-floor target per the field doc) but state what actually drives the alert, so the rationale is not read as describing behaviour the controller does not have. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
2bf37f8260 |
fix(postgres): anchor autoscaler SSRF guard to the cluster domain
The serverAddress in-cluster check accepted an unanchored trailing domain (`\.svc(\.[a-zA-Z0-9.-]+)?$`), so a crafted host like x.svc.attacker.com — a `.svc` label followed by any external domain — passed the guard. A tenant could then point the cluster-privileged KEDA operator off-cluster (the egress NetworkPolicy is the only remaining barrier, and it is inert where the CNI does not enforce CiliumNetworkPolicy). Anchor the optional suffix to this cluster's domain (from _cluster.cluster-domain, injected by the platform from networking.clusterDomain): a legitimate vmselect is <service>.<namespace>.svc or ...svc.<clusterDomain>, nothing else. The prior tests only exercised non-.svc hosts and gave false confidence; add a case for the .svc.<external> bypass. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
6083a9fa1d |
feat(postgres): guard replicas>maxReplicas and pin serverAddress to in-cluster
Close two render-guard follow-ups the design proposal names for this PR: - Fail closed when replicas exceeds autoscaling.maxReplicas. The stock HPA's currentReplicas > maxReplicaCount branch writes /scale = maxReplicaCount unconditionally on the first reconcile — even under the both-direction pause of dryRun/transition — so a resting/seed count above the ceiling would shed the surplus standbys and their PVCs in one step, past the scale-down pacing. Mirrors the existing effectiveMin > maxReplicas guard. - Refuse an autoscaling.serverAddress whose host is not an in-cluster Service DNS name (…svc[.<domain>]). The cluster-privileged KEDA operator fetches it every poll, so an external/IP address is an SSRF surface (cloud metadata endpoint, another tenant); the query body is already namespace-scoped, this restricts the destination. Defense-in-depth pairs with the operator egress policy (keda package). Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
c693ed36fb |
fix(postgres): force client-side apply so the autoscaler seed survives
Under active read-replica autoscaling the chart seeds spec.instances at a constant max(replicas, effectiveMin) rather than omitting it (an omitted field defaults to 1, which the CNPG quorum webhook rejects, and — under server-side apply — the phase-2 omit prunes the field whenever KEDA has not yet written /scale, collapsing an idle-at-floor cluster to 1 and shedding its replica PVCs). Keeping the constant only works if Flux does not force-own and revert it. The postgres ApplicationDefinition therefore carries release.cozystack.io/helm-server-side-apply: "false", so the tenant HelmRelease applies client-side: helm-controller patches the CNPG Cluster from the previous-vs-new rendered manifest without consulting live state, the unchanged constant yields no patch, and KEDA's higher live /scale count is preserved. The constant also equals the disabled/dryRun/transition render (instances: replicas) whenever replicas sits at/above the floor, so those flips are patch-free too. Correct the merge-semantics comments across db.yaml, _autoscaling.tpl and values.yaml to describe the two-way previous-vs-new patch precisely (not a "three-way no-op"), and document the residual floor-change hazard: editing replicas / minReplicas / quorum.maxSyncReplicas moves the seed, so that one reconcile does patch spec.instances to the new seed. Add autoscaling.serverAddress so a monitoring:true tenant can point the ScaledObject at a vmselect that carries both the CNPG metrics and kube_pod_labels. Regenerated README/schema/types/cozyrds. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
826f08972f |
fix(apps/postgres): make autoscaler fail-safe, dip-free, and observable
Address review findings on the KEDA read-replica autoscaler where the
implementation diverged from the merged design proposal.
Fail-safe read load. Gate the `or vector(0)` floor on the existence of ANY CNPG
instance pod (`count(kube_pod_labels{...instance_role=~".+"}) > 0`) — primary or
replica — so the query is empty ONLY when no CNPG pod labels exist at all (a
broken kube-state-metrics pipeline), not during initial provisioning when the
sole replica-side pod is CNPG's join Job (jobRole, not instanceRole). Also pin
`ignoreNullValues: "false"` on the trigger: KEDA's prometheus scaler defaults it
true, turning an EMPTY result into metric 0 (not an error) — the HPA would then
desire 0 and clamp to the floor, i.e. silently scale a broken-pipeline cluster
DOWN. With it false an empty result is a scaler error, so the HPA HOLDS and
DatabaseAutoscalerScalerErrors fires; gating on any instance role keeps a healthy
cluster (including a long basebackup) from tripping that alert.
Constant seed = max(replicas, effectiveMin), so enable/dryRun/transition flips
are a merge no-op with no dip and never undercut the quorum floor.
Fail closed when the quorum floor exceeds maxReplicas (was: effectiveMax silently
raised both bounds, unbounded for a large maxSyncReplicas).
Dry-run / transition keep the HPA (unidirectional pause pair, not the HPA-deleting
full pause) but both SelectPolicy=Disabled, so desired normalizes to current — the
recommendation is read from the read-load metric, not a diverging desired count.
Observability. DatabaseAutoscalerAtMax keys on desired >= maxReplicas, NOT the
ScalingLimited condition (which is also true at the floor, so it fired on every
idle cluster with wrong "raise maxReplicas" advice). Dashboard read-load panels
are per-cluster (label_cnpg_io_cluster) with a CPU panel for ReadCPUUtilization
and the exact clamp in descriptions; ScaleStuck's Pending intersection is scoped
to CNPG pods; alerts.yaml narrative comments moved to Go-template comments so they
do not ship into the PrometheusRule.
Guard the seeding invariant: TestBuildHelmReleaseSpecNoForceOrDriftDetection.
Regenerated README/schema/types.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
8769232997 |
docs(apps/postgres): correct autoscaling merge-safety notes
The disable case (3) note claimed disabling snaps instances back to .Values.replicas, but the client-side three-way merge compares the newly rendered value against the previously applied one, not the live count. When replicas == effectiveMin (the default) it is a no-op and KEDA's live count is preserved (silent over-provisioning), so it does not snap back. Reword case (3) and the "differs from the live one" framing to match the actual merge semantics. Re-point the no-Force/no-SSA invariant citation from internal/controller/applicationdefinition_helmreconciler.go (which holds no apply-strategy code) to internal/operator/package_reconciler.go buildHelmReleaseSpec, where the HelmRelease spec is actually built. Comment/doc only; helm unittest 52/52 unchanged. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
b40cf82642 |
fix(postgres): seed instances on both paths, add input guards, and harden alerts
Address IvanHunters' deep review round on #3954: - [MAJOR] Enabling autoscaling directly on an existing non-quorum database no longer collapses it to 1 instance and deletes every replica PVC. The chart now SEEDS spec.instances at the quorum floor (effectiveMin) on BOTH the quorum and non-quorum active paths instead of omitting the field in the non-quorum case; the constant seed is a three-way-merge no-op so KEDA still owns the runtime count via /scale, and the enable dip is capped at the floor (>=2) rather than collapsing to 1. Documents the client-side-3-way-merge (not SSA) assumption that the no-op relies on. - [MINOR] Fail closed on autoscaling.minReplicas > autoscaling.maxReplicas instead of silently resolving the ceiling upward (which defeated maxReplicas as a cost knob). The stray maxReplicas: 0 case now rejects rather than clamps. - [MINOR] The disable-wedge guard, its values note and its test no longer emit the invalid `kubectl scale --subresource=scale` (scale has no --subresource); use `kubectl scale cluster.postgresql.cnpg.io/<name> --replicas=N`. - [MINOR] Correct the enablement note: raising minReplicas/maxSyncReplicas on a live cluster is NOT harmless when KEDA's live count exceeds the new floor (it decommissions surplus replicas + PVCs); prescribe pin-first for raises too. - [MINOR] Add DatabaseAutoscalerKedaOperatorDown (absent(keda_build_info)): when the operator is unhealthy a ScaledObject never becomes an HPA and no other alert can fire. Intersect DatabaseAutoscalerScaleStuck with a Pending pod so a healthy long basebackup (Running pod, desired>current >15m) no longer false-fires. - [MINOR] Keep the KEDA CRDs on package uninstall (crds.additionalAnnotations resource-policy: keep via the shim) so removing the keda package doesn't cascade-delete every tenant ScaledObject. - [MINOR] cozy-lib helper: reject a non-numeric bound (int "two" -> 0 -> a scale-to-zero ScaledObject) and render an explicit pollingInterval/cooldownPeriod of 0 (`if not kindIs invalid`, not `with`). - [MINOR] Note the read-load fail-open: a broken metrics join reads as idle and silently pins autoscaling at the floor under real load. New/updated tests cover the non-quorum seed, the min>max guard, the corrected kubectl message, the operator-down + Pending-intersected alerts, the CRD keep policy, and the non-numeric-bound helper guard. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
1ebc021491 |
style(postgres): strip the autoscaling db.yaml comments from the rendered manifest
The autoscaling instance-count explanations were plain `#` YAML comments, so they
shipped verbatim into every rendered CNPG Cluster manifest on every customer
cluster (unlike the {{/* */}} comments elsewhere in the chart). Convert them to
Go-template comments so the rationale stays in-source but is stripped at render,
matching the treatment already applied to the apps.yaml clusterDomain comment.
Also document (as a stripped comment) that WorkloadMonitor.spec.replicas tracks
the quorum floor, not KEDA's live autoscaled target, under active autoscaling.
Addresses the deep cozy-review notes on #3954.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
5a5047ac23 |
fix(dashboards): bind the autoscaling dashboard to a datasource variable
The database-autoscaling dashboard hardcoded "datasource": "VictoriaMetrics" on
its namespace template variable, but cozystack provisions VictoriaMetrics
datasources as vm-shortterm / vm-longterm — there is no datasource named
"VictoriaMetrics", so on import the variable query failed ("datasource not
found"), the namespace variable never populated, and every panel (which filters
on namespace=~"$namespace") showed no data. Switch to the repo-standard pattern
used by db/cloudnativepg and the other dashboards: a `datasource`-typed template
variable (ds_prometheus) referenced as { type: prometheus, uid: ${ds_prometheus} }
from the namespace variable and every panel/target (the panels previously carried
no datasource at all and only worked by falling back to the default).
Also reframe the disable-wedge guard message to lead with the invariant (replicas
must exceed quorum.maxSyncReplicas) so a pure-quorum user who never enabled
autoscaling isn't told to "disable autoscaling"; the disable path is kept as a
parenthetical cause.
Addresses the adversarial branch-review findings on #3954.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
||
|
Alexey Artamonov
|
6e3da63586 |
fix(postgres): guard the disable-wedge, role-scope the freeze count, and harden edges
Address the remaining IvanHunters review points on #3954: - [MAJOR] Disabling autoscaling wedged the HelmRelease under synchronous quorum. An autoscaled + maxSyncReplicas>=1 cluster runs with instances seeded from effectiveMin while replicas stays at its default, so flipping autoscaling off rendered instances: <replicas> next to the quorum and CNPG's webhook rejected it ("maxSyncReplicas must be lower than the number of instances"). Add a symmetric render-time guard on the static-count path (covers off/dryRun/ transition): fail with an actionable message when replicas <= maxSyncReplicas. - [MINOR] Role-scope the freeze brake's currentInstances count (label_cnpg_io_instance_role=~".+") so lingering CNPG join/bootstrap Job pods (cnpg.io/cluster but jobRole, not instanceRole) don't inflate it and nudge desired up while the brake is meant to hold it. - [MINOR] Default maxReplicationLagSeconds to 0 (brake opt-in): only the non-braking path is validated live, so ship the validated default and let the freeze be enabled explicitly until its branch is exercised under load. - [MINOR] Document the floor-change and non-quorum-disable dips (same three-way- merge hazard) and the pin-replicas-first mitigation. - [NIT] Coerce the cozy-lib helper's `paused` (eq "true") so a caller passing the string "false" does not pause the ScaledObject. - [NIT] Trim the keda.clusterDomain injection comment so it no longer ships ~8 verbatim lines into the cozystack-values Secret on every cluster. Regenerate types/schema/README/postgres-rd. New tests cover the disable-wedge guard, the role-scoped freeze count, and the string-"false" paused coercion. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
70e4087432 |
fix(postgres): guard empty monitoring namespace; clarify alert scope and helper doc
Address the branch-review nits: - Fail closed when the monitoring namespace is unset. serverAddress builds vmselect-shortterm.<ns-monitoring>.svc, and an empty ns-monitoring rendered the malformed vmselect-shortterm..svc:8481, surfacing late as a KEDA scaler error. The platform always injects it, but a hand-rendered/misconfigured tenant now gets an actionable render-time failure mirroring the KEDA-CRD and target guards. Tested. - Document that the database-autoscaling PrometheusRule matches every KEDA HPA / ScaledObject error (the underlying series carry no owning-app label), which is correct only while database autoscaling is KEDA's sole consumer in cozystack; note to scope the rules if a non-database ScaledObject is ever added. - Correct the cozy-lib.keda.scaledObject usage example: `paused` should reflect dry-run OR migration transition, not dry-run alone, so a second engine copying the example does not drop the transition pause. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
9cf4a171f0 |
fix(postgres): stop coercing numeric-zero autoscaling bounds and floor each lag gate
Two review notes from IvanHunters: - effectiveMin/effectiveMax wrapped minReplicas/maxReplicas/maxSyncReplicas/ maxReplicationLagSeconds in Sprig `default`, which treats a numeric 0 as empty and silently substitutes the fallback (e.g. maxReplicas: 0 became 6, discarding the tenant's value). values.yaml/schema already default these when omitted, so read them directly; an explicit out-of-range value now flows into max() and is clamped to the quorum floor rather than swapped for a different number. Add a test that maxReplicas: 0 clamps to effectiveMin instead of becoming 6. - The replication-lag brake only floored the product $braking with `or vector(0)`, so if a CNPG build stops emitting cnpg_pg_replication_lag or cnpg_collector_wal_records the individual gate resolved to an empty vector. Floor each gate ($lagHigh, $writing) individually so the fail-open decision is explicit and per-gate: the brake engages only on positively-observed lag AND writing, and a missing signal degrades to "scale normally" (safer than freezing on absent data) rather than relying on the emptiness of the product. Assert both gates carry the floor. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
a7b5077da2 |
fix(postgres): floor the lag-brake frozen term so a KSM gap can't empty the query
The read-load query's base term is floored with `or vector(0)`, but the replication-lag brake's frozen additive term (count(kube_pod_labels) * target) was not. PromQL `+` is a set intersection, so if count(kube_pod_labels) is momentarily empty — a kube-state-metrics scrape gap or KSM rollout — the frozen term is empty and the whole `base + frozen*braking` sum collapses to empty, defeating the base term's own graceful degradation. KEDA's prometheus scaler (ignoreNullValues: true by default) then reads that as 0 and pins desired to minReplicaCount, ignoring real read load exactly when metrics are flaky. This hits the default config (maxReplicationLagSeconds: 30, brake active). Floor the frozen summand with `or vector(0)` so base + 0 = base, and assert the floor in the lag-brake unittest. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |