Skip to content

Return all LLDP configs for each switch port settings object #8665

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
internet-diglett merged 7 commits into from
Aug 12, 2025
Merged

Return all LLDP configs for each switch port settings object #8665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
47fa4b3
Return all LLDP configs for each link
internet-diglett Jul 23, 2025
2b39167
fix underlying type for lldp management_ip
internet-diglett Jul 23, 2025
1dfe5d1
Merge branch 'main' into bugfix-8640
internet-diglett Jul 25, 2025
a75a72a
Merge branch 'main' into bugfix-8640
internet-diglett Jul 25, 2025
7638c24
Test for multiple LLDP configs
internet-diglett Jul 29, 2025
3dfb410
Merge branch 'main' into bugfix-8640
internet-diglett Aug 8, 2025
a81277f
Merge branch 'main' into bugfix-8640
internet-diglett Aug 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion nexus/db-model/src/schema_versions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(177, 0, 0);
pub const SCHEMA_VERSION: Version = Version::new(178, 0, 0);

/// List of all past database schema versions, in *reverse* order
///
Expand All @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = 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(178, "change-lldp-management-ip-to-inet"),
KnownVersion::new(177, "add-host-ereport-part-number"),
KnownVersion::new(176, "audit-log"),
KnownVersion::new(175, "inv-host-phase-1-active-slot"),
Expand Down
2 changes: 0 additions & 2 deletions nexus/db-queries/src/db/datastore/switch_port.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::<LldpLinkConfig>(&conn)
.await?;

Expand All @@ -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::<TxEqConfig>(&conn)
.await?;
result.tx_eq = tx_eq_ids_and_nulls
Expand Down
74 changes: 60 additions & 14 deletions nexus/tests/integration_tests/switch_port.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,38 +112,70 @@ 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_ 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"),
),
Comment on lines +125 to +129
Copy link
Contributor Author

@internet-diglett internet-diglett Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the management_ip to Some triggers a 500 error in my local environment:

        FAIL [   5.922s] omicron-nexus::test_all integration_tests::switch_port::test_port_settings_basic_crud
  stdout ───

    running 1 test
    test integration_tests::switch_port::test_port_settings_basic_crud ... FAILED

    failures:

    failures:
        integration_tests::switch_port::test_port_settings_basic_crud

    test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 453 filtered out; finished in 5.82s

  stderr ───
    log file: /tmp/test_all-7489d72104d534f7-test_port_settings_basic_crud.19830.0.log
    note: configured to log to "/tmp/test_all-7489d72104d534f7-test_port_settings_basic_crud.19830.0.log"
    DB URL: postgresql://root@[::1]:56635/omicron?sslmode=disable
    DB address: [::1]:56635
    log file: /tmp/test_all-7489d72104d534f7-test_port_settings_basic_crud.19830.2.log
    note: configured to log to "/tmp/test_all-7489d72104d534f7-test_port_settings_basic_crud.19830.2.log"

    thread 'integration_tests::switch_port::test_port_settings_basic_crud' panicked at nexus/tests/integration_tests/switch_port.rs:174:6:
    called `Result::unwrap()` on an `Err` value: expected status code 201 Created, found 500 Internal Server Error

Upon looking into the logs I found this message:

23:27:44.137Z INFO b324e6ed-86b6-4096-82d2-c06317d1f86e (dropshot_external): request completed
    error_message_external = Internal Server Error
    error_message_internal = Unknown diesel error creating SwitchPortSettings called "portofino": Error deserializing field 'management_ip': invalid network address f
ormat. returned type isn't a Inet
    latency_us = 64232
    local_addr = 127.0.0.1:38416
    method = POST
    remote_addr = 127.0.0.1:39650
    req_id = 62680343-d827-4d25-9cd4-7ff21655065a
    response_code = 500
    uri = /v1/system/networking/switch-port-settings

Which is quite puzzling because the contents of the field appear to be valid:

    [
       LldpLinkConfig {
           id: 686fdcad-7fe2-444f-b2e7-3e22de4c836a,
           enabled: true,
           link_name: Some(
               "Link Name",
           ),
           link_description: Some(
               "link_ Dscription",
           ),
           chassis_id: Some(
               "Chassis ID",
           ),
           system_name: Some(
               "System Name",
           ),
           system_description: Some(
               "System description",
           ),
           management_ip: Some(
               V4(
                   Ipv4Network {
                       addr: 203.0.113.10,
                       prefix: 32,
                   },
               ),
           ),
           time_created: 2025-07-23T00:58:55.447616202Z,
           time_modified: 2025-07-23T00:58:55.447616202Z,
           time_deleted: None,
       },
   ]

We also appear to be inserting structs with Option<IpNetwork> fields in other parts of our db-queries. I will look more into this tomorrow.

Copy link
Contributor

@jgallagher jgallagher Jul 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

Unknown diesel error creating SwitchPortSettings called "portofino": Error deserializing field 'management_ip': invalid network address f
ormat. returned type isn't a Inet

This is a serialization error from diesel; it's failing to deserialize the response from cockroach. This almost always means there's a mismatch between the types defined in the database and what we've told diesel the types are. I think that's the case here too; in dbinit.sql, the management_ip column is specified as TEXT:

omicron/schema/crdb/dbinit.sql

Line 3108 in f6d41ad

management_ip TEXT,

but in the diesel schema, it's specified as Inet:

omicron/nexus/db-schema/src/schema.rs

Line 163 in f6d41ad

management_ip -> Nullable<Inet>,

I think this means:

  • No query fetching a non-NULL value for this column can ever succeed
  • If you're only seeing it in your local env, that probably means we had no tests that exercise a non-NULL value for this column until this change

Copy link
Contributor

@jgallagher jgallagher Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this was top of mind because @sudomateo ran into the same thing in #8587, where #6693 had made some changes to a type in the diesel schema but not the database, but we didn't catch it in CI because there were similarly no tests for non-NULL values.

Copy link
Contributor Author

@internet-diglett internet-diglett Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgallagher Great catch, thanks for the insight!

};

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: 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: lldp_params.clone(),
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(),
Expand All @@ -153,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,
Expand All @@ -173,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);

Expand All @@ -182,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");
Expand All @@ -208,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);

Expand All @@ -217,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");
Expand Down Expand Up @@ -259,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(),
Expand Down
2 changes: 2 additions & 0 deletions schema/crdb/change-lldp-management-ip-to-inet/up01.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ALTER TABLE IF EXISTS omicron.public.lldp_link_config
DROP COLUMN IF EXISTS management_ip;
2 changes: 2 additions & 0 deletions schema/crdb/change-lldp-management-ip-to-inet/up02.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ALTER TABLE IF EXISTS omicron.public.lldp_link_config
ADD COLUMN IF NOT EXISTS management_ip INET;
6 changes: 3 additions & 3 deletions schema/crdb/dbinit.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3108,10 +3108,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 (
Expand Down Expand Up @@ -6497,7 +6497,7 @@ INSERT INTO omicron.public.db_metadata (
version,
target_version
) VALUES
(TRUE, NOW(), NOW(), '177.0.0', NULL)
(TRUE, NOW(), NOW(), '178.0.0', NULL)
ON CONFLICT DO NOTHING;

COMMIT;
Loading