Add disposition to blueprint disks #7169
Conversation
This is the second PR related to fixing #7098. BlueprintPhysicalDisksConfig is no longer an alias of OmicronPhysicalDisksConfig, but instead wraps a new type BlueprintPhysicalDiskConfig which itself wraps an OmicronPhysicalDiskConfig and a BlueprintPhysicalDiskDisposition. This change brings blueprint physical disks in line with blueprint datasets and zones such that expungement is a first class notion in the blueprint, and not implicit in the disk not being present in a blueprint. This change only adds the new types and makes them work with the existing code. The underlying logic around expungement and decommissioning has not changed. That will come in a follow up PR.
| @@ -0,0 +1,7 @@ | |||
| ALTER TABLE omicron.public.bp_omicron_physical_disk | |||
| ADD COLUMN IF NOT EXISTS disposition omicron.public.bp_physical_disk_disposition | |||
There was a problem hiding this comment.
I think the schema migration test failure is legit: IIRC this puts the column at the end of the table; you'll have to do the same in dbinit.sql to get it passing. (Unless there's a way to put a column in the middle? I don't think there is but would love to be wrong.)
There was a problem hiding this comment.
Yep. Fixed in the latest push.
| }) | ||
| .collect(); | ||
| sled_info.request.disks = OmicronPhysicalDisksConfig { | ||
| sled_info.request.disks = BlueprintPhysicalDisksConfig { | ||
| generation: Generation::new(), |
There was a problem hiding this comment.
I realize this didn't change on this PR, but in the context of #7162 (comment) should this be Generation::new().next()?
nexus/types/src/deployment.rs
Outdated
| #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq)] | ||
| pub struct BlueprintPhysicalDiskConfig { | ||
| pub disposition: BlueprintPhysicalDiskDisposition, | ||
| pub config: OmicronPhysicalDiskConfig, |
There was a problem hiding this comment.
With apologies for a potentially-really-annoying suggestion: since we have to impl From<BlueprintPhysicalDiskConfig> for OmicronPhysicalDiskConfig anyway, should we inline all the fields of OmicronPhysicalDiskConfig here instead of embedding the whole struct as-is? IIRC we did that for zones, and it would avoid a lot of the config.config.fieldname stuttering that's now required in a bunch of places.
There was a problem hiding this comment.
Yes, this is annoying both in the code as it exists and thinking about changing it lol. I usually like nesting structures, but this did become a bit ridiculous at the call sites. I'll change it as you suggest.
There was a problem hiding this comment.
Done in 3f52153
| @@ -18,6 +18,68 @@ | |||
| } | |||
| }, | |||
| "definitions": { | |||
| "BlueprintPhysicalDiskConfig": { | |||
There was a problem hiding this comment.
Do we need to bump to RSS service plan v6? This change isn't backwards compatible 😬
There was a problem hiding this comment.
Damnit, I did consider this, but not closely enough. I was thinking this was one of the DeployStepVersions and that changing the last one wouldn't matter. Practically speaking, I'm not sure this does matter, since the plan only gets reloaded on RSS failing, which only happens on new runs of RSS. But for good measure, I should probably make a v6.
There was a problem hiding this comment.
I'm actually not sure it makes sense to add another version. The plan is only read during RSS time and we never clean up the chain of reads. This does mean that we'll have 2 formats of v5 in the wild, but the old ones are plans that never get loaded again.
There was a problem hiding this comment.
@jgallagher and I discussed this, and then we discussed in update huddle. It turns out that the plans aren't really useful at all given that we can't actually restart RSS without a clean-slate. Therefore the current plan (no pun intended) is to get rid of them altogether. I'm going to open an issue about this shortly and then open a PR resolving it.
For now though, I'm not going to go back and add a v6.
This is the second PR related to fixing #7098.
BlueprintPhysicalDisksConfig is no longer an alias of OmicronPhysicalDisksConfig, but instead wraps a new type BlueprintPhysicalDiskConfig which itself wraps an OmicronPhysicalDiskConfig and a BlueprintPhysicalDiskDisposition. This change brings blueprint physical disks in line with blueprint datasets and zones such that expungement is a first class notion in the blueprint, and not implicit in the disk not being present in a blueprint.
This change only adds the new types and makes them work with the existing code. The underlying logic around expungement and decommissioning has not changed. That will come in a follow up PR.