Conversation
|
|
oksana-grishchenko
requested review from
bogdanjeler-ev,
egegunes,
eleo007,
gkech,
hors,
jvpasinatto,
mayankshah1607,
nmarukovich,
pooknull and
valmiranogueira
as code owners
| return crunchyv1beta1.Backups{ | ||
| PGBackRest: crunchyv1beta1.PGBackRestArchive{ | ||
| Image: b.PGBackRest.Image, | ||
| }, | ||
| } |
There was a problem hiding this comment.
crunchy controller decides whether backups are disabled by comparing backup spec with crunchyv1beta1.Backups{PGBackRest: crunchyv1beta1.PGBackRestArchive{}}
will this affect that?
There was a problem hiding this comment.
It will (in a way).
That's why the backup-enable-disable test was updated to show how a cluster without repos but with dataSource is expected to work. Which is: in case we specify the image in the crunchyv1beta1.Backups{PGBackRest: crunchyv1beta1.PGBackRestArchive{}} section, crunchy considers backups as enabled indeed, however since there are no repos defined, it's only expected that the sts will be created for repohost but no backup job should be created until we enable the backups. It's being explicitly checked in this new test step
Please let me know if you have any concerns about this behaviour.
There was a problem hiding this comment.
is there a way to not create the sts for repohost?
There was a problem hiding this comment.
Apparently there is this RELATED_IMAGE_PGBACKREST env var for crunchy cluster that should do the trick without defining the image in the .backups.pgbackrest section. That could potentially allow to reach the goal but avoid creating sts. I'll play around with it and get back.
There was a problem hiding this comment.
posted the current state to the related ticket, the work on it is paused until the proper solution is found.
There was a problem hiding this comment.
Pull request overview
This pull request allows PostgreSQL clusters to be restored from a dataSource when backups are disabled. Previously, the restore job would fail with an error about the missing pgBackRest image when creating a cluster with a dataSource while backups were disabled.
Changes:
- Added an
Enabledfield to theBackupsstruct to explicitly control backup functionality - Added validation requiring
spec.backups.pgbackrest.imagewhenspec.dataSourceis set - Modified the backup reconciliation logic to preserve restore status and consider the
Enabledflag when determining if backups are active - Added comprehensive E2E tests covering cluster creation from dataSource with disabled backups
Reviewed changes
Copilot reviewed 19 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go | Added Enabled *bool field to Backups struct |
| pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go | Auto-generated deepcopy code for new Enabled field |
| pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go | Added validation rule and modified ToCrunchy conversion to pass image when backups disabled |
| internal/controller/postgrescluster/pgbackrest.go | Updated reconciliation logic to check Enabled flag and preserve Restore status |
| config/crd/bases/* | Updated CRD definitions with new Enabled field and validation rules |
| deploy/*.yaml | Updated deployment manifests and CRD bundles (includes test image reference) |
| e2e-tests/tests/backup-enable-disable/* | Added comprehensive E2E tests for datasource restore with disabled backups |
e2e-tests/tests/backup-enable-disable/06-check-backups-with-datasource.yaml
Outdated
Show resolved
Hide resolved
e2e-tests/tests/backup-enable-disable/04-enable-backups-datasource.yaml
Outdated
Show resolved
Hide resolved
| // Backups defines a PostgreSQL archive configuration | ||
| type Backups struct { | ||
| Enabled *bool `json:"enabled,omitempty"` | ||
|
|
||
| // pgBackRest archive configuration | ||
| // +optional | ||
| PGBackRest PGBackRestArchive `json:"pgbackrest"` |
There was a problem hiding this comment.
- Problem: Adding
Backups.EnabledchangesPostgresCluster.BackupSpecFound()semantics becauseBackupSpecFound()still usesreflect.DeepEqual(cr.Spec.Backups, Backups{PGBackRest: ...})and does not account forenabled=false. - Why it matters: A cluster with
spec.backups.enabled=falsewill now be treated as having a backup spec, which makesconfig.VerifyImageValues()start requiring a pgBackRest image even though backups are explicitly disabled (and other code paths likeObserveBackupUniversenow treatenabled=falseas “no backups spec”). - Fix: Update
BackupSpecFound()(and any similar checks) to return false whenEnabledis set to false (e.g., gate the existing DeepEqual check behindptr.Deref(cr.Spec.Backups.Enabled, true)or an explicitEnabled != nil && !*Enabledearly return).
commit: eed0c65 |
Allow no repos when restoring from dataSource
Problem:
K8SPG-904 When restoring from dataSource while backups are disabled, the restore job fails to be created.
Cause:
The
crunchyv1beta1.Backups.PGBackRest.Imagewas never set if the backups were disabled which caused the following error:Solution:
Init the
crunchyv1beta1.Backups.PGBackRest.Imagewith the given image from the PG CR.Note: if the
crunchyv1beta1.Backups.PGBackRest.Imageis set then the repohost sts appears which was the cause why thebackup-enable-disablee2e test was failing on step 2.So the following changes are also applied:
crunchyv1beta1.Backups.PGBackRest.Imageonly if the pg cluster has datasourcecrunchyv1beta1.Backups.PGBackRest.ImageCHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability