Two defects of the same class — the WAL-archive S3 prefix (serverName)
changing or defaulting differently between components:
Recovery default regression (review blocker): native CNPG defaulted an
absent externalClusters serverName to the entry name (= bootstrap.oldName),
but the barman-cloud plugin defaults it to the NEW cluster's name, so a
legacy-flow restore with bootstrap.serverName unset would read the wrong
S3 prefix (hard failure, or silently restoring another cluster's data).
The chart now always emits serverName, defaulting to bootstrap.oldName.
Live-cluster serverName flip (found by the gated postgres backup e2e on a
real cluster): the chart renders spec.plugins without serverName
(effective prefix = cluster name) while a strategy template may name a
different one; the driver's ForceOwnership patch then flipped the prefix
mid-stream, splitting the WAL archive across two prefixes — the WAL
holding the backup's start was lost between them and the restore failed
with "WAL not found". The driver now preserves the live Cluster's
effective serverName (explicit parameter, or the cluster name when the
plugin is attached without one) and only applies the strategy's value to
a Cluster with no plugin attached; both charts pin serverName explicitly,
and the example strategy no longer sets one.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Review follow-up (cozystack#3300, non-blocking): the barman-cloud ObjectStore CRD
validates spec.retentionPolicy against ^[1-9][0-9]*[dwm]$, so an operator who
explicitly sets backup.retentionPolicy="" would render retentionPolicy: "" and
the API server would reject the ObjectStore, breaking backup provisioning. Guard
the field in the apps/postgres and keycloak chart ObjectStores with
{{- if .Values.backup.retentionPolicy }} (the Go driver path is already safe via
omitempty). Add a helm-unittest asserting an empty retentionPolicy is omitted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Review follow-ups for the CNPG barman-cloud plugin migration (cozystack#3300):
- docs/operations/backup-classes.md: the 'Upgrade notes from chart-managed
backups' section still described the removed native spec.backup.barmanObjectStore
behavior. Rewrite it for the plugin flow (chart renders spec.plugins + a
barmancloud.cnpg.io/ObjectStore; the driver SSA-applies an ObjectStore and
patches spec.plugins), and document the real upgrade hazard: the
plugin-barman-cloud operator + cert-manager must be present in
cozy-postgres-operator or the Cluster references an unregistered plugin and
backups/WAL archiving stall.
- apps/postgres backup_storage_test.yaml: add a positive bootstrap-recovery
rendering test. Asserts externalClusters[].plugin references
barman-cloud.cloudnative-pg.io with barmanObjectName=<release>-recovery and
serverName on the plugin parameter, and that the -recovery ObjectStore carries
spec.configuration but NOT serverName (the plugin forbids it there). Previously
the recovery path had only fail-fast guard tests, no positive coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Address review feedback on #3069:
- ttlSecondsAfterFinished 120 -> 3600 so a failed init Job leaves a 1h
debugging window before the TTL controller collects it (the TTL also
reaps failed Jobs, not just successful ones).
- Load templates/init-script.yaml in the unittest suite: the Job's
checksum/config annotation includes that template, so rendering
init-job.yaml in isolation failed in CI. Select the Job via
documentSelector and assert the 3600 TTL.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
The post-install/post-upgrade init Job is a one-shot reconciler of
users/databases/roles. Its hook-delete-policy was `before-hook-creation`
only, which never fires on uninstall, so the completed Job and its Pod
lingered in the tenant namespace forever after the Postgres CR was
deleted (#3055).
Add `ttlSecondsAfterFinished: 120` so the Kubernetes TTL controller GCs
the Job and its Pod ~2 minutes after completion, regardless of the Helm
hook lifecycle. This deliberately avoids touching the hook-timeout
semantics that the bootstrap.recovery path depends on.
Add a helm-unittest suite locking in the TTL and the bootstrap-skip
behaviour to prevent regression.
Fixes#3055
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
Resolve the conflict in the generated packages/system/postgres-rd/cozyrds/postgres.yaml by regenerating openAPISchema and keysOrder from the merged values. The CRD now carries both the chart's tls.enabled field and main's backup useSystemBucket / BackupClass-driven fields.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Remove the jq post-processing patch that widened tls.enabled type to
["boolean","null"] in values.schema.json and the openAPISchema embedded
in the cozyrds CRD. Plain "type":"boolean" is emitted directly by
cozyvalues-gen and is the correct form.
The cozystack-api server unmarshals openAPISchema into
apiextv1.JSONSchemaProps whose Type field is a single string; an array
value silently zeros the field and disables server-side defaulting for
the entire app.
Tri-state semantics are preserved on the Helm template side via
kindIs "invalid": omitting tls.enabled triggers the external-based
fallback; null is no longer a valid input and is rejected by the schema.
Remove two helm-unittest cases that injected tls.enabled: null (now
intentionally rejected) and update the suite comment to match.
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Drop the templates/certmanager.yaml suite entirely (the file no longer
exists) and rewrite the spec.certificates assertions to match the new
operator-managed flow: certificates block emitted only when both TLS is
on AND external is true, with serverAltDNSNames carrying just the
external hostname.
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Pin the 1:1 equivalence between legacy flat preset names and their
instance-type counterparts in helm-unittest for three charts that
exercise different preset shapes:
- postgres — top-level only
- clickhouse — top + nested clickhouseKeeper preset
- opensearch — top + nested dashboards preset
Each case asserts that nano <-> t1.nano, small <-> t1.small,
medium <-> c1.small, large <-> c1.medium, and micro <-> t1.micro
render the same CPU and memory.
packages/apps/{clickhouse,opensearch}/Makefile gain a `test:` target
that calls helm unittest so hack/helm-unit-tests.sh actually runs the
new suites in CI; without it the suites would land as dead weight.
hack/check-rd-presets.sh enforces two invariants in `make unit-tests`:
- every packages/system/*-rd/cozyrds/*.yaml openAPISchema carries
the canonical 47-preset enum (40 instance-type + 7 legacy);
- no chart template carries a hardcoded legacy preset literal in
a defaultingSanitize (list "<legacy>" ...) form.
Both turn an accidental drop of a per-chart Makefile step or an
accidental legacy hardcode into a CI failure rather than silent
production-time validation rejection.
Co-authored-by: Matthieu ROBIN <matthieu@hidora.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
