K8SPG-786 - #1734
Open
nmarukovich wants to merge 14 commits into
Open
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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| case $test_name in | ||
| "custom-extensions" | "builtin-extensions" | major-upgrade* ) | ||
| "custom-extensions" | "builtin-extensions" | "operator-upgrade-extensions" | major-upgrade* ) |
Contributor
There was a problem hiding this comment.
[shfmt] reported by reviewdog 🐶
Suggested change
| "custom-extensions" | "builtin-extensions" | "operator-upgrade-extensions" | major-upgrade* ) | |
| "custom-extensions" | "builtin-extensions" | "operator-upgrade-extensions" | major-upgrade*) |
| 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 |
Contributor
There was a problem hiding this comment.
[shfmt] reported by reviewdog 🐶
Suggested change
| if [[ -z "$gap" || "$gap" -gt "$max_gap" ]]; then | |
| if [[ -z $gap || $gap -gt $max_gap ]]; then |
Collaborator
commit: 2b0ca7a |
nmarukovich
marked this pull request as ready for review
August 11, 2026 13:58
nmarukovich
requested review from
DhruthiKV,
egegunes,
eleo007,
gkech,
hors,
jvpasinatto,
mayankshah1607,
oksana-grishchenko,
pooknull and
valmiranogueira
as code owners
August 11, 2026 13:58
Contributor
Author
|
copilot |
Contributor
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.
Comment on lines
547
to
551
| 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 | ||
| } |
oksana-grishchenko
left a comment
oksana-grishchenko
left a comment
Contributor
There was a problem hiding this comment.
LGTM, few non-blocking suggestions
Comment on lines
547
to
+548
| AWS_DEFAULT_REGION=eu-central-1 \ | ||
| /usr/bin/aws --endpoint-url https://s3.amazonaws.com s3 cp s3://pg-extensions/ /tmp/ --recursive && | ||
| /usr/bin/aws --endpoint-url https://s3.eu-central-1.amazonaws.com s3 cp s3://pg-extensions/ /tmp/ --recursive && |
Contributor
There was a problem hiding this comment.
I would suggest not to hardcode the region since we have the AWS_DEFAULT_REGION var above.
In fact, I think we should not specifying the endpoint --endpoint-url https://s3.eu-central-1.amazonaws.com at all, it should be resolved automatically since we've set the AWS_DEFAULT_REGION env
Comment on lines
+286
to
+287
| // tri-state: nil means the user never asked for the extension, so the | ||
| // operator must leave it alone instead of dropping it |
Contributor
There was a problem hiding this comment.
it would be more straightforward if the comment specified what leaving alone technically means. something like:
// tri-state: nil means the CR does not express a desired state for the extension,
// so the operator must not create or drop the extension,
// it preserves whatever state already exists in the database
wdyt?
oksana-grishchenko
approved these changes
Aug 12, 2026
Contributor
|
sorry for accidental closing the PR, it was a misclick somehow🙈 |
Member
|
@nmarukovich please add a title |
egegunes
reviewed
Aug 13, 2026
egegunes
left a comment
egegunes
left a comment
Contributor
There was a problem hiding this comment.
I don't understand what's the result of these changes: if pg_cron was installed as a custom extension, what happens when you upgrade?
to me it seems like custom extension is not uninstalled even though you delete it from extensions.custom, and whatever version you have is kept. if this is true, this version is kept until when? is it possible that you think you're on pg_cron x.y.z while you're on x.y.z-6?
| SetUser bool `json:"setUser,omitempty"` | ||
| // tri-state: nil means the user never asked for the extension, so the | ||
| // operator must leave it alone instead of dropping it | ||
| PGCron *bool `json:"pgCron,omitempty"` |
Member
There was a problem hiding this comment.
should we allow to go from true -> nil?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CHANGE DESCRIPTION
Problem:
The operator drops the pg_cron extension from the database and destroys all
jobs stored in cron.job. This happens to users who install pg_cron as a custom
extension from their own storage. It also happens when a user moves pg_cron
from the custom list to the builtin flags.
Cause:
pgCronandsetUserwere plain bools. If the user did not set them, theoperator set them to
false. So the operator cannot see the difference between"the user did not ask for pg_cron" and "the user wants pg_cron removed", and it
runs
DROP EXTENSION IF EXISTS pg_cron.The custom extensions loop drops an extension as soon as it is removed from the
custom list, even if the user enabled the same extension as a builtin one.
pg_cron
**Solution:**PGCronandSetUserare*boolnow: nil means the flag is not set and the operator leaves the extension alone, false means drop it, true means create it. We do not setfalse` for these two flags anymore. With nil we also do not addpg_cron to shared_preload_libraries, so postgres does not restart.
The custom extensions loop does not drop an extension if the same extension is
enabled as a builtin one. Now a user can move pg_cron from the custom list to
the builtin flags and keep the data.
new upgrade e2e tests
custom-extensions and upgrade-minor were extended.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability