Implement record based Crucible reference counting #6805

jmpesp · 2024-10-09T00:29:36Z

Crucible volumes are created by layering read-write regions over a hierarchy of read-only resources. Originally only a region snapshot could be used as a read-only resource for a volume. With the introduction of read-only regions (created during the region snapshot replacement process) this is no longer true!

Read-only resources can be used by many volumes, and because of this they need to have a reference count so they can be deleted when they're not referenced anymore. The region_snapshot table uses a volume_references column, which counts how many uses there are. The region table does not have this column, and more over a simple integer works for reference counting but does not tell you what volume that use is from. This can be determined (see omdb's validate volume references command) but it's information that is tossed out, as Nexus knows what volumes use what resources! Instead, record what read-only resources a volume uses in a new table.

As part of the schema change to add the new volume_resource_usage table, a migration is included that will create the appropriate records for all region snapshots.

In testing, a few bugs were found: the worst being that read-only regions did not have their read_only column set to true. This would be a problem if read-only regions are created, but they're currently only created during region snapshot replacement. To detect if any of these regions were created, find all regions that were allocated for a snapshot volume:

SELECT id FROM region
WHERE volume_id IN (SELECT volume_id FROM snapshot);

A similar bug was found in the simulated Crucible agent.

This commit also reverts #6728, enabling region snapshot replacement again - it was disabled due to a lack of read-only region reference counting, so it can be enabled once again.

Crucible volumes are created by layering read-write regions over a hierarchy of read-only resources. Originally only a region snapshot could be used as a read-only resource for a volume. With the introduction of read-only regions (created during the region snapshot replacement process) this is no longer true! Read-only resources can be used by many volumes, and because of this they need to have a reference count so they can be deleted when they're not referenced anymore. The region_snapshot table uses a `volume_references` column, which counts how many uses there are. The region table does not have this column, and more over a simple integer works for reference counting but does not tell you _what_ volume that use is from. This can be determined (see omdb's validate volume references command) but it's information that is tossed out, as Nexus knows what volumes use what resources! Instead, record what read-only resources a volume uses in a new table. As part of the schema change to add the new `volume_resource_usage` table, a migration is included that will create the appropriate records for all region snapshots. In testing, a few bugs were found: the worst being that read-only regions did not have their read_only column set to true. This would be a problem if read-only regions are created, but they're currently only created during region snapshot replacement. To detect if any of these regions were created, find all regions that were allocated for a snapshot volume: SELECT id FROM region WHERE volume_id IN (SELECT volume_id FROM snapshot); A similar bug was found in the simulated Crucible agent. This commit also reverts oxidecomputer#6728, enabling region snapshot replacement again - it was disabled due to a lack of read-only region reference counting, so it can be enabled once again.

leftwo

I may have some more questions after I finish going through nexus/tests/integration_tests/volume_management.rs but I thought I should give you what I have so far.

nexus/db-model/src/volume_resource_usage.rs

leftwo · 2024-10-09T23:15:34Z

nexus/db-model/src/volume_resource_usage.rs

+                VolumeResourceUsage::ReadOnlyRegion {
+                    region_id: record
+                        .region_id
+                        .expect("valid read-only region usage record"),


For this and the .expects below, this message is printed when we panic, right? If so, should it be saying that we did not find a valid region usage record?

Conservatively, this should maybe be a TryFrom implementation. As it exists today, someone could modify a column in the database and cause Nexus to panic with these .expects

agreed, that's in 0a45763

nexus/db-queries/src/db/datastore/volume.rs

nexus/src/app/sagas/disk_create.rs

smklein · 2024-10-14T16:34:47Z

schema/crdb/dbinit.sql

+  'region_snapshot'
+);
+
+CREATE TABLE IF NOT EXISTS omicron.public.volume_resource_usage (


I know it exists elsewhere in this PR, but I think this table would benefit from some text explaining what it is.

you're right, done in bd2ef84

smklein · 2024-10-14T16:44:58Z

nexus/db-model/src/volume_resource_usage.rs

+    pub region_id: Option<Uuid>,
+
+    pub region_snapshot_dataset_id: Option<Uuid>,
+    pub region_snapshot_region_id: Option<Uuid>,
+    pub region_snapshot_snapshot_id: Option<Uuid>,


I had no idea reading the DB schema that these were two groups of columns, and "either one or the other is non-null".

If that's the intent -- and it seems to be, based on VolumeResourceUsage as an enum -- maybe we could add a CHECK on the table validating this?

Something like:

CONSTRAINT exactly_one_usage_source CHECK ( ( (usage_type = 'readonlyregion') AND (region_id IS NOT NULL) AND (region_snapshot_dataset_id IS NULL AND region_snaphshot_region_id IS NULL AND region_snapshot_snapshot_id IS NULL) ) OR ( (usage_type = 'regionsnapshot') AND (region_id NOT NULL) AND (region_snapshot_dataset_id IS NOT NULL AND region_snaphshot_region_id IS NOT NULL AND region_snapshot_snapshot_id IS NOT NULL) ) )

nice, done in bd2ef84

smklein · 2024-10-14T16:53:45Z

nexus/db-model/src/volume_resource_usage.rs

+                VolumeResourceUsage::ReadOnlyRegion {
+                    region_id: record
+                        .region_id
+                        .expect("valid read-only region usage record"),


Conservatively, this should maybe be a TryFrom implementation. As it exists today, someone could modify a column in the database and cause Nexus to panic with these .expects

smklein · 2024-10-14T16:54:41Z

nexus/db-queries/src/db/datastore/region.rs

@@ -264,7 +264,7 @@ impl DataStore {
                block_size,
                blocks_per_extent,
                extent_count,
-                read_only: false,
+                read_only: maybe_snapshot_id.is_some(),


Was this a bug before this PR?

Unfortunately yes

Note though that currently the only thing in Nexus that creates read-only regions is region snapshot replacement, so this wasn't a bug that was hit anywhere.

smklein · 2024-10-14T16:55:29Z

nexus/db-queries/src/db/datastore/volume.rs

-        enum VolumeCreationError {
-            #[error("Error from Volume creation: {0}")]
-            Public(Error),
+        let maybe_volume: Option<Volume> = dsl::volume


Volume has a time_deleted column -- do we not care about that here?

Not here, no - if the caller is trying to call volume_create with a volume that has the same ID as one that already exists or is soft-deleted, then we should disallow that.

smklein · 2024-10-14T17:55:52Z

nexus/db-queries/src/db/datastore/volume.rs

+                    // This function may be called with a replacement volume
+                    // that is completely blank, to be filled in later by this
+                    // function. `volume_create` will have been called but will
+                    // not have added any volume resource usage records, because
+                    // it was blank!


I'm confused about this comment, in this location - namely, I'm not sure who is calling volume_create in this situation where we're saying "volume_create will have been called". Is this something we're doing within the body of this function, and I'm not seeing it? Or is this something someone else could concurrently be doing?

No, this was bad wording! Changed in 58169fe, let me know if that reads better

smklein · 2024-10-14T17:56:30Z

nexus/db-queries/src/db/datastore/volume.rs

+                    // not have added any volume resource usage records, because
+                    // it was blank!
+                    //
+                    // The indention leaving this transaction is that the


Suggested change

// The indention leaving this transaction is that the

// The intention leaving this transaction is that the

also 58169fe

smklein · 2024-10-14T17:58:05Z

nexus/db-queries/src/db/datastore/volume.rs

+            // We don't support a pure Region VCR at the volume
+            // level in the database, so this choice should
+            // never be encountered.
+            panic!("Region not supported as a top level volume");


This seems like a footgun to me, to have a pub fn with an undocumented panic?

yeah, removed in 3849fe8

smklein · 2024-10-14T18:02:51Z

schema/crdb/crucible-ref-count-records/up08.sql

+      OVERLAY(
+       OVERLAY(
+        MD5(volume.id::TEXT || dataset_id::TEXT || region_id::TEXT || snapshot_id::TEXT || snapshot_addr || volume_references::TEXT)
+        PLACING '4' from 13


What is this doing? Why '4'? Why from 13?

This code creates a deterministic V4 UUID from the other columns, which has the shape of

xxxxxxxx-xxxx-Mxxx-Nxxx-xxxxxxxxxxxx 12345678 9111 1111 1112 222222222333 012 3456 7890 123456789012

where the first hexadecimal digit in the third group always starts with a 4 means that M = 4, and I put a "random" value (read: volume references) in for N (the variant field).

But from https://datatracker.ietf.org/doc/html/rfc9562#name-uuid-version-4:

Alternatively, an implementation MAY choose to randomly generate the exact required number of bits for random_a, random_b, and random_c (122 bits total) and then concatenate the version and variant in the required position.

I don't think I'm doing this right - position 17 shouldn't be "random":

var:
The 2-bit variant field as defined by Section 4.1, set to 0b10.

I can fix this tomorrow

done in 58169fe (plus a comment)

I am leaving a comment so that Github stops bringing me to this text box. Please ignore!

smklein · 2024-10-14T18:03:07Z

schema/crdb/crucible-ref-count-records/up08.sql

+        MD5(volume.id::TEXT || dataset_id::TEXT || region_id::TEXT || snapshot_id::TEXT || snapshot_addr || volume_references::TEXT)
+        PLACING '4' from 13
+       )
+       PLACING TO_HEX(volume_references) from 17


Why from 17?

See above comment.

The garbage collection of read/write regions must be separate from read-only regions: - read/write regions are garbage collected by either being deleted during a volume soft delete, or by appearing later during the "find deleted volume regions" section of the volume delete saga - read-only regions are garbage collected only in the volume soft delete code, when there are no more references to them `find_deleted_volume_regions` was changed to only operate on read/write regions, and no longer returns the optional RegionSnapshot object: that check was moved from the volume delete saga into the function, as it didn't make sense that it was separated. This commit also adds checks to validate that invariants related to volumes are not violated during tests. One invalid test was deleted (regions will never be deleted when they're in use!) In order to properly test the separate region deletion routines, the first part of the fixes for dealing with deleted volumes during region snapshot replacement were brought in from that branch: these are the changes to region_snapshot_replacement_step.rs and region_snapshot_replacement_start.rs.

add comment to table as well

jmpesp · 2024-10-16T22:01:52Z

@smklein @leftwo have a look at 2ff34d5, which contains a fix for the region deletion bug I've been talking about.

They can also have the use-after-free problem that region snapshots do!

pull more transaction bodies into "in_txn" functions

jmpesp · 2024-11-01T20:16:13Z

@leftwo @smklein I'm all done responding to comments, please have another look!

nexus/db-queries/src/db/datastore/region.rs

leftwo

Some questions, but I like it all

nexus/db-queries/src/db/datastore/volume.rs

leftwo · 2024-11-04T21:49:09Z

nexus/db-queries/src/db/datastore/volume.rs

-                        return Ok(VolumeReplaceResult::ExistingVolumeDeleted);
-                    };
+        if !old_region_in_vcr && new_region_in_vcr {
+            // It does seem like the replacement happened


If we have just one of: !old_region_in_vcr or new_region_in_vcr, will those cases be captured below?

You mean

if !old_region_in_vcr && new_region_in_vcr { // It does seem like the replacement happened return Ok(VolumeReplaceResult::AlreadyHappened); } else if old_region_in_vcr && !new_region_in_vcr { // Replacement hasn't happened yet } else if old_region_in_vcr && new_region_in_vcr { // XXX wat } else if !old_region_in_vcr && !new_region_in_vcr { // XXX wat }

?

Yes.

It seems like old_region_in_vcr && !new_region_in_vcr is what we expect, and the other two are things that should not happen?

Right, yeah. old_region_in_vcr && new_region_in_vcr means we performed a replacement (because new is there) but old is still there, meaning that the replacement couldn't have taken place, and !old_region_in_vcr && !new_region_in_vcr means that the thing we're being asked to replace isn't there to begin with, and a replacement hasn't taken place (because new isn't there).

Do you think I should have those cases in the code?

We chatted and I did put some comments in the code in f52fa63

leftwo · 2024-11-04T21:55:35Z

nexus/db-queries/src/db/datastore/volume.rs

-                            volume_id.0,
-                            replacement_usage,
-                        );
+    /// Replace a read-only target in a Volume with a new region


While it's true that we use "target" and "region" to mean the same thing, we should be consistent in a single sentence with the same name :)

This is kinda foreshadowing! Eventually "target" here could mean either a region snapshot or a read-only region, because I think the read-only region replacement is going to live along side the region snapshot replacement logic.

nexus/db-queries/src/db/datastore/volume.rs

…n soft-deleted volumes

smklein · 2024-11-07T19:06:09Z

schema/crdb/crucible-ref-count-records/up08.sql

+        MD5(volume.id::TEXT || dataset_id::TEXT || region_id::TEXT || snapshot_id::TEXT || snapshot_addr || volume_references::TEXT)
+        PLACING '4' from 13
+       )
+       PLACING '8' from 17


This still feels somewhat dangerous to me, and I'm not sure we have tests for the validity nor uniqueness of the UUIDs being produced by this transition.

The goal here is to produce a volume_resource_usage record for each volume INNER JOIN region_snapshot that isn't being deleted, right? Couldn't you do this by doing:

-- End goal: Find all volume/region_snapshot combos that **don't** have a volume_resource_usage record (volume INNER JOIN region_snapshot) LEFT JOIN (volume_resource_usage) ON ( volume_resource_usage.volume = volume.id AND volume_resource_usage.region_snapshot_region_id = region_snapshot.region_id )

And then write a WHERE clause to ensure that volume_resource_usage.id is NULL for these cases?

Basically:

INSERT INTO volume_resource_usage -- <New records, using normal UUID generation> FROM -- <The clause above - `volume JOIN region_snapshot`, but also `JOIN`-ing with `volume_resource_usage` to check a record doesn't already exist> WHERE volume.time_deleted IS NULL AND region_snapshot.deleting = false AND volume_resource_usage.id IS NULL

This would still be idempotent! Re-running it should be a no-op on the second iteration

This does work, see 2b511b1, thanks :) The test_migrate_.* tests confirm that it works.

jmpesp requested review from smklein and leftwo October 9, 2024 00:29

jmpesp added 2 commits October 9, 2024 00:44

Merge branch 'main' into read_only_regions_need_ref_counting_too

66c6797

full table scan required during clean slate check

410c79d

jmpesp mentioned this pull request Oct 9, 2024

Pantry crashed when processing a region with mismatched information #6353

Closed

leftwo reviewed Oct 11, 2024

View reviewed changes

smklein reviewed Oct 14, 2024

View reviewed changes

jmpesp added 7 commits October 16, 2024 21:12

be explicit in comment!

221751a

reduce denting

00d9f38

TODO be smart enough

412d665

use TryFrom instead of From, as then a database edit cannot panic Nexus

0a45763

fmt

89744ea

add CONSTRAINT to volume_resource_usage table to separate two enum uses

bd2ef84

add comment to table as well

jmpesp added 14 commits October 17, 2024 15:12

constraint typo

af6e6e5

assert read-only regions never have snapshots

47cc31e

DO NOT UNWRAP FOR THE LOVE OF ALL THAT IS HOLY

3d16f26

Implement a deleting flag for read-only regions

d4de663

They can also have the use-after-free problem that region snapshots do!

use REGION_REDUNDANCY_THRESHOLD instead of 3

1b57804

use an enum instead of bool

eae5f31

use SocketAddrV6 instead of passing around strings

0e0a639

fix bad wording

58169fe

correct 2 bit variant for v4 uuid

02ebcbe

actually correct ipv6

b8cccb2

we do not need to panic

3849fe8

no lines longer than 80!

ce56ca3

pull more transaction bodies into "in_txn" functions

Merge branch 'main' into read_only_regions_need_ref_counting_too

5a10c77

Merge branch 'main' into read_only_regions_need_ref_counting_too

f1e24f2

leftwo reviewed Nov 1, 2024

View reviewed changes

nexus/db-queries/src/db/datastore/region.rs Show resolved Hide resolved

leftwo approved these changes Nov 4, 2024

View reviewed changes

jmpesp added 3 commits November 5, 2024 21:57

expand on different old_region_in_vcr + new_region_in_vcr cases

f52fa63

Merge branch 'main' into read_only_regions_need_ref_counting_too

b020b78

retain previous find_deleted_volume_regions behaviour: only operate o…

ff590d6

…n soft-deleted volumes

smklein reviewed Nov 7, 2024

View reviewed changes

non-jank idempotent query to populate volume_resource_usage

2b511b1

smklein approved these changes Nov 12, 2024

View reviewed changes

jmpesp merged commit 7cf688c into oxidecomputer:main Nov 12, 2024
16 checks passed

jmpesp deleted the read_only_regions_need_ref_counting_too branch November 12, 2024 20:12

	// The indention leaving this transaction is that the
	// The intention leaving this transaction is that the

Implement record based Crucible reference counting #6805

Implement record based Crucible reference counting #6805

Conversation

jmpesp commented Oct 9, 2024

leftwo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmpesp commented Oct 16, 2024

jmpesp commented Nov 1, 2024

leftwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leftwo left a comment •

edited

Loading