Nullable lldp_link_config_id column (oxidecomputer#6436)

oxidecomputer#6185 introduced a schema
change which added a non-nullable column `lldp_link_config_id` to the
`switch_port_settings_link_config` table.

However, on systems where `switch_port_settings_link_config` had rows,
there was no "default" value, and the schema update appears to fail.
This is currently the case on our dogfood system.

To mitigate: this PR makes the `lldp_link_config_id` column nullable, by
updating the existing schema change (for cases where it could not
complete previously) and by adding a new schema change (for cases where
the `switch_port_settings_link_config` table was empty, and the schema
change **did** previously complete).

Fixes oxidecomputer#6433

common/src/api/external/mod.rs

@@ -2371,7 +2371,7 @@ pub struct SwitchPortLinkConfig {
 
     /// The link-layer discovery protocol service configuration id for this
     /// link.
-    pub lldp_link_config_id: Uuid,
+    pub lldp_link_config_id: Option<Uuid>,
 
     /// The name of this link.
     pub link_name: String,

nexus/db-model/src/schema.rs

@@ -144,7 +144,7 @@ table! {
         fec -> crate::SwitchLinkFecEnum,
         speed -> crate::SwitchLinkSpeedEnum,
         autoneg -> Bool,
-        lldp_link_config_id -> Uuid,
+        lldp_link_config_id -> Nullable<Uuid>,
     }
 }
 

nexus/db-model/src/schema_versions.rs

@@ -17,7 +17,7 @@ use std::collections::BTreeMap;
 ///
 /// This must be updated when you change the database schema.  Refer to
 /// schema/crdb/README.adoc in the root of this repository for details.
-pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(91, 0, 0);
+pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(92, 0, 0);
 
 /// List of all past database schema versions, in *reverse* order
 ///
@@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy<Vec<KnownVersion>> = Lazy::new(|| {
         // |  leaving the first copy as an example for the next person.
         // v
         // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
+        KnownVersion::new(92, "lldp-link-config-nullable"),
         KnownVersion::new(91, "add-management-gateway-producer-kind"),
         KnownVersion::new(90, "lookup-bgp-config-by-asn"),
         KnownVersion::new(89, "collapse_lldp_settings"),

nexus/db-model/src/switch_port.rs

@@ -381,7 +381,7 @@ impl Into<external::SwitchPortConfig> for SwitchPortConfig {
 #[diesel(table_name = switch_port_settings_link_config)]
 pub struct SwitchPortLinkConfig {
     pub port_settings_id: Uuid,
-    pub lldp_link_config_id: Uuid,
+    pub lldp_link_config_id: Option<Uuid>,
     pub link_name: String,
     pub mtu: SqlU16,
     pub fec: SwitchLinkFec,
@@ -401,7 +401,7 @@ impl SwitchPortLinkConfig {
     ) -> Self {
         Self {
             port_settings_id,
-            lldp_link_config_id,
+            lldp_link_config_id: Some(lldp_link_config_id),
             link_name,
             fec,
             speed,

nexus/db-queries/src/db/datastore/switch_port.rs

@@ -455,7 +455,7 @@ impl DataStore {
                 let lldp_link_ids: Vec<Uuid> = result
                     .links
                     .iter()
-                    .map(|link| link.lldp_link_config_id)
+                    .filter_map(|link| link.lldp_link_config_id)
                     .collect();
 
                 use db::schema::lldp_link_config;
@@ -1511,7 +1511,7 @@ async fn do_switch_port_settings_delete(
     // delete lldp configs
     use db::schema::lldp_link_config;
     let lldp_link_ids: Vec<Uuid> =
-        links.iter().map(|link| link.lldp_link_config_id).collect();
+        links.iter().filter_map(|link| link.lldp_link_config_id).collect();
     diesel::delete(lldp_link_config::dsl::lldp_link_config)
         .filter(lldp_link_config::id.eq_any(lldp_link_ids))
         .execute_async(conn)

openapi/nexus.json

@@ -19335,6 +19335,7 @@
             "type": "string"
           },
           "lldp_link_config_id": {
+            "nullable": true,
             "description": "The link-layer discovery protocol service configuration id for this link.",
             "type": "string",
             "format": "uuid"
@@ -19363,7 +19364,6 @@
           "autoneg",
           "fec",
           "link_name",
-          "lldp_link_config_id",
           "mtu",
           "port_settings_id",
           "speed"

schema/crdb/collapse_lldp_settings/up2.sql

@@ -1,4 +1,4 @@
 /*
  * Add a pointer to this link's LLDP config settings.
  */
-ALTER TABLE omicron.public.switch_port_settings_link_config ADD COLUMN IF NOT EXISTS lldp_link_config_id UUID NOT NULL;
+ALTER TABLE omicron.public.switch_port_settings_link_config ADD COLUMN IF NOT EXISTS lldp_link_config_id UUID;

schema/crdb/dbinit.sql

@@ -2657,7 +2657,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.switch_port_settings_link_config (
     fec omicron.public.switch_link_fec,
     speed omicron.public.switch_link_speed,
     autoneg BOOL NOT NULL DEFAULT false,
-    lldp_link_config_id UUID NOT NULL,
+    lldp_link_config_id UUID,
 
     PRIMARY KEY (port_settings_id, link_name)
 );
@@ -4214,7 +4214,7 @@ INSERT INTO omicron.public.db_metadata (
     version,
     target_version
 ) VALUES
-    (TRUE, NOW(), NOW(), '91.0.0', NULL)
+    (TRUE, NOW(), NOW(), '92.0.0', NULL)
 ON CONFLICT DO NOTHING;
 
 COMMIT;

schema/crdb/lldp-link-config-nullable/up1.sql

@@ -0,0 +1,20 @@
+-- Refer to https://github.com/oxidecomputer/omicron/issues/6433 for the justificaiton
+-- behind this schema change.
+--
+-- In short: the "collapse_lldp_settings" schema change was edited after
+-- merging. That change included a schema change which added a non-null column
+-- to an existing table. Such a data-modifying statement is only valid for
+-- tables with no rows - however, in our test systems, we observed rows, which
+-- prevented this schema change from progressing.
+--
+-- To resolve:
+-- 1. Within the old "collapse_lldp_settings" change, we retroactively dropped the
+-- non-null constraint. For systems with populated
+-- "switch_port_settings_link_config" tables, this allows the schema update to
+-- complete without an error.
+-- 2. Within this new "lldp-link-config-nullable" change, we ALSO dropped the
+-- non-null constraint. For systems without populated
+-- "switch_port_settings_link_config" tables -- which may have been able to
+-- apply the "collapse_lldp_settings" change successfully -- this converges the state
+-- of the database to the same outcome, where the columns is nullable.
+ALTER TABLE omicron.public.switch_port_settings_link_config ALTER COLUMN lldp_link_config_id DROP NOT NULL;