test(longhaul): add data-protection verifier (ScheduledBackup + retention) by WentingWu666666 · Pull Request #419 · documentdb/documentdb-kubernetes-operator

WentingWu666666 · 2026-07-08T23:24:48Z

Long-haul data-protection verifier (ScheduledBackup + retention)

Adds the Data protection category from the long-haul design (docs/designs/long-haul-test-design.md → Operations) to the driver introduced in #405. Scope: exercise a ScheduledBackup on the canary cluster and verify the properties only a multi-day run can establish — that backups keep being produced and completing, and that expired backups are actually garbage-collected so the backup population stays bounded.

What this PR adds

A new test/longhaul/backup package: a long-lived verifier that ensures a ScheduledBackup against the canary cluster and runs concurrently with the operation scheduler. Per the design, backup is deliberately not isolated from topology/chaos so that backup-vs-topology serialization bugs surface here.

Each verification cycle checks only what a multi-day canary can uniquely prove (unit/e2e tests can't):

Check	Signal
Scheduling liveness	`status.lastScheduledTime` keeps advancing; a stalled scheduler (past `nextScheduledTime` + grace) warns
Completion	child `Backup` CRs keep reaching `completed`; terminal `failed` counted (a `skipped` backup is an intentional standby no-op, not a failure)
Retention leak	no completed backup outlives its retention window (`stoppedAt + spec.retentionDays24h` + grace) -> a lingering backup is a FAIL*

A retention leak flips the run verdict to FAIL and is surfaced in the report's new Data Protection section.

Design notes

Black-box oracle. The leak deadline is derived from each backup's own declared spec.retentionDays (stamped by the ScheduledBackup at creation), not the operator-populated status.expiredAt. This verifies the outcome (expired backups disappear) without re-deriving the operator's expiration arithmetic — which is a pure function already covered by the operator's own unit tests (backup_funcs_test.go) and needs no accumulation. Keying off the per-backup value also keeps the check correct if a later run uses a different retention.
Bounded state. The dedup maps are pruned to the live backup set each cycle (backup names are timestamp-unique and never reused), so the verifier's own memory stays bounded over a multi-day run.
Because the minimum meaningful retention is 1 day, the leak check only fires on multi-day runs — exactly the accumulation window long-haul exists to cover.

Changes

test/longhaul/backup/{metrics,verifier}.go (+ unit tests, suite): the verifier, atomic metrics, and the retention-leak oracle.
monitor/k8sclient.go: EnsureScheduledBackup / GetScheduledBackup / ListScheduledChildBackups. EnsureScheduledBackup reconciles an existing CR in place when a run's schedule/retention differ (history preserved, never recreated). Child backups are selected by the scheduledbackup=<name> label.
config.go: LONGHAUL_BACKUP_ENABLED / LONGHAUL_BACKUP_SCHEDULE / LONGHAUL_BACKUP_RETENTION_DAYS + validation. (The verifier poll cadence is an internal package const, not an env knob.)
report.go: Data Protection report section; main.go: bootstrap + goroutine wiring + FAIL gating.
deploy/setup.yaml: corrected AKS prerequisite to k8s >= 1.35 (operator requires ImageVolume, GA in 1.35).
.gitignore: re-include test/longhaul/backup/ (the generic Visual Studio Backup*/ rule swallowed it on case-insensitive filesystems, same as the e2e backup dirs).

Follow-up dependency

The driver ServiceAccount needs create/get/list/update on scheduledbackups.documentdb.io and list on backups.documentdb.io. Those verbs belong in deploy/rbac.yaml (added by the CI/CD PR); without them the verifier logs an error and the rest of the run continues. Documented in test/longhaul/README.md.

Verification

```
cd test/longhaul
go build ./... # clean
go vet ./... # clean
go test ./... # all packages pass
```

Adds the data-protection component of the long-haul driver. A new test/longhaul/backup package bootstraps a ScheduledBackup against the canary cluster and runs a verifier concurrently with the operation scheduler (backup is deliberately not isolated, per the design). Each verification cycle checks: - scheduling liveness (status.lastScheduledTime advances) - child Backup completion and terminal failures - retention correctness (expiredAt == stoppedAt + retentionDays*24h) - retention garbage collection (no backup lingers past expiredAt) Retention or GC violations flip the run verdict to FAIL. New K8sClusterClient methods (EnsureScheduledBackup / GetScheduledBackup / ListScheduledChildBackups) back the verifier's Client interface, and new LONGHAUL_BACKUP_* env vars configure it. Also re-includes test/longhaul/backup/ in .gitignore (the generic Backup*/ VS rule swallowed it on case-insensitive filesystems). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

documentdb-triage-tool · 2026-07-08T23:36:49Z

🤖 Auto-triaged by documentdb-triage-tool.

Applied: test, documentation, enhancement
Project fields suggested: Component test · Priority P2 · Effort L · Status In Progress
Confidence: 0.85 (mixed)

Reasoning

component from path globs (test, docs); effort from diff stats (915+10 LOC, 12 files); LLM: Adds a new long-haul test verifier package for ScheduledBackup and retention correctness, spanning multiple files across test, monitor/k8sclient, config, report, and main wiring — a substantial multi-file addition within the test component.

If a label is wrong, remove it manually and ping @patty-chow so the rules can be tuned. The bot will not re-label items that already have component labels.

Drop the retention-arithmetic check (expiredAt == stoppedAt + retentionDays*24h). That is a pure function already covered by the operator unit tests (backup_funcs_test.go) and is evaluable on the very first backup, so it does not belong in a multi-day canary. Replace the two retention checks (correctness + GC) with a single black-box retention-leak oracle: a completed backup must not outlive its policy window (stoppedAt + cfg.RetentionDays*24h + grace). The deadline is derived from the driver's own config, not the operator's ExpiredAt field, so the check verifies the outcome (expired backups disappear, population stays bounded) without coupling to operator internals. Keeps: scheduling liveness + completion tracking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

Remove comments that merely restate the code (interface method and Config field doc lines), and fix two comments left stale by the trim: checkChildBackups no longer verifies retention arithmetic, and BackupRetentionDays is no longer validated against operator-computed expiration. De-duplicate the black-box rationale (now stated once in the package doc) from checkRetentionLeak and the RetentionLeaks field. Drop the unused retentionDays/expired parameters from the mkBackup test helper: the leak oracle derives its deadline from the verifier config, so Spec.RetentionDays and Status.ExpiredAt are never read. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

Remove three Metrics fields that had no report or verdict consumer: - VerifyCycles: incremented every pass but never surfaced in the report or verdict; only its own test read it. - StartTime / Elapsed: the backup snapshot's Elapsed was never read (the report's duration comes from the workload snapshot), and StartTime existed only to compute it. Remaining fields (Scheduled, Completed, Failed, RetentionLeaks, LastChildCount, LastScheduled) each back a report row and/or the FAIL verdict. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

