velero: put restore-test probe PVCs on a Delete class, sweep orphans from dead runs #422

2026-07-26T08:52:51Z

claude-bot commented

2026-07-26 08:52:51 +00:00

Scope correction up front

The ask was "wire restore-test into ceph-block-ephemeral, retire the purge hack" — that came from a suggestion of mine, and investigating it showed the suggestion was wrong on both halves. Recording that plainly:

ceph-block-ephemeral is the wrong target. The RBD probe path already uses ceph-block-snapshot-readonly, which is already reclaimPolicy: Delete — it never leaked. And its noload mountOption is load-bearing: it skips ext4 journal replay on the read-only snapshot mount. ceph-block-ephemeral lacks that option, so pointing the probe at it would reintroduce the dirty-journal mount failure this cluster already hit.
The actual leak on the probe path is CephFS, not RBD. sc_for_snapshot_probe returned plain ceph-filesystem (Retain), so every CephFS probe stranded a Released PV.
purge_released_pvs cannot be retired. Details below.

The three throwaway-PVC categories

Only one is ours to choose a StorageClass for.

#	PVC category	Created by	Class	Fixable here?
1	source snapshot probes (`rt-src-*`)	this script	was `ceph-filesystem` (Retain)	✅ fixed
2	destination PVCs in `*-restore-test`	Velero restore	inherited from backup (Retain)	❌
3	data-mover staging PVCs in `velero/` (`-rtest-`)	Velero node-agent	derived from target PVC (Retain)	❌

Category 1 — fixed. CephFS branch now returns ceph-filesystem-ephemeral. I diffed the two classes: identical apart from reclaimPolicy (same provisioner, parameters, mountOptions), so the snapshot restores into it unchanged.

Category 2 — rejected, not deferred. The only remap knob is the change-storage-class RestoreItemAction ConfigMap. Per the v1.18 restore reference it is identified by labels and applies to every restore in the installation — there is no per-restore scoping, no label selector, no Restore CR field. Wiring it up would silently strip Retain from a real disaster-recovery restore, i.e. remove the safety net exactly when it matters. Saving a cleanup loop is not worth that trade.

Category 3 — not available. The node-agent restorePVC config supports only ignoreDelayBinding; unlike backupPVC there is no storageClass field on v1.18 (docs).

So 2 and 3 keep purge_released_pvs load-bearing. It stays — now with a comment explaining why, so the next person doesn't repeat this investigation.

Bonus: a residual leak the ephemeral class does not fix

While mapping the above I found a genuine hole. cleanup_service's purge is scoped to ${RUN_ID}:

purge_released_pvs "^(${dst_ns}/.*|velero/.*-rtest-${RUN_ID}-.*|.*/rt-src-.*-${RUN_ID})$"

Correct during a run — it must not touch a concurrent run's artifacts. But a run that dies before cleanup (activeDeadlineSeconds, OOM, node eviction, killed Job) strands its PVs permanently: no later run's pattern will ever match an older RUN_ID. That is a standing leak path independent of reclaim policy, and it plausibly contributed to incident 2026-05-17.

Added an unscoped sweep at run start. Safe because:

concurrencyPolicy: Forbid guarantees no other restore-test Job is active, so anything matching is by definition abandoned;
it only ever touches PVs in Released phase — never anything Bound to a live workload;
the patterns are restore-test-only naming (*-restore-test/, velero/*-rtest-*, */rt-src-*).

Verification

Dry-ran the sweep's exact jq selector against the live cluster (read-only, no patch/delete):

it matches zero PVs today;
the cluster's only three Released PVs are the deliberate Retain safety copies — crowdsec/crowdsec-db-pvc, openbao/data-openbao-0, home-assistant/mosquitto-data — and none of them match. That was the property I most wanted to prove before adding an unscoped sweep.

Categories 2 and 3 were confirmed empirically by observing the in-flight 08:00 run: destination PVCs provisioned on ceph-filesystem/ceph-block and the staging PVCs audiobookshelf-daily-rtest-* on ceph-filesystem — all Retain.

bash -n clean.

Merge timing

Worth a moment's thought: the weekly run started at 08:00 UTC and takes several hours. Merging updates the restore-test-script ConfigMap under the running pod. In practice bash holds an open fd and Kubernetes swaps the ConfigMap via atomic symlink, so the running script should keep reading the old inode — but there is no upside to merging mid-run. Suggest merging after the current run reports its summary.

🤖 Generated with Claude Code

