From 282e6c5e561468de78dbcb19012c7e01162940fd Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 15 May 2026 11:30:18 -0700 Subject: [PATCH 1/3] wip --- nexus/db-model/src/schema_versions.rs | 3 +- .../ereport_trim_serial_trailing_nulls.rs | 165 ++++++++++++++++++ .../integration_tests/data_migrations/mod.rs | 2 + schema/crdb/dbinit.sql | 2 +- .../ereport-trim-serial-trailing-nulls/up.sql | 12 ++ 5 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 nexus/tests/integration_tests/data_migrations/ereport_trim_serial_trailing_nulls.rs create mode 100644 schema/crdb/ereport-trim-serial-trailing-nulls/up.sql diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 7aa942766e2..d4cafbc8f8f 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(257, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(258, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ pub 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(258, "ereport-trim-serial-trailing-nulls"), KnownVersion::new(257, "add-disk-adoption-requests"), KnownVersion::new(256, "bgp-unnumbered-peer-cleanup"), KnownVersion::new(255, "blueprint-add-external-networking-generation"), diff --git a/nexus/tests/integration_tests/data_migrations/ereport_trim_serial_trailing_nulls.rs b/nexus/tests/integration_tests/data_migrations/ereport_trim_serial_trailing_nulls.rs new file mode 100644 index 00000000000..f50ef3167f1 --- /dev/null +++ b/nexus/tests/integration_tests/data_migrations/ereport_trim_serial_trailing_nulls.rs @@ -0,0 +1,165 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use super::super::schema::{DataMigrationFns, MigrationContext}; +use futures::future::BoxFuture; +use pretty_assertions::assert_eq; +use uuid::Uuid; + +// Ereport restart IDs — one per test case. +const CLEAN_SERIAL: Uuid = + Uuid::from_u128(25800001_0000_0000_0000_000000000001); +const NULL_PADDED_SERIAL: Uuid = + Uuid::from_u128(25800001_0000_0000_0000_000000000002); +const NULL_SERIAL: Uuid = Uuid::from_u128(25800001_0000_0000_0000_000000000003); + +const COLLECTOR: Uuid = Uuid::from_u128(25800002_0000_0000_0000_000000000001); + +pub(crate) fn checks() -> DataMigrationFns { + DataMigrationFns::new().before(before).after(after) +} + +fn before<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { + Box::pin(async move { + // Insert ereport rows with various serial_number values: + // 1. A clean serial number with no trailing nulls. + // 2. A serial number with trailing null bytes. + // 3. A NULL serial_number (should remain NULL). + ctx.client + .batch_execute(&format!( + " + INSERT INTO omicron.public.ereport ( + restart_id, ena, time_collected, collector_id, + serial_number, report, reporter, slot_type, slot + ) VALUES + ('{CLEAN_SERIAL}', 1, now(), '{COLLECTOR}', + 'BRM42220031', + '{{}}', 'sp', 'sled', 0), + ('{NULL_PADDED_SERIAL}', 1, now(), '{COLLECTOR}', + E'2EKRJYKV\\x00\\x00\\x00', + '{{}}', 'sp', 'sled', 1), + ('{NULL_SERIAL}', 1, now(), '{COLLECTOR}', + NULL, + '{{}}', 'sp', 'sled', 2); + " + )) + .await + .expect( + "failed to insert test data for ereport-trim-serial-trailing-nulls" + ); + + // Verify that the null-padded serial number actually has the expected + // nulls in it. + let rows = ctx + .client + .query( + &format!( + "SELECT restart_id, serial_number + FROM omicron.public.ereport + WHERE restart_id IN ( + '{CLEAN_SERIAL}', + '{NULL_PADDED_SERIAL}', + '{NULL_SERIAL}' + ) + ORDER BY restart_id" + ), + &[], + ) + .await + .expect("failed to query ereport rows after migration"); + + assert_eq!(rows.len(), 3, "expected 3 test ereport rows"); + + for row in &rows { + let id: Uuid = row.get("restart_id"); + let serial: Option = row.get("serial_number"); + + match id { + id if id == CLEAN_SERIAL => { + assert_eq!(serial.as_deref(), Some("BRM42220031"),); + } + id if id == NULL_PADDED_SERIAL => { + assert_eq!( + serial.as_deref(), + Some("2EKRJYKV\0\0\0"), + "2EKRJYKV should be null-padded", + ); + } + id if id == NULL_SERIAL => { + assert_eq!(serial, None, "NULL serial_number be None"); + } + other => panic!("unexpected restart_id: {other}"), + } + } + }) +} + +fn after<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { + Box::pin(async move { + // Verify the serial numbers after the migration. + let rows = ctx + .client + .query( + &format!( + "SELECT restart_id, serial_number + FROM omicron.public.ereport + WHERE restart_id IN ( + '{CLEAN_SERIAL}', + '{NULL_PADDED_SERIAL}', + '{NULL_SERIAL}' + ) + ORDER BY restart_id" + ), + &[], + ) + .await + .expect("failed to query ereport rows after migration"); + + assert_eq!(rows.len(), 3, "expected 3 test ereport rows"); + + for row in &rows { + let id: Uuid = row.get("restart_id"); + let serial: Option = row.get("serial_number"); + + match id { + id if id == CLEAN_SERIAL => { + assert_eq!( + serial.as_deref(), + Some("BRM42220031"), + "clean serial number should be unchanged" + ); + } + id if id == NULL_PADDED_SERIAL => { + assert_eq!( + serial.as_deref(), + Some("2EKRJYKV"), + "serial with trailing NUL bytes should be trimmed" + ); + } + id if id == NULL_SERIAL => { + assert_eq!( + serial, None, + "NULL serial_number should remain NULL" + ); + } + other => panic!("unexpected restart_id: {other}"), + } + } + + // Clean up test data. + ctx.client + .batch_execute(&format!( + " + DELETE FROM omicron.public.ereport + WHERE restart_id IN ( + '{CLEAN_SERIAL}', + '{NULL_PADDED_SERIAL}', + '{NULL_SERIAL}' + ); + " + )) + .await + .unwrap(); + }) +} diff --git a/nexus/tests/integration_tests/data_migrations/mod.rs b/nexus/tests/integration_tests/data_migrations/mod.rs index d63210e7ccd..6f2b75396a1 100644 --- a/nexus/tests/integration_tests/data_migrations/mod.rs +++ b/nexus/tests/integration_tests/data_migrations/mod.rs @@ -35,6 +35,7 @@ mod delete_nexus_default_allow_firewall_rule; mod disk_types; mod drop_uninitialized_svc_enabled_not_online_state; mod ereport_everyone_gets_a_slot; +mod ereport_trim_serial_trailing_nulls; mod fix_leaked_bp_oximeter_read_policy_rows; mod fix_session_token_column_order; mod inv_clear_mupdate_override; @@ -90,6 +91,7 @@ pub(crate) fn get_migration_checks() -> BTreeMap { register!(delete_nexus_default_allow_firewall_rule); register!(drop_uninitialized_svc_enabled_not_online_state); register!(bgp_unnumbered_peer_cleanup); + register!(ereport_trim_serial_trailing_nulls); map } diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 9be0e8b1693..63bb87ee11c 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -8576,7 +8576,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '257.0.0', NULL) + (TRUE, NOW(), NOW(), '258.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql b/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql new file mode 100644 index 00000000000..9c10d83cf34 --- /dev/null +++ b/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql @@ -0,0 +1,12 @@ +-- Strip trailing NUL (\0) characters from the serial_number column in the +-- ereport table. The service processor null-pads OXV2 serial numbers. MGS +-- was stripping these trailing NULLs when collecting the inventory, but +-- not when ingesting ereports. This was fixed in +-- https://github.com/oxidecomputer/management-gateway-service#491 +-- so we can now update any previously-nully ereport serials to match. +SET LOCAL disallow_full_table_scans = off; + +UPDATE omicron.public.ereport + SET serial_number = rtrim(serial_number, '\0') + WHERE serial_number IS NOT NULL + AND serial_number != rtrim(serial_number, '\0'); From 4110abc33ee8ae2b4e84d199efa8f7d076e36b51 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 15 May 2026 11:30:30 -0700 Subject: [PATCH 2/3] wip --- schema/crdb/ereport-trim-serial-trailing-nulls/up.sql | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql b/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql index 9c10d83cf34..ee7fb117c29 100644 --- a/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql +++ b/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql @@ -8,5 +8,4 @@ SET LOCAL disallow_full_table_scans = off; UPDATE omicron.public.ereport SET serial_number = rtrim(serial_number, '\0') - WHERE serial_number IS NOT NULL - AND serial_number != rtrim(serial_number, '\0'); + WHERE serial_number IS NOT NULL; From 436a51c3f9f6a63ffa57652f8f866dc581e097e7 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 15 May 2026 11:55:28 -0700 Subject: [PATCH 3/3] chr(0) actually works --- schema/crdb/ereport-trim-serial-trailing-nulls/up.sql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql b/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql index ee7fb117c29..82fe8ace940 100644 --- a/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql +++ b/schema/crdb/ereport-trim-serial-trailing-nulls/up.sql @@ -7,5 +7,6 @@ SET LOCAL disallow_full_table_scans = off; UPDATE omicron.public.ereport - SET serial_number = rtrim(serial_number, '\0') - WHERE serial_number IS NOT NULL; + SET serial_number = rtrim(serial_number, chr(0)) + WHERE serial_number IS NOT NULL + AND serial_number != rtrim(serial_number, chr(0));