From 26d6417bfd51627e079a668706a498d3305c7334 Mon Sep 17 00:00:00 2001 From: Matthew Sanabria Date: Fri, 11 Jul 2025 09:54:03 -0400 Subject: [PATCH 1/4] nexus: update `switch_port_settings_route_config` schema I renamed the `local_pref` column to `rib_priority` to complete the rename in https://github.com/oxidecomputer/omicron/pull/6693. I also changed the type of the renamed column from `INT8` to `INT2`, clamping existing values to `0` or `255`. This was missed in https://github.com/oxidecomputer/omicron/pull/6693 and led to the following error when reading the value from the database. ``` {"msg":"request completed","v":0,"name":"omicron-dev","level":30,"time":"2025-07-11T13:57:48.670343242Z","hostname":"ms-ox01","pid":3113,"uri":"/v1/system/networking/switch-port-settings","method":"POST","req_id":"12b35f5a-2255-4244-a5aa-f9c84032fb81","remote_addr":"127.0.0.1:41766","local_addr":"127.0.0.1:12220","component":"dropshot_external","name":"e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c","error_message_external":"Internal Server Error","error_message_internal":"Unknown diesel error creating SwitchPortSettings called \"example\": Error deserializing field 'local_pref': Received more than 2 bytes while decoding an i16. Was an Integer expression accidentally marked as SmallInt?","latency_us":133766,"response_code":500} ``` The error occurred because the Rust type was `SqlU8` and the database type was `INT8`. Reads would fail because `INT8` columns could not be read into `SqlU8` types without loss of precision. This was caught in https://github.com/oxidecomputer/terraform-provider-oxide/pull/426 when implementing a Terraform provider resource for switch port settings. It's likely that this has been broken since https://github.com/oxidecomputer/omicron/pull/6693 and any user that attempted to set `rib_priority` in their Rack Setup Service (RSS) would have encountered this error. However, `rib_priority` is an uncommon configuration option and none of our customer's RSS configurations show this as being set. Given this information, it seems safe to assume that no customer has the `rib_priority` column set so the clamping logic implemented here should work well for customer installations. --- common/src/api/external/mod.rs | 3 ++- nexus/db-model/src/schema_versions.rs | 3 ++- nexus/db-model/src/switch_port.rs | 1 - nexus/db-schema/src/schema.rs | 2 +- nexus/types/src/external_api/params.rs | 4 ++-- openapi/nexus.json | 4 ++-- schema/crdb/dbinit.sql | 4 ++-- schema/crdb/route-config-rib-priority/up01.sql | 1 + schema/crdb/route-config-rib-priority/up02.sql | 9 +++++++++ schema/crdb/route-config-rib-priority/up03.sql | 1 + 10 files changed, 22 insertions(+), 10 deletions(-) create mode 100644 schema/crdb/route-config-rib-priority/up01.sql create mode 100644 schema/crdb/route-config-rib-priority/up02.sql create mode 100644 schema/crdb/route-config-rib-priority/up03.sql diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 7d415f2ba03..3ed8e980264 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -3016,7 +3016,8 @@ pub struct SwitchPortRouteConfig { /// over an 802.1Q tagged L2 segment. pub vlan_id: Option, - /// RIB Priority indicating priority within and across protocols. + /// Route RIB priority. Higher priority indicates precedence within and across + /// protocols. pub rib_priority: Option, } diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 31ee927bd20..2d2d48dfbbc 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// 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: Version = Version::new(164, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(165, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock> = LazyLock::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(165, "route-config-rib-priority"), KnownVersion::new(164, "fix-leaked-bp-oximeter-read-policy-rows"), KnownVersion::new(163, "bp-desired-host-phase-2"), KnownVersion::new(162, "bundle-by-creation"), diff --git a/nexus/db-model/src/switch_port.rs b/nexus/db-model/src/switch_port.rs index b768df6ed7e..67b77df9376 100644 --- a/nexus/db-model/src/switch_port.rs +++ b/nexus/db-model/src/switch_port.rs @@ -609,7 +609,6 @@ pub struct SwitchPortRouteConfig { pub dst: IpNetwork, pub gw: IpNetwork, pub vid: Option, - #[diesel(column_name = local_pref)] pub rib_priority: Option, } diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 16c116c8e44..c5a6caae9e0 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -202,7 +202,7 @@ table! { dst -> Inet, gw -> Inet, vid -> Nullable, - local_pref -> Nullable, + rib_priority -> Nullable, } } diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 05e1adc30aa..5640227982f 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -2003,8 +2003,8 @@ pub struct Route { /// VLAN id the gateway is reachable over. pub vid: Option, - /// Local preference for route. Higher preference indictes precedence - /// within and across protocols. + /// Route RIB priority. Higher priority indicates precedence within and across + /// protocols. pub rib_priority: Option, } diff --git a/openapi/nexus.json b/openapi/nexus.json index 239807b84aa..0d0834cf2d1 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -22294,7 +22294,7 @@ }, "rib_priority": { "nullable": true, - "description": "Local preference for route. Higher preference indictes precedence within and across protocols.", + "description": "Route RIB priority. Higher priority indicates precedence within and across protocols.", "type": "integer", "format": "uint8", "minimum": 0 @@ -24570,7 +24570,7 @@ }, "rib_priority": { "nullable": true, - "description": "RIB Priority indicating priority within and across protocols.", + "description": "Route RIB priority. Higher priority indicates precedence within and across protocols.", "type": "integer", "format": "uint8", "minimum": 0 diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 9b90c2b8e85..1a76f8b3422 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3148,7 +3148,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.switch_port_settings_route_config ( dst INET, gw INET, vid INT4, - local_pref INT8, + rib_priority INT2, /* TODO https://github.com/oxidecomputer/omicron/issues/3013 */ PRIMARY KEY (port_settings_id, interface_name, dst, gw) @@ -6209,7 +6209,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '164.0.0', NULL) + (TRUE, NOW(), NOW(), '165.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/route-config-rib-priority/up01.sql b/schema/crdb/route-config-rib-priority/up01.sql new file mode 100644 index 00000000000..80c6078dcec --- /dev/null +++ b/schema/crdb/route-config-rib-priority/up01.sql @@ -0,0 +1 @@ +ALTER TABLE omicron.public.switch_port_settings_route_config ADD COLUMN IF NOT EXISTS rib_priority INT2; diff --git a/schema/crdb/route-config-rib-priority/up02.sql b/schema/crdb/route-config-rib-priority/up02.sql new file mode 100644 index 00000000000..bc79a842298 --- /dev/null +++ b/schema/crdb/route-config-rib-priority/up02.sql @@ -0,0 +1,9 @@ +SET LOCAL disallow_full_table_scans = off; +UPDATE omicron.public.switch_port_settings_route_config +SET rib_priority = + CASE + WHEN local_pref > 255 THEN 255 + WHEN local_pref < 0 THEN 0 + ELSE local_pref::INT2 + END; +SET LOCAL disallow_full_table_scans = on; diff --git a/schema/crdb/route-config-rib-priority/up03.sql b/schema/crdb/route-config-rib-priority/up03.sql new file mode 100644 index 00000000000..e719e0b79b9 --- /dev/null +++ b/schema/crdb/route-config-rib-priority/up03.sql @@ -0,0 +1 @@ +ALTER TABLE omicron.public.switch_port_settings_route_config DROP COLUMN IF EXISTS local_pref; From 17dda23606a05ef1351688f349aada029280e6ef Mon Sep 17 00:00:00 2001 From: Matthew Sanabria Date: Mon, 14 Jul 2025 12:46:12 -0400 Subject: [PATCH 2/4] nexus: `switch_port_settings_route_config` integration tests --- nexus/tests/integration_tests/schema.rs | 84 +++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/nexus/tests/integration_tests/schema.rs b/nexus/tests/integration_tests/schema.rs index 1d289007dfc..9278515bf47 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -28,7 +28,9 @@ use similar_asserts; use slog::Logger; use slog_error_chain::InlineErrorChain; use std::collections::BTreeMap; +use std::collections::HashMap; use std::net::IpAddr; +use std::str::FromStr; use std::sync::Mutex; use tokio::time::Duration; use tokio::time::timeout; @@ -2711,6 +2713,84 @@ fn after_164_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { }) } +const ROUTE_CONFIG_PORT_SETTINGS_ID_0: &str = + "1e700b64-79e0-4515-9771-bcc2391b6d4d"; +const ROUTE_CONFIG_PORT_SETTINGS_ID_1: &str = + "c6b015ff-1c98-474f-b9e9-dfc30546094f"; +const ROUTE_CONFIG_PORT_SETTINGS_ID_2: &str = + "8b777d9b-62a3-4c4d-b0b7-314315c2a7fc"; +const ROUTE_CONFIG_PORT_SETTINGS_ID_3: &str = + "7c675e89-74b1-45da-9577-cf75f028107a"; +const ROUTE_CONFIG_PORT_SETTINGS_ID_4: &str = + "e2413d63-9307-4918-b9c4-bce959c63042"; + +// Insert records using the `local_pref` column before it's renamed and its +// database type is changed from INT8 to INT2. The receiving Rust type is u8 +// so 2 records are outside the u8 range, 2 records are at the edge of the u8 +// range, and 1 record is within the u8 range. +fn before_165_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { + Box::pin(async move { + ctx.client + .batch_execute(&format!(" + INSERT INTO omicron.public.switch_port_settings_route_config + (port_settings_id, interface_name, dst, gw, vid, local_pref) + VALUES + ( + '{ROUTE_CONFIG_PORT_SETTINGS_ID_0}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, -1 + ), + ( + '{ROUTE_CONFIG_PORT_SETTINGS_ID_1}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 0 + ), + ( + '{ROUTE_CONFIG_PORT_SETTINGS_ID_2}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 128 + ), + ( + '{ROUTE_CONFIG_PORT_SETTINGS_ID_3}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 255 + ), + ( + '{ROUTE_CONFIG_PORT_SETTINGS_ID_4}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 256 + ); + "), + ) + .await + .expect("failed to insert pre-migration rows for 165"); + }) +} + +// Query the records using the new `rib_priority` column and assert that the +// values were correctly clamped within the u8 range. +fn after_165_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { + Box::pin(async move { + let rows = ctx + .client + .query( + "SELECT * FROM omicron.public.switch_port_settings_route_config;", + &[], + ) + .await + .expect("failed to query post-migration switch_port_settings_route_config table"); + assert_eq!(rows.len(), 5); + + let records: HashMap = HashMap::from([ + (Uuid::from_str(ROUTE_CONFIG_PORT_SETTINGS_ID_0).unwrap(), 0), + (Uuid::from_str(ROUTE_CONFIG_PORT_SETTINGS_ID_1).unwrap(), 0), + (Uuid::from_str(ROUTE_CONFIG_PORT_SETTINGS_ID_2).unwrap(), 128), + (Uuid::from_str(ROUTE_CONFIG_PORT_SETTINGS_ID_3).unwrap(), 255), + (Uuid::from_str(ROUTE_CONFIG_PORT_SETTINGS_ID_4).unwrap(), 255), + ]); + + for row in rows { + let port_settings_id = row.get::<&str, Uuid>("port_settings_id"); + let rib_priority_got = row.get::<&str, i16>("rib_priority"); + + let rib_priority_want = records + .get(&port_settings_id) + .expect("unexpected port_settings_id value when querying switch_port_settings_route_config"); + assert_eq!(rib_priority_got, *rib_priority_want); + } + }) +} + // Lazily initializes all migration checks. The combination of Rust function // pointers and async makes defining a static table fairly painful, so we're // using lazy initialization instead. @@ -2801,6 +2881,10 @@ fn get_migration_checks() -> BTreeMap { Version::new(164, 0, 0), DataMigrationFns::new().before(before_164_0_0).after(after_164_0_0), ); + map.insert( + Version::new(165, 0, 0), + DataMigrationFns::new().before(before_165_0_0).after(after_165_0_0), + ); map } From 16b853e82c40580bd605eaf60abbced8e562f49b Mon Sep 17 00:00:00 2001 From: Matthew Sanabria Date: Thu, 17 Jul 2025 16:11:50 -0400 Subject: [PATCH 3/4] schema: do not reset `disallow_full_table_scans` --- schema/crdb/route-config-rib-priority/up02.sql | 1 - 1 file changed, 1 deletion(-) diff --git a/schema/crdb/route-config-rib-priority/up02.sql b/schema/crdb/route-config-rib-priority/up02.sql index bc79a842298..2179ee13d82 100644 --- a/schema/crdb/route-config-rib-priority/up02.sql +++ b/schema/crdb/route-config-rib-priority/up02.sql @@ -6,4 +6,3 @@ SET rib_priority = WHEN local_pref < 0 THEN 0 ELSE local_pref::INT2 END; -SET LOCAL disallow_full_table_scans = on; From a8a7d9bbc9af4476a9c25cbcb76bedb46f4ada71 Mon Sep 17 00:00:00 2001 From: Matthew Sanabria Date: Thu, 17 Jul 2025 16:25:28 -0400 Subject: [PATCH 4/4] nexus: add null test in `switch_port_settings_route_config` integration tests --- nexus/tests/integration_tests/schema.rs | 48 ++++++++++++------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/nexus/tests/integration_tests/schema.rs b/nexus/tests/integration_tests/schema.rs index 9278515bf47..b2f4e03aaf0 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -2713,16 +2713,12 @@ fn after_164_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { }) } -const ROUTE_CONFIG_PORT_SETTINGS_ID_0: &str = - "1e700b64-79e0-4515-9771-bcc2391b6d4d"; -const ROUTE_CONFIG_PORT_SETTINGS_ID_1: &str = - "c6b015ff-1c98-474f-b9e9-dfc30546094f"; -const ROUTE_CONFIG_PORT_SETTINGS_ID_2: &str = - "8b777d9b-62a3-4c4d-b0b7-314315c2a7fc"; -const ROUTE_CONFIG_PORT_SETTINGS_ID_3: &str = - "7c675e89-74b1-45da-9577-cf75f028107a"; -const ROUTE_CONFIG_PORT_SETTINGS_ID_4: &str = - "e2413d63-9307-4918-b9c4-bce959c63042"; +const PORT_SETTINGS_ID_165_0: &str = "1e700b64-79e0-4515-9771-bcc2391b6d4d"; +const PORT_SETTINGS_ID_165_1: &str = "c6b015ff-1c98-474f-b9e9-dfc30546094f"; +const PORT_SETTINGS_ID_165_2: &str = "8b777d9b-62a3-4c4d-b0b7-314315c2a7fc"; +const PORT_SETTINGS_ID_165_3: &str = "7c675e89-74b1-45da-9577-cf75f028107a"; +const PORT_SETTINGS_ID_165_4: &str = "e2413d63-9307-4918-b9c4-bce959c63042"; +const PORT_SETTINGS_ID_165_5: &str = "05df929f-1596-42f4-b78f-aebb5d7028c4"; // Insert records using the `local_pref` column before it's renamed and its // database type is changed from INT8 to INT2. The receiving Rust type is u8 @@ -2736,19 +2732,22 @@ fn before_165_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { (port_settings_id, interface_name, dst, gw, vid, local_pref) VALUES ( - '{ROUTE_CONFIG_PORT_SETTINGS_ID_0}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, -1 + '{PORT_SETTINGS_ID_165_0}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, -1 ), ( - '{ROUTE_CONFIG_PORT_SETTINGS_ID_1}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 0 + '{PORT_SETTINGS_ID_165_1}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 0 ), ( - '{ROUTE_CONFIG_PORT_SETTINGS_ID_2}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 128 + '{PORT_SETTINGS_ID_165_2}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 128 ), ( - '{ROUTE_CONFIG_PORT_SETTINGS_ID_3}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 255 + '{PORT_SETTINGS_ID_165_3}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 255 ), ( - '{ROUTE_CONFIG_PORT_SETTINGS_ID_4}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 256 + '{PORT_SETTINGS_ID_165_4}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, 256 + ), + ( + '{PORT_SETTINGS_ID_165_5}', 'phy0', '0.0.0.0/0', '0.0.0.0', NULL, NULL ); "), ) @@ -2769,19 +2768,20 @@ fn after_165_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { ) .await .expect("failed to query post-migration switch_port_settings_route_config table"); - assert_eq!(rows.len(), 5); - - let records: HashMap = HashMap::from([ - (Uuid::from_str(ROUTE_CONFIG_PORT_SETTINGS_ID_0).unwrap(), 0), - (Uuid::from_str(ROUTE_CONFIG_PORT_SETTINGS_ID_1).unwrap(), 0), - (Uuid::from_str(ROUTE_CONFIG_PORT_SETTINGS_ID_2).unwrap(), 128), - (Uuid::from_str(ROUTE_CONFIG_PORT_SETTINGS_ID_3).unwrap(), 255), - (Uuid::from_str(ROUTE_CONFIG_PORT_SETTINGS_ID_4).unwrap(), 255), + assert_eq!(rows.len(), 6); + + let records: HashMap> = HashMap::from([ + (Uuid::from_str(PORT_SETTINGS_ID_165_0).unwrap(), Some(0)), + (Uuid::from_str(PORT_SETTINGS_ID_165_1).unwrap(), Some(0)), + (Uuid::from_str(PORT_SETTINGS_ID_165_2).unwrap(), Some(128)), + (Uuid::from_str(PORT_SETTINGS_ID_165_3).unwrap(), Some(255)), + (Uuid::from_str(PORT_SETTINGS_ID_165_4).unwrap(), Some(255)), + (Uuid::from_str(PORT_SETTINGS_ID_165_5).unwrap(), None), ]); for row in rows { let port_settings_id = row.get::<&str, Uuid>("port_settings_id"); - let rib_priority_got = row.get::<&str, i16>("rib_priority"); + let rib_priority_got = row.get::<&str, Option>("rib_priority"); let rib_priority_want = records .get(&port_settings_id)