## Scope correction up front The ask was "wire restore-test into `ceph-block-ephemeral`, retire the purge hack" — that came from a suggestion of mine, and investigating it showed the suggestion was wrong on both halves. Recording that plainly: - **`ceph-block-ephemeral` is the wrong target.** The RBD probe path already uses `ceph-block-snapshot-readonly`, which is **already `reclaimPolicy: Delete`** — it never leaked. And its `noload` mountOption is load-bearing: it skips ext4 journal replay on the read-only snapshot mount. `ceph-block-ephemeral` lacks that option, so pointing the probe at it would reintroduce the dirty-journal mount failure this cluster already hit. - **The actual leak on the probe path is CephFS**, not RBD. `sc_for_snapshot_probe` returned plain `ceph-filesystem` (Retain), so every CephFS probe stranded a Released PV. - **`purge_released_pvs` cannot be retired.** Details below. ## The three throwaway-PVC categories Only one is ours to choose a StorageClass for. | # | PVC category | Created by | Class | Fixable here? | |---|---|---|---|---| | 1 | source snapshot probes (`rt-src-*`) | this script | was `ceph-filesystem` (Retain) | ✅ **fixed** | | 2 | destination PVCs in `*-restore-test` | Velero restore | inherited from backup (Retain) | ❌ | | 3 | data-mover staging PVCs in `velero/` (`*-rtest-*`) | Velero node-agent | derived from target PVC (Retain) | ❌ | **Category 1 — fixed.** CephFS branch now returns `ceph-filesystem-ephemeral`. I diffed the two classes: identical apart from `reclaimPolicy` (same provisioner, parameters, mountOptions), so the snapshot restores into it unchanged. **Category 2 — rejected, not deferred.** The only remap knob is the `change-storage-class` RestoreItemAction ConfigMap. Per the [v1.18 restore reference](https://velero.io/docs/v1.18/restore-reference/) it is identified by labels and applies to **every restore in the installation** — there is no per-restore scoping, no label selector, no Restore CR field. Wiring it up would silently strip `Retain` from a real disaster-recovery restore, i.e. remove the safety net exactly when it matters. Saving a cleanup loop is not worth that trade. **Category 3 — not available.** The node-agent `restorePVC` config supports only `ignoreDelayBinding`; unlike `backupPVC` there is no `storageClass` field on v1.18 ([docs](https://velero.io/docs/v1.18/data-movement-restore-pvc-configuration/)). So 2 and 3 keep `purge_released_pvs` load-bearing. It stays — now with a comment explaining why, so the next person doesn't repeat this investigation. ## Bonus: a residual leak the ephemeral class does not fix While mapping the above I found a genuine hole. `cleanup_service`'s purge is scoped to `${RUN_ID}`: ``` purge_released_pvs "^(${dst_ns}/.*|velero/.*-rtest-${RUN_ID}-.*|.*/rt-src-.*-${RUN_ID})$" ``` Correct *during* a run — it must not touch a concurrent run's artifacts. But a run that dies before cleanup (`activeDeadlineSeconds`, OOM, node eviction, killed Job) **strands its PVs permanently**: no later run's pattern will ever match an older `RUN_ID`. That is a standing leak path independent of reclaim policy, and it plausibly contributed to incident 2026-05-17. Added an unscoped sweep at run start. Safe because: - `concurrencyPolicy: Forbid` guarantees no other restore-test Job is active, so anything matching is by definition abandoned; - it only ever touches PVs in **`Released`** phase — never anything Bound to a live workload; - the patterns are restore-test-only naming (`*-restore-test/`, `velero/*-rtest-*`, `*/rt-src-*`). ## Verification Dry-ran the sweep's exact `jq` selector against the live cluster (read-only, no patch/delete): - it matches **zero** PVs today; - the cluster's only three `Released` PVs are the deliberate `Retain` safety copies — `crowdsec/crowdsec-db-pvc`, `openbao/data-openbao-0`, `home-assistant/mosquitto-data` — and **none of them match**. That was the property I most wanted to prove before adding an unscoped sweep. Categories 2 and 3 were confirmed empirically by observing the in-flight 08:00 run: destination PVCs provisioned on `ceph-filesystem`/`ceph-block` and the staging PVCs `audiobookshelf-daily-rtest-*` on `ceph-filesystem` — all Retain. `bash -n` clean. ## Merge timing Worth a moment's thought: the weekly run started at 08:00 UTC and takes several hours. Merging updates the `restore-test-script` ConfigMap under the running pod. In practice bash holds an open fd and Kubernetes swaps the ConfigMap via atomic symlink, so the running script should keep reading the old inode — but there is no upside to merging mid-run. Suggest merging after the current run reports its summary. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-07-26 08:52:51 +00:00

velero: put restore-test probe PVCs on a Delete class, sweep orphans from dead runs

ci/woodpecker/pr/pr-review Pipeline was successful

Details

ci/woodpecker/pr/ci-scripts-test Pipeline was successful

Details

e16dfd8603

Follow-up to #416 (ceph-block-ephemeral) for restore-test.sh's own PV leak
(incident 2026-05-17, ~2.7 TB of orphan CephFS subvolumes).

