From 47fa4b333aaf418c51de5f43ed843bea1d6b4239 Mon Sep 17 00:00:00 2001 From: Levon Tarver Date: Wed, 23 Jul 2025 02:16:31 +0000 Subject: [PATCH 1/3] Return all LLDP configs for each link * We allowed multiple LLDP link configs to be created on a single switch port settings object, but we would only return one when viewing the object. This has been fixed. * Note that until we support breakout connections, only one lldp configuration will be applied. This is not a regression. --- nexus/db-queries/src/db/datastore/switch_port.rs | 2 -- nexus/tests/integration_tests/switch_port.rs | 8 ++++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/switch_port.rs b/nexus/db-queries/src/db/datastore/switch_port.rs index 5ab8405bff8..c4093806eda 100644 --- a/nexus/db-queries/src/db/datastore/switch_port.rs +++ b/nexus/db-queries/src/db/datastore/switch_port.rs @@ -504,7 +504,6 @@ impl DataStore { result.link_lldp = lldp_link_config::dsl::lldp_link_config .filter(lldp_link_config::id.eq_any(lldp_link_ids)) .select(LldpLinkConfig::as_select()) - .limit(1) .load_async::(&conn) .await?; @@ -523,7 +522,6 @@ impl DataStore { let configs = tx_eq_config::dsl::tx_eq_config .filter(tx_eq_config::id.eq_any(tx_eq_ids)) .select(TxEqConfig::as_select()) - .limit(1) .load_async::(&conn) .await?; result.tx_eq = tx_eq_ids_and_nulls diff --git a/nexus/tests/integration_tests/switch_port.rs b/nexus/tests/integration_tests/switch_port.rs index a84753af9e2..d904fe20071 100644 --- a/nexus/tests/integration_tests/switch_port.rs +++ b/nexus/tests/integration_tests/switch_port.rs @@ -118,11 +118,15 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { let lldp_params = LldpLinkConfigCreate { enabled: true, link_name: Some("Link Name".into()), - link_description: Some("link_ Dscription".into()), + link_description: Some("link description".into()), chassis_id: Some("Chassis ID".into()), system_name: Some("System Name".into()), system_description: Some("System description".into()), - management_ip: None, + management_ip: Some( + "203.0.113.10" + .parse() + .expect("management_ip should be a valid address"), + ), }; // links From 2b3916794934595fd3122c08cb7ed4b03f7653a2 Mon Sep 17 00:00:00 2001 From: Levon Tarver Date: Wed, 23 Jul 2025 20:40:05 +0000 Subject: [PATCH 2/3] fix underlying type for lldp management_ip --- nexus/db-model/src/schema_versions.rs | 3 ++- schema/crdb/change-lldp-management-ip-to-inet/up01.sql | 2 ++ schema/crdb/change-lldp-management-ip-to-inet/up02.sql | 2 ++ schema/crdb/dbinit.sql | 6 +++--- 4 files changed, 9 insertions(+), 4 deletions(-) create mode 100644 schema/crdb/change-lldp-management-ip-to-inet/up01.sql create mode 100644 schema/crdb/change-lldp-management-ip-to-inet/up02.sql diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 3d54aea3ef6..cbb1faff63f 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(170, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(171, 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(171, "change-lldp-management-ip-to-inet"), KnownVersion::new(170, "add-pending-mgs-updates-rot-bootloader"), KnownVersion::new(169, "inv-ntp-timesync"), KnownVersion::new(168, "add-inv-host-phase-1-flash-hash"), diff --git a/schema/crdb/change-lldp-management-ip-to-inet/up01.sql b/schema/crdb/change-lldp-management-ip-to-inet/up01.sql new file mode 100644 index 00000000000..29776a38697 --- /dev/null +++ b/schema/crdb/change-lldp-management-ip-to-inet/up01.sql @@ -0,0 +1,2 @@ +ALTER TABLE IF EXISTS omicron.public.lldp_link_config +DROP COLUMN IF EXISTS management_ip; diff --git a/schema/crdb/change-lldp-management-ip-to-inet/up02.sql b/schema/crdb/change-lldp-management-ip-to-inet/up02.sql new file mode 100644 index 00000000000..3c7c485fb2d --- /dev/null +++ b/schema/crdb/change-lldp-management-ip-to-inet/up02.sql @@ -0,0 +1,2 @@ +ALTER TABLE IF EXISTS omicron.public.lldp_link_config +ADD COLUMN IF NOT EXISTS management_ip INET; diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index e4a08c623af..693a12b9586 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3105,10 +3105,10 @@ CREATE TABLE IF NOT EXISTS omicron.public.lldp_link_config ( chassis_id STRING(63), system_name STRING(63), system_description STRING(612), - management_ip TEXT, time_created TIMESTAMPTZ NOT NULL, time_modified TIMESTAMPTZ NOT NULL, - time_deleted TIMESTAMPTZ + time_deleted TIMESTAMPTZ, + management_ip INET ); CREATE TABLE IF NOT EXISTS omicron.public.tx_eq_config ( @@ -6295,7 +6295,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '170.0.0', NULL) + (TRUE, NOW(), NOW(), '171.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; From 7638c24538ef053806cb9956e93ef1b3caa71f44 Mon Sep 17 00:00:00 2001 From: Levon Tarver Date: Tue, 29 Jul 2025 02:58:25 +0000 Subject: [PATCH 3/3] Test for multiple LLDP configs --- nexus/tests/integration_tests/switch_port.rs | 66 ++++++++++++++++---- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/nexus/tests/integration_tests/switch_port.rs b/nexus/tests/integration_tests/switch_port.rs index d904fe20071..4c39f7cf65e 100644 --- a/nexus/tests/integration_tests/switch_port.rs +++ b/nexus/tests/integration_tests/switch_port.rs @@ -112,10 +112,10 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { description: "just a port".into(), }); - let link_name = + let link0_name = Name::from_str("phy0").expect("phy0 should be a valid name"); - let lldp_params = LldpLinkConfigCreate { + let lldp0_params = LldpLinkConfigCreate { enabled: true, link_name: Some("Link Name".into()), link_description: Some("link description".into()), @@ -129,25 +129,53 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { ), }; + let link1_name = + Name::from_str("phy1").expect("phy1 should be a valid name"); + + let lldp1_params = LldpLinkConfigCreate { + enabled: true, + link_name: Some("Link Name 2".into()), + link_description: Some("link description".into()), + chassis_id: Some("Chassis ID".into()), + system_name: Some("System Name".into()), + system_description: Some("System description".into()), + management_ip: Some( + "203.0.113.10" + .parse() + .expect("management_ip should be a valid address"), + ), + }; + // links settings.links.push(LinkConfigCreate { - link_name: link_name.clone(), + link_name: link0_name.clone(), mtu: 4700, - lldp: lldp_params.clone(), + lldp: lldp0_params.clone(), fec: Some(LinkFec::None), speed: LinkSpeed::Speed100G, autoneg: false, tx_eq: None, }); + + settings.links.push(LinkConfigCreate { + link_name: link1_name.clone(), + mtu: 4700, + lldp: lldp1_params.clone(), + fec: Some(LinkFec::None), + speed: LinkSpeed::Speed100G, + autoneg: false, + tx_eq: None, + }); + // interfaces settings.interfaces.push(SwitchInterfaceConfigCreate { - link_name: link_name.clone(), + link_name: link0_name.clone(), v6_enabled: true, kind: SwitchInterfaceKind::Primary, }); // routes settings.routes.push(RouteConfig { - link_name: link_name.clone(), + link_name: link0_name.clone(), routes: vec![Route { dst: "1.2.3.0/24".parse().unwrap(), gw: "1.2.3.4".parse().unwrap(), @@ -157,7 +185,7 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { }); // addresses settings.addresses.push(AddressConfig { - link_name: link_name.clone(), + link_name: link0_name.clone(), addresses: vec![Address { address: "203.0.113.10/24".parse().unwrap(), vlan_id: None, @@ -177,7 +205,7 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { .parsed_body() .unwrap(); - assert_eq!(created.links.len(), 1); + assert_eq!(created.links.len(), 2); assert_eq!(created.routes.len(), 1); assert_eq!(created.addresses.len(), 1); @@ -186,7 +214,14 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { assert_eq!(link0.mtu, 4700); let lldp0 = link0.lldp_link_config.clone().unwrap(); - assert_eq!(lldp0, lldp_params); + assert_eq!(lldp0, lldp0_params); + + let link1 = &created.links[1]; + assert_eq!(&link1.link_name.to_string(), "phy1"); + assert_eq!(link1.mtu, 4700); + + let lldp1 = link1.lldp_link_config.clone().unwrap(); + assert_eq!(lldp1, lldp1_params); let ifx0 = &created.interfaces[0]; assert_eq!(&ifx0.interface_name.to_string(), "phy0"); @@ -212,7 +247,7 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { .parsed_body() .unwrap(); - assert_eq!(roundtrip.links.len(), 1); + assert_eq!(roundtrip.links.len(), 2); assert_eq!(roundtrip.routes.len(), 1); assert_eq!(roundtrip.addresses.len(), 1); @@ -221,7 +256,14 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { assert_eq!(link0.mtu, 4700); let lldp0 = link0.lldp_link_config.clone().unwrap(); - assert_eq!(lldp0, lldp_params); + assert_eq!(lldp0, lldp0_params); + + let link1 = &roundtrip.links[1]; + assert_eq!(&link1.link_name.to_string(), "phy1"); + assert_eq!(link1.mtu, 4700); + + let lldp1 = link1.lldp_link_config.clone().unwrap(); + assert_eq!(lldp1, lldp1_params); let ifx0 = &roundtrip.interfaces[0]; assert_eq!(&ifx0.interface_name.to_string(), "phy0"); @@ -263,7 +305,7 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) { // Update port settings. Should not see conflict. settings.bgp_peers.push(BgpPeerConfig { - link_name: link_name.clone(), + link_name: link0_name.clone(), peers: vec![BgpPeer { bgp_config: NameOrId::Name("as47".parse().unwrap()), interface_name: "phy0".parse().unwrap(),