link fec is now an optional setting #6992
Conversation
| 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
| 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?
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.