Teach Nexus about snapshots #1475
Conversation
Create a "snapshot_create" saga, and fill out snapshot related code: - allow snapshots to be taken of disks attached to instances - stub out the path for taking snapshots of unattached disks - allow disks to be created from snapshots - implement related HTTP endpoint functions Note that Nexus will allow you to create multiple disks out of a single snapshot, and this currently will send multiple activations to a single downtairs set, causing a race where only one Upstairs can win. This will be addressed by future Crucible changes.
| // For a disk not attached to an instance, implementation requires | ||
| // constructing a volume and performing a snapshot through some other | ||
| // means. Currently unimplemented. | ||
| todo!(); | ||
| } |
There was a problem hiding this comment.
@jclulow this is where I imagine something like your new service plugging in - it has to construct the volume and therefore requires the volume construction request.
There was a problem hiding this comment.
Nit: can we propagate an error upwards, rather than crashing here? This is an endpoint exposed to clients; if for some horrible reason it doesn't make it into v1, we shouldn't be crashing when this happens
| // This also requires solving how to clean up the associated resources | ||
| // (on-disk snapshots, running read-only downstairs) because disks | ||
| // *could* still be using them (if the snapshot has not yet been turned | ||
| // into a regular crucible volume). It will involve some sort of | ||
| // reference counting for volumes, and probably means this needs to | ||
| // instead be a saga. |
There was a problem hiding this comment.
There's a comment in Datastore::project_delete_snapshot that's throwing me off a bit - it says:
A snapshot can be deleted in any state. It's never attached to an instance, and any disk launched from it will copy and modify the volume construction request it's based on
But this comment implies doing so is unsafe.
To be clear, the current implementation is leaking, by not reference counting / deleting volumes, correct?
Just verifying my understanding that this is a question of "when should we delete things that we can delete" (which sucks, but is okay to punt), rather than "the current implementation risks a use-after-free" (which we probably should not merge).
There was a problem hiding this comment.
Sorry yeah, that is a little confusing. "A snapshot can be deleted in any state." refers to the snapshot states creating, ready, faulted, destroyed. Simply deleting it won't affect any running instance.
The current implementation leaks on-disk snapshots and associated read-only running downstairs, yes. If I create a snapshot X out of disk A, create disk B from snapshot X, there will be three volumes in the database:
| thing | volume record |
|---|---|
| disk A | volume 1 |
| snapshot X | volume 2 |
| disk B | volume 3 |
Volume 2 will be a copy of volume 1 (with modifications). Volume 3 will include a copy of volume 2 as a read-only parent.
With this code, a delete of snapshot X will delete the snapshot record and the associated volume record (volume 2). It will not delete the on-disk snapshot or associated running read-only downstairs, which are (if it is attached to an instance) being used by volume 3.
There was a problem hiding this comment.
Got it - the TL;DR is that the resource being leaked is "volumes associated with snapshots". If we don't have one already, could we file an issue for this?
There was a problem hiding this comment.
Definitely, opened #1632
update sled agent openapi
make it "ensure" instead
Update Snapshot Create saga for new steno changes
|
Pretty sure this PR addresses all the points in #735 |
|
This should be good for a re-review now. |
There was a problem hiding this comment.
Thanks for all the hard work on this. I think there are a handful of issues we want to follow-up on, but we can certainly do those iteratively, rather than continuing to block this PR (which I imagine is already a pain to rebase at the 3500+ LoC mark).
Thanks again for all the tests!
Create a "snapshot_create" saga, and fill out snapshot related code:
Note that Nexus will allow you to create multiple disks out of a single
snapshot, and this currently will send multiple activations to a single
downtairs set, causing a race where only one Upstairs can win. This will
be addressed by future Crucible changes.