Snapshot deletion returns 500 error and subsequent disk deletion also results in 500 error afterwards

[askfongjojo]

After fixes related to #824 and #3698, once I've deleted a snapshot (which did remove the snapshot but return a 500 error), subsequent disk deletion still returns this kind of errors:

root@oxz_nexus_1d4b924f-3725-4636-92fe-c5412426018b:~# grep 'InternalError' `svcs -L nexus` | looker
16:46:41.376Z INFO 1d4b924f-3725-4636-92fe-c5412426018b (dropshot_external): request completed
    error_message_external = Internal Server Error
    error_message_internal = saga ACTION error at node "no_result_2": deserialize failed: unknown variant `failed to delete_crucible_running_snapshots: InvalidRequest { message: "Not Found" }`, expected one of `ObjectNotFound`, `ObjectAlreadyExists`, `InvalidRequest`, `Unauthenticated`, `InvalidValue`, `Forbidden`, `InternalError`, `ServiceUnavailable`, `MethodNotAllowed`, `TypeVersionMismatch`, `Conflict`
    file = /home/build/.cargo/git/checkouts/dropshot-a4a923d29dccc492/7622dc0/dropshot/src/server.rs:839
    local_addr = 172.30.2.5:443
    method = DELETE
    remote_addr = 172.20.16.218:38978
    req_id = 8372cb4d-ae55-430d-8165-c63e82bc8f77
    response_code = 500
    uri = /v1/snapshots/db76c742-c2e9-45ce-b72c-5c49c3153fcf
16:46:47.938Z INFO 1d4b924f-3725-4636-92fe-c5412426018b (dropshot_external): request completed
    error_message_external = Internal Server Error
    error_message_internal = saga ACTION error at node "no_result_1": deserialize failed: unknown variant `failed to delete_crucible_regions: InvalidRequest { message: "must delete snapshots first!" }`, expected one of `ObjectNotFound`, `ObjectAlreadyExists`, `InvalidRequest`, `Unauthenticated`, `InvalidValue`, `Forbidden`, `InternalError`, `ServiceUnavailable`, `MethodNotAllowed`, `TypeVersionMismatch`, `Conflict`
    file = /home/build/.cargo/git/checkouts/dropshot-a4a923d29dccc492/7622dc0/dropshot/src/server.rs:839
    local_addr = 172.30.2.5:443
    method = DELETE
    remote_addr = 172.20.16.218:38990
    req_id = 0a966210-729d-442c-a1f2-c8ad18a8d5cc
    response_code = 500
    uri = /v1/disks/20edc1c4-d4de-499a-b508-80cb0edb5b2a
17:15:45.059Z INFO 1d4b924f-3725-4636-92fe-c5412426018b (dropshot_external): request completed
    error_message_external = Internal Server Error
    error_message_internal = saga ACTION error at node "no_result_2": deserialize failed: unknown variant `failed to delete_crucible_running_snapshots: InvalidRequest { message: "Not Found" }`, expected one of `ObjectNotFound`, `ObjectAlreadyExists`, `InvalidRequest`, `Unauthenticated`, `InvalidValue`, `Forbidden`, `InternalError`, `ServiceUnavailable`, `MethodNotAllowed`, `TypeVersionMismatch`, `Conflict`
    file = /home/build/.cargo/git/checkouts/dropshot-a4a923d29dccc492/7622dc0/dropshot/src/server.rs:839
    local_addr = 172.30.2.5:443
    method = DELETE
    remote_addr = 172.20.16.218:55358
    req_id = 9de10d6a-9986-44a4-b290-4caad0342256
    response_code = 500
    uri = https://mercury.sys.rack2.eng.oxide.computer/v1/disks/test-disk-del-500?project=test-project

Despite the errors, disks were deleted in the end (at least from a user perspective) but @augustuswm observed that virtual disk bytes provisioned did not get updated afterwards, e.g.

// Pre    => c67808de-deee-4b96-8aef-a461c759f098 | 2023-08-10 16:46:47.914542+00 | Project         |                   128849018880 |                0 |               0
// Create => c67808de-deee-4b96-8aef-a461c759f098 | 2023-08-10 17:08:46.548842+00 | Project         |                   139586437120 |                0 |               0
// Delete => c67808de-deee-4b96-8aef-a461c759f098 | 2023-08-10 17:15:45.038063+00 | Project         |                   139586437120 |                0 |               0

[augustuswm]

After triggering this trying to delete the containing project will fail due to a Nexus panic:

thread 'tokio-runtime-worker' panicked at 'Collection deleted while non-empty: VirtualProvisioningCollection { id: c67808de-deee-4b96-8aef-a461c759f098, time_modified: Some(2023-08-10T16:46:47.914542Z), collection_type: "Project", virtual_disk_bytes_provisioned: ByteCount(ByteCount(128849018880)), cpus_provisioned: 0, ram_provisioned: ByteCount(ByteCount(0)) }', /work/oxidecomputer/omicron/nexus/db-queries/src/db/datastore/virtual_provisioning_[collection.rs:147](http://collection.rs:147/):9
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

[jmpesp]

There were a few issues in the functions that the snapshot create and delete sagas call that I believe I have a fix for, tested by creating a snapshot antagonist for the omicron-stress tool. See the next comments for more info.

[jmpesp]

project_ensure_snapshot

project_ensure_snapshot performs the following block of code to insert or ensure that a snapshot exists:

diesel::insert_into(dsl::snapshot)
    .values(snapshot)
    .on_conflict((dsl::project_id, dsl::name))
    .filter_target(dsl::time_deleted.is_null())
    .do_update()
    .set(dsl::time_modified.eq(dsl::time_modified))

Unfortunately, this does not do the correct thing in the presence of a project id + name conflict: if a snapshot is created with the same project and name, but a different ID, this insert will set the time_modified column of the existing snapshot record, not insert a new one! Here is a simple reproduction case that can be done in CRDB (output below shown for v22.1.9):

# create a new table, plus a unique index on name + project

root@:26257/omicron> create table tbl (id INT8 PRIMARY KEY, name varchar, project varchar, deleted bool, modified int4);
CREATE TABLE


Time: 6ms total (execution 6ms / network 0ms)

root@:26257/omicron> create unique index on tbl (name, project) where deleted is false;
CREATE INDEX


Time: 117ms total (execution 117ms / network 0ms)

# create a resource

root@:26257/omicron> insert into tbl values (1, 'resource', 'project', false, 0);
INSERT 1


Time: 2ms total (execution 2ms / network 0ms)

# creating a resource with a different name in the same project works

root@:26257/omicron> insert into tbl values (2, 'testpost', 'project', false, 0);
INSERT 1


Time: 2ms total (execution 2ms / network 0ms)

# Creating a resource with the same name in the same project does not work:

root@:26257/omicron> insert into tbl values (3, 'resource', 'project', false, 0);
ERROR: duplicate key value violates unique constraint "tbl_name_project_key"
SQLSTATE: 23505
DETAIL: Key (name,project)=('resource','project') already exists.
CONSTRAINT: tbl_name_project_key

# if the the original resource is deleted, then the above insert of a resource with the same name will work:

root@:26257/omicron> update tbl set deleted=true where id=1;
UPDATE 1


Time: 3ms total (execution 3ms / network 0ms)

root@:26257/omicron> insert into tbl values (3, 'resource', 'project', false, 0);
INSERT 1


Time: 3ms total (execution 2ms / network 0ms)

root@:26257/omicron> select * from tbl;
  id |   name   | project | deleted | modified
-----+----------+---------+---------+-----------
   1 | resource | project |  true   |        0
   2 | testpost | project |  false  |        0
   3 | resource | project |  false  |        0
(3 rows)


Time: 1ms total (execution 1ms / network 0ms)

# reinserting that same resource doesn't work

root@:26257/omicron> insert into tbl values (3, 'resource', 'project', false, 0);
ERROR: duplicate key value violates unique constraint "tbl_pkey"
SQLSTATE: 23505
DETAIL: Key (id)=(3) already exists.
CONSTRAINT: tbl_pkey

# reinserting with a conflict check similar to the diesel code does work, and modifies the `modified` column:

root@:26257/omicron> insert into tbl values (3, 'resource', 'project', false) on conflict (name, project) where deleted = false do update set modified = 1;
INSERT 1


Time: 3ms total (execution 3ms / network 0ms)

root@:26257/omicron> select * from tbl;
  id |   name   | project | deleted | modified
-----+----------+---------+---------+-----------
   1 | resource | project |  true   |        0
   2 | testpost | project |  false  |        0
   3 | resource | project |  false  |        1
(3 rows)


Time: 1ms total (execution 1ms / network 0ms)

# here's the problem: if a resource with an id of 4 but the same project and name is used here, then it will update the modified column of the resource with id 3:

root@:26257/omicron> insert into tbl values (4, 'resource', 'project', false) on conflict (name, project) where deleted = false do update set modified = 2;
INSERT 1


Time: 3ms total (execution 3ms / network 0ms)

root@:26257/omicron> select * from tbl;
  id |   name   | project | deleted | modified
-----+----------+---------+---------+-----------
   1 | resource | project |  true   |        0
   2 | testpost | project |  false  |        0
   3 | resource | project |  false  |        2
(3 rows)


Time: 1ms total (execution 1ms / network 0ms)

project_ensure_snapshot, given two Snapshots with different IDs but the same name and project, will return Ok for the both of them. This causes conflicting snapshot_create sagas to run if the user submits many requests at the same time, and this causes problems.

The correct thing to do here would be to add a where clause after the do update set:

root@:26257/omicron> select * from tbl;
  id |   name   | project | deleted | modified
-----+----------+---------+---------+-----------
   1 | resource | project |  true   |        0
   2 | testpost | project |  false  |        0
   3 | resource | project |  false  |        2
(3 rows)


Time: 1ms total (execution 1ms / network 0ms)

root@:26257/omicron> insert into tbl values (4, 'resource', 'project', false) on conflict (name, project) where deleted = false do update set modified = 3 where tbl.id = 4;
INSERT 0


Time: 2ms total (execution 2ms / network 0ms)

root@:26257/omicron> select * from tbl;
  id |   name   | project | deleted | modified
-----+----------+---------+---------+-----------
   1 | resource | project |  true   |        0
   2 | testpost | project |  false  |        0
   3 | resource | project |  false  |        2
(3 rows)


Time: 1ms total (execution 1ms / network 0ms)

The diesel for this would look something like:

diesel::insert_into(dsl::snapshot)
    .values(snapshot)
    .on_conflict((dsl::project_id, dsl::name))
    .filter_target(dsl::time_deleted.is_null())
    .do_update()
    .set(dsl::time_modified.eq(dsl::time_modified))
    .filter(dsl::id.eq(snapshot.id())

But it doesn't seem like this is supported by the CRDB backend of diesel (I tried the code above and it wouldn't compile). It is supported by postgres.

The fix that works here is to use a transaction that checks for the existence of a snapshot with a conflicting ID, and only apply the upsert if the ID matches.

[jmpesp]

project_delete_snapshot

The code that deletes a snapshot does not do any check for the snapshot's current state. Indeed, the comment in that function says "A snapshot can be deleted in any state". This is wrong! Snapshots should not be deleted if they're in state Creating (except by the snapshot_create saga that is creating them, during unwind).

The fix for this is to pass a list of ok_to_delete_states, much like disks and instances already have, and spell out the states from which a snapshot can be deleted.

[jmpesp]

#3947 bundles fixes for the issues described above this comment.

Importantly, it is possible to do

diesel::insert_into(dsl::snapshot)
    .values(snapshot)
    .on_conflict((dsl::project_id, dsl::name))
    .filter_target(dsl::time_deleted.is_null())
    .do_update()
    .set(dsl::time_modified.eq(dsl::time_modified))
    .filter(dsl::id.eq(snapshot.id())

but the failure (snapshot exists already) isn't disambiguated from the case where the project ID doesn't exist, so the "use a transaction that checks for the existence of a snapshot with a conflicting ID" method was retained.

[jmpesp]

#3997 contains the next round of fixes, both related to concurrent snapshot delete sagas running for the same snapshot. I'm testing now with the snapshot antagonist to make sure this is the last of the issues that would cause 500s.

[jmpesp]

I've closed #3997, and will open a series of smaller PRs instead that will be easier to review.