…retention oracle Previously EnsureScheduledBackup was create-if-not-exists: starting a run with a different schedule/retention silently kept the old CR, while the verifier judged backups against the new config. A retention mismatch could raise a false-positive leak FAIL. Fix both sides: - EnsureScheduledBackup now reconciles an existing CR in place when the requested schedule/retention differ (history preserved, never recreated), so a run's parameters actually take effect. - The retention-leak oracle derives each backup's deadline from that backup's own spec.retentionDays (stamped by the ScheduledBackup at creation), falling back to run config, so it stays correct regardless of later parameter changes or a pre-existing CR. Update Bootstrap/README docs and note the added 'update' RBAC verb on scheduledbackups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

The seenCompleted/seenFailed/leakFlagged maps were keyed by backup name and never pruned, so they grew monotonically for the life of the process. The rate is tiny (~4/day at the default schedule), but a driver built to catch accumulation bugs should not itself accumulate unbounded state. Prune each map to the set of backups present in the current list every cycle. Backup names are timestamp-unique and never reused, so a pruned entry cannot reappear and be double-counted; the cumulative counters are unaffected. This bounds the maps to the live backup population. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

The operator requires ImageVolume support (GA in Kubernetes 1.35) and crash-loops on older nodes, so the setup prerequisite of k8s >= 1.30 was incorrect. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

BackupPhaseSkipped is set by the operator when a backup is intentionally not taken (e.g. the cluster is a non-primary/standby) — an expected no-op, not a failure of the backup machinery. Lumping it into isTerminalFailure inflated the Failed count and logged an intentional skip as a 'terminal failure phase'. Treat only BackupPhaseFailed as a genuine terminal failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

The backup verifier's poll cadence is an internal implementation detail, not a test-shaping parameter. Collapse LONGHAUL_BACKUP_VERIFY_INTERVAL (env var, Config field, default, parsing, validation) into a verifyInterval const in the backup package, matching workload.verifier's existing pattern. Also drop the stale 'validated against operator-computed expiration' note from the retention-days README row. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Wenting Wu <wentingwu@microsoft.com>

Copilot

Pull request overview

Adds the Data Protection long-haul verification component to the test/longhaul canary driver, ensuring ScheduledBackups continue to be scheduled/completed over multi-day runs and that retention actually garbage-collects expired backups (bounded backup population).

Changes:

Introduces a new test/longhaul/backup package with a verifier + atomic metrics + unit tests for ScheduledBackup liveness, completion, and retention-leak detection.
Extends the long-haul monitor client with helpers to ensure/read a ScheduledBackup and list its child Backup CRs via the scheduledbackup=<name> label.
Wires the verifier into cmd/longhaul, adds env configuration + validation, and surfaces backup metrics + FAIL gating in the final report.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`test/longhaul/backup/metrics.go`	Adds atomic counters + snapshot API for backup verification metrics.
`test/longhaul/backup/metrics_test.go`	Unit tests for metrics snapshot + retention-leak signal.
`test/longhaul/backup/verifier.go`	Implements verifier loop for scheduling liveness, completion/failure counting, and retention-leak detection.
`test/longhaul/backup/verifier_test.go`	Unit tests for verifier behavior, dedup/pruning, and retention oracle.
`test/longhaul/backup/suite_test.go`	Ginkgo suite bootstrap for the new package.
`test/longhaul/monitor/k8sclient.go`	Adds ScheduledBackup ensure/get + child-backup listing filtered by scheduledbackup label.
`test/longhaul/config/config.go`	Adds env knobs for enabling backups, schedule, and retention days + validation.
`test/longhaul/config/config_test.go`	Adds validation coverage for backup-related config.
`test/longhaul/cmd/longhaul/main.go`	Boots the backup verifier concurrently with operations and includes retention-leak FAIL gating in summary.
`test/longhaul/report/report.go`	Adds “Data Protection” section to the generated markdown report.
`test/longhaul/deploy/setup.yaml`	Updates documented AKS prerequisite version to k8s >= 1.35.
`test/longhaul/README.md`	Documents the new data-protection verifier configuration/behavior and RBAC needs.
`.gitignore`	Re-includes `test/longhaul/backup/` so it isn’t ignored by the generic `Backup*/` rule on case-insensitive filesystems.

+- **Completion** — child `Backup` CRs keep reaching `completed`; terminal
+  failures (`failed` / `skipped`) are counted.


xgerman · 2026-07-10T21:01:05Z

