Skip to content

nexus: update switch_port_settings_route_config schema #8587

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sudomateo
Copy link
Contributor

I renamed the local_pref column to rib_priority to complete the rename in #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 #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
oxidecomputer/terraform-provider-oxide#426 when implementing a Terraform provider resource for switch port settings. It's likely that this has been broken since #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.

I renamed the `local_pref` column to `rib_priority` to complete the
rename in #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 #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
oxidecomputer/terraform-provider-oxide#426
when implementing a Terraform provider resource for switch
port settings. It's likely that this has been broken since
#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.
@sudomateo sudomateo force-pushed the sudomateo/wuxkwlxksyvk branch from b40fcbd to 75d9f46 Compare July 12, 2025 18:11
Comment on lines +2 to +8
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;
Copy link
Contributor Author

@sudomateo sudomateo Jul 12, 2025

Choose a reason for hiding this comment

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

Should I check for the existence of the local_pref column before running the update? The failing tests pass as-is.

$ cargo nextest run -p omicron-nexus \
  integration_tests::schema::dbinit_equals_sum_of_all_up \
  integration_tests::schema::nexus_applies_update_on_boot \
  integration_tests::schema::validate_data_migration \
  integration_tests::schema::versions_have_idempotent_up
info: experimental features enabled: setup-scripts
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.59s
────────────
 Nextest run ID 203a1279-f293-4d27-9798-e7567a3f4e40 with nextest profile: default
    Starting 4 tests across 4 binaries (618 tests skipped)
             [ 00:00:00]                                                                                                                                            0/622:
       SETUP [      1/1] crdb-seed: cargo run -p crdb-seed --profile test
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.43s
     Running `target/debug/crdb-seed`
Jul 12 18:17:18.961 INFO Using existing CRDB seed tarball: `/tmp/crdb-base-sudomateo/95d6982d0e09b7c2fe1faf36c7b5468930c6e78e2198dccc411c51e5b6d0d362.tar`
  SETUP PASS [   0.510s] crdb-seed: cargo run -p crdb-seed --profile test
        PASS [  31.601s] omicron-nexus::test_all integration_tests::schema::validate_data_migration
        PASS [  34.053s] omicron-nexus::test_all integration_tests::schema::nexus_applies_update_on_boot
        PASS [  35.028s] omicron-nexus::test_all integration_tests::schema::versions_have_idempotent_up
        PASS [  46.814s] omicron-nexus::test_all integration_tests::schema::dbinit_equals_sum_of_all_up
────────────
     Summary [  47.334s] 4 tests run: 4 passed, 618 skipped

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rib_priority needs additional validation / better error handling
1 participant