From 13c3274aaf2c1397eb0e1c9eebef547b892a34c7 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Wed, 7 Oct 2026 10:01:19 +0200 Subject: [PATCH] feat(clone): reap the internal snapshot a clone took on its source A clone of a source with volumes takes `clone-` on the source and restores from it, and nothing ever deleted that snapshot. Deleting the clone left it behind, and the source could then never be deleted again through the API, which refuses a definition that has snapshots. Every CSI clone goes through this path. `rd d` of a clone now reaps that snapshot, on both the REST and the CLI door. Ownership is a prop the clone path stamps on the snapshot, never its name, so an operator's snapshot called `clone-` is never touched. The snapshot is visible in `s l`, so another definition can have been restored from it, and that definition keeps reading it: new replicas are pinned to its nodes and restored from it by name. A dependent appears in two steps, the snapshot read and the definition create, so no list on the reap's side alone can see one between them. The reap marks the snapshot, then lists dependents past the cache, and only then deletes; a restore creates its definition, then reads the snapshot back past the cache and withdraws on a live mark, on a snapshot whose delete is in flight, or on one that is gone. Whichever of the list and the create comes second sees the other. A snapshot the reap keeps has the mark taken off again. The reap runs detached from its caller on a bounded budget, since the delete has already been reported, and a mark left by a process that died expires. A delete that was issued is read off the snapshot itself: the store now surfaces DELETE on a snapshot a finalizer still holds, the way it does for resources, and the snapshot list no longer stamps SUCCESSFUL beside it. A later `rd d` of the source names what is still there: a snapshot safe to delete, one kept because a named definition restores from it, and one that looks like a clone's but was never stamped. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/cli-parity-known-deltas.md | 4 +- internal/cli/definition.go | 17 +- internal/cli/rd_delete_clone_snapshot_test.go | 112 +++++ internal/cli/snapshot.go | 28 +- .../cli/snapshot_restore_withdraw_test.go | 81 ++++ internal/cli/write.go | 26 ++ pkg/api/v1/snapshot.go | 3 + pkg/rest/cache_invalidation_bug_124_test.go | 4 + pkg/rest/rd_clone.go | 16 + pkg/rest/rd_delete_clone_snapshot_test.go | 315 ++++++++++++++ pkg/rest/resource_definitions.go | 86 +++- pkg/rest/snapshot_restore.go | 45 ++ pkg/rest/snapshot_restore_withdraw_test.go | 196 +++++++++ pkg/rest/snapshots.go | 8 +- pkg/rest/snapshots_test.go | 40 ++ pkg/store/clone_snapshot.go | 405 ++++++++++++++++++ pkg/store/clone_snapshot_mark_test.go | 212 +++++++++ pkg/store/clone_snapshot_test.go | 48 +++ pkg/store/clone_snapshot_uncached_test.go | 200 +++++++++ pkg/store/inmemory_resource_definition.go | 5 + pkg/store/k8s/deleting_flag_internal_test.go | 28 ++ pkg/store/k8s/rd_list_uncached_test.go | 68 +++ pkg/store/k8s/resource_definitions.go | 38 +- pkg/store/k8s/snapshots.go | 8 + pkg/store/local_props.go | 18 +- pkg/store/store.go | 7 + 26 files changed, 1974 insertions(+), 44 deletions(-) create mode 100644 internal/cli/rd_delete_clone_snapshot_test.go create mode 100644 internal/cli/snapshot_restore_withdraw_test.go create mode 100644 pkg/rest/rd_delete_clone_snapshot_test.go create mode 100644 pkg/rest/snapshot_restore_withdraw_test.go create mode 100644 pkg/store/clone_snapshot.go create mode 100644 pkg/store/clone_snapshot_mark_test.go create mode 100644 pkg/store/clone_snapshot_test.go create mode 100644 pkg/store/clone_snapshot_uncached_test.go create mode 100644 pkg/store/k8s/rd_list_uncached_test.go diff --git a/docs/cli-parity-known-deltas.md b/docs/cli-parity-known-deltas.md index c6b55014..84fd37c2 100644 --- a/docs/cli-parity-known-deltas.md +++ b/docs/cli-parity-known-deltas.md @@ -50,12 +50,12 @@ Row IDs match the command-catalogue indexes used by `cli-parity-refresh.sh` (see | 80 | `r td --migrate-from ` (disklessOnRemaining on the vacated source) | BEHAVIOR | permanent | Upstream-issue U435 (P2, "No diskless replica recreated on source after `--migrate-from` despite disklessOnRemaining"). RG `disklessOnRemaining` IS implemented in blockstor — the placer's `placeDisklessOnRemaining` (`pkg/placer/placer.go`) fans a DISKLESS witness onto every healthy node not already hosting a replica, on the AUTOPLACE path (pinned by `pkg/placer/placer_test.go::TestPlaceDisklessOnRemaining`). The divergence is scoped strictly to the `--migrate-from` path: the `ResourceMigrationReconciler` (`internal/controller/resource_migration_controller.go`) prunes the source's DISKFUL Resource CRD once the destination is UpToDate but does NOT itself re-stamp a `disklessOnRemaining` witness on the vacated source — it relies on the auto-tiebreaker machinery instead. On a cluster where the post-migration replica set has even parity (e.g. the canonical 2-diskful result), `ensureTiebreaker` immediately RE-OCCUPIES the vacated source node with a DISKLESS `TIE_BREAKER` Resource — the same node-presence outcome `disklessOnRemaining` intends (every node retains DRBD-network presence / a quorum voter; see row 78). The witness's FLAG differs (`TIE_BREAKER` vs a plain `disklessOnRemaining` DISKLESS stub) but the operator-visible effect — the source node is left with a diskless replica, not "nothing" (the U435 symptom) — holds. Where parity is odd (no auto-TB needed), the vacated node is genuinely left bare, matching upstream's pre-fix behaviour for that case. CSI/Cozystack never sets `disklessOnRemaining` on its RGs (it relies on auto-TB for quorum), so the migrate-path gap is operator-invisible for the target workload. The auto-TB re-occupation of the migrate source is pinned by `tests/operator-harness/replay/toggle-disk-migrate-from.yaml` (`assert-src-removed` awaits `replica_diskless` on the source node) + `tests/e2e/cli-matrix/toggle-disk-migrate-from.sh`. No fix required; documented so a future `disklessOnRemaining + --migrate-from` report maps here. | | 81 | `rd l` / `rd s` (Layers column / `layer_data` on unplaced RDs) | WIRE_SHAPE | permanent | Bug 349: the python CLI's `rd l` Layers column `,`-joins `layer_data[].type`, but blockstor's k8s store persists only `Spec.LayerStack` — so the read path re-synthesises a flat `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` on EVERY RD GET / list response (`stampRDLayerDataFromStack`, pkg/rest/resource_definitions.go), falling back to the upstream default stack `DRBD,STORAGE` when the stored stack is empty, so the column never renders blank. Upstream LINSTOR 1.33.2 emits `layer_data` on RD responses only once DRBD layer data actually exists (resources placed → port/secret allocated), and then with a volatile `data` payload (`peer_slots`, `port`, `secret`, …) blockstor never synthesises; a bare `rd c` and an RD with only a volume-definition both omit the field entirely (verified live against the dev-stand oracle 2026-06-12). Operator-visible effect: BS renders the Layers column for unplaced RDs where upstream renders an empty cell — BLOCKSTOR_SUPERSET; once resources exist both sides list the same `type` sequence (the CLI renders only the types, never upstream's `data` payload). linstor-csi / piraeus-operator do not gate on RD `layer_data`. The L3 oracle-replay harness tolerates exactly this field on RD-shaped objects (scoped per-field drop in tests/contract/normalize.go keyed on `resource_group_name`; resources' `layer_object` / volumes' `layer_data_list` still diff). Pinned by `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped` (L1) + the 4 RD traces in `tests/contract/testdata/oracle/` (L3). | -| 82 | `rd clone` data plane (`use_zfs_clone` vs `zfs send\|recv`) | BEHAVIOR | permanent | Bug-020. Upstream LINSTOR clones a VD-bearing RD by internal snapshot + either `zfs clone` (when the request carries `use_zfs_clone=true`, golinstor v0.58+/linstor-csi) or `zfs send \| zfs recv` (default, fully independent copy). blockstor's clone routes through the snapshot-restore machinery: internal snapshot `clone-` + `BlockstorRestoreFromSnapshot` marker, whose ZFS provider materialises the target with `zfs clone`. Bug 038: clone/restore replicas are stamped on the snapshot-holding nodes in the SOURCE replica's storage pool (upstream restore semantics, oracle-verified), and the placer refuses to land later replicas of a restore-marked RD on a different backend — the snapshot stream formats are not interchangeable (FILE_THIN stream into `zfs recv` loops on bad magic). Upstream `rd clone` likewise operates on the source's own pools (it refuses unsupported source pools: "Clone source contains unsupported storage pools"). Consequences accepted: (a) `use_zfs_clone=true` — the linstor-csi case — gets exactly the requested semantics; (b) `use_zfs_clone=false`/absent ALSO lands on the snapshot-clone path instead of an independent full copy, so same-node clone targets stay dependent on the origin snapshot (the snapshot is visible in `linstor s l` and must outlive the clone); (c) sources on non-snapshot-capable (thick) pools refuse the clone with an actionable envelope where upstream would full-copy. Pinned by `pkg/rest/clone_use_zfs_clone_bug020_test.go` + `pkg/rest/clone_placement_bug038_test.go` + `pkg/placer/restore_source_constraints_bug038_test.go` (L1) + `tests/integration` Group J `CSICreateVolumeFromClone` (Tier 2). | +| 82 | `rd clone` data plane (`use_zfs_clone` vs `zfs send\|recv`) | BEHAVIOR | permanent | Bug-020. Upstream LINSTOR clones a VD-bearing RD by internal snapshot + either `zfs clone` (when the request carries `use_zfs_clone=true`, golinstor v0.58+/linstor-csi) or `zfs send \| zfs recv` (default, fully independent copy). blockstor's clone routes through the snapshot-restore machinery: internal snapshot `clone-` + `BlockstorRestoreFromSnapshot` marker, whose ZFS provider materialises the target with `zfs clone`. Bug 038: clone/restore replicas are stamped on the snapshot-holding nodes in the SOURCE replica's storage pool (upstream restore semantics, oracle-verified), and the placer refuses to land later replicas of a restore-marked RD on a different backend — the snapshot stream formats are not interchangeable (FILE_THIN stream into `zfs recv` loops on bad magic). Upstream `rd clone` likewise operates on the source's own pools (it refuses unsupported source pools: "Clone source contains unsupported storage pools"). Consequences accepted: (a) `use_zfs_clone=true` — the linstor-csi case — gets exactly the requested semantics; (b) `use_zfs_clone=false`/absent ALSO lands on the snapshot-clone path instead of an independent full copy, so same-node clone targets stay dependent on the origin snapshot (the snapshot is visible in `linstor s l` and must outlive the clone; `rd d ` reaps it from the source through both the REST and the CLI door, since the dependency ends with the target, and until it did the source could not be deleted at all, because `rd d` refuses a definition that has snapshots. Ownership is the `Blockstor/CloneSnapshotOf` prop the clone path stamps on the snapshot, never its name, so an operator's snapshot that happens to be called `clone-` is never reaped; a snapshot another definition was restored from is kept, asked of the API server rather than a cache, after the snapshot is marked as being reaped, and a restore reads its snapshot back once its definition exists and withdraws on that mark or on a snapshot that is gone; the reap outlives its caller, and a mark it leaves anyway expires after five minutes; and one taken by a version before the prop existed is left for the operator. A later `rd d` of the source names each kind: one safe to delete, one kept for a named restore, one that looks like a clone's but was never stamped); (c) sources on non-snapshot-capable (thick) pools refuse the clone with an actionable envelope where upstream would full-copy. Pinned by `pkg/rest/clone_use_zfs_clone_bug020_test.go` + `pkg/rest/clone_placement_bug038_test.go` + `pkg/placer/restore_source_constraints_bug038_test.go` (L1) + `tests/integration` Group J `CSICreateVolumeFromClone` (Tier 2). | | 83 | `rg spawn-resources` on an over-committed RG (place_count > available nodes) | BEHAVIOR | permanent | Corner D1. Upstream LINSTOR fails `spawn-resources` SHORT when the RG's place_count exceeds the placeable node count: it returns `FAIL_NOT_ENOUGH_NODES` (ret_code 996, "Not enough available nodes") and places NOTHING. blockstor instead takes a DEFERRED, best-effort autoplace path: it spawns the RD + VDs, places as many diskful replicas as the topology allows (e.g. 3 of 7 on a 3-node cluster), and surfaces the shortfall as an INFO in the SUCCESS envelope (`resource definition spawned, autoplace deferred: : not enough candidate storage pools: placed N of M`, exit 0). The `RGRebalanceReconciler` (`internal/controller/rg_rebalance_controller.go`) then tops the replica count back up additively once more nodes appear. The over-commit `rg create` itself is ACCEPTED by both controllers (parity — never an early create-time error). Rationale: the deferred path lets the CSI external-provisioner retry loop converge on a created RD instead of hard-failing on a cluster that is one node away from satisfying the request, and is strictly additive (scale-down stays an explicit `r d`). Switching spawn back to the upstream 996 short-fail would be a deliberate API change, not a silent regression. Pinned by `pkg/rest/spawn_test.go::TestSpawnImpossiblePlacementReturnsActionableError` + `TestSpawnPartialFlagAllowsShortPlacement` (L1) and `tests/e2e/cli-matrix/rg-c-overcommit-spawn-defers.sh` (L6). | | 84 | `r c ` (tiebreaker→diskful promotion runs full SyncTarget, not skip-sync) | BEHAVIOR | permanent | Promoting a tiebreaker (diskless witness) to diskful runs a FULL SyncTarget on the promoted node instead of the upstream skip-sync. DELIBERATE: skip-sync would require BS to pre-stamp a matching day0 GI and let the fresh replica win the auto-primary election — but a diskful peer already holds data (`anyDiskfulPeerHasData == true`, `pkg/dispatcher/dispatcher.go`), so force-priming the fresh replica mints an UNRELATED Current UUID; the data-bearing peer then declines the handshake (`uuid_compare()=unrelated-data`, "Unrelated data, aborting!") and the pair wedges in mutual StandAlone that never auto-recovers (the respawn-StandAlone P0). BS therefore gates the auto-primary seed on `!anyDiskfulPeerHasData(peers)` and `!rdInitialized(rd)`: with a data-bearing peer present the promoted replica comes up Inconsistent and SyncTargets the real bytes off the peer. Data converges correctly; the only cost is a full resync of the volume — the safe trade vs. a StandAlone wedge. Upstream's skip-sync-on-promotion is not reproducible without reintroducing the wedge. Pinned by `tests/e2e/cli-matrix/r-c-over-tiebreaker-skip-sync.sh` (L6, asserts SyncTarget→UpToDate convergence + Bug 348 SyncSource shape) + the dispatcher auto-primary gate tests (`pkg/dispatcher/dispatcher_test.go`, respawn-StandAlone wedge regression). | | 85 | `r l` State column — bare `SyncSource`/`SyncTarget` literal (no `(NN%)` suffix) when OutOfSyncKib<=0 | WIRE_SHAPE | permanent | A replica actively resyncing renders `SyncTarget(NN%)` / `SyncSource(NN%)` WHILE there is data to copy. When the per-volume `OutOfSyncKib` is <= 0 (or the VD size is unknown), BS DELIBERATELY renders the BARE literal `SyncSource` / `SyncTarget` with NO `(NN%)` suffix: `withSyncPercent` (pkg/rest/resources.go, called from `annotateSyncProgress`) short-circuits on `outOfSyncKib<=0` rather than emit a misleading `(0%)`/`(100%)`. The drbd replication-state literal can be observed for a brief window before/after the peer-device OutOfSyncKib counter carries a non-zero value, so a bare Sync* token is a real and intended shape. The REGRESSION guarded against is the opposite — a TERMINAL `UpToDate` must never carry a `(NN%)` suffix (Bug A). So the contract is: a Sync* token may render bare OR with `(NN%)`, but a terminal disk_state never carries a percent. Pinned by `tests/e2e/cli-matrix/r-l-conns-shapes.sh` sub-test D (L6) + `annotateSyncProgress`/`withSyncPercent` unit coverage (pkg/rest, Bug 348 + Bug A/B). | | 86 | `rd clone --external-name` / `--volume-passphrase` / a `--layer-list` that changes the source's layer set | MISSING_FEATURE | permanent | `external_name` gives the clone an identity of its own upstream and `volume_passphrases` carries the LUKS keys for its volumes; blockstor names a clone by `name` and takes its keys from the cluster passphrase, so honouring neither is possible and both are refused with an explicit 501 rather than accepted and dropped. Every refusal on this endpoint, the 501 included, goes out in the shape the caller decodes: the CloneStarted object for python-linstor, which names itself `PythonLinstor/…` and reads the clone answer as that object whatever the status, and an ApiCallRc array for every other client, since golinstor decodes a non-2xx body as nothing else (linstor-csi's 404s reach it bare either way) — a clone that reports success under a different name, or with keys the caller does not hold, is the failure mode `src_snap_name` is already refused for. `layer_list` IS honoured, with one refusal: on a source that has volumes the requested stack must be the source's, compared as a set (order is the stack's own and case folds). The clone data plane restores the source's bytes and brings the layer stack up over them, in that order, and every layer's bring-up writes to the device — LUKS formats a device carrying no header, DRBD stamps `meta-disk internal` metadata with `create-md --force` — so a layer added here lands across the bytes just restored and the clone reports COMPLETE over them, while a layer dropped leaves the target reading data the missing layer wrote. A source with no recorded stack means the upstream default `[DRBD, STORAGE]`, which is what linstor-csi sends, so the CSI clone path is unaffected. Upstream clones the layer list with the volume, re-encrypting LUKS under the cluster key. A volume-less source has no bytes to lose and keeps taking any stack. A clone resumed over a leftover also keeps that leftover's shape: a retry naming a different `resource_group` or stack is refused rather than answered 201 with the request's shape dropped, and a leftover internal snapshot that has fallen behind the source (a volume added, or resized) is refused rather than cloned from. Pinned by `pkg/rest/rd_clone_golinstor_shape_test.go` + `pkg/rest/rd_clone_client_envelope_test.go` + `pkg/rest/rd_clone_layer_stack_test.go` + `pkg/rest/rd_clone_resume_test.go` + `pkg/rest/rd_clone_leftover_shape_test.go` + `pkg/rest/rd_clone_server_owned_props_test.go` + `pkg/rest/rd_modify_server_owned_props_test.go` + `internal/cli/snapshot_restore_prepared_test.go` (L1) and `tests/e2e/cli-matrix/rd-clone-retry-semantics.sh` (L6, stand-only: `make e2e-list` covers `tests/e2e/*.sh` and no CI lane runs the cli-matrix cells) + `tests/operator-harness/replay/rd-clone-retry-semantics.yaml` (L7). `rd modify` and `rd set-property` / `delete-property` refuse the same server-owned props, with 400 and an error respectively, since a `delete_namespaces` of `Blockstor` mid-restore took the adoption mark with it, and `rd create` refuses a definition carrying one with 400. Prop edits on a clone (`override_props`, `delete_props`, `delete_namespaces`) that would rewrite or remove a prop blockstor sets on the clone itself, the restore marker, the record of the snapshot's volumes, the rollback mark or the adoption mark, are refused with 400 before anything is written, rather than applied after the marker and leaving the satellite to bring the volumes up blank; linstor-csi's own `delete_props` edit touches none of them. | -| 87 | `rd clone` / `s resource restore` retry over an unfinished target | BEHAVIOR | permanent | This is the data path: a clone of a source with volumes, and every restore. A clone of a volume-less source is a plain copy with no marker, so a repeat of it stays the already-exists refusal upstream gives. Both endpoints stamp `BlockstorRestoreFromSnapshot` with the target definition and hydrate its volumes and place its replicas afterwards, so a target carrying the marker says the operation STARTED, not that it finished. A repeat under the same name RESUMES it — every step tolerates an object a previous attempt already created — and says so (`… completed on retry`); upstream has no equivalent notion, a repeat there is a plain already-exists failure. This is what makes CSI's CreateVolume idempotent: external-provisioner has no other way to make progress after a partial failure, and answering success on the marker alone would leave CSI seeing a ready volume nothing ever finishes. A definition under that name that carries no marker is taken when it is in the state LINSTOR's own sequence leaves (`rd c`, `s vd restore`, then `s rsc restore`): its volumes exactly the snapshot's and no live replica. linstor-csi restores every volume that way, creating the definition itself first, and the marker is stamped on it at that point. What stays a refusal, each before anything is written to the definition: any other definition under that name this operation did not create and that is not prepared, meaning its volumes are not the snapshot's or it holds a live replica (409); a prepared definition whose layer stack is not the snapshot source's, compared as a set, since the restore would put the source's bytes under layers it never had or without layers it had (409); a prepared definition, or a leftover still to be finished or one the request places replicas into, whose every replica is still being deleted, which placing over would race (409; a bare REST restore's replay over a finished leftover still answers 201 while its snapshot exists, since it places none); a prepared definition or a leftover parented to a resource group that no longer exists (409, or 500 asking for a retry when the group cannot be read); this operation's own leftover carrying the DELETE flag, since finishing it races the tear-down reaping what it writes (409); a leftover whose earlier attempt's rollback gave up, which may hold less than the operation intended, with the way out named (409); a leftover holding a volume the snapshot never recorded beside one it did (409); and a leftover whose creating attempt has started rolling it back (409). Placing a replica, after the definition and its volumes are written, refuses an operator's diskless replica already on a node the request places onto, which holds no copy of the data and is not counted as placed: a 409 on a retry over this operation's own leftover, and on a fresh restore a failed placement, which rolls the new definition back and answers 500, neither with a FAIL_EXISTS band. The controller's tie-breaker witness there is promoted to the replica the request asked for instead, as autoplace promotes one, and its volumes are restored from the snapshot like any other replica of the target; a witness no storage pool can be found for is left as it is and refused the same way, naming `linstor resource create --storage-pool ` to give it one. Once the marker is on a prepared definition it stays: a concurrent retry that finds it resumes and places the replica, and the request that wrote it meets that replica as one already placed, never as a reason to take the marker back off. The window this leaves is a replica placed by hand between the judgement and the marker, which is then judged as part of the restore; on a target prepared for exactly this restore nothing else places one in that moment. A replica the operation would place that is still being deleted on its node fails the request rather than counting as placed (a 409 over a leftover this request adopted, and a 500 with the rollback's answer over one it created), with no FAIL_EXISTS_RSC band on either. Two restores of different snapshots into one prepared definition do not both take it: the second finds the first's marker and is refused. The CLI door settles the parent group, and the nodes and pools the replicas go to, before it writes the definition or the marker, so a request refused for its own shape, or over a prepared definition whose group is gone, leaves that definition unmarked; and a CLI placement that fails after the marker is finished by running the same command again, unless a replica already landed: the re-run then judges it finished, so the failure names the replicas that did not land and the `blockstor resource create` that places them. The CLI judges its own leftover with the same assessment and gate order the REST door uses (a tear-down, then a volume the restore would not have written, then a parent group that is gone, then a rollback that gave up): a leftover with the snapshot's volumes and one replica with a disk is finished and left alone rather than re-placed on a node the operator emptied, and only one with no such replica gets its placement finished, under the same adoption mark the REST door takes, so a rollback by the attempt that created it yields. A definition already under the name is judged before anything about the source is planned, so a finished restore is left alone even once the source can no longer be planned from. Replicas are counted past the cache on both doors, and a rollback still in progress is refused as one still running rather than as one that gave up. A clone resume whose internal snapshot is gone is refused once the leftover holds a volume with a replica that has a disk, since finishing it from a new snapshot would join two moments of the source; a leftover with no volume or no replica is finished from the new snapshot like a fresh clone, unless its volumes do not fit the source as it is now (one smaller than the source's, or one the source no longer has), which is refused with `delete and clone again` before any snapshot is taken. The volumes and replicas of a prepared definition, and a colliding replica, are read from the API server, not a cache, and so is the definition the request judges right after stamping its marker: linstor-csi's volume restore and the resource restore after it can be served by different replicas of the server. A finished clone or restore is judged by its volumes and, where the operation placed replicas, by holding one with a disk, never by where they are now: a replay after a node was evacuated or the target scaled down answers success and leaves placement alone. A finished clone is a copy of a point-in-time and its replay answers the same way whatever happens to the source afterwards — including a `vd set-size`, which a guard comparing the internal snapshot to the live source turned into a permanent refusal of every retry. Pinned by `pkg/rest/rd_clone_idempotency_test.go` + `pkg/rest/rd_clone_replay_test.go` + `pkg/rest/rd_clone_leftover_shape_test.go` + `pkg/rest/snapshot_restore_idempotency_test.go` + `pkg/rest/snapshot_restore_replay_test.go` + `pkg/rest/snapshot_restore_csi_flow_test.go` + `pkg/rest/snapshot_restore_prepared_target_test.go` + `pkg/rest/snapshot_restore_prepared_cache_test.go` + `pkg/rest/snapshot_restore_resume_gates_test.go` + `pkg/rest/leftover_gates_test.go` + `pkg/rest/rollback_handshake_failclosed_test.go` + `pkg/store/restore_target_test.go` + `internal/cli/snapshot_restore_prepared_test.go` (L1) and `tests/e2e/cli-matrix/rd-clone-retry-semantics.sh` (L6, stand-only: `make e2e-list` covers `tests/e2e/*.sh` and no CI lane runs the cli-matrix cells) + `tests/operator-harness/replay/rd-clone-retry-semantics.yaml` (L7). Every marker is written with `Blockstor/RestoreVolumes`, the volumes the snapshot held, so a leftover whose snapshot is gone (an operator deleted it) is still judged against them: a clone that restored only some of its volumes is unfinished, not complete. A leftover from before the record existed carries none and is judged on its own volumes, as before. A CLI rollback that could not delete takes its in-progress mark back off so the next run finishes the definition, and a CLI re-run over a finished restore says so on stdout. The CLI's rollback of a definition it created holds the same handshake as REST before deleting: a definition another request adopted is left to it, and one whose mark could not be written or whose adoption could not be read is not deleted. A rollback after a failed clone or restore also leaves a definition that already reads as finished, holding every volume and, where the operation places replicas, one replica with a disk: the clone status poll answers such a definition complete without writing anything, which can happen while the attempt that created it is still stamping its last replica, so deleting it afterwards took a volume the driver had been told exists. A replay takes the adoption mark before its 201, so the rollback yields to it like to any adopter, whatever the request placed. The rollback over a group deleted under the operation is the exception: it removes a finished definition too, since one parented to a group that is gone is what it exists to remove, so a status poll that answered complete a moment before it starts is followed by the definition going away; a POST replay's mark makes it yield. That attempt fails, and its retry answers for what it left; a restore that asked for replicas on named nodes names the ones it never placed, with the `linstor resource create` that places them, since the retry judges the definition finished and places nothing. A clone's prop edits (`override_props`, `delete_props`, `delete_namespaces`) land with the definition when it is created, so no leftover carries a prop the caller asked to remove: linstor-csi removes `Aux/csi-provisioning-completed-by` on every clone and takes a definition carrying it, with volumes of the right size, as one already provisioned. The clone status poll asks a finished target what the POST's replay asks before its 201: one carrying the DELETE flag, parented to a group that is gone, or carrying a rollback in progress or one that gave up is answered 404, which sends the driver back to the POST that refuses it, and a group that cannot be read is a 500. A replay of a finished clone is answered before the source is read, so it survives the source being deleted or losing its volumes. A retry after the snapshot was deleted, the cleanup that follows a finished restore, is judged against the volumes the definition recorded when it was restored: a finished one answers 201 (the CLI exits 0) without reading the snapshot or the source, an unfinished one is refused with `delete and restore it from another snapshot`, and a target restored before the record existed keeps the 404 it got. A restored or cloned definition is stamped with the source's data-plane layer stack (its own stored stack, or DRBD over STORAGE when it has none): the dispatcher reads only the definition's stack, so a target group with another stack never changed how the bytes are brought up, but the control plane reads the group's stack for a definition stored without one, and the stamp keeps the two in step. A REST replay of a finished restore checks its node list only while the snapshot exists; once it is gone the replay is judged by the target alone. A bare restore counts as finished without a replica only while its snapshot exists: once the snapshot is gone, a target with no replica holds its data nowhere and is refused with `delete and restore it from another snapshot`, as the CLI refuses it. A clone that names no layer stack takes the source's and needs the cluster passphrase when that stack carries LUKS, as one that names it does; the check runs after the replay of a finished clone, which writes no new LUKS volume. A target already under the name is judged, hydrated and placed under the name it is stored with, whatever case the request spells it in: its replicas are selected by that name exactly, and judged under another spelling a finished clone or restore read as having none; a read of that name that fails is the 500 below. A target that cannot be read is answered 500 asking for a retry, not taken as absent: the create that followed collided and was adopted past every judgement of a leftover. The CLI refuses a snapshot that recorded no volumes before it writes or resumes anything, as the REST door does. A rollback that leaves a definition whole and cannot take its in-progress mark back off says so and names `linstor resource-definition set-property BlockstorRollbackAbandoned` with no value (python-linstor's delete; the blockstor CLI also has `delete-property`), which `rd modify` (`delete_props`) and the CLI accept as the one edit of a server-owned prop an operator may make, and only while the mark sits on a definition the rollback left whole (`in-progress`, or `snapshots` / `read-snapshots`, where it stopped before touching anything): one naming a later step sits on a definition the rollback took partly apart, and stays; setting the mark stays refused, and an empty `override_props` value of it is read as the delete. The refusal over an in-progress mark names the clear only once the rollback has had its time; a clear while it still runs stops that rollback before it deletes anything, since the rollback checks its own mark is still there before its first delete. A CLI rollback that leaves the target in place names the `blockstor snapshot resource restore` that finishes it, including for `rd clone`, whose own re-run is refused over a target that exists. | +| 87 | `rd clone` / `s resource restore` retry over an unfinished target | BEHAVIOR | permanent | This is the data path: a clone of a source with volumes, and every restore. A clone of a volume-less source is a plain copy with no marker, so a repeat of it stays the already-exists refusal upstream gives. Both endpoints stamp `BlockstorRestoreFromSnapshot` with the target definition and hydrate its volumes and place its replicas afterwards, so a target carrying the marker says the operation STARTED, not that it finished. A repeat under the same name RESUMES it — every step tolerates an object a previous attempt already created — and says so (`… completed on retry`); upstream has no equivalent notion, a repeat there is a plain already-exists failure. This is what makes CSI's CreateVolume idempotent: external-provisioner has no other way to make progress after a partial failure, and answering success on the marker alone would leave CSI seeing a ready volume nothing ever finishes. A definition under that name that carries no marker is taken when it is in the state LINSTOR's own sequence leaves (`rd c`, `s vd restore`, then `s rsc restore`): its volumes exactly the snapshot's and no live replica. linstor-csi restores every volume that way, creating the definition itself first, and the marker is stamped on it at that point. What stays a refusal, each before anything is written to the definition: any other definition under that name this operation did not create and that is not prepared, meaning its volumes are not the snapshot's or it holds a live replica (409); a prepared definition whose layer stack is not the snapshot source's, compared as a set, since the restore would put the source's bytes under layers it never had or without layers it had (409); a prepared definition, or a leftover still to be finished or one the request places replicas into, whose every replica is still being deleted, which placing over would race (409; a bare REST restore's replay over a finished leftover still answers 201 while its snapshot exists, since it places none); a prepared definition or a leftover parented to a resource group that no longer exists (409, or 500 asking for a retry when the group cannot be read); this operation's own leftover carrying the DELETE flag, since finishing it races the tear-down reaping what it writes (409); a leftover whose earlier attempt's rollback gave up, which may hold less than the operation intended, with the way out named (409); a leftover holding a volume the snapshot never recorded beside one it did (409); and a leftover whose creating attempt has started rolling it back (409). Placing a replica, after the definition and its volumes are written, refuses an operator's diskless replica already on a node the request places onto, which holds no copy of the data and is not counted as placed: a 409 on a retry over this operation's own leftover, and on a fresh restore a failed placement, which rolls the new definition back and answers 500, neither with a FAIL_EXISTS band. The controller's tie-breaker witness there is promoted to the replica the request asked for instead, as autoplace promotes one, and its volumes are restored from the snapshot like any other replica of the target; a witness no storage pool can be found for is left as it is and refused the same way, naming `linstor resource create --storage-pool ` to give it one. Once the marker is on a prepared definition it stays: a concurrent retry that finds it resumes and places the replica, and the request that wrote it meets that replica as one already placed, never as a reason to take the marker back off. The window this leaves is a replica placed by hand between the judgement and the marker, which is then judged as part of the restore; on a target prepared for exactly this restore nothing else places one in that moment. A replica the operation would place that is still being deleted on its node fails the request rather than counting as placed (a 409 over a leftover this request adopted, and a 500 with the rollback's answer over one it created), with no FAIL_EXISTS_RSC band on either. Two restores of different snapshots into one prepared definition do not both take it: the second finds the first's marker and is refused. The CLI door settles the parent group, and the nodes and pools the replicas go to, before it writes the definition or the marker, so a request refused for its own shape, or over a prepared definition whose group is gone, leaves that definition unmarked; and a CLI placement that fails after the marker is finished by running the same command again, unless a replica already landed: the re-run then judges it finished, so the failure names the replicas that did not land and the `blockstor resource create` that places them. The CLI judges its own leftover with the same assessment and gate order the REST door uses (a tear-down, then a volume the restore would not have written, then a parent group that is gone, then a rollback that gave up): a leftover with the snapshot's volumes and one replica with a disk is finished and left alone rather than re-placed on a node the operator emptied, and only one with no such replica gets its placement finished, under the same adoption mark the REST door takes, so a rollback by the attempt that created it yields. A definition already under the name is judged before anything about the source is planned, so a finished restore is left alone even once the source can no longer be planned from. Replicas are counted past the cache on both doors, and a rollback still in progress is refused as one still running rather than as one that gave up. A clone resume whose internal snapshot is gone is refused once the leftover holds a volume with a replica that has a disk, since finishing it from a new snapshot would join two moments of the source; a leftover with no volume or no replica is finished from the new snapshot like a fresh clone, unless its volumes do not fit the source as it is now (one smaller than the source's, or one the source no longer has), which is refused with `delete and clone again` before any snapshot is taken. The volumes and replicas of a prepared definition, and a colliding replica, are read from the API server, not a cache, and so is the definition the request judges right after stamping its marker: linstor-csi's volume restore and the resource restore after it can be served by different replicas of the server. A finished clone or restore is judged by its volumes and, where the operation placed replicas, by holding one with a disk, never by where they are now: a replay after a node was evacuated or the target scaled down answers success and leaves placement alone. A finished clone is a copy of a point-in-time and its replay answers the same way whatever happens to the source afterwards — including a `vd set-size`, which a guard comparing the internal snapshot to the live source turned into a permanent refusal of every retry. Pinned by `pkg/rest/rd_clone_idempotency_test.go` + `pkg/rest/rd_clone_replay_test.go` + `pkg/rest/rd_clone_leftover_shape_test.go` + `pkg/rest/snapshot_restore_idempotency_test.go` + `pkg/rest/snapshot_restore_replay_test.go` + `pkg/rest/snapshot_restore_csi_flow_test.go` + `pkg/rest/snapshot_restore_prepared_target_test.go` + `pkg/rest/snapshot_restore_prepared_cache_test.go` + `pkg/rest/snapshot_restore_resume_gates_test.go` + `pkg/rest/leftover_gates_test.go` + `pkg/rest/rollback_handshake_failclosed_test.go` + `pkg/store/restore_target_test.go` + `internal/cli/snapshot_restore_prepared_test.go` (L1) and `tests/e2e/cli-matrix/rd-clone-retry-semantics.sh` (L6, stand-only: `make e2e-list` covers `tests/e2e/*.sh` and no CI lane runs the cli-matrix cells) + `tests/operator-harness/replay/rd-clone-retry-semantics.yaml` (L7). Every marker is written with `Blockstor/RestoreVolumes`, the volumes the snapshot held, so a leftover whose snapshot is gone (an operator deleted it, or it was a clone's internal one and was reaped) is still judged against them: a clone that restored only some of its volumes is unfinished, not complete. A leftover from before the record existed carries none and is judged on its own volumes, as before. A CLI rollback that could not delete takes its in-progress mark back off so the next run finishes the definition, and a CLI re-run over a finished restore says so on stdout. The CLI's rollback of a definition it created holds the same handshake as REST before deleting: a definition another request adopted is left to it, and one whose mark could not be written or whose adoption could not be read is not deleted. A rollback after a failed clone or restore also leaves a definition that already reads as finished, holding every volume and, where the operation places replicas, one replica with a disk: the clone status poll answers such a definition complete without writing anything, which can happen while the attempt that created it is still stamping its last replica, so deleting it afterwards took a volume the driver had been told exists. A replay takes the adoption mark before its 201, so the rollback yields to it like to any adopter, whatever the request placed. The rollback over a group deleted under the operation is the exception: it removes a finished definition too, since one parented to a group that is gone is what it exists to remove, so a status poll that answered complete a moment before it starts is followed by the definition going away; a POST replay's mark makes it yield. That attempt fails, and its retry answers for what it left; a restore that asked for replicas on named nodes names the ones it never placed, with the `linstor resource create` that places them, since the retry judges the definition finished and places nothing. A clone's prop edits (`override_props`, `delete_props`, `delete_namespaces`) land with the definition when it is created, so no leftover carries a prop the caller asked to remove: linstor-csi removes `Aux/csi-provisioning-completed-by` on every clone and takes a definition carrying it, with volumes of the right size, as one already provisioned. The clone status poll asks a finished target what the POST's replay asks before its 201: one carrying the DELETE flag, parented to a group that is gone, or carrying a rollback in progress or one that gave up is answered 404, which sends the driver back to the POST that refuses it, and a group that cannot be read is a 500. A replay of a finished clone is answered before the source is read, so it survives the source being deleted or losing its volumes. A retry after the snapshot was deleted, the cleanup that follows a finished restore, is judged against the volumes the definition recorded when it was restored: a finished one answers 201 (the CLI exits 0) without reading the snapshot or the source, an unfinished one is refused with `delete and restore it from another snapshot`, and a target restored before the record existed keeps the 404 it got. A restored or cloned definition is stamped with the source's data-plane layer stack (its own stored stack, or DRBD over STORAGE when it has none): the dispatcher reads only the definition's stack, so a target group with another stack never changed how the bytes are brought up, but the control plane reads the group's stack for a definition stored without one, and the stamp keeps the two in step. A REST replay of a finished restore checks its node list only while the snapshot exists; once it is gone the replay is judged by the target alone. A bare restore counts as finished without a replica only while its snapshot exists: once the snapshot is gone, a target with no replica holds its data nowhere and is refused with `delete and restore it from another snapshot`, as the CLI refuses it. A clone that names no layer stack takes the source's and needs the cluster passphrase when that stack carries LUKS, as one that names it does; the check runs after the replay of a finished clone, which writes no new LUKS volume. A target already under the name is judged, hydrated and placed under the name it is stored with, whatever case the request spells it in: its replicas are selected by that name exactly, and judged under another spelling a finished clone or restore read as having none; a read of that name that fails is the 500 below. A target that cannot be read is answered 500 asking for a retry, not taken as absent: the create that followed collided and was adopted past every judgement of a leftover. The CLI refuses a snapshot that recorded no volumes before it writes or resumes anything, as the REST door does. A rollback that leaves a definition whole and cannot take its in-progress mark back off says so and names `linstor resource-definition set-property BlockstorRollbackAbandoned` with no value (python-linstor's delete; the blockstor CLI also has `delete-property`), which `rd modify` (`delete_props`) and the CLI accept as the one edit of a server-owned prop an operator may make, and only while the mark sits on a definition the rollback left whole (`in-progress`, or `snapshots` / `read-snapshots`, where it stopped before touching anything): one naming a later step sits on a definition the rollback took partly apart, and stays; setting the mark stays refused, and an empty `override_props` value of it is read as the delete. The refusal over an in-progress mark names the clear only once the rollback has had its time; a clear while it still runs stops that rollback before it deletes anything, since the rollback checks its own mark is still there before its first delete. A CLI rollback that leaves the target in place names the `blockstor snapshot resource restore` that finishes it, including for `rd clone`, whose own re-run is refused over a target that exists. | | 88 | `rg vg m --gross` / `rg vg m` with `flags` | MISSING_FEATURE | permanent | Upstream's one volume-group flag is `GROSS_SIZE`, which sizes a spawned volume gross. Nothing in blockstor sizes a spawned volume gross, so the volume-group modify door answers 501 to any `flags` in the body and writes nothing, the props half included, rather than a 200 for a request that changes nothing. | | 89 | `delete_namespaces` on every props-modify door | BEHAVIOR | permanent | Not a divergence; recorded so it is not reopened. As upstream's `PropsContainer.removeNamespace` does, a namespace removes the keys below it and leaves a key spelled exactly as the namespace: `DrbdOptions` takes `DrbdOptions/Net/protocol` and keeps `DrbdOptions`. Trailing slashes name the same namespace. Every door and every guard in front of one read it through the same helper. | diff --git a/internal/cli/definition.go b/internal/cli/definition.go index 837d6e6b..0f4b64c0 100644 --- a/internal/cli/definition.go +++ b/internal/cli/definition.go @@ -22,6 +22,7 @@ import ( "context" "errors" "fmt" + "maps" "slices" "sort" "strings" @@ -29,6 +30,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" "github.com/cozystack/blockstor/internal/cli/command" "github.com/cozystack/blockstor/internal/cli/view" @@ -156,7 +158,7 @@ func resourceDefinitionClone(ctx context.Context, run *runContext) error { snapName := cloneSnapshotName(target) - err = ensureCloneSnapshot(ctx, run, &src, snapName) + err = ensureCloneSnapshot(ctx, run, &src, snapName, target) if err != nil { return err } @@ -234,7 +236,9 @@ func checkCloneSnapshotIsCurrent( return nil } -func ensureCloneSnapshot(ctx context.Context, run *runContext, src *apiv1.ResourceDefinition, snapName string) error { +func ensureCloneSnapshot( + ctx context.Context, run *runContext, src *apiv1.ResourceDefinition, snapName, target string, +) error { existing, err := run.Store.Snapshots().Get(ctx, src.Name, snapName) if err == nil { return checkCloneSnapshotIsCurrent(ctx, run, src, &existing) @@ -255,6 +259,15 @@ func ensureCloneSnapshot(ctx context.Context, run *runContext, src *apiv1.Resour return err } + // Stamped as the clone's own, the way the REST door stamps it, so that + // deleting the clone may reap it; see store.CloneSnapshotOwnerProp. + snap.Props = maps.Clone(snap.Props) + if snap.Props == nil { + snap.Props = map[string]string{} + } + + snap.Props[store.CloneSnapshotOwnerProp] = target + err = run.Store.Snapshots().Create(ctx, snap) if err != nil { return fmt.Errorf("create clone snapshot %s: %w", snapName, err) diff --git a/internal/cli/rd_delete_clone_snapshot_test.go b/internal/cli/rd_delete_clone_snapshot_test.go new file mode 100644 index 00000000..529f805e --- /dev/null +++ b/internal/cli/rd_delete_clone_snapshot_test.go @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: Apache-2.0 + +package cli_test + +import ( + "context" + "errors" + "strings" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// `rd clone` takes the same internal snapshot the REST door takes, and `rd d` +// of the clone reaped nothing, so the source could never be deleted through +// the CLI either. +func TestResourceDefinitionDeleteOfACloneReapsItsSnapshot(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, seedSnapshotSource) + + if got := app.Run(t.Context(), []string{"rd", "clone", "pvc-x", "pvc-reap"}); got != 0 { + t.Fatalf("clone exit = %d (stderr: %s)", got, errBuf.String()) + } + + // The owner prop is the snapshot's. Carried onto the clone, it would be + // copied into every snapshot later taken of the clone as a claim of + // ownership nobody made. + clone, err := appStore(t, app).ResourceDefinitions().Get(t.Context(), "pvc-reap") + if err != nil { + t.Fatalf("get the clone: %v", err) + } + + if owner, ok := clone.Props[store.CloneSnapshotOwnerProp]; ok { + t.Errorf("the clone carries the snapshot's owner prop %q", owner) + } + + if got := app.Run(t.Context(), []string{"rd", "d", "pvc-reap"}); got != 0 { + t.Fatalf("delete of the clone exit = %d (stderr: %s)", got, errBuf.String()) + } + + backend := appStore(t, app) + + if _, err := backend.Snapshots().Get(t.Context(), "pvc-x", "clone-pvc-reap"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the internal snapshot outlived the clone it backed: %v", err) + } + + // seedSnapshotSource also leaves an operator snapshot on pvc-x, which the + // delete has to keep refusing over. The clone's own must not be among what + // it names. + app.Run(t.Context(), []string{"rd", "d", "pvc-x"}) + + if strings.Contains(errBuf.String(), "clone-pvc-reap") { + t.Errorf("the source delete still meets the clone's snapshot: %s", errBuf.String()) + } +} + +// An operator can name a snapshot anything, `clone-` included, and +// restore it under that target. Only the prop the clone path writes makes a +// snapshot the clone's to reap. +func TestResourceDefinitionDeleteKeepsAnOperatorSnapshotNamedLikeAClone(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) { + seedSnapshotSource(ctx, backend) + _ = backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "clone-pvc-op", ResourceName: "pvc-x", + Nodes: []string{"node-1", "node-2"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 1 << 20}}, + }) + }) + + argv := []string{ + "s", "resource", "restore", + "--from-resource", "pvc-x", "--from-snapshot", "clone-pvc-op", "--to-resource", "pvc-op", + } + + if got := app.Run(t.Context(), argv); got != 0 { + t.Fatalf("restore exit = %d (stderr: %s)", got, errBuf.String()) + } + + if got := app.Run(t.Context(), []string{"rd", "d", "pvc-op"}); got != 0 { + t.Fatalf("delete exit = %d (stderr: %s)", got, errBuf.String()) + } + + if _, err := appStore(t, app).Snapshots().Get(t.Context(), "pvc-x", "clone-pvc-op"); err != nil { + t.Errorf("the operator's snapshot was reaped because of its name: %v", err) + } +} + +// The CLI refusal over a source still carrying a snapshot its clone left had no +// case that reached it: every fixture's list came back empty. +func TestResourceDefinitionDeleteNamesACloneSnapshotLeftBehind(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) { + _ = backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-left"}) + _ = backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "clone-pvc-gone", ResourceName: "pvc-left", + Props: map[string]string{store.CloneSnapshotOwnerProp: "pvc-gone"}, + }) + }) + + if got := app.Run(t.Context(), []string{"rd", "d", "pvc-left"}); got == 0 { + t.Fatal("delete of a source still carrying a snapshot succeeded") + } + + if !strings.Contains(errBuf.String(), "clone-pvc-gone") { + t.Errorf("refusal %q does not name the snapshot the clone left", errBuf.String()) + } +} diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 59135b9f..ef87610c 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -347,6 +347,13 @@ func snapshotRestoreResource(ctx context.Context, run *runContext) error { return fmt.Errorf("create resource definition %s: %w", def.Name, err) } + // The snapshot was read before the definition existed; see + // store.ReapClonedSnapshot. + err = store.RestoreSourceWithdrawn(ctx, run.Store, snap.ResourceName, snap.Name) + if err != nil { + return rollbackRestore(ctx, run, def.Name, planned, err) + } + // The definition is this command's own, but a concurrent run may already // have adopted it and be hydrating the same volumes. A volume that is // already there at the snapshot's size is the restore's, not a collision @@ -387,6 +394,10 @@ func newRestoredDefinition(rdName string, src *apiv1.ResourceDefinition, snap *a // the placer looks the source half up as a store key. def.Props = store.WithRestoreMarker(def.Props, snap) + // The owner prop belongs to the snapshot; see store.CloneSnapshotOwnerProp. + delete(def.Props, store.CloneSnapshotOwnerProp) + delete(def.Props, store.CloneSnapshotReapingProp) + return def } @@ -469,6 +480,21 @@ func restoreIntoExistingTarget( return finishConcurrentRestore(ctx, run, args, existing.Name, snap, createErr) } + return placeOverMarkedTarget(ctx, run, existing.Name, snap, planned) +} + +// placeOverMarkedTarget places the planned replicas on a target that carries +// this restore's marker. The snapshot was read before the marker went on, or +// before this run found it there, so it is read back first; see +// store.ReapClonedSnapshot. +func placeOverMarkedTarget( + ctx context.Context, run *runContext, rdName string, snap *apiv1.Snapshot, planned []apiv1.Resource, +) error { + err := store.RestoreSourceWithdrawn(ctx, run.Store, snap.ResourceName, snap.Name) + if err != nil { + return fmt.Errorf("restore into %s: %w", rdName, err) + } + return placeOverExisting(ctx, run, planned) } @@ -630,7 +656,7 @@ func finishRestoreLeftover( return err } - return placeOverExisting(ctx, run, planned) + return placeOverMarkedTarget(ctx, run, rdName, snap, planned) } // answerFinishedLeftover reports a finished leftover of this restore as diff --git a/internal/cli/snapshot_restore_withdraw_test.go b/internal/cli/snapshot_restore_withdraw_test.go new file mode 100644 index 00000000..d48aa5c1 --- /dev/null +++ b/internal/cli/snapshot_restore_withdraw_test.go @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: Apache-2.0 + +package cli_test + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// The CLI restore reads the snapshot back after it creates the definition, +// the same way the REST door does, and withdraws from one a reap has marked. +func TestSnapshotRestoreWithdrawsFromASnapshotBeingReaped(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) { + seedSnapshotSource(ctx, backend) + _ = backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "clone-pvc-old", ResourceName: "pvc-x", + Nodes: []string{"node-1", "node-2"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 1 << 20}}, + Props: map[string]string{ + store.CloneSnapshotOwnerProp: "pvc-old", + store.CloneSnapshotReapingProp: "pvc-old@" + time.Now().UTC().Format(time.RFC3339), + }, + }) + }) + + if got := app.Run(t.Context(), []string{ + "s", "resource", "restore", "--from-resource", "pvc-x", + "--from-snapshot", "clone-pvc-old", "--to-resource", "pvc-third", + }); got == 0 { + t.Fatal("restore from a snapshot being reaped succeeded") + } + + if !strings.Contains(errBuf.String(), "being deleted") { + t.Errorf("refusal %q does not say the snapshot is going", errBuf.String()) + } + + if _, err := appStore(t, app).ResourceDefinitions().Get(t.Context(), "pvc-third"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the definition restored from a snapshot being reaped was left behind: %v", err) + } +} + +// The CLI restore drops the snapshot's reaping mark the way the REST door does. +func TestSnapshotRestoreDropsAnExpiredReapingMark(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) { + seedSnapshotSource(ctx, backend) + _ = backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-stale", ResourceName: "pvc-x", + Nodes: []string{"node-1", "node-2"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 1 << 20}}, + Props: map[string]string{ + store.CloneSnapshotReapingProp: "gone@" + time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), + }, + }) + }) + + if got := app.Run(t.Context(), []string{ + "s", "resource", "restore", "--from-resource", "pvc-x", + "--from-snapshot", "snap-stale", "--to-resource", "pvc-stale", + }); got != 0 { + t.Fatalf("restore over an expired mark exit = %d (stderr: %s)", got, errBuf.String()) + } + + rd, err := appStore(t, app).ResourceDefinitions().Get(t.Context(), "pvc-stale") + if err != nil { + t.Fatalf("read the restored definition: %v", err) + } + + if mark, ok := rd.Props[store.CloneSnapshotReapingProp]; ok { + t.Errorf("the restored definition carries the snapshot's reaping mark %q", mark) + } +} diff --git a/internal/cli/write.go b/internal/cli/write.go index 35ec6a18..b8e49760 100644 --- a/internal/cli/write.go +++ b/internal/cli/write.go @@ -340,10 +340,27 @@ func resourceDefinitionDelete(ctx context.Context, run *runContext) error { } if len(snaps) > 0 { + // A snapshot a clone took for itself outlives the clone when its + // reap was skipped, failed or kept it for a restore; nothing else + // names it now. + left, leftErr := store.CloneSnapshotsLeftBehind(ctx, run.Store, snaps) + if leftErr != nil { + _, _ = fmt.Fprintf(run.Err, "warning: could not tell which snapshots clones left behind: %v\n", leftErr) + } else if !left.Empty() { + cause, correction := left.Explain(name) + + return fmt.Errorf("%w: %s has %d snapshot(s): %s; %s", + errDefinitionHasSnapshots, name, len(snaps), cause, correction) + } + return fmt.Errorf("%w: %s has %d snapshot(s); delete them first", errDefinitionHasSnapshots, name, len(snaps)) } + // Read before the delete: the definition's own props are the only record + // of the internal snapshot a clone took on its source. + clonedFrom := store.OwnedCloneSnapshot(ctx, run.Store, name) + // Then the replicas. Dropping the definition alone leaves them with no // parent to stamp a deletion on, so the satellite finalizer never runs // and the DRBD minor, port and peer entries stay live on every node. @@ -357,6 +374,15 @@ func resourceDefinitionDelete(ctx context.Context, run *runContext) error { return fmt.Errorf("delete resource definition %s: %w", name, err) } + // The internal snapshot the clone took on its source goes with it, the + // way the REST door reaps it, or the source could never be deleted again. + // Not fatal: the definition is gone, and a kept snapshot is named by the + // refusal a later delete of the source meets. + reapErr := store.ReapClonedSnapshot(ctx, run.Store, clonedFrom) + if reapErr != nil { + _, _ = fmt.Fprintf(run.Err, "warning: internal clone snapshot kept: %v\n", reapErr) + } + return nil } diff --git a/pkg/api/v1/snapshot.go b/pkg/api/v1/snapshot.go index fe59b651..6815dc9c 100644 --- a/pkg/api/v1/snapshot.go +++ b/pkg/api/v1/snapshot.go @@ -129,4 +129,7 @@ const ( SnapshotFlagFailedDeployment = "FAILED_DEPLOYMENT" SnapshotFlagFailedDisconnect = "FAILED_DISCONNECT" SnapshotFlagSuccessful = "SUCCESSFUL" + // SnapshotFlagDelete marks a snapshot whose delete was issued and has + // not finished: a finalizer still holds it. + SnapshotFlagDelete = "DELETE" ) diff --git a/pkg/rest/cache_invalidation_bug_124_test.go b/pkg/rest/cache_invalidation_bug_124_test.go index 77f6b066..ef2b08f5 100644 --- a/pkg/rest/cache_invalidation_bug_124_test.go +++ b/pkg/rest/cache_invalidation_bug_124_test.go @@ -151,6 +151,10 @@ func (l *laggingRDs) List(ctx context.Context) ([]apiv1.ResourceDefinition, erro return l.inner.List(ctx) //nolint:wrapcheck // test helper } +func (l *laggingRDs) ListUncached(ctx context.Context) ([]apiv1.ResourceDefinition, error) { + return l.inner.ListUncached(ctx) //nolint:wrapcheck // test helper +} + func (l *laggingRDs) Get(ctx context.Context, name string) (apiv1.ResourceDefinition, error) { return l.inner.Get(ctx, name) //nolint:wrapcheck // test helper } diff --git a/pkg/rest/rd_clone.go b/pkg/rest/rd_clone.go index f85d78c7..5591fbc5 100644 --- a/pkg/rest/rd_clone.go +++ b/pkg/rest/rd_clone.go @@ -1474,6 +1474,8 @@ func (s *Server) ensureCloneSnapshot( return nil, false, false } + stampCloneSnapshotOwner(&snap, cloneName) + snap.Snapshots = makeSnapshotPerNode(snapName, snap.Nodes, snap.VolumeDefinitions) err = s.Store.Snapshots().Create(ctx, &snap) @@ -1490,6 +1492,20 @@ func (s *Server) ensureCloneSnapshot( return &snap, false, true } +// stampCloneSnapshotOwner marks the snapshot as the clone's own, so deleting +// the clone may reap it. A name cannot carry that: an operator can call a +// snapshot `clone-` and restore it under that target, and the marker a +// restore writes is the one a clone writes. The map is copied first, since the +// hydration hands over the source definition's own. +func stampCloneSnapshotOwner(snap *apiv1.Snapshot, cloneName string) { + snap.Props = maps.Clone(snap.Props) + if snap.Props == nil { + snap.Props = map[string]string{} + } + + snap.Props[store.CloneSnapshotOwnerProp] = cloneName +} + // cloneSourceIsNotBeingDeleted mirrors the snapshot-create Bug 180 gate: a // source RD mid-tear-down would reap the internal snapshot and the clone // marker from under the satellite's restore. diff --git a/pkg/rest/rd_delete_clone_snapshot_test.go b/pkg/rest/rd_delete_clone_snapshot_test.go new file mode 100644 index 00000000..be168eca --- /dev/null +++ b/pkg/rest/rd_delete_clone_snapshot_test.go @@ -0,0 +1,315 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "encoding/json" + "net/http" + "strings" + "testing" + + "github.com/cockroachdb/errors" + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +func deleteRD(t *testing.T, base, name string) int { + t.Helper() + + resp := httpDelete(t, base+"/v1/resource-definitions/"+name) + _ = resp.Body.Close() + + return resp.StatusCode +} + +// The clone takes an internal snapshot on the SOURCE, and nothing reaped it: +// the target could be deleted and the snapshot stayed, so the source could +// never be deleted again through the API, which refuses a definition that has +// snapshots. Every clone from CSI goes through this path. +func TestRDDeleteReapsTheInternalCloneSnapshotFromTheSource(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, st, "src-reap") + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := cloneOnce(t, base, "src-reap", "dst-reap", nil); code != http.StatusCreated { + t.Fatalf("clone = %d, want 201", code) + } + + if _, err := st.Snapshots().Get(ctx, "src-reap", cloneSnapshotName("dst-reap")); err != nil { + t.Fatalf("fixture: the clone took no internal snapshot: %v", err) + } + + if clone, err := st.ResourceDefinitions().Get(ctx, "dst-reap"); err != nil { + t.Fatalf("read the clone: %v", err) + } else if owner, ok := clone.Props[store.CloneSnapshotOwnerProp]; ok { + t.Errorf("the clone carries the snapshot's owner prop %q", owner) + } + + if code := deleteRD(t, base, "dst-reap"); code != http.StatusOK { + t.Fatalf("delete of the clone = %d, want 200", code) + } + + if _, err := st.Snapshots().Get(ctx, "src-reap", cloneSnapshotName("dst-reap")); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the internal snapshot outlived the clone it backed: %v", err) + } + + if code := deleteRD(t, base, "src-reap"); code != http.StatusOK { + t.Errorf("delete of the source = %d, want 200: the source is undeletable while that snapshot stands", code) + } +} + +// A restore's marker names an operator's snapshot, which is somebody's data. +// Only the snapshot a clone derived from its own target name is reaped. +func TestRDDeleteLeavesTheSnapshotARestoreCameFrom(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, st, "src-keep") + seedRestoreSnapshot(t, st, "src-keep", "snap-keep", []string{"node-a"}) + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := restoreOnce(t, base, "src-keep", "snap-keep", + map[string]any{"to_resource": "dst-keep"}); code != http.StatusCreated { + t.Fatalf("restore = %d, want 201", code) + } + + if code := deleteRD(t, base, "dst-keep"); code != http.StatusOK { + t.Fatalf("delete of the restored definition = %d, want 200", code) + } + + if _, err := st.Snapshots().Get(ctx, "src-keep", "snap-keep"); err != nil { + t.Errorf("the operator's snapshot was reaped with the restore that used it: %v", err) + } +} + +// A restore writes the same marker a clone does, and an operator can name a +// snapshot `clone-` and restore it under that target. Ownership read +// off the name destroyed that snapshot with the definition. +func TestRDDeleteKeepsAnOperatorSnapshotNamedLikeTheClonesOwn(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, st, "src-op") + seedRestoreSnapshot(t, st, "src-op", cloneSnapshotName("dst-op"), []string{"node-a"}) + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := restoreOnce(t, base, "src-op", cloneSnapshotName("dst-op"), + map[string]any{"to_resource": "dst-op"}); code != http.StatusCreated { + t.Fatalf("restore = %d, want 201", code) + } + + if code := deleteRD(t, base, "dst-op"); code != http.StatusOK { + t.Fatalf("delete = %d, want 200", code) + } + + if _, err := st.Snapshots().Get(ctx, "src-op", cloneSnapshotName("dst-op")); err != nil { + t.Errorf("the operator's snapshot was reaped because of its name: %v", err) + } +} + +// The internal snapshot is visible in `s l`, so a third definition can be +// restored from it, and that definition keeps reading it through its marker. +func TestRDDeleteKeepsTheCloneSnapshotAnotherDefinitionWasRestoredFrom(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, st, "src-dep") + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := cloneOnce(t, base, "src-dep", "dst-dep", nil); code != http.StatusCreated { + t.Fatalf("clone = %d, want 201", code) + } + + if code := restoreOnce(t, base, "src-dep", cloneSnapshotName("dst-dep"), + map[string]any{"to_resource": "third-dep"}); code != http.StatusCreated { + t.Fatalf("restore from the internal snapshot = %d, want 201", code) + } + + if code := deleteRD(t, base, "dst-dep"); code != http.StatusOK { + t.Fatalf("delete of the clone = %d, want 200", code) + } + + if _, err := st.Snapshots().Get(ctx, "src-dep", cloneSnapshotName("dst-dep")); err != nil { + t.Errorf("the snapshot third-dep was restored from was reaped with the clone: %v", err) + } +} + +// A reap that was skipped or failed cannot be re-run by deleting the clone +// again, since it is gone. The refusal a delete of the source meets is the one +// place that still sees the snapshot, so it names it. +func TestRDDeleteRefusalNamesAnInternalSnapshotItsCloneLeftBehind(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, st, "src-orph") + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: cloneSnapshotName("gone-orph"), + ResourceName: "src-orph", + Nodes: []string{"node-a"}, + Props: map[string]string{store.CloneSnapshotOwnerProp: "gone-orph"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 64 * 1024}}, + }); err != nil { + t.Fatalf("seed the orphaned snapshot: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := httpDelete(t, base+"/v1/resource-definitions/src-orph") + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusConflict { + t.Fatalf("delete of the source = %d, want 409", resp.StatusCode) + } + + var rcs []apiv1.APICallRc + if err := json.NewDecoder(resp.Body).Decode(&rcs); err != nil || len(rcs) == 0 { + t.Fatalf("decode the refusal: %v", err) + } + + if !strings.Contains(rcs[0].Cause, cloneSnapshotName("gone-orph")) { + t.Errorf("refusal cause %q does not name the snapshot the clone left", rcs[0].Cause) + } +} + +// A snapshot the reap kept because another definition was restored from it is +// not deletable. The refusal on the source used to call it an orphan and tell +// the operator to delete it, which takes the point-in-time from under the +// definition still restoring from it. +func TestRDDeleteRefusalNamesTheDefinitionThatKeepsACloneSnapshot(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedDeployedCloneSource(t, st, "src-use") + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := cloneOnce(t, base, "src-use", "dst-use", nil); code != http.StatusCreated { + t.Fatalf("clone = %d, want 201", code) + } + + if code := restoreOnce(t, base, "src-use", cloneSnapshotName("dst-use"), + map[string]any{"to_resource": "third-use"}); code != http.StatusCreated { + t.Fatalf("restore from the internal snapshot = %d, want 201", code) + } + + if code := deleteRD(t, base, "dst-use"); code != http.StatusOK { + t.Fatalf("delete of the clone = %d, want 200", code) + } + + resp := httpDelete(t, base+"/v1/resource-definitions/src-use") + defer func() { _ = resp.Body.Close() }() + + var rcs []apiv1.APICallRc + if err := json.NewDecoder(resp.Body).Decode(&rcs); err != nil || len(rcs) == 0 { + t.Fatalf("decode the refusal (status %d): %v", resp.StatusCode, err) + } + + rc := rcs[0] + + if !strings.Contains(rc.Correc, "keep "+cloneSnapshotName("dst-use")+" while third-use exists") { + t.Errorf("correction %q does not name the definition restoring from the snapshot", rc.Correc) + } + + // The snapshot delete door does not check for dependents, so a command + // next to "kept because" is one an operator runs. + if strings.Contains(rc.Correc, "s d src-use "+cloneSnapshotName("dst-use")) { + t.Errorf("correction %q tells the operator to delete a snapshot still in use", rc.Correc) + } +} + +// A snapshot a version before the owner prop took is never reaped, and the +// operator was never told which one keeps the source refused. +func TestRDDeleteRefusalNamesAnUnownedCloneSnapshot(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedDeployedCloneSource(t, st, "src-old") + seedRestoreSnapshot(t, st, "src-old", cloneSnapshotName("gone-old"), []string{"node-a"}) + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := httpDelete(t, base+"/v1/resource-definitions/src-old") + defer func() { _ = resp.Body.Close() }() + + var rcs []apiv1.APICallRc + if err := json.NewDecoder(resp.Body).Decode(&rcs); err != nil || len(rcs) == 0 { + t.Fatalf("decode the refusal (status %d): %v", resp.StatusCode, err) + } + + if !strings.Contains(rcs[0].Cause, cloneSnapshotName("gone-old")) { + t.Errorf("cause %q does not name the unowned clone snapshot", rcs[0].Cause) + } +} + +func refusalOf(t *testing.T, base, rdName string) apiv1.APICallRc { + t.Helper() + + resp := httpDelete(t, base+"/v1/resource-definitions/"+rdName) + defer func() { _ = resp.Body.Close() }() + + var rcs []apiv1.APICallRc + if err := json.NewDecoder(resp.Body).Decode(&rcs); err != nil || len(rcs) == 0 { + t.Fatalf("decode the refusal (status %d): %v", resp.StatusCode, err) + } + + return rcs[0] +} + +// The bucket decides what the operator is told to run: a stamped orphan gets +// the command outright, one never stamped gets it behind a warning. Both carry +// the snapshot's own name, since a placeholder pasted into a shell is a +// redirect and `s d` of a name that does not exist exits 0. +func TestRDDeleteRefusalWordsEachLeftSnapshotByItsBucket(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, st, "src-bkt") + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "clone-owned", ResourceName: "src-bkt", + Props: map[string]string{store.CloneSnapshotOwnerProp: "owned"}, + }); err != nil { + t.Fatalf("seed the stamped orphan: %v", err) + } + + seedRestoreSnapshot(t, st, "src-bkt", "clone-legacy", []string{"node-a"}) + + base, stop := startServerWithStore(t, st) + defer stop() + + rc := refusalOf(t, base, "src-bkt") + + if !strings.Contains(rc.Correc, "`linstor s d src-bkt clone-owned`; ") || + strings.Contains(rc.Correc, "depends on clone-owned") { + t.Errorf("correction %q does not give the stamped orphan its command outright", rc.Correc) + } + + if !strings.Contains(rc.Correc, + "if nothing of yours depends on clone-legacy, `linstor s d src-bkt clone-legacy`") { + t.Errorf("correction %q does not put the unstamped snapshot behind a warning", rc.Correc) + } + + if !strings.HasSuffix(rc.Correc, "then delete src-bkt again") { + t.Errorf("correction %q does not end on the source delete", rc.Correc) + } +} diff --git a/pkg/rest/resource_definitions.go b/pkg/rest/resource_definitions.go index 3e893dc4..77bd6980 100644 --- a/pkg/rest/resource_definitions.go +++ b/pkg/rest/resource_definitions.go @@ -1207,24 +1207,14 @@ func (s *Server) handleRDDelete(w http.ResponseWriter, r *http.Request) { // stamps DeletionTimestamp on every replica, a failed RD-delete // leaves the cluster half-torn-down (children gone, parent // kept, snapshots orphaned) which no retry can reconcile. - snaps, err := s.Store.Snapshots().ListByDefinitionUncached(r.Context(), name) - if err != nil && !errors.Is(err, store.ErrNotFound) { - writeStoreError(w, err) - + if !s.rdHasNoSnapshots(w, r, name) { return } - if len(snaps) > 0 { - writeJSON(w, http.StatusConflict, []apiv1.APICallRc{{ - RetCode: apiCallRcError | apiCallRcFailExistsSnapshotDfn, - Message: "Cannot delete resource definition '" + name + "' because it has snapshots.", - ObjRefs: map[string]string{ - objRefRscDfn: name, - }, - }}) - - return - } + // Read before the delete, used after it: the definition's own props + // are the only record of the internal snapshot a clone left on its + // source. + clonedFrom := store.OwnedCloneSnapshot(r.Context(), s.Store, name) // Cascade the delete to all child Resource replicas BEFORE // dropping the RD itself. Without this, child Resources are @@ -1243,7 +1233,7 @@ func (s *Server) handleRDDelete(w http.ResponseWriter, r *http.Request) { // existing `blockstor.cozystack.io/satellite-resource` // finalizer then drains DRBD before the apiserver removes // the object. - err = s.cascadeDeleteResources(r.Context(), name) + err := s.cascadeDeleteResources(r.Context(), name) if err != nil { writeStoreError(w, err) @@ -1289,6 +1279,14 @@ func (s *Server) handleRDDelete(w http.ResponseWriter, r *http.Request) { // close: either ordering yields a clean cluster. s.sweepOrphanSnapshotsAfterRDDelete(r.Context(), name) + // And the internal snapshot the clone of this definition took on its + // SOURCE. Nothing else reaps it: the operator-facing snapshot doors + // and the auto-snapshot reaper are all driven by a name or a label + // this one never carries, so it outlived the target it was taken for + // and left the source undeletable through this very handler, which + // refuses a definition that has snapshots. + reapClonedSnapshot(r.Context(), s.Store, clonedFrom) + // Bug 124: block the response until the local informer cache has // observed the RD + child Resource deletions. Without this gate, // `linstor rd d ` returns SUCCESS and the very next @@ -1302,6 +1300,62 @@ func (s *Server) handleRDDelete(w http.ResponseWriter, r *http.Request) { }}) } +// rdHasNoSnapshots is handleRDDelete's pre-walk: it refuses, and answers +// false, while any Snapshot still hangs off the definition. +func (s *Server) rdHasNoSnapshots(w http.ResponseWriter, r *http.Request, name string) bool { + // Uncached: a snapshot that raced the delete and has not reached the + // informer is exactly the one this refusal exists for. + snaps, err := s.Store.Snapshots().ListByDefinitionUncached(r.Context(), name) + if err != nil && !errors.Is(err, store.ErrNotFound) { + writeStoreError(w, err) + + return false + } + + if len(snaps) > 0 { + refusal := apiv1.APICallRc{ + RetCode: apiCallRcError | apiCallRcFailExistsSnapshotDfn, + Message: "Cannot delete resource definition '" + name + "' because it has snapshots.", + ObjRefs: map[string]string{ + objRefRscDfn: name, + }, + } + + // A snapshot a clone took for itself outlives the clone when its + // reap was skipped or failed, and a repeated delete of the clone + // cannot re-run it: the clone is already gone. This refusal is the + // one place that still sees it, so it names it. + left, leftErr := store.CloneSnapshotsLeftBehind(r.Context(), s.Store, snaps) + if leftErr != nil { + log.FromContext(r.Context()).V(1).Info("could not tell which snapshots clones left behind", + "resourceDefinition", name, "error", leftErr.Error()) + } else if !left.Empty() { + refusal.Cause, refusal.Correc = left.Explain(name) + } + + writeJSON(w, http.StatusConflict, []apiv1.APICallRc{refusal}) + + return false + } + + return true +} + +// reapClonedSnapshot drops the internal snapshot the deleted definition was +// cloned from, when the clone path stamped it as that definition's own; see +// store.ReapClonedSnapshot. +// +// Best-effort, like the sweep below: the delete has already succeeded and the +// operator has been told so. A snapshot kept because another definition was +// restored from it, or one whose delete failed, stays visible in `linstor s l`, +// and the refusal a later delete of the source meets names it. +func reapClonedSnapshot(ctx context.Context, st store.Store, ref store.ClonedSnapshotRef) { + err := store.ReapClonedSnapshot(ctx, st, ref) + if err != nil { + log.FromContext(ctx).V(1).Info("internal clone snapshot kept", "reason", err.Error()) + } +} + // sweepOrphanSnapshotsAfterRDDelete drops any Snapshot rows that // raced past the pre-Delete `ensureNoSnapDfns` walk in handleRDDelete. // Bug 180: a concurrent `linstor s c ` can slip between diff --git a/pkg/rest/snapshot_restore.go b/pkg/rest/snapshot_restore.go index 793e4f76..09c99f2f 100644 --- a/pkg/rest/snapshot_restore.go +++ b/pkg/rest/snapshot_restore.go @@ -1364,6 +1364,12 @@ func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *s // every replica of the new RD clones from the same source. newRD.Props = store.WithRestoreMarker(newRD.Props, snap) + // The owner prop is the snapshot's, not the definition's: copied onward, + // every snapshot later taken of this definition would inherit a claim of + // ownership it was never given. + delete(newRD.Props, store.CloneSnapshotOwnerProp) + delete(newRD.Props, store.CloneSnapshotReapingProp) + created, adoptedRG, err := s.createOrAdoptRestoredRD(ctx, &newRD, snap, overrides) if err != nil { return materialisedRD{}, err @@ -1377,6 +1383,16 @@ func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *s made = createdRD(newRD.Name, newRD.ResourceGroupName) } + // The snapshot was read before the definition existed, and a reap that + // listed dependents in between could not see this one. Read it back now + // the definition is there; see store.ReapClonedSnapshot. The withdraw has + // already taken back what this request created, so its error is not one + // to roll back over. + err = store.RestoreSourceWithdrawn(ctx, s.Store, snap.ResourceName, snap.Name) + if err != nil { + return materialisedRD{}, s.withdrawRestoredRD(ctx, newRD.Name, created, err) + } + err = hydrateVolumesFromSnapshot(ctx, s, newRD.Name, snap, true) if err != nil { return made, made.failedAfterWrite(err) @@ -1475,6 +1491,35 @@ func (s *Server) claimAdoptedLeftover(ctx context.Context, rdName string, snap * return err //nolint:wrapcheck // store names the definition } +// withdrawRestoredRD takes back a definition this request created from a +// snapshot that went away under it, before anything was hydrated into it, and +// answers as for a snapshot that does not exist. +// +// A definition an earlier attempt left is not this request's to delete: it +// may hold what that attempt hydrated. It cannot be finished without the +// snapshot either, so the answer names it and the way out, rather than a 404 +// every retry repeats with nothing pointing at the leftover. +func (s *Server) withdrawRestoredRD(ctx context.Context, rdName string, created bool, cause error) error { + if created { + err := s.Store.ResourceDefinitions().Delete(ctx, rdName) + if err != nil && !errors.Is(err, store.ErrNotFound) { + log.FromContext(ctx).Info("could not withdraw a definition whose snapshot went away", + "resourceDefinition", rdName, "error", err.Error()) + } + } + + if !errors.Is(cause, store.ErrRestoreSourceWithdrawn) { + return cause + } + + if created { + return errors.Wrap(store.ErrNotFound, cause.Error()) + } + + return errors.Wrapf(store.ErrNotFound, "%s; '%s', left by an earlier attempt at this restore, "+ + "cannot be finished without it: delete it with `linstor rd d %s`", cause.Error(), rdName, rdName) +} + // materialisedRD is what a materialisation wrote, and whether the definition // under that name is this call's to undo. // diff --git a/pkg/rest/snapshot_restore_withdraw_test.go b/pkg/rest/snapshot_restore_withdraw_test.go new file mode 100644 index 00000000..47a6327a --- /dev/null +++ b/pkg/rest/snapshot_restore_withdraw_test.go @@ -0,0 +1,196 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "encoding/json" + "net/http" + "strings" + "testing" + "time" + + "github.com/cockroachdb/errors" + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// snapshotGoesOnCreate deletes one snapshot just before a definition is +// created: the reap of a clone landing between the restore's snapshot read +// and its definition create. +type snapshotGoesOnCreate struct { + store.Store + + source, snapshot string +} + +type snapshotGoesOnCreateRDs struct { + store.ResourceDefinitionStore + + outer snapshotGoesOnCreate +} + +func (s snapshotGoesOnCreate) ResourceDefinitions() store.ResourceDefinitionStore { + return snapshotGoesOnCreateRDs{ResourceDefinitionStore: s.Store.ResourceDefinitions(), outer: s} +} + +func (r snapshotGoesOnCreateRDs) Create(ctx context.Context, rd *apiv1.ResourceDefinition) error { + _ = r.outer.Store.Snapshots().Delete(ctx, r.outer.source, r.outer.snapshot) + + return r.ResourceDefinitionStore.Create(ctx, rd) //nolint:wrapcheck // pass-through test double +} + +// A reap whose dependent list was taken before the restore created its +// definition deleted the snapshot from under it, and the restore answered 201 +// over a definition the satellite can only bring up blank. +func TestSnapshotRestoreWithdrawsWhenItsSnapshotGoesBeforeTheDefinitionLands(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, backend, "src-gone") + seedRestoreSnapshot(t, backend, "src-gone", "snap-gone", []string{"node-a"}) + + base, stop := startServerWithStore(t, snapshotGoesOnCreate{ + Store: backend, source: "src-gone", snapshot: "snap-gone", + }) + defer stop() + + code := restoreOnce(t, base, "src-gone", "snap-gone", map[string]any{"to_resource": "dst-gone"}) + if code != http.StatusNotFound { + t.Errorf("restore over a snapshot deleted under it = %d, want 404", code) + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-gone"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the definition restored from a deleted snapshot was left behind: %v", err) + } +} + +// The reap marks before it lists, so a restore whose create landed after the +// list reads the mark back and withdraws. +func TestSnapshotRestoreWithdrawsFromASnapshotBeingReaped(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, st, "src-reap") + + seedRestoreSnapshot(t, st, "src-reap", "clone-dst-reap", []string{"node-a"}) + + snap, err := st.Snapshots().Get(ctx, "src-reap", "clone-dst-reap") + if err != nil { + t.Fatalf("read the seeded snapshot: %v", err) + } + + snap.Props = map[string]string{ + store.CloneSnapshotOwnerProp: "dst-reap", + store.CloneSnapshotReapingProp: "dst-reap@" + time.Now().UTC().Format(time.RFC3339), + } + + if err := st.Snapshots().Update(ctx, &snap); err != nil { + t.Fatalf("mark the snapshot as being reaped: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + code := restoreOnce(t, base, "src-reap", "clone-dst-reap", map[string]any{"to_resource": "third-reap"}) + if code != http.StatusNotFound { + t.Errorf("restore from a snapshot being reaped = %d, want 404", code) + } + + if _, err := st.ResourceDefinitions().Get(ctx, "third-reap"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the definition restored from a snapshot being reaped was left behind: %v", err) + } +} + +// A mark no live reap owns is not withdrawn on, and it is the snapshot's: a +// definition restored from the snapshot does not carry it onward to every +// snapshot later taken of it. +func TestSnapshotRestoreDropsAnExpiredReapingMark(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, st, "src-stale") + seedRestoreSnapshot(t, st, "src-stale", "snap-stale", []string{"node-a"}) + + snap, err := st.Snapshots().Get(ctx, "src-stale", "snap-stale") + if err != nil { + t.Fatalf("read the seeded snapshot: %v", err) + } + + snap.Props = map[string]string{ + store.CloneSnapshotReapingProp: "gone@" + time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), + } + + if err := st.Snapshots().Update(ctx, &snap); err != nil { + t.Fatalf("leave an expired mark: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := restoreOnce(t, base, "src-stale", "snap-stale", + map[string]any{"to_resource": "dst-stale"}); code != http.StatusCreated { + t.Fatalf("restore over an expired mark = %d, want 201", code) + } + + rd, err := st.ResourceDefinitions().Get(ctx, "dst-stale") + if err != nil { + t.Fatalf("read the restored definition: %v", err) + } + + if mark, ok := rd.Props[store.CloneSnapshotReapingProp]; ok { + t.Errorf("the restored definition carries the snapshot's reaping mark %q", mark) + } +} + +// A leftover an earlier attempt left is not this request's to delete, and the +// answer names it: without that every retry repeats a 404 about the snapshot +// with nothing pointing at the definition that can no longer be finished. +func TestSnapshotRestoreKeepsAnEarlierLeftoverWhenItsSnapshotGoes(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, backend, "src-left") + seedRestoreSnapshot(t, backend, "src-left", "snap-left", []string{"node-a"}) + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "dst-left", + Props: map[string]string{restoreFromSnapshotKey: restoreMarker("src-left", "snap-left")}, + }); err != nil { + t.Fatalf("seed the leftover: %v", err) + } + + base, stop := startServerWithStore(t, snapshotGoesOnCreate{ + Store: backend, source: "src-left", snapshot: "snap-left", + }) + defer stop() + + raw, err := json.Marshal(map[string]any{"to_resource": "dst-left"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + resp := httpPost(t, base+"/v1/resource-definitions/src-left/snapshot-restore-resource/snap-left", raw) + defer func() { _ = resp.Body.Close() }() + + var rcs []apiv1.APICallRc + if err := json.NewDecoder(resp.Body).Decode(&rcs); err != nil || len(rcs) == 0 { + t.Fatalf("decode the answer (status %d): %v", resp.StatusCode, err) + } + + if resp.StatusCode != http.StatusNotFound { + t.Errorf("restore over a leftover whose snapshot went = %d, want 404", resp.StatusCode) + } + + if !strings.Contains(rcs[0].Message, "linstor rd d dst-left") { + t.Errorf("answer %q does not name the leftover", rcs[0].Message) + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-left"); err != nil { + t.Errorf("the earlier attempt's leftover was deleted: %v", err) + } +} diff --git a/pkg/rest/snapshots.go b/pkg/rest/snapshots.go index f6f48715..3fb9e4b1 100644 --- a/pkg/rest/snapshots.go +++ b/pkg/rest/snapshots.go @@ -396,9 +396,13 @@ const legacySnapshotFlagFailed = "FAILED" // isTerminalSnapshotFlag reports whether a flag value already pins the // snapshot to a non-Incomplete State that outranks `SUCCESSFUL`. -// Mirrors the python CLI's elif-chain order in snapshot_cmds.show. +// Mirrors the python CLI's elif-chain order in snapshot_cmds.show, where +// DELETE comes first: a snapshot whose delete is in flight is Deleting, and +// stamping SUCCESSFUL beside it hands a client that reads the flags in any +// other order a snapshot being destroyed as a usable one. func isTerminalSnapshotFlag(flag string) bool { - return flag == apiv1.SnapshotFlagFailedDeployment || + return flag == apiv1.SnapshotFlagDelete || + flag == apiv1.SnapshotFlagFailedDeployment || flag == apiv1.SnapshotFlagFailedDisconnect || flag == apiv1.SnapshotFlagSuccessful || flag == legacySnapshotFlagFailed diff --git a/pkg/rest/snapshots_test.go b/pkg/rest/snapshots_test.go index ebaedad4..5dc45cb8 100644 --- a/pkg/rest/snapshots_test.go +++ b/pkg/rest/snapshots_test.go @@ -2417,3 +2417,43 @@ func TestSnapshotCreateScopesGateToTargetNodes_G5(t *testing.T) { t.Errorf("targeting thick n2: got %d, want 400 (thick pool refused)", badResp.StatusCode) } } + +// A snapshot whose delete is in flight is Deleting: the python CLI checks the +// DELETE flag first, and SUCCESSFUL stamped beside it reads as a usable +// snapshot to any client that checks the flags in another order. +func TestSnapshotStateNotMarkedSuccessfulWhileDeleting(t *testing.T) { + ctx := t.Context() + + st := store.NewInMemory() + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "snap-deleting"}); err != nil { + t.Fatalf("seed RD: %v", err) + } + + if err := st.Resources().Create(ctx, &apiv1.Resource{Name: "snap-deleting", NodeName: "worker-1"}); err != nil { + t.Fatalf("seed Resource: %v", err) + } + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap1", + ResourceName: "snap-deleting", + Nodes: []string{"worker-1"}, + Flags: []string{apiv1.SnapshotFlagDelete}, + Snapshots: []apiv1.SnapshotPerNode{ + {SnapshotName: "snap1", NodeName: "worker-1", CreateTimestamp: 1714000000}, + }, + }); err != nil { + t.Fatalf("seed Snapshot: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + got := decodeSnapshotPage(t, base+"/v1/view/snapshots") + if len(got) != 1 { + t.Fatalf("view len: got %d, want 1", len(got)) + } + + if slices.Contains(got[0].Flags, apiv1.SnapshotFlagSuccessful) { + t.Errorf("Flags: SUCCESSFUL stamped beside DELETE: %v", got[0].Flags) + } +} diff --git a/pkg/store/clone_snapshot.go b/pkg/store/clone_snapshot.go new file mode 100644 index 00000000..2a147433 --- /dev/null +++ b/pkg/store/clone_snapshot.go @@ -0,0 +1,405 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package store + +import ( + "context" + "errors" + "fmt" + "slices" + "sort" + "strings" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" +) + +// CloneSnapshotOwnerProp marks a snapshot the clone path took for itself. Its +// value is the name of the clone the snapshot was taken for. +// +// The marker alone cannot say whether a snapshot is the clone's own: a restore +// writes the same marker, pointing at an operator's snapshot, and an operator +// can name a snapshot anything, `clone-` included. Only a prop this +// server wrote when it took the snapshot separates the two, so a snapshot +// without it is somebody's data and is never reaped. +const CloneSnapshotOwnerProp = "Blockstor/CloneSnapshotOf" + +// ClonedSnapshotRef names the internal snapshot a clone left on its source, +// and the clone it was taken for. +type ClonedSnapshotRef struct { + Source string + Snapshot string + Clone string +} + +// OwnedCloneSnapshot answers which internal snapshot a definition was cloned +// from, read before the definition is deleted since its props are the only +// record of it. The zero value means there is none to reap: the definition is +// not a clone, the snapshot is gone, or the snapshot does not carry the owner +// prop naming this definition. +func OwnedCloneSnapshot(ctx context.Context, st Store, rdName string) ClonedSnapshotRef { + rd, err := st.ResourceDefinitions().Get(ctx, rdName) + if err != nil { + return ClonedSnapshotRef{} + } + + source, snapName, found := strings.Cut(rd.Props[RestoreFromSnapshotProp], ":") + if !found || source == "" || snapName == "" { + return ClonedSnapshotRef{} + } + + snap, err := st.Snapshots().Get(ctx, source, snapName) + if err != nil || !strings.EqualFold(snap.Props[CloneSnapshotOwnerProp], rdName) { + return ClonedSnapshotRef{} + } + + return ClonedSnapshotRef{Source: source, Snapshot: snapName, Clone: rdName} +} + +// CloneSnapshotReapingProp marks an internal clone snapshot the reap has +// started on, as `@`. A restore that created its +// definition from the snapshot reads it back past the cache and withdraws when +// it finds a live mark or no snapshot at all; see ReapClonedSnapshot for why +// both sides are needed. +const CloneSnapshotReapingProp = "Blockstor/CloneSnapshotReaping" + +// cloneSnapshotReapBudget bounds a reap from its first write to its last: no +// call the reap makes, the delete included, can land after it. +const cloneSnapshotReapBudget = 30 * time.Second + +// cloneSnapshotReapingMarkTTL is how long a reaping mark stays live. It covers +// the reap up to its delete: a mark older than this belongs to a reap that can +// no longer issue one, since cloneSnapshotReapBudget is far shorter, so it is +// ignored, and a mark left by a reap that died, or whose clean-up failed, +// cannot refuse restores from a snapshot the reap never deleted for longer +// than this. A delete that was issued is a different state and outlives any +// budget while a satellite finalizer holds the snapshot; that one is read off +// the snapshot's DELETE flag, not off the mark. The gap between the two +// budgets also absorbs clock skew between the replica that wrote the mark and +// the one that reads it. +const cloneSnapshotReapingMarkTTL = 5 * time.Minute + +// ErrCloneSnapshotInUse reports an internal clone snapshot another definition +// was restored from, which is therefore kept. +var ErrCloneSnapshotInUse = errors.New("internal clone snapshot is still a restore source") + +// ErrRestoreSourceWithdrawn reports a snapshot that was deleted, or is being +// reaped, while a definition was being restored from it. +var ErrRestoreSourceWithdrawn = errors.New("snapshot was withdrawn while a definition was restored from it") + +// ReapClonedSnapshot drops the internal snapshot once the clone it was taken +// for is gone, which is what lets the source be deleted again: both doors +// refuse to delete a definition that still has snapshots. +// +// The snapshot is visible in `s l`, so it can have been used as the source of +// a restore, and that definition keeps reading it through its own marker: the +// placer pins new replicas to the snapshot's nodes and the satellite restores +// a replica from it by name. Such a snapshot is kept, and the error says why. +// +// A dependent appears in two steps, the snapshot read and the definition +// create, so no list taken here alone can see one that is between them. The +// reap therefore marks the snapshot first, then asks the API server for +// dependents, and only then deletes; a restore creates its definition first, +// then reads the snapshot back past the cache and withdraws on a live mark or +// on a snapshot that is gone (RestoreSourceWithdrawn). Whichever of the list +// and the create comes second sees the other: a create before the list is a +// dependent the list finds, and a create after it is followed by a read that +// finds the mark. +// +// The reap runs on a context the caller cannot cancel, bounded by +// cloneSnapshotReapBudget: the delete has already been reported, and a reap +// cut between its mark and its clean-up would leave the mark behind. A mark +// that is left anyway, by a process that died or a clean-up that failed, +// expires (cloneSnapshotReapingMarkTTL). +// +// When the dependents cannot be listed the snapshot is kept and the mark +// taken off again; the refusal a later delete of the source meets names it. +func ReapClonedSnapshot(ctx context.Context, st Store, ref ClonedSnapshotRef) error { + if ref.Source == "" || ref.Snapshot == "" { + return nil + } + + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), cloneSnapshotReapBudget) + defer cancel() + + err := setReapingMark(ctx, st, ref.Source, ref.Snapshot, ref.Clone+"@"+time.Now().UTC().Format(time.RFC3339)) + if errors.Is(err, ErrNotFound) { + return nil + } + + if err != nil { + return fmt.Errorf("mark internal clone snapshot %s/%s for reaping: %w", ref.Source, ref.Snapshot, err) + } + + definitions, err := st.ResourceDefinitions().ListUncached(ctx) + if err != nil { + return keepCloneSnapshot(ctx, st, ref, + fmt.Errorf("list definitions restored from %s/%s: %w", ref.Source, ref.Snapshot, err)) + } + + dependents := restoredFrom(definitions, ref.Source, ref.Snapshot) + if len(dependents) > 0 { + return keepCloneSnapshot(ctx, st, ref, fmt.Errorf("%w: %s was restored from %s:%s", + ErrCloneSnapshotInUse, dependents[0], ref.Source, ref.Snapshot)) + } + + err = st.Snapshots().Delete(ctx, ref.Source, ref.Snapshot) + if err != nil && !errors.Is(err, ErrNotFound) { + return keepCloneSnapshot(ctx, st, ref, + fmt.Errorf("delete internal clone snapshot %s/%s: %w", ref.Source, ref.Snapshot, err)) + } + + return nil +} + +// keepCloneSnapshot takes the reaping mark back off a snapshot the reap +// decided to keep, so restores from it are not refused, and returns why it +// was kept. +func keepCloneSnapshot(ctx context.Context, st Store, ref ClonedSnapshotRef, why error) error { + err := setReapingMark(ctx, st, ref.Source, ref.Snapshot, "") + if err != nil && !errors.Is(err, ErrNotFound) { + return fmt.Errorf("%w; the reaping mark stayed on it, so restores from it are refused "+ + "for up to %s: %w", why, cloneSnapshotReapingMarkTTL, err) + } + + return why +} + +// setReapingMark writes mark as the reaping mark, or removes the mark when it +// is empty. The snapshot is read past the cache: the mark this reap wrote one +// round trip earlier may not have reached the cache yet, and a removal decided +// on that read would find nothing to remove. +func setReapingMark(ctx context.Context, st Store, source, snapshot, mark string) error { + snap, err := uncachedSnapshot(ctx, st, source, snapshot) + if err != nil { + return err + } + + if mark == "" { + if _, marked := snap.Props[CloneSnapshotReapingProp]; !marked { + return nil + } + + delete(snap.Props, CloneSnapshotReapingProp) + } else { + if snap.Props == nil { + snap.Props = map[string]string{} + } + + snap.Props[CloneSnapshotReapingProp] = mark + } + + return st.Snapshots().Update(ctx, &snap) //nolint:wrapcheck // callers wrap with the snapshot they name +} + +// uncachedSnapshot reads one snapshot from the API server. +func uncachedSnapshot(ctx context.Context, st Store, source, snapshot string) (apiv1.Snapshot, error) { + snaps, err := st.Snapshots().ListByDefinitionUncached(ctx, source) + if err != nil { + return apiv1.Snapshot{}, fmt.Errorf("read snapshot %s/%s: %w", source, snapshot, err) + } + + for i := range snaps { + if strings.EqualFold(snaps[i].Name, snapshot) { + return snaps[i], nil + } + } + + return apiv1.Snapshot{}, fmt.Errorf("snapshot %s/%s: %w", source, snapshot, ErrNotFound) +} + +// reapingMarkIsLive reports whether a reaping mark belongs to a reap that can +// still delete the snapshot. A mark whose time cannot be read is not. +func reapingMarkIsLive(mark string, now time.Time) bool { + at := strings.LastIndex(mark, "@") + if at < 0 { + return false + } + + written, err := time.Parse(time.RFC3339, mark[at+1:]) + if err != nil { + return false + } + + return now.Sub(written) < cloneSnapshotReapingMarkTTL +} + +// RestoreSourceWithdrawn is the restore's half of the protocol described on +// ReapClonedSnapshot: called after the restored definition was created, it +// reads the snapshot back past the cache and reports ErrRestoreSourceWithdrawn +// when it is gone or a reap that is still live has started on it. Any other +// error means the answer could not be had, and the caller withdraws as well. +func RestoreSourceWithdrawn(ctx context.Context, st Store, source, snapshot string) error { + snap, err := uncachedSnapshot(ctx, st, source, snapshot) + if errors.Is(err, ErrNotFound) { + return fmt.Errorf("%w: %s/%s was deleted", ErrRestoreSourceWithdrawn, source, snapshot) + } + + if err != nil { + return err + } + + if slices.Contains(snap.Flags, apiv1.SnapshotFlagDelete) || + reapingMarkIsLive(snap.Props[CloneSnapshotReapingProp], time.Now()) { + return fmt.Errorf("%w: %s/%s is being deleted", ErrRestoreSourceWithdrawn, source, snapshot) + } + + return nil +} + +// restoredFrom names the definitions whose marker says they were restored +// from source:snapshot. The clone the snapshot was taken for is not skipped by +// name: by the time the reap lists, the clone is gone from the API server, as +// a definition has no finalizer to linger behind, and a definition that has +// taken its name since is a dependent like any other. +func restoredFrom(definitions []apiv1.ResourceDefinition, source, snapshot string) []string { + marker := source + ":" + snapshot + + var names []string + + for i := range definitions { + if strings.EqualFold(definitions[i].Props[RestoreFromSnapshotProp], marker) { + names = append(names, definitions[i].Name) + } + } + + return names +} + +// LeftCloneSnapshots is what the snapshots a source still carries say about +// clones that are gone. +type LeftCloneSnapshots struct { + // Deletable were taken by a clone that no longer exists, and no + // definition was restored from them. + Deletable []string + // Unowned look like a clone's internal snapshot, `clone-` for a + // definition that no longer exists, but carry no owner prop: a version + // before the prop existed took them, or an operator named one that way. + // Nothing here reaps them; they are named so the operator knows why the + // source is still refused. + Unowned []string + // InUse were taken by a clone that is gone but still have a definition + // restored from them, keyed by snapshot. + InUse map[string]string +} + +// Empty reports whether there is nothing to tell the operator. +func (l LeftCloneSnapshots) Empty() bool { + return len(l.Deletable) == 0 && len(l.Unowned) == 0 && len(l.InUse) == 0 +} + +// CloneSnapshotsLeftBehind sorts a source's snapshots that outlived the clone +// they belong to. A delete that reaped nothing, a reap that failed, and a reap +// that kept a snapshot another definition was restored from all leave one, +// and a repeated delete of the clone cannot revisit them: the clone is gone. +// The refusal a delete of the source meets is the one place that still sees +// them, so it names them, and never calls a snapshot deletable while a +// definition still restores from it. +func CloneSnapshotsLeftBehind(ctx context.Context, st Store, snaps []apiv1.Snapshot) (LeftCloneSnapshots, error) { + out := LeftCloneSnapshots{InUse: map[string]string{}} + + var definitions []apiv1.ResourceDefinition + + for i := range snaps { + owner := snaps[i].Props[CloneSnapshotOwnerProp] + legacy := owner == "" + + if legacy { + var found bool + + owner, found = strings.CutPrefix(snaps[i].Name, "clone-") + if !found || owner == "" { + continue + } + } + + _, err := st.ResourceDefinitions().Get(ctx, owner) + if !errors.Is(err, ErrNotFound) { + continue + } + + // One list answers every snapshot, taken only once one needs it. + if definitions == nil { + definitions, err = st.ResourceDefinitions().ListUncached(ctx) + if err != nil { + return LeftCloneSnapshots{}, fmt.Errorf("list definitions restored from %s: %w", snaps[i].ResourceName, err) + } + } + + dependents := restoredFrom(definitions, snaps[i].ResourceName, snaps[i].Name) + + switch { + case len(dependents) > 0: + out.InUse[snaps[i].Name] = dependents[0] + case legacy: + out.Unowned = append(out.Unowned, snaps[i].Name) + default: + out.Deletable = append(out.Deletable, snaps[i].Name) + } + } + + return out, nil +} + +// Explain words the report for the refusal both delete doors give, so the two +// cannot drift. Every command it prints names the snapshot it acts on, and the +// steps come in the order they have to run: what can go now, then what has to +// wait, then the source. A snapshot a definition still restores from gets no +// command at all: the snapshot delete door does not check for dependents, and +// a pasteable delete next to "kept because" is one an operator runs. +func (l LeftCloneSnapshots) Explain(source string) (string, string) { + size := len(l.Deletable) + len(l.Unowned) + len(l.InUse) + causes := make([]string, 0, size) + steps := make([]string, 0, size+1) + + deleteCmd := func(snapshot string) string { + return "`linstor s d " + source + " " + snapshot + "`" + } + + for _, snapshot := range l.Deletable { + causes = append(causes, "internal clone snapshot "+snapshot+ + " outlived the clone it was taken for, and nothing was restored from it") + steps = append(steps, deleteCmd(snapshot)) + } + + for _, snapshot := range l.Unowned { + causes = append(causes, "snapshot "+snapshot+" looks like the internal snapshot of a clone "+ + "that no longer exists, but was not stamped as one, so it is never reaped") + steps = append(steps, "if nothing of yours depends on "+snapshot+", "+deleteCmd(snapshot)) + } + + kept := make([]string, 0, len(l.InUse)) + for snapshot := range l.InUse { + kept = append(kept, snapshot) + } + + sort.Strings(kept) + + for _, snapshot := range kept { + causes = append(causes, "internal clone snapshot "+snapshot+" is kept because "+ + l.InUse[snapshot]+" was restored from it") + steps = append(steps, "keep "+snapshot+" while "+l.InUse[snapshot]+" exists, since "+ + l.InUse[snapshot]+" still reads it; once "+l.InUse[snapshot]+ + " is deleted, the next delete of "+source+" names it again") + } + + steps = append(steps, "then delete "+source+" again") + + return strings.Join(causes, "; "), strings.Join(steps, "; ") +} diff --git a/pkg/store/clone_snapshot_mark_test.go b/pkg/store/clone_snapshot_mark_test.go new file mode 100644 index 00000000..f8095439 --- /dev/null +++ b/pkg/store/clone_snapshot_mark_test.go @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: Apache-2.0 + +package store_test + +import ( + "context" + "errors" + "maps" + "strings" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// cacheWithoutTheMark answers every cached snapshot read without the reaping +// mark, the way a cache that has not seen the reap's own write does. Only the +// uncached list carries it. +type cacheWithoutTheMark struct{ store.SnapshotStore } + +func withoutTheMark(snap apiv1.Snapshot) apiv1.Snapshot { + snap.Props = maps.Clone(snap.Props) + delete(snap.Props, store.CloneSnapshotReapingProp) + + return snap +} + +func (c cacheWithoutTheMark) Get(ctx context.Context, rdName, snapName string) (apiv1.Snapshot, error) { + snap, err := c.SnapshotStore.Get(ctx, rdName, snapName) + + return withoutTheMark(snap), err +} + +func (c cacheWithoutTheMark) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { + snaps, err := c.SnapshotStore.ListByDefinition(ctx, rdName) + for i := range snaps { + snaps[i] = withoutTheMark(snaps[i]) + } + + return snaps, err //nolint:wrapcheck // pass-through test double +} + +type cacheWithoutTheMarkStore struct{ store.Store } + +func (c cacheWithoutTheMarkStore) Snapshots() store.SnapshotStore { + return cacheWithoutTheMark{c.Store.Snapshots()} +} + +func seedKeptCloneSnapshot(t *testing.T, st store.Store) { + t.Helper() + + seedOwnedCloneSnapshot(t, st) + + if err := st.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{ + Name: "third", + Props: map[string]string{store.RestoreFromSnapshotProp: "src:clone-dst"}, + }); err != nil { + t.Fatalf("seed the restored definition: %v", err) + } +} + +// The mark came off through a cached read, which right after the reap's own +// write may not hold it yet, and a kept snapshot then refused every restore. +func TestReapClonedSnapshotTakesTheMarkOffPastTheCache(t *testing.T) { + t.Parallel() + + ctx := t.Context() + backend := store.NewInMemory() + seedKeptCloneSnapshot(t, backend) + + err := store.ReapClonedSnapshot(ctx, cacheWithoutTheMarkStore{backend}, + store.ClonedSnapshotRef{Source: "src", Snapshot: "clone-dst", Clone: "dst"}) + if !errors.Is(err, store.ErrCloneSnapshotInUse) { + t.Fatalf("reap = %v, want ErrCloneSnapshotInUse", err) + } + + if err := store.RestoreSourceWithdrawn(ctx, backend, "src", "clone-dst"); err != nil { + t.Errorf("a restore from the kept snapshot is refused: %v", err) + } +} + +// honoursCancel fails every call on a context that is done, which the +// in-memory store does not. +type honoursCancel struct{ store.SnapshotStore } + +func (h honoursCancel) ListByDefinitionUncached(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { + if err := ctx.Err(); err != nil { + return nil, err //nolint:wrapcheck // the context error is the point + } + + return h.SnapshotStore.ListByDefinitionUncached(ctx, rdName) //nolint:wrapcheck // pass-through test double +} + +func (h honoursCancel) Update(ctx context.Context, snap *apiv1.Snapshot) error { + if err := ctx.Err(); err != nil { + return err //nolint:wrapcheck // the context error is the point + } + + return h.SnapshotStore.Update(ctx, snap) //nolint:wrapcheck // pass-through test double +} + +type honoursCancelStore struct{ store.Store } + +func (h honoursCancelStore) Snapshots() store.SnapshotStore { + return honoursCancel{h.Store.Snapshots()} +} + +// The delete has been reported by the time the reap runs, so a caller that +// hangs up, or a SIGTERM, must not cut it between its mark and its clean-up. +func TestReapClonedSnapshotOutlivesItsCaller(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedOwnedCloneSnapshot(t, backend) + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + err := store.ReapClonedSnapshot(ctx, honoursCancelStore{backend}, + store.ClonedSnapshotRef{Source: "src", Snapshot: "clone-dst", Clone: "dst"}) + if err != nil { + t.Fatalf("reap on a cancelled caller: %v", err) + } + + if _, err := backend.Snapshots().Get(t.Context(), "src", "clone-dst"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the reap stopped with its caller: %v", err) + } +} + +// A mark left by a reap that died, or whose clean-up failed, refused restores +// from a healthy snapshot for good. It expires, and one whose time cannot be +// read is not a live reap's. +func TestRestoreSourceWithdrawnIgnoresAMarkNoLiveReapCanOwn(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + mark string + withdrawn bool + }{ + {name: "fresh", mark: "dst@" + time.Now().UTC().Format(time.RFC3339), withdrawn: true}, + {name: "expired", mark: "dst@" + time.Now().Add(-time.Hour).UTC().Format(time.RFC3339)}, + {name: "unreadable", mark: "dst"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx := t.Context() + backend := store.NewInMemory() + + if err := backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "clone-dst", ResourceName: "src", + Props: map[string]string{store.CloneSnapshotReapingProp: tc.mark}, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } + + err := store.RestoreSourceWithdrawn(ctx, backend, "src", "clone-dst") + if got := errors.Is(err, store.ErrRestoreSourceWithdrawn); got != tc.withdrawn { + t.Errorf("withdrawn = %v (%v), want %v", got, err, tc.withdrawn) + } + }) + } +} + +// The steps come in the order they can run, and the source comes last. +func TestLeftCloneSnapshotsExplainOrdersTheSteps(t *testing.T) { + t.Parallel() + + _, steps := store.LeftCloneSnapshots{ + Deletable: []string{"clone-a"}, + Unowned: []string{"clone-b"}, + InUse: map[string]string{"clone-c": "third"}, + }.Explain("src") + + last := -1 + + for _, want := range []string{"s d src clone-a", "s d src clone-b", "keep clone-c", "then delete src again"} { + at := strings.Index(steps, want) + if at <= last { + t.Fatalf("steps %q: %q out of order", steps, want) + } + + last = at + } +} + +// A delete that was issued is held by the satellite finalizer until the +// on-disk delete succeeds, which outlives the reaping mark's lifetime. The +// restore reads that state off the snapshot, not off the mark's age. +func TestRestoreSourceWithdrawnOnASnapshotBeingDeleted(t *testing.T) { + t.Parallel() + + ctx := t.Context() + backend := store.NewInMemory() + + if err := backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "clone-dst", ResourceName: "src", + Flags: []string{apiv1.SnapshotFlagDelete}, + Props: map[string]string{ + store.CloneSnapshotReapingProp: "dst@" + time.Now().Add(-time.Hour).UTC().Format(time.RFC3339), + }, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } + + err := store.RestoreSourceWithdrawn(ctx, backend, "src", "clone-dst") + if !errors.Is(err, store.ErrRestoreSourceWithdrawn) { + t.Errorf("restore from a snapshot whose delete is in flight = %v, want withdrawn", err) + } +} diff --git a/pkg/store/clone_snapshot_test.go b/pkg/store/clone_snapshot_test.go new file mode 100644 index 00000000..9eda676e --- /dev/null +++ b/pkg/store/clone_snapshot_test.go @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: Apache-2.0 + +package store_test + +import ( + "errors" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// The clone was skipped by name, on the theory that it could still be listed +// after its own delete. It cannot: the list is the API server's, and a +// definition has no finalizer to linger behind. What the skip did let through +// is a different definition that has taken the clone's name since, restored +// from the same snapshot; the reap deleted the snapshot from under it. +func TestReapClonedSnapshotCountsADefinitionThatTookTheClonesName(t *testing.T) { + t.Parallel() + + ctx := t.Context() + backend := store.NewInMemory() + + if err := backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "clone-dst", ResourceName: "src", + Props: map[string]string{store.CloneSnapshotOwnerProp: "dst"}, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "dst", + Props: map[string]string{store.RestoreFromSnapshotProp: "src:clone-dst"}, + }); err != nil { + t.Fatalf("seed the new definition under the clone's name: %v", err) + } + + err := store.ReapClonedSnapshot(ctx, backend, store.ClonedSnapshotRef{ + Source: "src", Snapshot: "clone-dst", Clone: "dst", + }) + if !errors.Is(err, store.ErrCloneSnapshotInUse) { + t.Errorf("reap = %v, want ErrCloneSnapshotInUse", err) + } + + if _, err := backend.Snapshots().Get(ctx, "src", "clone-dst"); err != nil { + t.Errorf("the snapshot the new definition restores from was reaped: %v", err) + } +} diff --git a/pkg/store/clone_snapshot_uncached_test.go b/pkg/store/clone_snapshot_uncached_test.go new file mode 100644 index 00000000..edbdb3bf --- /dev/null +++ b/pkg/store/clone_snapshot_uncached_test.go @@ -0,0 +1,200 @@ +// SPDX-License-Identifier: Apache-2.0 + +package store_test + +import ( + "context" + "errors" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// cacheMissesOne hides one definition from List, the way an informer that has +// not seen its create yet does, and answers ListUncached from the backend. +type cacheMissesOne struct { + store.ResourceDefinitionStore + + hidden string +} + +func (c cacheMissesOne) List(ctx context.Context) ([]apiv1.ResourceDefinition, error) { + all, err := c.ResourceDefinitionStore.List(ctx) + + out := all[:0] + for i := range all { + if all[i].Name != c.hidden { + out = append(out, all[i]) + } + } + + return out, err //nolint:wrapcheck // pass-through test double +} + +func (c cacheMissesOne) ListUncached(ctx context.Context) ([]apiv1.ResourceDefinition, error) { + return c.ResourceDefinitionStore.List(ctx) //nolint:wrapcheck // pass-through test double +} + +type cacheMissesOneStore struct { + store.Store + + hidden string +} + +func (c cacheMissesOneStore) ResourceDefinitions() store.ResourceDefinitionStore { + return cacheMissesOne{ResourceDefinitionStore: c.Store.ResourceDefinitions(), hidden: c.hidden} +} + +// Deleting the clone before the replica serving the delete has listed the +// definition restored from its snapshot took the point-in-time from under that +// definition. The question is asked of the API server. +func TestReapClonedSnapshotAsksPastTheCacheForDependents(t *testing.T) { + t.Parallel() + + ctx := t.Context() + backend := store.NewInMemory() + + if err := backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "clone-dst", ResourceName: "src", + Props: map[string]string{store.CloneSnapshotOwnerProp: "dst"}, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "third", + Props: map[string]string{store.RestoreFromSnapshotProp: "src:clone-dst"}, + }); err != nil { + t.Fatalf("seed the restored definition: %v", err) + } + + err := store.ReapClonedSnapshot(ctx, cacheMissesOneStore{Store: backend, hidden: "third"}, + store.ClonedSnapshotRef{Source: "src", Snapshot: "clone-dst", Clone: "dst"}) + if !errors.Is(err, store.ErrCloneSnapshotInUse) { + t.Errorf("reap = %v, want ErrCloneSnapshotInUse", err) + } + + if _, err := backend.Snapshots().Get(ctx, "src", "clone-dst"); err != nil { + t.Errorf("the snapshot third restores from was reaped: %v", err) + } +} + +var errAPIServerUnavailable = errors.New("apiserver unavailable") + +// markSeenAtList records whether the snapshot carried the reaping mark when +// the reap listed dependents, and optionally fails the list. +type markSeenAtList struct { + store.ResourceDefinitionStore + + snaps store.SnapshotStore + seen *bool + listErr error +} + +func (m markSeenAtList) ListUncached(ctx context.Context) ([]apiv1.ResourceDefinition, error) { + snap, err := m.snaps.Get(ctx, "src", "clone-dst") + if err == nil { + _, *m.seen = snap.Props[store.CloneSnapshotReapingProp] + } + + if m.listErr != nil { + return nil, m.listErr + } + + return m.ResourceDefinitionStore.ListUncached(ctx) //nolint:wrapcheck // pass-through test double +} + +type markSeenAtListStore struct { + store.Store + + seen *bool + listErr error +} + +func (m markSeenAtListStore) ResourceDefinitions() store.ResourceDefinitionStore { + return markSeenAtList{ + ResourceDefinitionStore: m.Store.ResourceDefinitions(), + snaps: m.Snapshots(), seen: m.seen, listErr: m.listErr, + } +} + +func seedOwnedCloneSnapshot(t *testing.T, st store.Store) { + t.Helper() + + if err := st.Snapshots().Create(t.Context(), &apiv1.Snapshot{ + Name: "clone-dst", ResourceName: "src", + Props: map[string]string{store.CloneSnapshotOwnerProp: "dst"}, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } +} + +// A list taken before the mark cannot see a restore created after it, and +// nothing on the restore's side would then tell it the snapshot is going. +func TestReapClonedSnapshotMarksBeforeItLists(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedOwnedCloneSnapshot(t, backend) + + var seen bool + + err := store.ReapClonedSnapshot(t.Context(), markSeenAtListStore{Store: backend, seen: &seen}, + store.ClonedSnapshotRef{Source: "src", Snapshot: "clone-dst", Clone: "dst"}) + if err != nil { + t.Fatalf("reap: %v", err) + } + + if !seen { + t.Error("the reap listed dependents before it marked the snapshot") + } +} + +// A snapshot the reap keeps must take restores again, or the one it kept for +// a dependent is refused to the next. +func TestReapClonedSnapshotTakesTheMarkOffWhatItKeeps(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + listErr error + want error + }{ + {name: "dependent", want: store.ErrCloneSnapshotInUse}, + {name: "list-failed", listErr: errAPIServerUnavailable}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx := t.Context() + backend := store.NewInMemory() + seedOwnedCloneSnapshot(t, backend) + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "third", + Props: map[string]string{store.RestoreFromSnapshotProp: "src:clone-dst"}, + }); err != nil { + t.Fatalf("seed the restored definition: %v", err) + } + + var seen bool + + err := store.ReapClonedSnapshot(ctx, + markSeenAtListStore{Store: backend, seen: &seen, listErr: tc.listErr}, + store.ClonedSnapshotRef{Source: "src", Snapshot: "clone-dst", Clone: "dst"}) + if err == nil || (tc.want != nil && !errors.Is(err, tc.want)) { + t.Fatalf("reap = %v, want a refusal", err) + } + + snap, err := backend.Snapshots().Get(ctx, "src", "clone-dst") + if err != nil { + t.Fatalf("the kept snapshot is gone: %v", err) + } + + if mark, ok := snap.Props[store.CloneSnapshotReapingProp]; ok { + t.Errorf("the kept snapshot still carries the reaping mark %q", mark) + } + }) + } +} diff --git a/pkg/store/inmemory_resource_definition.go b/pkg/store/inmemory_resource_definition.go index 06234153..6270d03d 100644 --- a/pkg/store/inmemory_resource_definition.go +++ b/pkg/store/inmemory_resource_definition.go @@ -33,6 +33,11 @@ type inMemoryResourceDefinitions struct { m map[string]apiv1.ResourceDefinition } +// ListUncached has nothing to bypass here: this store is the API server. +func (s *inMemoryResourceDefinitions) ListUncached(ctx context.Context) ([]apiv1.ResourceDefinition, error) { + return s.List(ctx) +} + func (s *inMemoryResourceDefinitions) List(_ context.Context) ([]apiv1.ResourceDefinition, error) { s.mu.RLock() defer s.mu.RUnlock() diff --git a/pkg/store/k8s/deleting_flag_internal_test.go b/pkg/store/k8s/deleting_flag_internal_test.go index faf3d437..a5cf6972 100644 --- a/pkg/store/k8s/deleting_flag_internal_test.go +++ b/pkg/store/k8s/deleting_flag_internal_test.go @@ -169,3 +169,31 @@ func TestCrdToWireResourceSurfacesDeletingFlag(t *testing.T) { t.Fatalf("crdToWireResource surfaced DELETE on a live Resource") } } + +// A snapshot delete only stamps a DeletionTimestamp while a satellite +// finalizer holds it, and a restore that read it as live was restored from +// data being destroyed. +func TestCrdToWireSnapshotSurfacesDeletingFlag(t *testing.T) { + t.Parallel() + + now := metav1.NewTime(time.Now()) + crd := &crdv1alpha1.Snapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "src.clone-dst", + DeletionTimestamp: &now, + Finalizers: []string{"blockstor.cozystack.io/satellite-snapshot"}, + }, + Spec: crdv1alpha1.SnapshotSpec{ResourceDefinitionName: "src", SnapshotName: "clone-dst"}, + } + + if wire := crdToWireSnapshot(crd, nil); !slices.Contains(wire.Flags, apiv1.SnapshotFlagDelete) { + t.Fatalf("crdToWireSnapshot did not surface DELETE on a deleting Snapshot: flags=%v", wire.Flags) + } + + live := &crdv1alpha1.Snapshot{ + Spec: crdv1alpha1.SnapshotSpec{ResourceDefinitionName: "src", SnapshotName: "snap"}, + } + if slices.Contains(crdToWireSnapshot(live, nil).Flags, apiv1.SnapshotFlagDelete) { + t.Fatalf("crdToWireSnapshot surfaced DELETE on a live Snapshot") + } +} diff --git a/pkg/store/k8s/rd_list_uncached_test.go b/pkg/store/k8s/rd_list_uncached_test.go new file mode 100644 index 00000000..4815d0b7 --- /dev/null +++ b/pkg/store/k8s/rd_list_uncached_test.go @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: Apache-2.0 + +package k8s_test + +import ( + "context" + "testing" + + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + + crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store/k8s" +) + +// blindToDefinitions is a client whose cache has seen no definition at all. +type blindToDefinitions struct{ ctrlclient.Client } + +func (b blindToDefinitions) List(ctx context.Context, list ctrlclient.ObjectList, opts ...ctrlclient.ListOption) error { + if _, ok := list.(*crdv1alpha1.ResourceDefinitionList); ok { + return nil + } + + return b.Client.List(ctx, list, opts...) //nolint:wrapcheck // pass-through +} + +// ListUncached exists for a decision that destroys something when the answer +// comes back short, so it has to reach past a cache that trails. +func TestResourceDefinitionListUncachedReadsPastTheCache(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + ctx := t.Context() + + if err := k8s.New(fixture.client).ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: "pvc-uncached-list"}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + t.Cleanup(func() { + _ = k8s.New(fixture.client).ResourceDefinitions().Delete(context.Background(), "pvc-uncached-list") + }) + + st := k8s.NewWithAPIReader(blindToDefinitions{fixture.client}, fixture.client) + + cached, err := st.ResourceDefinitions().List(ctx) + if err != nil || len(cached) != 0 { + t.Fatalf("fixture: the cached list saw %d definition(s) (%v), want none", len(cached), err) + } + + live, err := st.ResourceDefinitions().ListUncached(ctx) + if err != nil { + t.Fatalf("ListUncached: %v", err) + } + + found := false + + for i := range live { + if live[i].Name == "pvc-uncached-list" { + found = true + } + } + + if !found { + t.Errorf("ListUncached missed a definition the API server holds: %d returned", len(live)) + } +} diff --git a/pkg/store/k8s/resource_definitions.go b/pkg/store/k8s/resource_definitions.go index 1b8a9984..2a0e742c 100644 --- a/pkg/store/k8s/resource_definitions.go +++ b/pkg/store/k8s/resource_definitions.go @@ -51,21 +51,17 @@ type resourceDefinitions struct { } func (s *resourceDefinitions) List(ctx context.Context) ([]apiv1.ResourceDefinition, error) { - var crdList crdv1alpha1.ResourceDefinitionList - - err := s.c.List(ctx, &crdList) - if err != nil { - return nil, errors.Wrap(err, "list ResourceDefinition CRDs") - } + return s.listThrough(ctx, s.c) +} - out := make([]apiv1.ResourceDefinition, 0, len(crdList.Items)) - for i := range crdList.Items { - out = append(out, crdToWireRD(&crdList.Items[i])) +// ListUncached lists through the direct reader when one is wired, and through +// the cache otherwise, where the cache is all there is. +func (s *resourceDefinitions) ListUncached(ctx context.Context) ([]apiv1.ResourceDefinition, error) { + if s.apiReader == nil { + return s.listThrough(ctx, s.c) } - sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) - - return out, nil + return s.listThrough(ctx, s.apiReader) } func (s *resourceDefinitions) Get(ctx context.Context, name string) (apiv1.ResourceDefinition, error) { @@ -307,6 +303,24 @@ func (s *resourceDefinitions) Delete(ctx context.Context, name string) error { return nil } +func (s *resourceDefinitions) listThrough(ctx context.Context, reader ctrlclient.Reader) ([]apiv1.ResourceDefinition, error) { + var crdList crdv1alpha1.ResourceDefinitionList + + err := reader.List(ctx, &crdList) + if err != nil { + return nil, errors.Wrap(err, "list ResourceDefinition CRDs") + } + + out := make([]apiv1.ResourceDefinition, 0, len(crdList.Items)) + for i := range crdList.Items { + out = append(out, crdToWireRD(&crdList.Items[i])) + } + + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + + return out, nil +} + // getForPatch reads the object a patch starts from. A cache NotFound is // re-read live, the way Get does: a caller that patches a definition it has // just created reads it at the peak of the informer's lag, and taking the diff --git a/pkg/store/k8s/snapshots.go b/pkg/store/k8s/snapshots.go index 5a040ba3..40573f21 100644 --- a/pkg/store/k8s/snapshots.go +++ b/pkg/store/k8s/snapshots.go @@ -351,6 +351,14 @@ func crdToWireSnapshot( copy(out.Flags, crd.Status.Flags) } + // A delete only stamps a DeletionTimestamp while a satellite finalizer + // holds the snapshot, and that lasts until the on-disk delete succeeds. + // Served as a plain snapshot, a restore read it as live and was restored + // from data being destroyed. Same flag resources carry for the same state, + // and like theirs it outranks every other State: the REST view stamps no + // SUCCESSFUL beside it. + out.Flags = withDeletingFlag(out.Flags, crd.DeletionTimestamp != nil) + if len(crd.Spec.VolumeDefinitions) > 0 { out.VolumeDefinitions = wireVolumeDefinitions(crd.Spec.VolumeDefinitions, vdPropsByNumber) } diff --git a/pkg/store/local_props.go b/pkg/store/local_props.go index 31065b60..948edd29 100644 --- a/pkg/store/local_props.go +++ b/pkg/store/local_props.go @@ -61,15 +61,15 @@ const RestoreAdoptedProp = "Blockstor/RestoreAdopted" // the snapshot it was restored from held, as `=` pairs. // // The snapshot is the reference a retry judges a leftover against, and it does -// not always outlive the leftover: an operator can delete it. Judged without -// it, a definition with one volume and one live replica read as finished -// whatever the snapshot held, so a clone that had restored only some of its -// volumes before the snapshot went was reported complete. The record is -// written with the marker, and it is the reference whenever the caller does -// not hand the snapshot in, whether or not the snapshot still exists: every -// rollback's spare-or-delete judgement (ReadsAsFinished) reads it. A leftover -// written before the record existed carries none and is judged as before, on -// its own volumes alone. +// not always outlive the leftover: an operator can delete it, and a clone's +// internal one is reaped. Judged without it, a definition with one volume and +// one live replica read as finished whatever the snapshot held, so a clone +// that had restored only some of its volumes before the snapshot went was +// reported complete. The record is written with the marker, and it is the +// reference whenever the caller does not hand the snapshot in, whether or not +// the snapshot still exists: every rollback's spare-or-delete judgement +// (ReadsAsFinished) reads it. A leftover written before the record existed +// carries none and is judged as before, on its own volumes alone. const RestoreVolumesProp = "Blockstor/RestoreVolumes" // WithRestoreMarker stamps a definition being restored from snap with the diff --git a/pkg/store/store.go b/pkg/store/store.go index f43d5732..173772c6 100644 --- a/pkg/store/store.go +++ b/pkg/store/store.go @@ -177,6 +177,13 @@ type ResourceGroupStore interface { // ResourceGroupStore (nil = untouched, empty = clear). type ResourceDefinitionStore interface { List(ctx context.Context) ([]apiv1.ResourceDefinition, error) + + // ListUncached answers the same question from the API server when the + // store has a direct reader, bypassing a cache that can trail a create. + // For a decision that destroys something when the answer comes back + // short, like reaping a snapshot no definition seems to use. + ListUncached(ctx context.Context) ([]apiv1.ResourceDefinition, error) + Get(ctx context.Context, name string) (apiv1.ResourceDefinition, error) Create(ctx context.Context, rd *apiv1.ResourceDefinition) error Update(ctx context.Context, rd *apiv1.ResourceDefinition) error