Repository navigation
fix(restore): keep a restore's source, refuse orphan targets, take over stale rollback marks - #218
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
52d0763 to
2bbc42c
Compare
2bbc42c to
d9d2c29
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. A concurrent delete can still take a restore's source, deleting a snapshot that is already gone now returns 409, and nothing tests that the real rollback writers put the time on the mark.
The PR covers three problems. A restored definition keeps reading new replicas from its source snapshot while the marker stays (#197). An in-progress rollback mark can outlive a killed writer forever (#213). A restore into a name with orphan diskless replicas cascades over them (#203).
I tested on a trial merge with main at 1652387. That is the merge base, so the tree is the head. pkg/store, internal/cli and internal/controller pass, and pkg/rest fails only the known "server did not stop within 2s" shutdown tests. CI is red only on E2E lane 3 (recovery-deleting-convert). The same scenario failed on 2026-10-24 on two unrelated branches, fix/create-spawn-prop-fields and fix/apiserver-plain-localhost, so it is not this PR.
Blockers
1. A restore can still lose its source to a concurrent delete
The delete lists dependents once and the restore reads the snapshot once, so neither side sees the other. ReleaseRestoreSource (pkg/store/restore_source.go:54) lists definitions, then the caller deletes the snapshot. materializeRestoredRD reads the snapshot before it creates the marked definition and never reads it again. A delete that lands between those two steps finds no dependent, and the restore then writes a marker naming a snapshot that no longer exists. On the k8s store the list also goes through the informer cache, so it can miss a definition created just before it.
Repro: I wrapped the in-memory store so that creating a marked definition first sends DELETE /v1/resource-definitions/src/snapshots/snap to the running server, then ran a normal restore. The delete answered 200 and the restore 201. The snapshot is gone and dst carries BlockstorRestoreFromSnapshot=src:snap, which is the #197 end state. CSI may be shielded by the snapshotter, but the CLI and the auto-snapshot prune are not.
#200 already describes this gap on ReapClonedSnapshot ("no list taken here alone can see one that is between them") and closes it there. The reap marks the snapshot, lists past the cache, then deletes. The restore reads the snapshot back after it creates the definition (RestoreSourceWithdrawn). The general delete here, on REST, in the CLI and in the prune, has neither half. I'd use the same protocol for every snapshot delete. If #200 is supposed to bring the restore half, this PR still needs the delete half, and #200 has to land first.
2. Deleting a snapshot that does not exist now returns 409
The release runs before the existence check, so a missing snapshot can be refused. In handleSnapshotDelete the call is at pkg/rest/snapshots.go:1262, above the not-found path. A definition whose marker names an absent snapshot, with an unfinished restore, turns the idempotent 200 into a 409. That restore can never finish because its source is gone, so linstor-csi retries DeleteSnapshot forever. I checked it: definition dst with marker src:snap-gone, no such snapshot, and DELETE .../snapshots/snap-gone returns 409. The CLI snapshotDelete behaves the same way. The race in point 1 produces exactly this state, and so does any snapshot removed before this change.
Read the snapshot past the cache first and answer success when it is absent. Only a snapshot that exists needs holding.
3. The time on the mark is not tested where it is written
Every takeover test seeds the mark with store.MarkRollback directly. I reverted the two real writers to the old raw prop write without a time. One is markRollbackAbandoned in pkg/rest/rg_deleted_race.go. The other is setRollbackMark in pkg/store/restore_target.go, which the CLI's HoldRollback uses. All tests stayed green. In that state no real mark ever goes stale, so #213 is not fixed. A test should run a REST rollback and a CLI rollback up to the in-progress mark and check that Blockstor/RollbackSince is on it.
These mutations survived too:
- removing
RollbackSincePropfromserverOwnedDefinitionProps, the guard that stops an operator from setting the time and forcing a takeover of a live mark UnmarkRollbackleaving the time behindMarkRollbackkeeping the time on a step mark- the skip of definitions flagged DELETE in
ReleaseRestoreSource - the skip of replicas flagged DELETE in
RefuseReplicasWithoutDefinition, which the d298ee0 message promises ("one already being deleted is no obstacle")
The PR body says every new guard was reverted and its test failed. That holds for the other guards, not for these.
4. Commit d9d2c29 describes the review instead of the change
Main squashes with the commit messages, so this text lands as it is. The subject is "close the gaps review found in the last three fixes". The body talks about earlier states of the branch ("The orphan refusal moves to the store", "sits above handleSnapshotDelete's doc comment rather than under it, and the unused RestoreSourceInUse is gone"). Please fold each part into the commit it belongs to (7d5a63c, d298ee0, 44e250d), so every message describes the change itself.
Non-blocking
- The takeover depends on clocks. The CLI stamps the mark with the operator machine's time, so a clock more than 60s behind makes a live CLI rollback look stale to the server. The check in
EnterDestructiveRollbackandenterDestructiveRollbackcompares only the valuein-progress, so it also accepts a mark another rollback wrote after a takeover. Now that the mark has a time, the rollback could compare against theRollbackSinceit wrote itself, and the same inreleaseInProgress. Then clock skew would stop mattering. - Some facts in the PR body are off. The REST rollback budget is 15.6s (
detachedRollbackBudget); 21s is about the termination grace. #190 is merged, so "Stacked on #190" is stale. The conflict with #200 is bigger than two lines:git merge-treeshows four conflicting hunks ininternal/cli/snapshot.goand row 87 ofdocs/cli-parity-known-deltas.md. - The site in #205 shows
snapshot resource restorefollowed bysnapshot deleteof the same snapshot, in two places. After this change that delete is refused until the restored replica is UpToDate, so the page should say so.
d9d2c29 to
6cdd037
Compare
|
Thanks, head 6cdd037. The history is reworked into one commit per change, so no commit describes a fix to another.
18 mutations over these paths each turn a test red. The L6 cell |
2ab7129 to
af59bdd
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. The four blockers from my last review are fixed, but the new snapshot delete has a regression: deleting a snapshot that the apiserver's cache has not seen yet answers 200 and deletes nothing.
I tested on a trial merge with main at 7bb6606. The branch sits on 1652387 and main has #206 on top, with no conflicts. pkg/store, pkg/store/k8s (envtest), internal/cli and internal/controller pass. pkg/rest fails only the known "server did not stop within 2s" shutdown tests. CI on af59bdd is green.
Previous blockers
- The delete and restore race is fixed. The delete marks the snapshot, lists definitions past the cache and only then deletes. The restore reads the snapshot back once its definition exists, on REST (new and adopted targets) and in the CLI (new, prepared and leftover targets).
- A delete of a missing snapshot answers 200 again. The snapshot is read first, before any restore is judged.
- The time on the mark is now tested where it is written. Every mutation I listed last time now turns a test red: both real writers back to a raw write,
RollbackSincePropout of the server-owned list,UnmarkRollbackkeeping the time,MarkRollbackkeeping it on a step, and both DELETE-flag skips. - Commit d9d2c29 is gone and its parts live in the commits they belong to.
I mutated 22 points in total: the deleting mark, the uncached list, each read-back on both doors, the missing-snapshot order, the mark TTL, the mark leaving TravellingProps, the keep path dropping the mark, the CLI rollback budget, the prune skip, both directions of the staleness check and the step write in EnterDestructiveRollback. Every one turned at least one test red.
Blocker: a delete of a fresh snapshot answers 200 and keeps it
DeleteSnapshot finds the snapshot past the cache, then writes the deleting mark with Snapshots().Update (pkg/store/restore_source.go:109). On the k8s store Update starts with a cached Get (pkg/store/k8s/snapshots.go:204), and on the apiserver the stores get the manager's cached client (NewWithAPIReader(mgr.GetClient(), ...) in field_index.go:67). When the replica's cache has not seen the snapshot yet, Update returns ErrNotFound. DeleteSnapshot wraps it, and handleSnapshotDelete answers it as "snapshot already absent" with 200.
Nothing is deleted. Before this PR the handler called Snapshots().Delete, which is a direct DELETE and does not depend on the cache. linstor-csi takes 200 on DeleteSnapshot as final, so the snapshot and its storage stay behind with nothing pointing at them. The window is a delete soon after the create, and it is wider when the two land on different apiserver replicas.
Repro in pkg/store/k8s with envtest: a store from NewWithAPIReader whose client returns NotFound for every Snapshot Get and an empty Snapshot List, with the real client as the reader. I created the snapshot through the real client and called store.DeleteSnapshot:
DeleteSnapshot err=mark snapshot snap1 of w194src as being deleted: update Snapshot w194src/snap1: snapshot "snap1" on RD "w194src": object not found
after delete: 1 snapshot(s) remain
The mark write has to read past the cache like the rest of the protocol does. At minimum, a NotFound from it after the uncached read found the snapshot must not reach the caller as ErrNotFound. dropDeletingMark has the same shape: there a cache miss leaves the mark on for up to five minutes, and restores from that snapshot get 409 with nothing in the delete's answer saying so.
Non-blocking
- The ABA point from last time is still open.
EnterDestructiveRollbackandenterDestructiveRollbackstill compare only the valuein-progress. The two-minute margin covers normal skew, so this is mostly about an operator machine with a bad clock. - Commit de6d5b8 ends with "the two branches meet on one definition", which is about #200. That sentence will be stale in main whichever lands first.
- With #200,
git merge-treeshows conflicts in five files. On top of that,RestoreSourceWithdrawnandErrRestoreSourceWithdrawnare defined in both branches but in different files (restore_source.gohere,clone_snapshot.goin #200). Git merges that cleanly and the build then fails, so it will not show up as a conflict. - Row 93 in
docs/cli-parity-known-deltas.mdsays the behaviour is pinned bysnap-d-restore-source.shand the replay. Neither runs in CI and neither has run on a stand yet, so today only the unit tests pin it. - #205 still shows
snapshot resource restorefollowed bysnapshot deleteof the same snapshot (site/content/en/docs/cli.mdandoperations.md). After this PR that delete is refused until the restored replica is UpToDate.
|
Thanks, head 06f6775 (rebased onto main, which now has #206, #215 and #219). Blocker: the snapshot store's Non-blocking: row 93 now says the unit tests pin it and the L6/L7 cells have not run on a stand, and the sentence about #200 is out of the de6d5b8 message. The ABA point I left for now. The |
af59bdd to
06f6775
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) LGTM. A delete of a snapshot the apiserver's cache has not seen yet now deletes it. Snapshots().Update reads through the direct reader when the store has one, so the deleting mark lands and the delete no longer answers 200 "already absent" with the snapshot left behind. Taking the mark off goes through the same Update, so dropDeletingMark is covered too.
I tested at 06f6775, which already sits on the current main (ffd7b90). pkg/store, pkg/store/k8s with envtest, internal/cli and internal/controller pass. pkg/rest fails only the known "server did not stop within 2s" shutdown tests, and the PR's own tests there pass when run by name. CI on 06f6775 is green.
The new test holds the fix. With the cached Get back in Update, TestDeletingASnapshotTheCacheHasNotSeenDeletesIt fails with the same "object not found" as my repro. The branch moved onto #206, #215 and #219, so I repeated two earlier mutations as well. Dropping the read-back in materializeRestoredRD turns TestSnapshotDeleteRacingARestoreLeavesNoMarkerOnAGoneSnapshot red. Skipping releaseRestoreSource turns seven tests red in pkg/store, internal/cli, internal/controller and pkg/rest.
The switch to the direct reader adds no apiserver reads elsewhere. The only production callers of Snapshots().Update are the two mark writes in restore_source.go.
Earlier non-blocking points:
- ABA on
in-progressis unchanged. Still the bad-clock case, not blocking. - The sentence about #200 in the
ListUncachedcommit is gone. - With #200,
git merge-treestill conflicts in the same five files.RestoreSourceWithdrawnandErrRestoreSourceWithdrawnare still defined inrestore_source.gohere and inclone_snapshot.gothere, so whichever PR lands second has to drop its copy. #216, #217 and #220 merge cleanly, and this branch plus #217 builds and passes the store, CLI and controller tests. - Row 93 now says only the unit tests pin it and the stand cells have not run.
- #205 still runs
blockstor snapshot delete my-volume before-upgraderight after the restore incli.mdandoperations.md. That one is for #205.
Nit: the comment on the apiReader field (pkg/store/k8s/snapshots.go:52) still says only ListByDefinitionUncached reads through it. Update does too now.
06f6775 to
e05cc3a
Compare
|
Rebased on main after #200, head e05cc3a. #200 brought its own |
038347e to
f94b272
Compare
|
Thanks. Your approval went with the rebase onto #200, head e92b128 now. The rebase keeps one copy of Nit: the Non-blocking 5: #205 is in main, so I fixed it here. cli.md and operations.md now say the delete is refused with 409 until the restore has an UpToDate replica. |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) The rebase onto #200 itself is clean. Each shared symbol is defined once now, go build ./... and go vet pass, and the five former conflicts keep both sides. RestoreSourceWithdrawn withdraws on a gone snapshot, the DELETE flag, a live reaping mark and a live deleting mark. Both mark cases answer the same retryable 409 in storeErrorRc and withdrawRestoredRD, and TravellingProps drops both marks. One thing does not hold, and it only exists now that both PRs are on the same tree.
Blocker: the deleting mark can wipe a live reaping mark. setDeletingMark and dropDeletingMark (pkg/store/restore_source.go) write through Snapshots().Update. Update re-reads the object but then replaces the whole Spec with wireToCRDSnapshotSpec(in), so the props it writes are the ones the delete read earlier. #200 moved its own mark to PatchSnapshotProps for exactly this reason: a write that comes in between makes the update conflict, and mutate runs again on the fresh object.
The race is reachable. An operator can s d an internal clone snapshot; it goes through DeleteSnapshot like any other. Suppose that delete reads the snapshot, a reap of the same snapshot writes its mark, and then the delete writes its own. The reaping mark is gone. The same happens between dropDeletingMark's read and its write. Once the delete keeps the snapshot and takes its mark off (a transient list or delete error is enough), a restore that reads back sees no mark at all and goes on, while the reap, which has already listed dependents, deletes the snapshot under it. That is the race the reaping mark exists to close. Repro, as a test in pkg/store:
type racingSnaps struct {
SnapshotStore
inject func()
}
func (s *racingSnaps) ListByDefinitionUncached(ctx context.Context, rd string) ([]apiv1.Snapshot, error) {
out, err := s.SnapshotStore.ListByDefinitionUncached(ctx, rd)
if f := s.inject; f != nil {
s.inject = nil
f()
}
return out, err
}
type racingStore struct {
Store
snaps *racingSnaps
}
func (s *racingStore) Snapshots() SnapshotStore { return s.snaps }
func TestDeleteKeepsAReapMarkWrittenInBetween(t *testing.T) {
inner := NewInMemory()
seedRestoreSource(t, inner, "src", "snap")
seedRestoredFrom(t, inner, "dst", "src:snap") // unfinished: the delete keeps the snapshot
reap := newReapingMark("clone1", time.Now())
st := &racingStore{Store: inner, snaps: &racingSnaps{SnapshotStore: inner.Snapshots()}}
st.snaps.inject = func() { _ = setReapingMark(t.Context(), inner, "src", "snap", reap) }
_ = DeleteSnapshot(t.Context(), st, "src", "snap")
snaps, _ := inner.Snapshots().ListByDefinitionUncached(t.Context(), "src")
if snaps[0].Props[CloneSnapshotReapingProp] != reap {
t.Fatalf("reap mark lost: props=%v", snaps[0].Props)
}
}On e92b128 it fails with reap mark lost: props=map[]. When both helpers write through PatchSnapshotProps (set the mark; take it off only while it is still this delete's), the test passes, and so do the rest of pkg/store. The apiReader read in Update can then stay or go; nothing in this PR would need it any more.
Everything else I checked holds. The head e92b128 sits on current main 536e935, so I tested it as is. pkg/store, pkg/store/k8s with envtest, internal/cli and internal/controller pass. pkg/rest fails only the known "server did not stop within 2s" tests, and those pass when run by name. golangci-lint with the repo config is clean on the changes. I broke each piece of both PRs on the head in turn, and every break turns tests red: the read-back in materializeRestoredRD, the deleting-mark and reaping-mark checks in RestoreSourceWithdrawn, #200's other-reap check in setReapingMark, the direct read in Update, the 409 for ErrRestoreSourceDeleting in storeErrorRc and in withdrawRestoredRD, SnapshotDeletingProp in TravellingProps, releaseRestoreSource, and the ErrNotFound wrap on a deleted snapshot. The apiReader comment is fixed.
Non-blocking:
- No test covers the stored-spelling lookup that
uncachedSnapshotgained in the rebase. DropsrcRD = rd.Nameand nothing goes red, because the in-memoryListByDefinitionUncachedfolds case and the k8s one does not. An envtest case inpkg/store/k8swith a differently cased name would pin it. - The new docs sentence in
cli.mdandoperations.mdsays "refused with 409" right aboveblockstor snapshot delete. That command callsstore.DeleteSnapshotdirectly (internal/cli/write_more.go:612) and fails with "a restore from this snapshot is not finished". It never sees an HTTP status. The 409 is whatlinstor s dgets. - CI: attempt 1 failed in
TestGroupFRAutoPlace3WithTieBreaker, lane 1recovery-auto-place-real-drbd, and lane 6, where the stand never became ready. The integration test failed the same way on #224's head the same evening. Neither failure touches the restore or snapshot-delete paths. Attempt 2 is still running. - Sibling PRs: merging with #222 conflicts only on adjacent rows in
tests/e2e/cli-matrix/README.md. #217, #224, #225 and #226 merge cleanly.
e92b128 to
d58b169
Compare
|
Thanks, head d58b169, rebased on main with #222 and #224 (the cli-matrix README keeps both rows). Blocker:
Non-blocking 1: Non-blocking 2: cli.md and operations.md say the blockstor CLI fails with |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) The reap-mark race is fixed, and the fix is pinned. With the old Update helpers put back on d58b169, both cases of TestDeleteKeepsAReapMarkWrittenInBetween fail with reap mark lost: props=map[]. With only setDeletingMark put back, the "before the mark is set" case fails. The other production writers of snapshot props are a patch (the reaping mark) or a get-modify-update that conflicts on a stale resourceVersion, and nothing outside storetest calls Snapshots().Update now. The cache-miss fix moved into PatchSnapshotProps and still holds: read through the cache there instead of apiReader, and TestDeletingASnapshotTheCacheHasNotSeenDeletesIt fails with "object not found". Removing the nil normalisation from the in-memory patch fails TestDroppingTheDeletingMarkLeavesNoProps.
The two non-blocking points are fixed too. Without srcRD = rd.Name the new envtest case fails, and the CLI error does carry the text the docs now quote.
Blocker: some commit messages describe states of this branch that are gone. Main squashes with the commit messages (#222 went in with all 24), so each one lands in main as written.
- 1af28da says "The snapshot store's update now reads through the direct reader when it has one." 0725538 reverts that, and on the head
Updateis the same as on main. What is left of 1af28da is the cache-miss test and an edit to parity row 93 that its message does not mention. - f81d24e writes a comment that lists
Updateas an uncached reader. 0725538 rewrites it again. - 0725538 opens with "The snapshot delete set and dropped its mark through Update" and says "Update no longer needs to read past the cache for the mark, so it reads as it did". Both sentences are about an earlier version of this PR, not about main.
- 661ad68 says the delete "is refused with 409", and d58b169 corrects that for the CLI.
Please fold each fix into the commit that introduced what it fixes: 0725538 and the cache-miss test into b674461, the row 93 edit into 8b90cc5, d58b169 into 661ad68. Then drop what remains of 1af28da and f81d24e. Same thing as d9d2c29 in the first round.
Non-blocking:
- No test checks that a delete takes off only its own deleting mark. I removed the
props[SnapshotDeletingProp] != markcheck indropDeletingMark, andpkg/store,pkg/store/k8s,internal/cliand the snapshot and restore tests inpkg/reststill pass. The check matters when two deletes of one snapshot overlap. B writes its mark after A, then A keeps the snapshot (after a transient list error, say) and takes B's mark off. A restore that reads back in that gap sees no mark and goes on while B deletes the snapshot. A third case inTestDeleteKeepsAReapMarkWrittenInBetweenthat writes another delete's mark before write 2 and checks it survives would pin it. - The head sits on current main e49b274, so I tested it as is.
pkg/store,pkg/store/k8swith envtest,internal/cliandinternal/controllerpass, andgo vetis clean.pkg/restfails only the known "server did not stop within 2s" tests, which pass when rerun by name. On CI, Unit, Lint, Contract and Integration are green; the E2E lanes were still running when I posted this. - This PR adds no merge conflicts with #192, #199, #217, #225 or #226. #221 already conflicts with main.
A definition restored or cloned from a snapshot keeps its restore marker, and while the marker is there every new replica is brought up from that snapshot and placed on its nodes. Nothing protected the snapshot: `s d` deleted it straight away and the auto-snapshot prune deleted by age, so the definition's next replica came up blank and never reached UpToDate. A snapshot delete, on the REST door and in the CLI, now first ends the dependency: a definition whose restore is finished, by the rule an operator's own delete of the marker passes, has the marker taken off, and its later replicas sync from their peers. One whose restore is still running keeps it, and the delete is refused for as long as that lasts: a retryable 409 on the REST door, an error naming the definition in the CLI. The prune skips such a snapshot for the pass. The clone door already refused rewriting the marker through override_props. Fixes #197 Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A restore or clone into a name with no definition but with a diskless replica under it created the definition over that replica. The stamp on its node then failed, and the rollback of the definition cascaded over every replica under the name, deleting one the request had not made. Both restore doors, REST and CLI, now refuse such a name with 409 before they write anything, and name the replicas. A replica holding data is still taken as placed, the controller's tie-breaker witness is still promoted by the stamp, and a replica already being deleted is no obstacle. Fixes #203 Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A clone or restore rollback marks the definition in-progress while it runs, and every retry over that mark answers 409. An apiserver replica OOM-killed or lost between writing the mark and taking it off left it for good, and linstor-csi retried CreateVolume against the 409 until an operator cleared the prop by hand. The mark now carries the time it was written. A retry, on the REST doors and in the CLI, takes over a mark older than two minutes and resumes the leftover; a mark another retry already took off counts as taken over. Every rollback that writes the mark runs on a budget well under that: the REST door's detached one, and a one-minute budget the CLI rollback now gets as well, which had no deadline before. The mark only ever stands over a definition left whole, and a rollback still alive past its budget finds the mark gone at its next step and stands down without deleting. Fixes #213 Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A snapshot delete listed the definitions restored from the snapshot once, and a restore read the snapshot once, before it created its marked definition. A delete landing between the two found no dependent and took the snapshot, and the restore then answered 201 over a definition whose marker named a snapshot that was gone, so its next replica came up blank. The list also went through the informer cache, which can miss a definition created a moment earlier. Every operator-facing delete, on the REST door, in the CLI and in the auto-snapshot prune, now marks the snapshot first, lists the definitions past the cache, and only then deletes; a snapshot it keeps gets its mark back off. A restore, on both doors, reads the snapshot back past the cache once its marked definition stands, and withdraws when the snapshot is gone or carries a live delete mark, taking back a definition it created; the REST door answers 404 and a retryable 409 for the two. Whichever of the list and the create comes second sees the other. The mark expires, so one a dead process left stops refusing restores. The mark goes on and comes off as a patch of the props the API server holds, read past the cache. A clone's reaping mark written in between survives it, a snapshot the cache has not seen yet is marked rather than reported absent, and a delete takes off only its own mark, never another delete's. An in-memory patch that empties the props leaves none, as the API server does. A delete of a snapshot that does not exist is judged before any restore again: a definition whose marker named a missing snapshot, with a restore that could never finish, turned the idempotent 200 into a 409 that linstor-csi retried forever. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The takeover of a stale in-progress mark reads the time written next to it, and every takeover test seeded that mark by hand. Writers that dropped the time, or a time left behind when the mark was replaced or removed, passed the whole suite while no real mark could ever go stale. The REST rollback and the store calls the CLI rollback runs on now have tests that drive them to the in-progress mark and check its time, and check that the step mark replacing it and a stand-down removing it take the time with them. An operator setting the time by hand is refused like any other server-owned prop. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Both snapshot examples delete the snapshot right after restoring from it. That delete is refused until the restore has an UpToDate replica: the blockstor CLI fails with "a restore from this snapshot is not finished", and linstor snapshot delete gets a 409. Say so next to it. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
d58b169 to
1150d36
Compare
|
Thanks, head 1150d36. Folded as you asked, the tree is d58b169 plus one test.
1af28da and f81d24e are gone. I also reread the other messages: d422218 and 042feef said the delete answers 409 on both doors, now the 409 is the REST door's and the CLI gets an error. Non-blocking 1: |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) Approving. The history rewrite didn't touch the code: the diff from d58b169 to 1150d36 is the new test and nothing else, on the same base, and every commit builds and passes go vet on its own.
The commit messages are fine now. I read each of the eight against its own diff and the head, and none describes a state that is gone. 042feef fixes the cached list that d422218 leaves behind, which is a real step of the series. Row 93 lands after the race fix, so its sentence about the mark is true where it lands.
The own-mark check is pinned. With the != mark check disabled in dropDeletingMark, TestDeleteTakesOffOnlyItsOwnDeletingMark fails with deleting mark = "", want the other delete's .... The earlier mutations still fail too. With the old Update helpers put back, both cases of TestDeleteKeepsAReapMarkWrittenInBetween fail with reap mark lost. With PatchSnapshotProps reading through the cache, TestDeletingASnapshotTheCacheHasNotSeenDeletesIt fails with "object not found".
Non-blocking:
- The PR body still says the delete "answers a retryable 409" for both doors, while the commits now say the CLI gets an error. The body doesn't land in main with the squash settings, so it only matters to readers here.
pkg/store,pkg/store/k8swith envtest,internal/cliandinternal/controllerpass. Inpkg/restonly the "server did not stop within 2s" tests fail, a different set each run, and the same failure shows up on main. Unit, Lint and Contract are green on CI. Integration and E2E were still running when I posted this.- No new conflicts with the other open PRs. #192 and #221 already conflict with main on the same files.
Three fixes on the restore and rollback paths.
Restore source (#197)
A restored or cloned definition keeps its restore marker, and while it is there every new replica is brought up from that snapshot and placed on its nodes. Nothing protected the snapshot:
s ddeleted it straight away and the auto-snapshot prune deleted by age, so the definition's next replica came up blank and never reached UpToDate. A snapshot delete, on the REST door and in the CLI, now ends the dependency first: a definition whose restore is finished, by the same rule an operator's own delete of the marker passes, has the marker taken off, and its later replicas sync from their peers. One whose restore is still running keeps it, and the delete answers a retryable 409 for as long as that lasts. The prune skips such a snapshot for the pass. The clone door already refused rewriting the marker throughoverride_props.A delete and a restore also race: the delete could miss a definition the restore was about to create, and the restore answered 201 over a snapshot that was gone. Now the delete marks the snapshot, lists definitions past the cache and only then deletes, and the restore, once its definition exists, reads the snapshot back past the cache and withdraws through its rollback on a live delete mark (409, retryable) or a missing snapshot (404). Deleting a snapshot that does not exist answers as before, without a 409.
Orphan replicas under the target name (#203)
A restore or clone into a name with no definition but with a diskless replica or a witness under it created the definition over that replica, failed the stamp on its node, and the rollback cascade then deleted a replica the request had not made. Both restore doors, REST and CLI, now refuse such a name with 409 before writing anything and name the replicas. A replica holding data is still taken as placed, as #190 does, and the controller's tie-breaker witness is still promoted.
Stale rollback mark (#213)
An apiserver replica killed between writing the in-progress rollback mark and taking it off left the mark for good, and linstor-csi retried against the 409 until somebody cleared the prop. The mark now carries the time it was written, and a retry on either door takes over a mark older than two minutes, well past the rollback's 15.6-second budget, and resumes the leftover. The mark only ever stands over a definition left whole, and a rollback still alive past its budget finds the mark gone at its next step and stands down without deleting. The CLI rollback, which had no deadline, now runs on a one-minute budget of its own, so the two-minute rule holds on both doors. A mark written before this carries no time and is never taken over.
Fixes #197
Fixes #203
Fixes #213
Testing
go test ./...: green apart from the knownpkg/restshutdown flake.go test -tags=integration ./tests/integration/...: green.