Use a typed UUID for Dataset #7048
Conversation
clients/nexus-client/src/lib.rs
Outdated
| @@ -51,6 +51,7 @@ progenitor::generate_api!( | |||
| TypedUuidForDemoSagaKind = omicron_uuid_kinds::DemoSagaUuid, | |||
| TypedUuidForDownstairsKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::DownstairsKind>, | |||
| TypedUuidForPropolisKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::PropolisKind>, | |||
| TypedUuidForDatasetKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::DatasetKind>, | |||
There was a problem hiding this comment.
Absurdly tiny nit - it looks like this list was alphabetized; maybe move this up a few lines to keep that?
There was a problem hiding this comment.
right, done in 994cc4c
nexus/db-model/src/dataset.rs
Outdated
| @@ -45,7 +47,7 @@ pub struct Dataset { | |||
| ip: Option<ipv6::Ipv6Addr>, | |||
| port: Option<SqlU16>, | |||
|
|
|||
| pub kind: DatasetKind, | |||
| pub kind: DbDatasetKind, | |||
There was a problem hiding this comment.
This name collision is pretty unfortunate. Since we only use the UUID DatasetKind once in this file AFAICT, can we import that one as a different name instead? #[asset(uuid_kind = DatasetUuidKind)] or something, if the proc macro allows renamed imports?
There was a problem hiding this comment.
cool, done in 0d24cc6
| @@ -27,7 +28,7 @@ use chrono::Utc; | |||
| use diesel::prelude::*; | |||
| use diesel::upsert::excluded; | |||
| use futures::FutureExt; | |||
| use nexus_db_model::DatasetKind; | |||
| use nexus_db_model::DatasetKind as DbDatasetKind; | |||
There was a problem hiding this comment.
Is this rename necessary?
| p.found_batch(&batch, &|d: &nexus_db_model::Dataset| d.id()); | ||
| paginator = p | ||
| .found_batch(&batch, &|d: &nexus_db_model::Dataset| { | ||
| *d.id().as_untyped_uuid() |
There was a problem hiding this comment.
IIRC there's also
| *d.id().as_untyped_uuid() | |
| d.id().into_untyped_uuid() |
| @@ -1043,7 +1044,8 @@ mod test { | |||
|
|
|||
| for (dataset, region) in dataset_and_regions { | |||
| // Must be 3 unique datasets | |||
| assert!(disk_datasets.insert(dataset.id())); | |||
| let dataset_id = dataset.id(); | |||
| assert!(disk_datasets.insert(*dataset_id.as_untyped_uuid())); | |||
There was a problem hiding this comment.
| assert!(disk_datasets.insert(*dataset_id.as_untyped_uuid())); | |
| assert!(disk_datasets.insert(dataset_id.into_untyped_uuid())); |
nexus/test-utils/src/lib.rs
Outdated
| @@ -254,8 +255,10 @@ impl RackInitRequestBuilder { | |||
| let zone = self | |||
| .internal_dns_config | |||
| .host_zone( | |||
| // TODO-cleanup use TypedUuid everywhere | |||
| OmicronZoneUuid::from_untyped_uuid(dataset_id), | |||
| // XXX why is zone id being created from dataset id? | |||
There was a problem hiding this comment.
This doesn't seem right; maybe related to #7000 (comment) (cc @smklein)?
There was a problem hiding this comment.
I think this dates back to #3364 , and there isn't really a strong justification for it, other than it's deterministic for tests. We probably got away with this for a while because there's only one caller (adding CockroachDB)
Given that we're already supplying a service_name here, it probably makes sense to force the caller to also supply a OmicronZoneUuid. I'm 99% sure the (one) caller could just use OmicronZoneUuid::new_v4() and this would work.
No description provided.