-
Notifications
You must be signed in to change notification settings - Fork 183
feat: Replace deprecated qpress with configurable compression options for backups #4340
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
base: v3
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| switch j.compression { | ||
| case backuppb.BackupCompression_BACKUP_COMPRESSION_DEFAULT: |
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.
specifies PBMs configuration options, and by default no specific compression
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.
looks to me like it's defaulting to gzip though.
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.
Its an empty case block, allowing to use PBM's default option to compress with s2, can explicitly specify that if that's preferred.
| if _, err := exec.LookPath(qpressBin); err != nil { | ||
| return errors.Wrapf(err, "lookpath: %s", qpressBin) | ||
| if j.compression == backuppb.BackupCompression_BACKUP_COMPRESSION_QUICKLZ { | ||
| if _, err := exec.LookPath(qpressBin); err != nil { |
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.
Ensures we only check that qpress is installed if user wants to explicitly use quicklz, other compression options are installed as part of xtrabackup, so no need to explicitly check for them
| } | ||
|
|
||
| // ListServiceCompression returns available compression methods for a service. | ||
| func (s *BackupService) ListServiceCompression(ctx context.Context, req *backupv1.ListServiceCompressionRequest) (*backupv1.ListServiceCompressionResponse, error) { |
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.
As described, provided an input service, checks for the available compression methods for the related service type
| } | ||
|
|
||
| switch j.compression { | ||
| case backuppb.BackupCompression_BACKUP_COMPRESSION_DEFAULT: |
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.
looks to me like it's defaulting to gzip though.
| case backuppb.BackupCompression_BACKUP_COMPRESSION_S2: | ||
| pbmArgs = append(pbmArgs, "--compression=s2") |
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.
are we sure pbm supports this compression method?
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.
Yeah, also documented as the default -- https://docs.percona.com/percona-backup-mongodb/reference/backup-options.html#backupcompression
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3 #4340 +/- ##
==========================================
- Coverage 44.56% 44.48% -0.08%
==========================================
Files 363 363
Lines 45706 45937 +231
==========================================
+ Hits 20369 20437 +68
- Misses 23677 23831 +154
- Partials 1660 1669 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…cross multiple files
|
@idoqo ICYMI, I've fixed the linter error that caused the workflow trigger to fail, might want to help trigger again |
|
@the-wunmi could you resolve conflicts please? |
@JiriCtvrtka resolved. |
…sts to include compression parameter in BackupTaskParams.
7fdd25e to
43f5800
Compare
…finitions. Remove references to Qpress in version cache tests.
|
hello @JiriCtvrtka @idoqo conflict on this has been resolved, alongside failing workflows, except for a linkspector check, unrelated to my changes here |
|
|
Summary
This PR updates backup compression by replacing the deprecated qpress dependency with configurable compression options, allowing users to choose from multiple compression algorithms for MySQL and MongoDB backups.
Background
Previous MySQL backups enforced qpress compression by default, but qpress has been disabled in newer PMM versions due to deprecation. This change removes the qpress dependency and provides users with flexible compression options.
Changes Made
Compression Algorithm Options
MySQL Backup Updates
MongoDB Backup Enhancement
API and Data Model Updates
Impact
Migration
Existing backups remain compatible, legacy artifacts are updated to use quicklz and s2 for MySQL and MongoDB respectively as currently supported
New backups default to default compression provided by the backup tool (xtrabackup/pbm) unless explicitly specified
Users who need QuickLZ can still select it, but qpress binary must be available
API Docs updated
Link to related pull request: Backup: Replace deprecated qpress with configurable compression options grafana#839
Demo
pmm-backup-bug.mov
After:
pmm-backup-compression-demo.mov