+		It("fails when backups enabled but schedule is empty", func() {
+			cfg := DefaultConfig()
+			cfg.ClusterName = "test"
+			cfg.BackupSchedule = ""


[Required] Please also clear the new backup env vars (LONGHAUL_BACKUP_ENABLED, LONGHAUL_BACKUP_SCHEDULE, LONGHAUL_BACKUP_RETENTION_DAYS) in the LoadFromEnv BeforeEach. Right now tests are not hermetic: with LONGHAUL_BACKUP_RETENTION_DAYS=abc in the shell, multiple LoadFromEnv specs fail before asserting their target behavior.

xgerman · 2026-07-10T21:01:05Z

+- **Scheduling liveness** — `status.lastScheduledTime` keeps advancing; a stalled
+  scheduler (past `status.nextScheduledTime` + grace) raises a warning.
+- **Completion** — child `Backup` CRs keep reaching `completed`; terminal
+  failures (`failed` / `skipped`) are counted.


[Required] This sentence conflicts with the implementation in backup/verifier.go: BackupPhaseSkipped is explicitly treated as a non-failure (expected no-op on standby) and is not counted in Failed. Please align the README wording so operators dont interpret skipped backups as failures.

xgerman

Summary

The backup verifier design and structure are good overall (clear separation into backup package, bounded dedup state, and focused metrics). I found two required fixes before merge, both around correctness/clarity rather than style.

Critical Issues

[Required] README behavior mismatch: docs currently say failed/skipped are counted as failures, but implementation intentionally excludes skipped from failure counting.
[Required] config tests are no longer hermetic for the new backup env vars. Repro: LONGHAUL_BACKUP_RETENTION_DAYS=abc go test ./test/longhaul/config causes multiple unrelated LoadFromEnv specs to fail.

Suggestions

[Suggestion] Once the above is fixed, add a small LoadFromEnv coverage case for the new backup env knobs (enabled, schedule, retentionDays) to keep parser behavior explicit.

Questions

None.

Local kind validation

Ran unit/module tests: cd test/longhaul && go test ./... ✅
Attempted local kind runtime validation:
- go run ./cmd/longhaul against existing pgcosmos-emulator service via port-forward failed during Mongo ping (length of read message too large), so it was not a valid longhaul target for this driver run.
- Tried provisioning a temporary DocumentDB CR for longhaul testing, but this cluster currently has a cert-manager webhook certificate expiry (x509: certificate has expired or is not yet valid), so reconciliation could not complete.

Given the current kind environment state, runtime E2E verification of this PR could not be completed here, but the unit/module coverage for the new code is passing.

documentdb-triage-tool Bot added documentation Improvements or additions to documentation enhancement New feature or request test labels Jul 8, 2026

wentingwu000 and others added 8 commits July 9, 2026 13:23

WentingWu666666 changed the title ~~test(longhaul): add ScheduledBackup + retention verifier~~ test(longhaul): add data-protection verifier (ScheduledBackup + retention) Jul 9, 2026

WentingWu666666 marked this pull request as ready for review July 9, 2026 21:14

WentingWu666666 requested review from alaye-ms, hossain-rayhan and xgerman as code owners July 9, 2026 21:14

Copilot AI review requested due to automatic review settings July 9, 2026 21:14

Copilot started reviewing on behalf of WentingWu666666 July 9, 2026 21:14 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

Comment thread test/longhaul/README.md

Comment on lines +154 to +155

- **Completion** — child `Backup` CRs keep reaching `completed`; terminal

failures (`failed` / `skipped`) are counted.

xgerman reviewed Jul 10, 2026

View reviewed changes

xgerman requested changes Jul 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test(longhaul): add data-protection verifier (ScheduledBackup + retention)#419

test(longhaul): add data-protection verifier (ScheduledBackup + retention)#419
WentingWu666666 wants to merge 9 commits into
documentdb:mainfrom
WentingWu666666:developer/wentingwu/longhaul-backup-ops

WentingWu666666 commented Jul 8, 2026 •

edited

Loading

Uh oh!

documentdb-triage-tool Bot commented Jul 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

xgerman Jul 10, 2026

Uh oh!

xgerman Jul 10, 2026

Uh oh!

xgerman left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		- Completion — child `Backup` CRs keep reaching `completed`; terminal
		failures (`failed` / `skipped`) are counted.

Uh oh!

Conversation

WentingWu666666 commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Long-haul data-protection verifier (ScheduledBackup + retention)

What this PR adds

Design notes

Changes

Follow-up dependency

Verification

Uh oh!

documentdb-triage-tool Bot commented Jul 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

xgerman Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

xgerman Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

xgerman left a comment

Choose a reason for hiding this comment

Summary

Critical Issues

Suggestions

Questions

Local kind validation

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

WentingWu666666 commented Jul 8, 2026 •

edited

Loading