-
Notifications
You must be signed in to change notification settings - Fork 48
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.
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
base: main
Are you sure you want to change the base?
Conversation
* 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.
management_ip: Some( | ||
"203.0.113.10" | ||
.parse() | ||
.expect("management_ip should be a valid address"), | ||
), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Just noting that the latest changes here work correctly with Terraform now since the API is returning the correct results.
|
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.
Related
Resolves #8640