Skip to content

test(e2e): add expired backup cleanup coverage#393

Open
xgerman wants to merge 1 commit into
documentdb:mainfrom
xgerman:developer/e2e-backup-expiration-370
Open

test(e2e): add expired backup cleanup coverage#393
xgerman wants to merge 1 commit into
documentdb:mainfrom
xgerman:developer/e2e-backup-expiration-370

Conversation

@xgerman

@xgerman xgerman commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Closes #370.

What

Adds an e2e spec verifying the operator garbage-collects Backup CRs once status.expiredAt is in the past, plus two reusable helpers in test/e2e/pkg/e2eutils/backup:

  • MarkExpired — patches status.expiredAt via the status subresource to fast-forward expiration without waiting out the real retentionDays window. Routing through the status subresource matters because the Backup type declares +kubebuilder:subresource:status, so a plain c.Patch on the same field is a silent no-op.
  • WaitForBackupDeleted — polls until the Backup CR is gone, treating NotFound as success.

Why

Issue #370 — the operator's retention path (backup_controller.go IsExpired()r.Delete) had no e2e coverage. The new helpers also unblock similar cleanup-style specs later (scheduled-backup retention, restore-source pruning, etc.).

How the spec works

  1. Provision a 1-node DocumentDB.
  2. Create an on-demand Backup and wait for Completed — marking an in-flight backup expired would race the snapshot controller and the retentionDays contract is defined against completed backups.
  3. MarkExpired(now - 1h) via status patch.
  4. Assert the Backup CR is gone within 3 minutes (status patch fires a watch event, so deletion is event-driven, not bound to the 1-minute periodic requeue).

Labels: [backup, needs-csi-snapshots, level:medium].

Validation

  • go vet ./... clean across test/e2e
  • go test ./pkg/e2eutils/backup/... 11/11 pass (5.6s) — 4 new unit tests cover both status-subresource patch path and Get/NotFound semantics
  • ginkgo --focus 'expired backup cleanup' ./tests/backupPASS in 67s on local kind (CSI hostpath + external-snapshotter)
  • Fresh-checkout simulation (git archive HEAD | tar -x; go vet + test) clean

Out of scope

  • No CHANGELOG.md entry — test-only change, borderline per CONTRIBUTING.md examples.
  • No retry.RetryOnConflict wrapper on MarkExpired — single in-flight test writer, conflict surface is near-zero. Easy to add later if other specs adopt the helper concurrently.

Copilot AI review requested due to automatic review settings May 28, 2026 20:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds E2E coverage for expired Backup CR garbage collection, restoring coverage for the retention cleanup path described in issue #370.

Changes:

  • Adds a medium-level backup E2E spec that creates a completed backup, patches status.expiredAt into the past, and waits for deletion.
  • Adds reusable backup test helpers for marking backups expired via the status subresource and waiting for Backup CR deletion.
  • Adds unit tests for the new helper behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
test/e2e/tests/backup/backup_expiration_test.go Adds the expired-backup cleanup E2E scenario.
test/e2e/pkg/e2eutils/backup/backup.go Adds MarkExpired and WaitForBackupDeleted helpers.
test/e2e/pkg/e2eutils/backup/backup_test.go Adds unit coverage for the new backup helpers.

@documentdb-triage-tool documentdb-triage-tool Bot added enhancement New feature or request test labels May 28, 2026
@documentdb-triage-tool

Copy link
Copy Markdown

🤖 Auto-triaged by documentdb-triage-tool.

Applied: test, enhancement
Project fields suggested: Component test · Priority P2 · Effort M · Status Needs Review
Confidence: 0.88 (mixed)

Reasoning

component from path globs (test); effort from diff stats (239+0 LOC, 3 files); LLM: Adds e2e coverage for the operator's backup expiration/garbage-collection path (previously untested), plus two reusable test helpers, all scoped to the test component with no production code changes.

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.

@WentingWu666666 WentingWu666666 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small suggestions:

1. Downstream cleanup isn't asserted. The spec confirms the Backup CR disappears, but Reconcile also drives cleanup of the underlying VolumeSnapshot (and any CNPG-side artifacts) when a backup expires. Today the test would still pass if the controller deleted the CR but leaked the snapshot — which is the more user-visible failure mode (storage cost, orphaned objects). A quick Eventually that the associated VolumeSnapshot is also gone would close that loop.

2. No negative case. The spec only proves "expired → deleted." It doesn't prove "not-yet-expired → NOT deleted," which is the property that actually protects user data. A sibling spec that creates a backup, waits a reconcile interval or two without marking it expired, and asserts it's still present would guard against a regression where the controller becomes overly aggressive (e.g., a future refactor inverts the IsExpired check). Cheap to add — same fixture, no MarkExpired call.

@xgerman
xgerman force-pushed the developer/e2e-backup-expiration-370 branch from 0f191b5 to f60c69c Compare June 12, 2026 20:12
@xgerman
xgerman force-pushed the developer/e2e-backup-expiration-370 branch from f60c69c to 277ba6a Compare July 21, 2026 21:25
Closes documentdb#370.

Adds e2e coverage for the operator's backup retention path plus reusable
helpers, in two complementary specs:

  - expired → garbage-collected: cluster ready → completed on-demand
    Backup → MarkExpired(now-1h) → assert both the Backup CR and its
    underlying VolumeSnapshot are removed. The snapshot assertion closes
    the more user-visible leak (orphaned storage), exercising the full
    Backup → CNPG Backup → VolumeSnapshot cascade (operator sets
    SnapshotOwnerReference=backup).
  - not-yet-expired → retained: a completed Backup whose status.expiredAt
    is in the future must NOT be deleted, and neither must its snapshot.
    Guards user data against an inverted/over-aggressive IsExpired.

Helpers:
  - MarkExpired — status-subresource patch fast-forwarding expiration
    (avoids the silent no-op when patching through c.Patch on a
    +kubebuilder:subresource:status type).
  - WaitForBackupDeleted / WaitForSnapshotsDeletedForBackup — poll until
    the Backup CR / its labeled VolumeSnapshots are gone.
  - provisionReadyCluster / createCompletedBackup — shared spec setup
    that also de-duplicates the on-demand backup spec.

Labels: [backup, needs-csi-snapshots, level:medium].

Validation:
- go vet ./... clean
- go test ./pkg/e2eutils/backup/... pass (incl. new snapshot-deletion
  unit tests with label/namespace filtering distractors)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6285c813-739d-4ebc-9511-1439884e2c9f
Signed-off-by: German Eichberger <geeichbe@microsoft.com>
@xgerman
xgerman force-pushed the developer/e2e-backup-expiration-370 branch from 277ba6a to 0a31706 Compare July 21, 2026 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(e2e): add expired backup cleanup coverage

3 participants