K8SPG-786 K8SPG-1040 add upgrade test and fix operator behaviuor with extentions. - #1734
K8SPG-786 K8SPG-1040 add upgrade test and fix operator behaviuor with extentions. #1734nmarukovich wants to merge 29 commits into
Conversation
… into K8SPG-786 # Conflicts: # e2e-tests/functions # e2e-tests/run-pr.csv # e2e-tests/run-release.csv # e2e-tests/tests/custom-extensions/04-check-extensions.yaml # e2e-tests/tests/custom-extensions/08-check-extensions.yaml # e2e-tests/tests/custom-extensions/13-check-extensions.yaml # e2e-tests/tests/custom-extensions/16-check-extensions.yaml # pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go # pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_types.go # pkg/apis/upstream.pgv2.percona.com/v1beta1/zz_generated.deepcopy.go
|
copilot |
There was a problem hiding this comment.
Pull request overview
This PR addresses K8SPG-786 by making pg_cron and set_user builtin extension management tri-state so the operator can distinguish between “unset/leave as-is” vs “explicitly disable/drop”, preventing unintended DROP EXTENSION (and data loss) during upgrades and when migrating extensions from spec.extensions.custom to builtin flags. It also expands E2E coverage to validate operator upgrades, minor upgrades, and cert-manager upgrade scenarios without triggering unwanted rollouts or data loss.
Changes:
- Change upstream
PostgresClusterextension flags forPGCron/SetUserfromboolto*bool(nil = leave alone), and update deepcopy behavior. - Update reconciliation logic to respect nil vs false/true, and prevent custom-extension removal from dropping extensions that are now enabled via builtin flags.
- Add/extend E2E tests for operator upgrades (extensions + cert-manager) and minor upgrade flow (backup/restore, rollout checks, log checks).
Reviewed changes
Copilot reviewed 70 out of 72 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/apis/upstream.pgv2.percona.com/v1beta1/zz_generated.deepcopy.go | DeepCopy updates for new *bool extension fields. |
| pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_types.go | Convert PGCron/SetUser to tri-state *bool in upstream API. |
| pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go | Stop defaulting PGCron/SetUser; map Percona CR -> upstream tri-state fields. |
| pkg/apis/pgv2.percona.com/v2/perconapgcluster_types_test.go | Add unit tests covering tri-state passthrough + stored-spec clearing behavior. |
| percona/controller/pgcluster/controller.go | Avoid dropping custom extensions when the same extension is enabled via builtin flags. |
| internal/controller/postgrescluster/postgres.go | Treat nil builtin-extension flags as “do not touch”; keep explicit enable/disable behavior. |
| internal/controller/postgrescluster/controller.go | Only add shared-preload params when tri-state flags are explicitly enabled. |
| e2e-tests/tests/upgrade-minor/conf/upgrade-minor.yaml | New base config for richer upgrade-minor scenario (multi-repo, resources, etc.). |
| e2e-tests/tests/upgrade-minor/conf/log-allowlist.txt | New allowlist for upgrade-minor log scanning. |
| e2e-tests/tests/upgrade-minor/13-check-logs.yaml | Add post-upgrade log scan step. |
| e2e-tests/tests/upgrade-minor/12-collation-mismatch.sql | Add SQL used to detect collation mismatch after upgrade. |
| e2e-tests/tests/upgrade-minor/12-check-collation-mismatch.yaml | Point check to the new SQL file name/location. |
| e2e-tests/tests/upgrade-minor/11-read-from-primary.yaml | Fix configmap naming to match step index. |
| e2e-tests/tests/upgrade-minor/11-assert.yaml | Assert data read-back from primary after additional write. |
| e2e-tests/tests/upgrade-minor/10-write-data.yaml | Add extra write step (post-upgrade) for read verification. |
| e2e-tests/tests/upgrade-minor/09-upgrade-cluster.yaml | Strengthen upgrade step (timeouts, image waits, consistency checks, preconditions). |
| e2e-tests/tests/upgrade-minor/09-assert.yaml | Update expected generations and status assertions for new flow. |
| e2e-tests/tests/upgrade-minor/08-restore.yaml | Add restore step to validate cross-version backup/restore behavior. |
| e2e-tests/tests/upgrade-minor/08-assert.yaml | Assert restore CR succeeded and cluster returned to ready. |
| e2e-tests/tests/upgrade-minor/07-backup-old-crversion.yaml | Add backup step while cluster still has released crVersion. |
| e2e-tests/tests/upgrade-minor/07-assert.yaml | Assert backup succeeded. |
| e2e-tests/tests/upgrade-minor/06-check-logs.yaml | Add mid-upgrade log scan step. |
| e2e-tests/tests/upgrade-minor/05-sleep-after-operator-update.yaml | Remove fixed sleep step (replaced by consistency/no-rollout checks). |
| e2e-tests/tests/upgrade-minor/05-check-no-rollout.yaml | Add explicit “no operator-driven rollout” + heartbeat gap assertion. |
| e2e-tests/tests/upgrade-minor/05-assert.yaml | Update expected generations/status due to new backup/flow behavior. |
| e2e-tests/tests/upgrade-minor/04-upgrade-operator.yaml | Add baseline snapshot + heartbeat before operator update. |
| e2e-tests/tests/upgrade-minor/02-write-data.yaml | Extend to include a full backup taken by released operator. |
| e2e-tests/tests/upgrade-minor/02-assert.yaml | Assert pre-upgrade backup succeeded. |
| e2e-tests/tests/upgrade-minor/01-create-cluster.yaml | Compose CR from released template + suite config; pin init image; repo2 wiring. |
| e2e-tests/tests/upgrade-minor/01-assert.yaml | Increase timeout and assert repo2 stanza creation. |
| e2e-tests/tests/upgrade-minor/00-deploy-operator.yaml | Add MinIO + repo2 credentials/bootstrap for cross-version backup/restore. |
| e2e-tests/tests/operator-upgrade-extensions/conf/operator-upgrade-extensions.yaml | New suite config for extension survival across operator/cluster upgrade. |
| e2e-tests/tests/operator-upgrade-extensions/conf/log-allowlist.txt | New allowlist for operator-upgrade-extensions log scanning. |
| e2e-tests/tests/operator-upgrade-extensions/99-remove-cluster-gracefully.yaml | New cleanup step for suite teardown. |
| e2e-tests/tests/operator-upgrade-extensions/08-check-logs.yaml | New log scan step after upgrade. |
| e2e-tests/tests/operator-upgrade-extensions/07-check-extensions-after-cluster-upgrade.yaml | Validate extension set + pg_cron ticking + vector data after cluster upgrade. |
| e2e-tests/tests/operator-upgrade-extensions/06-upgrade-cluster.yaml | Cluster upgrade step for the suite. |
| e2e-tests/tests/operator-upgrade-extensions/05-check-extensions-survived.yaml | Validate no rollout and extension set unchanged after operator update. |
| e2e-tests/tests/operator-upgrade-extensions/04-upgrade-operator.yaml | Operator upgrade step for the suite. |
| e2e-tests/tests/operator-upgrade-extensions/03-enable-extensions-and-write-data.yaml | Install extensions + create data that must survive upgrades. |
| e2e-tests/tests/operator-upgrade-extensions/02-add-custom-pg-cron.yaml | Add pg_cron as custom extension as part of migration scenario. |
| e2e-tests/tests/operator-upgrade-extensions/01-create-cluster.yaml | Create suite cluster from released CR + suite config; canary for CRD pruning. |
| e2e-tests/tests/operator-upgrade-extensions/01-assert.yaml | Assert suite cluster readiness. |
| e2e-tests/tests/operator-upgrade-extensions/00-deploy-operator.yaml | Deploy released operator + MinIO/custom extensions bootstrap for suite. |
| e2e-tests/tests/operator-upgrade-extensions/00-assert.yaml | Assert operator and MinIO are ready. |
| e2e-tests/tests/operator-upgrade-cert-manager/conf/log-allowlist.txt | New allowlist for operator-upgrade-cert-manager log scanning. |
| e2e-tests/tests/operator-upgrade-cert-manager/99-remove-cluster-gracefully.yaml | New cleanup step for suite teardown. |
| e2e-tests/tests/operator-upgrade-cert-manager/10-check-logs.yaml | New log scan step after upgrade. |
| e2e-tests/tests/operator-upgrade-cert-manager/09-check-restored-data.yaml | Verify restored data correctness. |
| e2e-tests/tests/operator-upgrade-cert-manager/08-restore.yaml | Restore step to validate cross-version backup/restore. |
| e2e-tests/tests/operator-upgrade-cert-manager/08-assert.yaml | Assert restore succeeded and cluster is ready post-restore. |
| e2e-tests/tests/operator-upgrade-cert-manager/07-backup-after-update.yaml | Backup step after operator upgrade. |
| e2e-tests/tests/operator-upgrade-cert-manager/07-assert.yaml | Assert post-upgrade backup succeeded. |
| e2e-tests/tests/operator-upgrade-cert-manager/06-check-tls-unchanged.yaml | Validate cert-manager presence doesn’t take over PKI-issued secrets or roll pods. |
| e2e-tests/tests/operator-upgrade-cert-manager/05-upgrade-operator.yaml | Operator upgrade step for cert-manager scenario. |
| e2e-tests/tests/operator-upgrade-cert-manager/04-backup.yaml | Backup step before operator upgrade. |
| e2e-tests/tests/operator-upgrade-cert-manager/04-assert.yaml | Assert pre-upgrade backup succeeded. |
| e2e-tests/tests/operator-upgrade-cert-manager/03-deploy-cert-manager.yaml | Introduce cert-manager after PKI cluster exists (K8SPG-1017 trigger). |
| e2e-tests/tests/operator-upgrade-cert-manager/02-write-data-and-save-tls.yaml | Write data + snapshot TLS/revisions baseline; premise checks. |
| e2e-tests/tests/operator-upgrade-cert-manager/01-create-cluster.yaml | Create released cluster pinned to released init image; add repo2 config. |
| e2e-tests/tests/operator-upgrade-cert-manager/01-assert.yaml | Assert cert-manager suite cluster readiness. |
| e2e-tests/tests/operator-upgrade-cert-manager/00-deploy-operator.yaml | Deploy released operator + MinIO/TLS bootstrap for repo2 backups. |
| e2e-tests/tests/operator-upgrade-cert-manager/00-assert.yaml | Assert operator and MinIO are ready. |
| e2e-tests/tests/custom-extensions/21-check-migration.yaml | New migration verification: custom -> builtin must not drop pg_cron or its data. |
| e2e-tests/tests/custom-extensions/20-migrate-to-builtin.yaml | Add migration step enabling builtin pg_cron while removing it from custom list. |
| e2e-tests/tests/custom-extensions/20-assert.yaml | Assert expected status after migration step. |
| e2e-tests/tests/custom-extensions/19-enable-pg_cron.yaml | Install pg_cron extension + create durable data (cron.job) for migration test. |
| e2e-tests/tests/custom-extensions/17-install-pg_cron.yaml | Install pg_cron as custom extension to set up migration scenario. |
| e2e-tests/tests/custom-extensions/17-assert.yaml | Assert expected status after custom pg_cron installation. |
| e2e-tests/run-release.csv | Add new upgrade suites to release run list. |
| e2e-tests/run-pr.csv | Add new upgrade suites to PR run list. |
| e2e-tests/functions | Add shared helper functions for rollout/TLS/log assertions; adjust AWS endpoint. |
Suppressed comments (1)
pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go:556
-
- Problem:
new(*cr.Spec.Extensions.SetUser.Enabled)is invalid Go (the built-innewrequires a type, not a value), so this code will not compile.
- Problem:
- Why it matters: This breaks
PerconaPGCluster.ToCrunchycompilation and prevents reconciliation/builds. - Fix: Allocate the bool pointer via
ptr.To(*...)(orb := *...; postgresCluster.Spec.Extensions.SetUser = &b).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if cr.Spec.Extensions.PGCron.Enabled != nil { | ||
| postgresCluster.Spec.Extensions.PGCron = *cr.Spec.Extensions.PGCron.Enabled | ||
| postgresCluster.Spec.Extensions.PGCron = new(*cr.Spec.Extensions.PGCron.Enabled) | ||
| } else { | ||
| postgresCluster.Spec.Extensions.PGCron = nil | ||
| } |
|
my version question was not about the patch version. what i'm trying to understand how to enable users who had pg_cron as a custom extension to start using the built-in pg_cron. because with these changes, once you have pg_cron custom extension you're stuck with it. at least we need to document how to do the migration |
The migration is supported in this PR. You do it in a single apply: The extension and its data survive. This PR added a guard for exactly this case: when the builtin flag is enabled, removing the extension from custom does not drop it — the builtin path takes over (its CREATE EXTENSION IF NOT EXISTS is a no-op on the existing extension): It's covered by e2e (custom-extensions steps 17–21): install pg_cron as custom → create a cron.schedule entry → switch to builtin in one apply → assert the extension is still there and cron.job still has the row. Without this PR you really were stuck: removing it from custom dropped the extension with all schedules, and the builtin path would re-create it empty. One rule to document : enable the builtin flag in the same apply as removing the custom entry. Removing from custom alone is an uninstall and drops the extension — that part is intentional. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 75 out of 77 changed files in this pull request and generated no new comments.
Suppressed comments (1)
e2e-tests/functions:580
- The AWS CLI invocation that downloads extension artifacts from AWS S3 no longer sets AWS_DEFAULT_REGION, unlike other aws-cli calls in this test harness; this can cause
awsto fail with "You must specify a region" depending on the container/config and makes the behavior inconsistent.
Set AWS_DEFAULT_REGION for the first copy step (and remove the now-empty line) to keep the script robust and consistent.
bash -c "
AWS_ACCESS_KEY_ID=$(get_aws_access_key_id aws-s3-secret) \
AWS_SECRET_ACCESS_KEY=$(get_aws_access_key aws-s3-secret) \
/usr/bin/aws --endpoint-url https://s3.amazonaws.com s3 cp s3://pg-extensions/ /tmp/ --recursive &&
| local gap | ||
| gap=$(run_psql_local 'SELECT COALESCE(ceil(EXTRACT(EPOCH FROM max(ts - prev)))::int,0) FROM (SELECT ts, lag(ts) OVER (ORDER BY ts) AS prev FROM heartbeat) g' "$uri" | tr -d '[:space:]') | ||
| echo "heartbeat max gap: ${gap}s (limit ${max_gap}s)" | ||
| if [[ -z "$gap" || "$gap" -gt "$max_gap" ]]; then |
There was a problem hiding this comment.
[shfmt] reported by reviewdog 🐶
| if [[ -z "$gap" || "$gap" -gt "$max_gap" ]]; then | |
| if [[ -z $gap || $gap -gt $max_gap ]]; then |
commit: 28ecf4c |
CHANGE DESCRIPTION
Problem:
When a cluster has pg_cron installed through spec.extensions.custom and the operator gets upgraded, the new operator drops the extension together with its data. Two reasons:
Solution:
The annotation is temporary: it only matters for upgrades from versions that didn't write the custom list (3.0.0 and older). Once those versions are out of the supported upgrade path, it can be removed.
new upgrade e2e tests
custom-extensions and upgrade-minor were extended.
Limitations:
This fixes pg_cron and set_user only. Other builtin extensions (pgaudit, pg_stat_monitor, pg_stat_statements, pgvector, pg_repack) have the same problem: if you install one of them as custom and its builtin flag is off, the operator drops it on every reconcile.
Question: do we fix the rest in a follow-up, or do we say "don't install builtin extensions as custom"? If the latter — let's reject it in validation, silent drop is worse.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability