Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses XtraBackup’s requirement that --defaults-file must be the first option by (a) enforcing ordering at the CRD/schema level for user-provided xtrabackup args and (b) ensuring the backup sidecar promotes --defaults-file ahead of operator-generated arguments during backup execution.
Changes:
- Add CRD validations and bounds (
maxItems,maxLength,maxProperties) to restrict and validate xtrabackup args, including enforcing--defaults-fileas the first xtrabackup argument when present. - Update backup sidecar argument construction to place
--defaults-filebefore generated xtrabackup options when provided as the first custom arg. - Add unit coverage for
xtrabackupArgsordering behavior and reorder${XB_EXTRA_ARGS}earlier in restore script xtrabackup invocations.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| deploy/cw-bundle.yaml | Adds schema validation/limits for xtrabackup args (including --defaults-file ordering) in the CW bundle output. |
| deploy/crd.yaml | Adds schema validation/limits for xtrabackup args (including --defaults-file ordering) in the CRD manifest. |
| deploy/bundle.yaml | Adds schema validation/limits for xtrabackup args (including --defaults-file ordering) in the operator bundle output. |
| config/crd/bases/ps.percona.com_perconaservermysqls.yaml | Propagates xtrabackup arg validation/limits into the base CRD for PerconaServerMySQL. |
| config/crd/bases/ps.percona.com_perconaservermysqlrestores.yaml | Propagates xtrabackup arg validation/limits into the base CRD for restores. |
| config/crd/bases/ps.percona.com_perconaservermysqlbackups.yaml | Propagates xtrabackup arg validation/limits into the base CRD for backups. |
| cmd/sidecar/handler/backup/create.go | Promotes --defaults-file (when provided as the first custom arg) to the front of the xtrabackup argv. |
| cmd/sidecar/handler/backup/create_test.go | Adds unit tests verifying xtrabackup arg ordering, including defaults-file handling. |
| build/run-restore.sh | Reorders ${XB_EXTRA_ARGS} before other xtrabackup options in restore flows. |
| api/v1/perconaservermysql_types.go | Adds kubebuilder validations for xtrabackup args and limits storages map size via MaxProperties. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pooknull
marked this pull request as ready for review
August 19, 2026 13:06
pooknull
requested review from
egegunes,
gkech,
hors,
mayankshah1607,
nmarukovich and
oksana-grishchenko
as code owners
August 19, 2026 13:06
egegunes
reviewed
Aug 20, 2026
Comment on lines
559
to
560
| // +kubebuilder:validation:XValidation:rule="!self.exists(arg, arg == '--defaults-file' || arg.startsWith('--defaults-file=')) || self[0] == '--defaults-file' || self[0].startsWith('--defaults-file=')",message="--defaults-file must be the first xtrabackup argument" | ||
| Xtrabackup []string `json:"xtrabackup,omitempty"` |
Contributor
There was a problem hiding this comment.
please add an envtest to cover this
Comment on lines
+241
to
+243
| // kubebuilder validation guarantees that --defaults-file is the first custom argument if specified. | ||
| // XtraBackup requires it to be the first option: https://docs.percona.com/percona-xtrabackup/8.0/xtrabackup-option-reference.html#defaults-file | ||
| end := 0 |
Contributor
There was a problem hiding this comment.
if we already guarantee this on CRD level, what's the purpose of this code?
Contributor
There was a problem hiding this comment.
I believe because here we take this config over HTTP so we can trust nothing from the request body I guess
Contributor
There was a problem hiding this comment.
maybe the comment needs a little rewording
gkech
reviewed
Aug 25, 2026
| @@ -236,7 +237,23 @@ func xtrabackupArgs(user, pass string, conf *xb.BackupConfig) []string { | |||
| } | |||
| } | |||
| if conf != nil && conf.ContainerOptions != nil { | |||
Contributor
There was a problem hiding this comment.
maybe not related to your changes, but in this function in general we could move conf !=nil early, revert the clause and return it instead of repeating it all the time
gkech
reviewed
Aug 25, 2026
| end := 0 | ||
| if len(customArgs) > 0 { | ||
| switch { | ||
| case strings.HasPrefix(customArgs[0], "--defaults-file=") && customArgs[0] != "--defaults-file=": |
Contributor
There was a problem hiding this comment.
Does xtrabackup actually honor the space-separated form here? As I read MySQL's get_defaults_options / load_defaults, --defaults-file is consumed before normal option parsing and only the --defaults-file= prefix is matched, so --defaults-file /etc/my.cnf wouldn't be picked up as a defaults file at all, and the path would fall through as an unrecognized positional arg. Is that correct?
Comment on lines
+242
to
+251
| if conf.ContainerOptions != nil { | ||
| customArgs := conf.ContainerOptions.Args.Xtrabackup | ||
| // kubebuilder validation guarantees that --defaults-file=<path> is the first custom argument if specified. | ||
| // https://docs.percona.com/percona-xtrabackup/8.0/xtrabackup-option-reference.html#defaults-file | ||
| // We should move it to the beginning of args. Other custom arguments should be appended after the generated arguments. | ||
| if len(customArgs) > 0 && strings.HasPrefix(customArgs[0], "--defaults-file=") && customArgs[0] != "--defaults-file=" { | ||
| args = append([]string{customArgs[0]}, args...) | ||
| customArgs = customArgs[1:] | ||
| } | ||
| args = append(args, customArgs...) |
Member
There was a problem hiding this comment.
If we're re-ordering it anyway, do we need a CRD validation? Can we also do the same for restore?
egegunes
approved these changes
Sep 1, 2026
Collaborator
commit: 8e6ad52 |
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.
https://perconadev.atlassian.net/browse/K8SPS-763
DESCRIPTION
Problem:
Xtrabackup requires
--defaults-fileto be the first option. The operator placed custom xtrabackup arguments after its own, causing backups using--defaults-fileto fail.Solution:
Place
--defaults-filebefore all generated arguments during backups.CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability