-
Notifications
You must be signed in to change notification settings - Fork 12
Upgrade PGO to 5.7.0; Add option to disable backups #191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| # backupsEnabled sets whether pgBackRest backups are enabled. | ||
| # This is enabled by default. | ||
| # Disabling backups is only supported on CrunchyData Postgres Operator v5.7.0 and above. | ||
| # If enabled, please carefully review the pgBackRest config below. | ||
| # Check out CrunchyData's Postgres Operator docs for more info: | ||
| # https://access.crunchydata.com/documentation/postgres-operator/v5/tutorials/backups-disaster-recovery/backups | ||
| backupsEnabled: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this default to false until we can recommend a more robust default configuration for pgBackrest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ividito Yeah, that was my first instinct too. But if the default is false, we could break existing setups where someone upgrades eoapi without also updating PGO. The bigger risk is that users with a working backup config might upgrade to the latest chart and miss this new flag -- suddenly their working backup mechanism will be silently ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at the moment we can still be a bit aggressive on introducing "breaking changes". I would put is as false and make sure in the next release notes we mention this new flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, happy to make the change if we think breaking changes are ok for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ividito @pantierra - updated the PR. Can I get another round of review before we merge please?
pantierra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Related to #181 (comment)
Disabling backups is only an option if using PGO >= 5.7.0
Along with disabling the backups, we can also set some Postgres settings to keep the storage consumed by WAL files in check. Here's an example: