Skip to content

Perform volume and local storage delete in tasks - #11027

Open
jmpesp wants to merge 2 commits into
oxidecomputer:mainfrom
jmpesp:background_disk_delete
Open

Perform volume and local storage delete in tasks#11027
jmpesp wants to merge 2 commits into
oxidecomputer:mainfrom
jmpesp:background_disk_delete

Conversation

@jmpesp

@jmpesp jmpesp commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Disk, snapshot, and image deletes have historically been the source of a lot of problems, especially when hardware is flaky. The whole associated delete saga had to complete before any of the endpoints would return an OK message, and they would hang if the saga was in a retry loop, often until an expungement occurred that would unblock said retry loop. Even worse was if the "delete freed regions" step was the one that was blocked, as that would hang any and every delete of a volume until resolved.

In accordance with the guidance in RFD 704, we're looking at replacing problematic sagas with one or more background tasks. In particular, the volume delete saga had to go. The whole saga was comprised of nodes that only have forward actions, so it was an ideal candidate for this conversion, and sagas that are like this don't make much sense in the first place.

Now all the disk delete saga does is:

  • soft-delete the disk record
  • update the virtual provisioning accounting
  • if the disk was backed by crucible, soft-delete the associated volume

Note each of these steps could still fail due to transient database issues, but those issues are less likely than the ones caused by either physical disk related issues or software bugs (like the recent Pantry lock-up). The snapshot and image delete sagas were similarly modified: wherever the volume delete sub saga was embedded, instead soft-delete the volume and let the background task handle it.

The disk delete saga also recently grown the ability to clean up local storage backed disks, and this was also moved into a different background task. Occasionally there were issues (like if the dataset is busy error was returned) that would cause those local storage backed disk deletes to unwind as well, where a rerun would succeed.

Note that merging this commit will cause a behaviour change: today, the disk / snapshot / image delete API calls do not return until all the associated resources are cleaned up (in the case of Crucible backed disks, cleaned up as much as possible). After this commit is merged, it will be possible to delete a disk, quickly create another one, and see an INSUFFICIENT_STORAGE error, because the background task that would have cleaned up either the regions or the local storage allocation hasn't fired yet. It would be incorrect to spin in the disk / snapshot / image delete endpoints as that would lead us into the same problems we had before, so this commit simply returns when the associated saga completes. The relevant clean-up background task is activated after each (now much shorter) delete saga in an attempt to close this window.

jmpesp added 2 commits August 7, 2026 15:25
Disk, snapshot, and image deletes have historically been the source of a
lot of problems, especially when hardware is flaky. The whole associated
delete saga had to complete before any of the endpoints would return an
OK message, and they would hang if the saga was in a retry loop, often
until an expungement occurred that would unblock said retry loop. Even
worse was if the "delete freed regions" step was the one that was
blocked, as that would hang _any and every_ delete of a volume until
resolved.

In accordance with the guidance in RFD 704, we're looking at replacing
problematic sagas with one or more background tasks. In particular, the
volume delete saga _had_ to go. The whole saga was comprised of nodes
that only have forward actions, so it was an ideal candidate for this
conversion, and sagas that are like this don't make much sense in the
first place.

Now all the disk delete saga does is:

- soft-delete the disk record
- update the virtual provisioning accounting
- if the disk was backed by crucible, soft-delete the associated volume

Note each of these steps could still fail due to transient database
issues, but those issues are less likely than the ones caused by either
physical disk related issues or software bugs (like the recent Pantry
lock-up). The snapshot and image delete sagas were similarly modified:
wherever the volume delete sub saga was embedded, instead soft-delete
the volume and let the background task handle it.

The disk delete saga also recently grown the ability to clean up local
storage backed disks, and this was also moved into a different
background task. Occasionally there were issues (like if the `dataset is
busy` error was returned) that would cause those local storage backed
disk deletes to unwind as well, where a rerun would succeed.

Note that merging this commit will cause a behaviour change: today, the
disk / snapshot / image delete API calls do not return until all the
associated resources are cleaned up (in the case of Crucible backed
disks, cleaned up as much as possible). After this commit is merged, it
will be possible to delete a disk, quickly create another one, and see
an INSUFFICIENT_STORAGE error, because the background task that would
have cleaned up either the regions or the local storage allocation
hasn't fired yet. It would be incorrect to spin in the disk / snapshot /
image delete endpoints as that would lead us into the same problems we
had before, so this commit simply returns when the associated saga
completes. The relevant clean-up background task is activated after each
(now much shorter) delete saga in an attempt to close this window.
@hawkw

hawkw commented Aug 7, 2026

Copy link
Copy Markdown
Member

this CI run makes it look like we apparently have a bunch of tests that relied on disk deletes happening synchronously, and they now consistently fail, at least on CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants