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.

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

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

nexus: update switch_port_settings_route_config schema #8587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 common/src/api/external/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2998,7 +2998,8 @@ pub struct SwitchPortRouteConfig {
/// over an 802.1Q tagged L2 segment.
pub vlan_id: Option<u16>,

/// RIB Priority indicating priority within and across protocols.
/// Route RIB priority. Higher priority indicates precedence within and across
/// protocols.
pub rib_priority: Option<u8>,
}

Expand Down
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(161, 0, 0);
pub const SCHEMA_VERSION: Version = Version::new(162, 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(162, "route-config-rib-priority"),
KnownVersion::new(161, "inv_cockroachdb_status"),
KnownVersion::new(160, "tuf-trust-root"),
KnownVersion::new(159, "sled-config-desired-host-phase-2"),
Expand Down
1 change: 0 additions & 1 deletion nexus/db-model/src/switch_port.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,6 @@ pub struct SwitchPortRouteConfig {
pub dst: IpNetwork,
pub gw: IpNetwork,
pub vid: Option<SqlU16>,
#[diesel(column_name = local_pref)]
pub rib_priority: Option<SqlU8>,
}

Expand Down
2 changes: 1 addition & 1 deletion nexus/db-schema/src/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ table! {
dst -> Inet,
gw -> Inet,
vid -> Nullable<Int4>,
local_pref -> Nullable<Int2>,
rib_priority -> Nullable<Int2>,
}
}

Expand Down
4 changes: 2 additions & 2 deletions nexus/types/src/external_api/params.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2003,8 +2003,8 @@ pub struct Route {
/// VLAN id the gateway is reachable over.
pub vid: Option<u16>,

/// 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<u8>,
}

Expand Down
4 changes: 2 additions & 2 deletions openapi/nexus.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions schema/crdb/dbinit.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3144,7 +3144,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)
Expand Down Expand Up @@ -6198,7 +6198,7 @@ INSERT INTO omicron.public.db_metadata (
version,
target_version
) VALUES
(TRUE, NOW(), NOW(), '161.0.0', NULL)
(TRUE, NOW(), NOW(), '162.0.0', NULL)
ON CONFLICT DO NOTHING;

COMMIT;
1 change: 1 addition & 0 deletions schema/crdb/route-config-rib-priority/up01.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE omicron.public.switch_port_settings_route_config ADD COLUMN IF NOT EXISTS rib_priority INT2;
9 changes: 9 additions & 0 deletions schema/crdb/route-config-rib-priority/up02.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Comment on lines +2 to +8
Copy link
Contributor Author

@sudomateo sudomateo Jul 12, 2025 •
edited
Loading

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

Copy link
Contributor

@jgallagher jgallagher Jul 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The tests aren't super helpful with migration steps that change data instead of structure; in general they run against an empty database, so any UPDATEs are effectively no-ops and will always pass.

We do have a spot where you can check migrations that affect data, though; I just left a comment on another PR with details. It's probably straightforward to add a similar tests here - before adds rows that hit all three of these CASEs (plus NULL?), and after would confirm the clamping performed as expected?

SET LOCAL disallow_full_table_scans = on;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to turn this back on; each step of the migration is run within its own transaction.

1 change: 1 addition & 0 deletions schema/crdb/route-config-rib-priority/up03.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE omicron.public.switch_port_settings_route_config DROP COLUMN IF EXISTS local_pref;
Loading