Use an enum instead of TEXT when storing SwitchLocation in CRDB#9984
Use an enum instead of TEXT when storing SwitchLocation in CRDB#9984jgallagher merged 16 commits intomainfrom
TEXT when storing SwitchLocation in CRDB#9984Conversation
smklein
left a comment
smklein
left a comment
There was a problem hiding this comment.
Looks solid; avoiding an arbitrary string here makes sense.
As a nitpick: I'd be down with "switch_index" instead of "switch_loc" if we are renaming things anyway (then maybe SwitchIndex as the name of the enum), but honestly, don't care too much. I think it's clear enough from context if you think that would be a pain.
| -- This should not remove any rows. Any existing rows where `switch_location` | ||
| -- has a value other than 'switch0' or 'switch1' is unusable and would have been | ||
| -- causing runtime errors anyway. | ||
| DELETE FROM omicron.public.switch_port | ||
| WHERE switch_loc IS NULL; |
There was a problem hiding this comment.
Makes sense; this feels like a slightly safer version of what i had in #9976 anyway
| @@ -0,0 +1,2 @@ | |||
| -- DO NOT EDIT. Generated by test_migration_verification_files. | |||
| SELECT CAST(IF((SELECT true WHERE EXISTS (SELECT index_name FROM omicron.crdb_internal.table_indexes WHERE descriptor_name = 'bfd_session' AND index_name = 'lookup_bfd_session')),'true','Schema change verification failed: index lookup_bfd_session on table bfd_session does not exist') AS BOOL); | |||
There was a problem hiding this comment.
ayyyy the index backfill queries are coming in handy; happy to see this as "not another bug manifesting in some horrible way"
Ran this by @internet-diglett who suggested struct SwitchLocation {
rack: RackUuid,
slot: SwitchSlot,
}I'll rename the db columns and the new db enum in this PR, then rename the existing |
Followup from #9984 which added the database-level `DbSwitchSlot` enum, this renames our `SwitchLocation` enum to `SwitchSlot` (and renames local variables in many spots, although I'm sure I missed a few). This is a combination of "consistency between Rust types and db types" and "free up the name `SwitchLocation` which implies both a rack ID + a switch slot". This bumps the sled-agent and external API versions, but in a wire-compatible way (just a type rename).
Reasoning here is similar to #10010: this isn't really a naturally-displayable type, and in this case we were previously abusing this to stringify it both in the db (fixed by #9984) and the external API (fixed by #10014, on which this PR is based). Maybe worth noting: I expected this to be entirely futureproofing, but by doing this I found a few external API types that I'd missed in an earlier draft of #10014 (types where we stringified `SwitchSlot`s for _output_ but never parsed them as _input_). All our remaining uses of the display impl are either for logging or constructing internal error messages; I changed all of these to use `Debug`, and added a manual `Debug` implementation that matches the old `Display` implementation. This should keep the error messages and logs consistent while not leading us to the pit of sadness of assuming we can format and parse these values as strings without consideration.
2 participants
I left a bunch of
// TODO-correctnessbreadcrumbs for changing the external API to take an enum instead ofNamein various spots where it wants a switch location; I'm planning on tackling that in a followup.This changes three column names to make the migrations simpler to write:
loopback_address.switch_location->loopback_address.switch_locswitch_port.switch_location->switch_port.switch_locbfd_session.switch->bfd_session.switch_locI don't love
switch_locand would preferswitch_location, but I don't think it's possible to do that without copying all the data twice (once to something liketmp_switch_location, then drop the originalswitch_location, then copy again to a newswitch_location) since CRDB doesn't allow idempotent column renames.This is the top of the yak-stack I built for myself while working on #9832. Closes #3594. Removes one instance of diesel <-> CRDB drift (
switch_port.switch_location).