Block size must be given during disk create

[jmpesp]

Block size of disks must be variable and declared during disk creation.
In other instances, it can be inherited (for example, creating a disk
from a snapshot will set the block size to whatever the snapshot's was).
Store this in the disks table.

Also, prepare for creating disks from images and snapshots by adding
the appropriate columns to the disks table.

Externally, the API should be returning names, not IDs, but this commit
doesn't tackle that. At minimum this will require Nexus to convert a
db::model::Disk into an external::Disk by looking up ids and filling in
the appropriate names (instead of a simple From impl). I'll create an
issue to track this work.

[zephraph]

Are there only two valid block_sizes or is it unconstrained?

[rmustacc]

It's an interesting question here about what we want to store in the database. In general today we only have block sizes like 512 and 4096; however, there could be others that we want to logically advertise especially as SSDs continue to increase their internal block size or we want to give VMs more flexibility here. While an integer is the simplest, maybe it's worth considering a semantic enum or otherwise figuring out how we ensure that invalid and unsupported values don't make it in. I'm not sure what our general db strategy is in that respect.

[jmpesp]

I'm also not sure what our general db strategy is here.

CRDB supports constraints, something like:

block_size INT NOT NULL CONSTRAINT block_size CHECK (block_size = 512 OR block_size = 4096),

Enums are more Rust work but possible too.

These check constraints can be changed using ALTER TABLE in the future if we plan on supporting more block sizes. If we only ever add supported block sizes, then that means adding another OR block_size = X clause:

root@:26257/omicron> alter table disk drop constraint block_size;
ALTER TABLE

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

root@:26257/omicron> alter table disk add constraint block_size check (block_size = 512 OR block_size = 4096 OR block_size = 2048);
ALTER TABLE

Time: 102ms total (execution 101ms / network 0ms)

Rolling back that schema change is problematic because rows could have made it in with block size X, however the documentation mentions that:

The CHECK constraint specifies that values for the column in INSERT or UPDATE statements must return TRUE or NULL for a Boolean expression.

so at least SELECTs will still work.

There would also have to be an additional change to Crucible to support any new block sizes.

[zephraph]

cc @davepacheco do you have thoughts?

[davepacheco]

My inclination would be to use an enum. We've done that in a bunch of places. I don't think we use check much today. check may well be just as good but on the surface it seems like an enum (with a corresponding Rust enum) would ensure only valid values could be represented in Rust as well as in SQL, while with check I think you could still easily construct invalid Rust representations.

[jmpesp]

Added an enum in a20f0eb

[zephraph]

Overall, looks good. None of my feedback/questions are blocking.

[jmpesp]

after c5d7620, this is now ready for a final review

[zephraph]

[not blocking]

Instead of carrying over these labels could we just do the byte value of the sizes in string form?

Suggested change

	CREATE TYPE omicron.public.block_size AS ENUM (
	'traditional',
	'iso',
	'advancedformat'
	);
	CREATE TYPE omicron.public.block_size AS ENUM (
	'512',
	'2048',
	'4096'
	);

The labels as are don't seem to have as much meaning in context.

[jmpesp]

nice, done in d5db0b5

[zephraph]

I think it'd be nice if we had a BlockSize entry in the schema that look something like this:

{
  "BlockSize": {
    "description": "Valid block sizes for a disk",
    "type": "integer",
    "enum": [
      512,
      2048,
      4096
    ]
  }
}

It's not something we need to do in this PR, but it'd be nice to work towards. This would generate better types for us on the client and make it easier to automatically infer client side validation. As-is now we'd have to add special handling on the client for validation which is fine for where we are now but less desirable long term.

[jmpesp]

No better time than the present :) done in ce7e8fd