nexus: trim trailing nulls in omicron.public.ereport serials

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<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(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"),

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<String> = 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<String> = 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();
+    })
+}

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<Version, DataMigrationFns> {
     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
 }

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;

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, chr(0))
+    WHERE serial_number IS NOT NULL
+    AND serial_number != rtrim(serial_number, chr(0));