link fec is now an optional setting #6992
Conversation
schema/crdb/dbinit.sql
Outdated
| port_settings_id UUID, | ||
| link_name TEXT, | ||
| mtu INT4, | ||
| port_settings_id UUID NOT NULL, |
There was a problem hiding this comment.
These changes aren't strictly related to the FEC part of this PR. These fields should always have been NOT NULL, and that oversight came to light when I realized that the fec field was already nullable.
There was a problem hiding this comment.
Is it possible to split this into two PRs? Marking the "rust side" of things as Nullable is an easy change to approve, but altering these columns to be non-NULL is a more involved change
There was a problem hiding this comment.
I backed out the changes that weren't directly related to FEC, and I'll submit a follow-on PR.
| ALTER TABLE omicron.public.switch_port_settings_link_config ALTER COLUMN speed SET NOT NULL; | ||
| ALTER TABLE omicron.public.switch_port_settings_link_config ALTER COLUMN mtu SET NOT NULL; | ||
| ALTER TABLE omicron.public.switch_port_settings_link_config ALTER COLUMN port_settings_id SET NOT NULL; |
There was a problem hiding this comment.
This looks a lot like https://github.com/oxidecomputer/omicron/tree/main/schema/crdb#211-adding-a-new-column-without-a-default-value -- if there are any databases which exist, where any of these values are NULL, then we'd fail to apply the schema upgrade and the rack would be stuck.
- Are we confident that these are non-NULL across all deployments? Why?
- Could we use a default value here, in the case that there are NULL values?
There was a problem hiding this comment.
The rust code that writes and reads this data does not have these fields as Option<>. So, the only way we could find a NULL value in the database is if it was entered manually, and the subsequent read would fail as the receiving struct wouldn't be able to represent a NULL value.
| /// The forward error correction mode of the link. | ||
| pub fec: LinkFec, |
There was a problem hiding this comment.
The internal API got a nice doc update; can we do the same here?
We no longer require that FEC be specified when creating a new link, instead letting dendrite choose a default value based on the transceiver type. This PR adapts to that change in the dendrite API and allows a NULL FEC value to be persisted to the database.