SEP-1648: Ship per-upload-selection xtrabackup payload variants - #1302
SEP-1648: Ship per-upload-selection xtrabackup payload variants#1302marcuscruz-percona wants to merge 7 commits into
Conversation
The xtrabackup payload ships all three upload-provider classes but a task only ever exercises the providers named in its form, and the payload sits two bytes under the 16 KiB Nomad dispatch limit. Delimit each provider's imports, constants, class and dispatch entry with GEN:UPLOAD marker comments and add a generator that emits one variant per selection with the unreachable providers stripped. The canonical payload is the all-providers variant, so it stays hand-edited and is never rewritten; the other seven are generated beside it. Markers cost nothing at dispatch because the executor minifies before sending. Variant filenames end in _payload so the check-nomad-payload-size hook's file pattern matches every one of them. The generator refuses to write a variant that parses but still references an omitted provider's exclusive names -- the payloads are never executed by CI, so that orphan would otherwise only surface mid-backup on a customer host. A --check mode guards against drift, wired as a pre-commit hook and as make regen-xtrabackup-variants[-check].
build_backup_spec now picks the payload by the set of upload providers on the form, so a task carries only the provider code it can reach: from 1,712 bytes of reclaimed dispatch budget for a backup with no upload configured down to zero for one selecting all three. Selection is keyed on the set, not the list, so form ordering cannot change the dispatched payload. boto3 is requested only when the shipped variant actually imports it -- asking for it otherwise makes the task install a dependency it never loads. Mydumper and binlog requirements are untouched.
marcuscruz-percona
added
python
qa in progress
There was a problem hiding this comment.
Pull request overview
This PR introduces size-optimized dispatch payloads for the MySQL backups XtraBackup task by generating per-upload-selection payload variants, keeping the canonical all-providers payload as the single hand-edited source while stripping unreachable provider code from the other variants.
Changes:
- Added a generator (
scripts/gen_xtrabackup_payload_variants.py) that renders and validates seven generatedxtrabackup_*_payloadvariants from the canonicalxtrabackup_payload. - Updated XtraBackup spec building to select the dispatched payload variant based on the set of selected upload providers, and to include
boto3only when an S3-capable variant is dispatched. - Added tests, snapshots, Make targets, pre-commit drift checking, and a changelog fragment to guard against variant drift and ensure correct selection.
Reviewed changes
Copilot reviewed 10 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/gen_xtrabackup_payload_variants.py |
New generator that strips marked provider regions, adds a banner, validates omissions, and supports --check drift detection. |
app/sep/apps/mysql_backups/xtrabackup_payload |
Adds # GEN:UPLOAD-BEGIN/END markers around provider-specific imports, constants, classes, and dispatch entries. |
app/sep/apps/mysql_backups/xtrabackup_noupload_payload |
New generated “no upload” payload variant emitted from the canonical source. |
app/sep/apps/mysql_backups/xtrabackup_rsync_payload |
Generated variant for rsync-only selection. |
app/sep/apps/mysql_backups/xtrabackup_s3_payload |
Generated variant for s3-only selection. |
app/sep/apps/mysql_backups/xtrabackup_gsutil_payload |
Generated variant for gsutil-only selection. |
app/sep/apps/mysql_backups/xtrabackup_rsync_s3_payload |
Generated variant for rsync+s3 selection. |
app/sep/apps/mysql_backups/xtrabackup_rsync_gsutil_payload |
Generated variant for rsync+gsutil selection. |
app/sep/apps/mysql_backups/xtrabackup_s3_gsutil_payload |
Generated variant for s3+gsutil selection. |
app/sep/apps/mysql_backups/spec.py |
Selects the correct payload variant for XtraBackup and conditionally appends boto3 only for S3 selections. |
tests/scripts/test_gen_xtrabackup_payload_variants.py |
New unit tests for generator correctness, drift guarding, validation, and size gating. |
tests/app/sep/apps/mysql_backups/test_payload_variant_selection.py |
New tests that pin selection → payload mapping and boto3 requirements behavior. |
tests/app/sep/apps/mysql_backups/test_deps.py |
Updates expected payload name for XtraBackup dependency test case to match variant selection. |
tests/app/sep/snapshots/payload/mysql_backups__spec_path.json |
Snapshot update to reflect variant payload selection in the generated spec. |
.pre-commit-config.yaml |
Adds a pre-commit hook to enforce that generated variants are in sync (--check). |
Makefile |
Adds regen-xtrabackup-variants and regen-xtrabackup-variants-check targets. |
changelog.d/SEP-1648.changed.md |
Documents the user-visible change in payload dispatch behavior and conditional boto3 inclusion. |
Suppressed comments (6)
tests/scripts/test_gen_xtrabackup_payload_variants.py:102
- Test-function docstring should start with an imperative verb (e.g., “Assert …”); starting with “The …” doesn’t meet the docstring mood rule applied to function docstrings.
"""The canonical payload is the all-providers variant and stays hand-edited."""
tests/scripts/test_gen_xtrabackup_payload_variants.py:170
- Test-function docstring should start with an imperative verb; starting with “An …” doesn’t follow the docstring mood rule for functions.
"""An opened region with no END is a hard error, not a silent truncation."""
tests/scripts/test_gen_xtrabackup_payload_variants.py:175
- Test-function docstring should start with an imperative verb; starting with “Two …” doesn’t follow the docstring mood rule for functions.
"""Two overlapping regions are rejected -- the omission would be ambiguous."""
tests/scripts/test_gen_xtrabackup_payload_variants.py:181
- Test-function docstring should start with an imperative verb; starting with “A …” doesn’t follow the docstring mood rule for functions.
"""A stray END marker is rejected rather than ignored."""
tests/scripts/test_gen_xtrabackup_payload_variants.py:186
- Test-function docstring should start with an imperative verb; starting with “A …” doesn’t follow the docstring mood rule for functions.
"""A marker naming a provider the generator does not know is a hard error."""
tests/scripts/test_gen_xtrabackup_payload_variants.py:191
- Docstring first line should start with an imperative verb; starting with “A region …” doesn’t follow the docstring mood rule for function docstrings.
"""A region drawn too narrowly leaves an orphan, which must fail at build time.
Single-source the variant naming rule, widen the orphan guard, and stop the test suite writing into the payload tree. The naming rule had two independent implementations -- one in the generator, one in build_backup_spec -- that had to agree or the dispatcher would name a payload that does not exist. Both now read payload_variants.py. It is stdlib-only and the generator loads it by path: importing the package would stand up the FastAPI app, which a pre-commit hook must not need. The size hook's file pattern now also fires on the rule file and the generator. The orphan check only intersected against a hand-maintained exclusive-name list, so a symbol added to a region later would ship stranded. validate() now also sweeps for names nothing in the variant binds, which needs no bookkeeping. test_canonical_is_never_rewritten called the generator against the working tree, so a suite run rewrote the seven variants and silently repaired any drift the --check guard exists to surface. It now runs against a tmp_path copy, and two new tests cover drift detection and repair. No variant shifts a byte.
|
Pushed CorrectnessThe orphan guard was narrower than the hazard it names. The test suite was writing into the payload tree. StructureThe variant naming rule had two independent implementations —
Also: dead MeasurementsThe margin table in the description was stale — it has been corrected against a fresh measurement through the gate's own minify+gzip pipeline. The Jira comment's numbers were the accurate ones. The floor is No variant shifts a single byte through any of this: Not changed, deliberately
Verification
Two acceptance criteria still need a decision, both flagged in the description and in a comment on SEP-1648: AC7's "at least 900 bytes" is unachievable for |
Eleven test docstrings opened with a noun phrase or a literal, which breaks the repo's imperative-mood docstring rule.
marcuscruz-percona
added
qa passed
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: marcuscruz-percona <272857389+marcuscruz-percona@users.noreply.github.com>
Resolved. I merged |
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: marcuscruz-percona <272857389+marcuscruz-percona@users.noreply.github.com>
Done. I merged |
github-actions
Bot
added
the
app:mysql_backups
github-actions
Bot
added
the
large-diff
Summary
# GEN:UPLOAD-BEGIN/ENDmarkers andscripts/gen_xtrabackup_payload_variants.pyemits one payload per upload selection with the unreachable providers stripped. The canonical payload is the all-providers variant, so it stays hand-edited and is never rewritten; the other seven are generated beside it. Markers cost 0 bytes at dispatch (16,382 before and after) because the executor minifies first.build_backup_specselects the variant by the set of providers on the form, so form ordering cannot change the dispatched payload, and requestsboto3only when the shipped variant actually imports it.app/sep/apps/mysql_backups/payload_variants.py, read by both the dispatcher and the generator, so the two cannot disagree on a name. It is stdlib-only and the generator loads it by path — importing the package would stand up the FastAPI app, which a pre-commit hook must not need.--check, wired as a pre-commit hook and asmake regen-xtrabackup-variants[-check]. Tests cover the generator, its malformed-region handling, drift detection and repair, per-variant size, and dispatch selection.Reclaimed margin against the 16 KiB gate (minify + gzip, the pipeline the gate applies):
Two acceptance criteria need amending
s3alone lands at 730 — S3 is the largest provider block, so an s3-only variant can only shed the other two. The other single-provider variants clear 900 comfortably.Three implementation notes worth a reviewer's attention
_payload(xtrabackup_s3_payload, notxtrabackup_payload_s3) because the size gate's pattern is(?:payload|[^/]*_payload)$. The other naming would have been silently skipped by the very gate this ticket exists to satisfy.restore/spec.pystill dispatches the canonical payload for every backup type, because a restore may have to reach any provider. Only the backup path is variant-selected.Existing stored tasks keep their recorded payload reference and the full-provider payload they point at still ships, so this reaches tasks created or updated after the upgrade only.
Tested
xtrabackup_rsync_payloadboto3is installed by the taskboto3is absent from requirementsAutomated: full suite green;
make lintclean; all pre-commit hooks pass; the size gate passes for all eight variants;--checkis idempotent on the checked-in tree; the suite no longer writes into the payload tree while it runs.Checklist
make test)make run-pre-commit)make makemigrations) — N/A, no model changeschangelog.d/if the change is user-facing (make changelog-add), or confirmed N/A