The intent was to retire purge_released_pvs entirely. That is not safely
possible, and the reason is worth recording. Three categories of throwaway
PVC exist per iteration; only one is ours to pick a StorageClass for:

  1. source snapshot-probe PVCs (rt-src-*) — this script creates them.
     FIXED HERE: the CephFS branch of sc_for_snapshot_probe returned
     `ceph-filesystem` (Retain), so every probe leaked a Released PV. It
     now returns `ceph-filesystem-ephemeral` (Delete). The two classes are
     identical apart from reclaimPolicy — same provisioner, parameters and
     mountOptions — so the snapshot restores into it unchanged.

     The ceph-block branch needed no change and must not get one: it
     already uses ceph-block-snapshot-readonly, which is Delete, and whose
     `noload` mountOption is load-bearing (skips ext4 journal replay on the
     RO snapshot mount). ceph-block-ephemeral lacks noload, so pointing the
     probe at it would reintroduce the dirty-journal mount failure.

  2. destination PVCs in *-restore-test namespaces — Velero creates them
     from the backed-up PVC spec, so the class is whatever production uses
     (Retain). The only remap knob is the change-storage-class
     RestoreItemAction ConfigMap, which Velero applies to EVERY restore in
     the installation with no per-restore scoping. Using it would strip
     Retain from a real disaster-recovery restore. Rejected.

  3. Velero data-mover staging PVCs in velero/ (*-rtest-*) — derived from
     the target PVC. The node-agent restorePVC config supports only
     `ignoreDelayBinding`; unlike backupPVC there is no storageClass field
     on Velero v1.18.

So 2 and 3 keep purge_released_pvs load-bearing; it stays, now documented.

Also fixes a real residual leak the ephemeral class does not address:
cleanup_service's purge is scoped to ${RUN_ID}, which is right during a run
but means a run that dies before cleanup (activeDeadlineSeconds, OOM, node
eviction, killed Job) strands its PVs forever — no later run's pattern ever
matches an older RUN_ID. Adds an unscoped sweep at run start. Safe because
concurrencyPolicy Forbid guarantees no other restore-test Job is active, and
it only ever touches Released PVs whose claimRef matches restore-test-only
naming patterns.

Verified against the live cluster: the sweep pattern matches zero PVs today,
and in particular does not match the three intentional Retain safety PVs
(crowdsec/crowdsec-db-pvc, openbao/data-openbao-0,
home-assistant/mosquitto-data). Categories 2 and 3 were confirmed to
provision on Retain classes by observing an in-flight run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

review-bot reviewed

2026-07-26 08:55:46 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit e16dfd860386c6135e50aaf4f011154f60e676b9.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

Single-file change to kubernetes/infrastructure/velero/restore-test.sh: (1) points the CephFS snapshot-probe PVCs at the new ceph-filesystem-ephemeral (Delete) class instead of ceph-filesystem (Retain), closing a real leak; (2) documents why purge_released_pvs and the RBD probe's ceph-block-snapshot-readonly class stay as-is; (3) adds an unscoped sweep of Released, restore-test-named PVs at the start of each run to catch orphans left by a run that died before its own cleanup. I checked the claims against the actual StorageClass/VolumeSnapshotClass/CronJob/RBAC manifests in the checkout and they hold up. No blocking issues.

🟡 Suggestions

kubernetes/infrastructure/velero/restore-test.sh:118 vs :378 — the scoped pattern in cleanup_service ("^(${dst_ns}/.*|velero/.*-rtest-${RUN_ID}-.*|.*/rt-src-.*-${RUN_ID})$") and the new unscoped sweep pattern ('^(.*-restore-test/.*|velero/.*-rtest-.*|.*/rt-src-.*)$') encode the same three naming conventions independently, with the ${RUN_ID} scoping just dropped for the sweep. If the naming scheme for one of the three categories ever changes, it's easy to update one and forget the other, silently narrowing or widening the sweep. Not worth blocking on for a 3-line duplication, but a shared helper (e.g. a function returning the three sub-patterns, with the caller adding scoping) would remove the drift risk.

Tests

