Repository navigation
[release-2.17] Fix backup schedule timeout and scheduling state (#818) - #821
Merged
Merged
Conversation
Backport of #818 to release-2.17. Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
force-pushed
the
backport-818-to-release-2.17
branch
from
September 2, 2026 14:52
f50278c to
dd3523c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Timeout cleanup can skip PVC deletion retries once a timed-out Job has a DeletionTimestamp, potentially leaking vtbackup PVCs after transient PVC cleanup errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Backports the fix for VitessBackupSchedule reconcile loops by making scheduling state durable (status-based cursor) and by measuring timeouts from the Kubernetes Job’s actual status.startTime rather than the cron slot timestamp.
Changes:
- Switch scheduling cursor behavior to rely on persisted
status.lastScheduledTimes, with repair from newer Job annotations before history cleanup. - Rework timeout cleanup to use
Job.status.startTime, supportjobTimeoutMinute: -1, and delete timed-out Jobs with foreground propagation. - Update CRD/OpenAPI schema + generated API docs and add regression tests to prevent reintroduction.
File summaries
| File | Description |
|---|---|
| test/endtoend/operator/operator-latest.yaml | Updates OpenAPI schema minimum to allow jobTimeoutMinute: -1. |
| pkg/controller/vitessbackupschedule/vtctldclient_backup_test.go | Adjusts timeout test to set Job.status.startTime and pass a deterministic now. |
| pkg/controller/vitessbackupschedule/vitessbackupschedule_controller.go | Core controller changes: durable scheduling cursor, startTime-based timeout cleanup, conditional status updates. |
| pkg/controller/vitessbackupschedule/scheduling_regression_test.go | Adds targeted regression coverage for scheduling cursor + timeout semantics + deletion behavior. |
| pkg/apis/planetscale/v2/vitessbackupschedule_types.go | Updates API docs/validation for JobTimeoutMinutes and allows -1. |
| docs/api/index.html | Regenerates HTML API docs to reflect updated timeout semantics. |
| docs/api.md | Regenerates Markdown API docs to reflect updated timeout semantics. |
| deploy/crds/planetscale.com_vitessclusters.yaml | Updates CRD schema minimum for jobTimeoutMinute to -1. |
| deploy/crds/planetscale.com_vitessbackupschedules.yaml | Updates CRD schema minimum for jobTimeoutMinute to -1. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mhamza15
approved these changes
Sep 2, 2026
migue
approved these changes
Sep 2, 2026
Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
force-pushed
the
backport-818-to-release-2.17
branch
from
September 2, 2026 17:26
7ad3e80 to
a08c521
Compare
mattlord
marked this pull request as draft
September 2, 2026 17:43
* Migrate end-to-end tests to GitHub Actions The end-to-end suite still depends on Buildkite-specific workers and sibling-container networking. This replaces the eight Buildkite steps with a parallel GitHub Actions matrix on 16-core Depot runners, removes the obsolete agent setup, and keeps the test tool versions pinned across CI hosts. Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * Update pinned vtctldclient to Vitess 24.0.0-rc1 The pinned vtctldclient was still 21.0.0-rc1 (from October 2024) while the e2e clusters run `vitess/lite:v24.0.0-rc1`, so the client was driving servers three major versions ahead of it. Bumps the pin and its checksum to match the server images. Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * use same version for kube as kubectl Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * Surface pod-startup failures in e2e jobs before the job timeout Every e2e job hung waiting for the vitess-operator pod and was killed by GitHub with no diagnostics: `checkPodStatusWithTimeout` waits up to 20 minutes before dumping pod state and exiting, but `timeout-minutes` was also 20, so the job died a few minutes before the helper's error path could run. Raises the job timeouts above the helper's wait and adds a failure-only step that dumps pod status, recent events, pod descriptions and operator logs while the kind cluster is still alive. Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * Rename default kind cluster so it can't match pod-wait regexes Every e2e job hung waiting for the vitess-operator pod even though it was Running. The kind cluster was renamed to "vitess-operator" in this branch, which makes the node (and the static kube-system pods) embed that string: `kube-apiserver-vitess-operator-control-plane` and three siblings all match `checkPodStatusWithTimeout`'s `vitess-operator(.*)1/1(.*)Running` regex, so the helper counts 5 matches instead of the expected 1 and never succeeds. Defaults `KIND_CLUSTER_NAME` to "vtop-e2e", which is not a substring of any regex the tests wait on. Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * Drop explanatory comment on KIND_CLUSTER_NAME default Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * Work around Ubuntu 24.04 AppArmor killing mysqld inside kind vttablet pods crashloop on the GitHub Actions runners: Ubuntu 24.04's AppArmor docker-default profile and unprivileged-userns restrictions kill mysqld in the nested vttablet -> kind -> docker stack, which Buildkite's older kernels never enforced. Validated in PR #781 on the same runners. Relaxes the userns sysctls and raises kind's inotify limits, then builds the operator image before tearing AppArmor down (BuildKit can't start build containers once the profile is unloaded). setupBuildContainerImage now skips the build when the image already exists so the make target reuses the pre-built image. Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * Dump per-pod logs in the e2e failure diagnostics The vtorc pod crashloops with exit code 1 but the failure dump only captured pod listings, events and descriptions, none of which say why. Dumps current and previous container logs for every pod in the example namespace, and fixes the operator log dump to use the default namespace the operator actually deploys into. Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * vtorc: add required `--cell` flag Current VTOrc builds reject startup when `--cell` is absent, leaving the pod in CrashLoopBackOff. This passes the tablet pool cell already stored in `vtorc.Spec` to the VTOrc command. Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * Update end-to-end-test.yaml Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * Fix end-to-end test dependency installation The dependency script installs kind and vtctldclient in top-level blocks, while kubectl uses a named helper. The VTAdmin job also installs Ubuntu's snap-based Chromium launcher, which stops working after the workflow tears down AppArmor. This adds matching installer functions for kind and vtctldclient. It also installs a normal Chrome package for the VTAdmin job and exposes it as chromium before AppArmor is disabled. Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> * Apply suggestions from code review Signed-off-by: Mohamed Hamza <mhamza@fastmail.com> --------- Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
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.
Summary
Backport of #818 (fixes #817) to
release-2.17.Fix backup-schedule create/delete loops caused by stale cron timestamps and lost Job history. The controller now uses persisted per-strategy status as its scheduling cursor and measures timeouts from the actual Job start time.
This prevents completed or timed-out Jobs from resetting scheduling state, including when
successfulJobsHistoryLimit: 0or the operator catches up a missed cron slot after downtime.This also backports the following CI changes as we could not otherwise merge anything in the release-2.17 branch:
mainas wellChanges
status.lastScheduledTimesas the durable scheduling cursor.jobTimeoutMinutefromJob.status.startTime; Jobs that have not started cannot time out.jobTimeoutMinute: -1to disable timeout cleanup. The default remains 10 minutes.concurrencyPolicy: Forbid, a terminating Job remains active until Kubernetes removes its Pods, blocking a replacement.VitessBackupSchedulestatus only when it changes.The cherry-pick of 168d0fd applied cleanly with no conflicts.
🤖 Generated with Claude Code