No automated tests exist for this script (confirmed no shellcheck/test step references it in .woodpecker/), consistent with the rest of the file — verification here is manual/empirical, as documented at length in the PR body (dry-ran the sweep's jq selector against the live cluster, confirmed it matches zero PVs today and correctly excludes the three legitimate Retain safety copies). I independently verified the two factual claims underpinning the fix by reading the manifests in the checkout:

ceph-filesystem-ephemeral (kubernetes/infrastructure/rook-ceph-cluster/storageclass.yaml:118-134) is identical to ceph-filesystem (:95-111) except reclaimPolicy: Delete vs Retain — same provisioner, parameters, and no mountOptions on either, so the snapshot-restore-into-ephemeral-class swap is safe.
ceph-block-snapshot-readonly (kubernetes/infrastructure/velero/snapshot-readonly-storageclass.yaml) is already reclaimPolicy: Delete and carries the noload mountOption that ceph-block-ephemeral (storageclass.yaml:41-59) lacks, confirming the RBD leg is correctly left untouched.
concurrencyPolicy: Forbid and backoffLimit: 0 on the CronJob (restore-test-cronjob.yaml:34,40) support the "unscoped sweep is safe" argument: a dead run's Job reaches a terminal Failed state (no retries) before the next scheduled run can start, so nothing concurrent can be racing the sweep.
No namespace or PVC naming under kubernetes/apps/ collides with the *-restore-test, velero/*-rtest-*, or */rt-src-* patterns, so the broadened, unscoped regex has no plausible false-positive target in this repo.

Recommendation

Looks reasonable — no blocking issues; the suggestion above is optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `e16dfd860386c6135e50aaf4f011154f60e676b9`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Single-file change to `kubernetes/infrastructure/velero/restore-test.sh`: (1) points the CephFS snapshot-probe PVCs at the new `ceph-filesystem-ephemeral` (Delete) class instead of `ceph-filesystem` (Retain), closing a real leak; (2) documents why `purge_released_pvs` and the RBD probe's `ceph-block-snapshot-readonly` class stay as-is; (3) adds an unscoped sweep of Released, restore-test-named PVs at the start of each run to catch orphans left by a run that died before its own cleanup. I checked the claims against the actual StorageClass/VolumeSnapshotClass/CronJob/RBAC manifests in the checkout and they hold up. No blocking issues. ## 🟡 Suggestions - `kubernetes/infrastructure/velero/restore-test.sh:118` vs `:378` — the scoped pattern in `cleanup_service` (`"^(${dst_ns}/.*|velero/.*-rtest-${RUN_ID}-.*|.*/rt-src-.*-${RUN_ID})$"`) and the new unscoped sweep pattern (`'^(.*-restore-test/.*|velero/.*-rtest-.*|.*/rt-src-.*)$'`) encode the same three naming conventions independently, with the `${RUN_ID}` scoping just dropped for the sweep. If the naming scheme for one of the three categories ever changes, it's easy to update one and forget the other, silently narrowing or widening the sweep. Not worth blocking on for a 3-line duplication, but a shared helper (e.g. a function returning the three sub-patterns, with the caller adding scoping) would remove the drift risk. ## Tests No automated tests exist for this script (confirmed no shellcheck/test step references it in `.woodpecker/`), consistent with the rest of the file — verification here is manual/empirical, as documented at length in the PR body (dry-ran the sweep's `jq` selector against the live cluster, confirmed it matches zero PVs today and correctly excludes the three legitimate `Retain` safety copies). I independently verified the two factual claims underpinning the fix by reading the manifests in the checkout: - `ceph-filesystem-ephemeral` (`kubernetes/infrastructure/rook-ceph-cluster/storageclass.yaml:118-134`) is identical to `ceph-filesystem` (`:95-111`) except `reclaimPolicy: Delete` vs `Retain` — same provisioner, `parameters`, and no `mountOptions` on either, so the snapshot-restore-into-ephemeral-class swap is safe. - `ceph-block-snapshot-readonly` (`kubernetes/infrastructure/velero/snapshot-readonly-storageclass.yaml`) is already `reclaimPolicy: Delete` and carries the `noload` mountOption that `ceph-block-ephemeral` (`storageclass.yaml:41-59`) lacks, confirming the RBD leg is correctly left untouched. - `concurrencyPolicy: Forbid` and `backoffLimit: 0` on the CronJob (`restore-test-cronjob.yaml:34,40`) support the "unscoped sweep is safe" argument: a dead run's Job reaches a terminal Failed state (no retries) before the next scheduled run can start, so nothing concurrent can be racing the sweep. - No namespace or PVC naming under `kubernetes/apps/` collides with the `*-restore-test`, `velero/*-rtest-*`, or `*/rt-src-*` patterns, so the broadened, unscoped regex has no plausible false-positive target in this repo. ## Recommendation Looks reasonable — no blocking issues; the suggestion above is optional.

k.renner merged commit e710b5d207 into master

2026-07-26 19:53:35 +00:00

k.renner referenced this issue from a commit

2026-07-26 19:53:36 +00:00

Merge pull request 'velero: put restore-test probe PVCs on a Delete class, sweep orphans from dead runs' (#422) from claude-bot/restore-test-ephemeral-sc into master

claude-bot referenced this pull request

2026-07-27 10:41:36 +00:00

[alert] KubeJobFailed (velero) #380

claude-bot referenced this pull request

2026-07-27 19:23:58 +00:00

[alert] KubeJobFailed (velero) #427

Sign in to join this conversation.