Option 2: Retroactively modify migration 229 to shrink sessions table (#9868)

Alternate version of #9867 that does not expire all existing console
sessions.

See more detailed description in
#9866 and in the comment
in the migration file.

Author: david-crespo

nexus/tests/integration_tests/schema.rs

@@ -4129,12 +4129,19 @@ fn after_222_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
 // Migration 229 fixes column ordering in console_session and device_access_token.
 // The "id" column was added via ALTER TABLE in migration 145, placing it at the
 // end. This migration recreates the tables with "id" as the first column.
+// It also drops console sessions created more than 24 hours ago.
 const SESSION_229_ID: Uuid =
     Uuid::from_u128(0x22900001_0000_0000_0000_000000000001);
 const SESSION_229_TOKEN: &str = "tok-console-229-migration-test";
 const SESSION_229_SILO_USER: Uuid =
     Uuid::from_u128(0x22900001_0000_0000_0000_000000000002);
 
+const SESSION_229_OLD_ID: Uuid =
+    Uuid::from_u128(0x22900001_0000_0000_0000_000000000003);
+const SESSION_229_OLD_TOKEN: &str = "tok-console-229-old-session";
+const SESSION_229_OLD_SILO_USER: Uuid =
+    Uuid::from_u128(0x22900001_0000_0000_0000_000000000004);
+
 const DEVICE_229_ID: Uuid =
     Uuid::from_u128(0x22900002_0000_0000_0000_000000000001);
 const DEVICE_229_TOKEN: &str = "tok-device-229-migration-test";
@@ -4148,6 +4155,8 @@ fn before_229_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
     Box::pin(async move {
         // Insert test data into console_session and device_access_token.
         // These tables currently have "id" as the last column (from migration 145).
+        // We insert two console sessions: one recent (should be kept) and one
+        // created over 24 hours ago (should be dropped).
         ctx.client
             .batch_execute(&format!(
                 "
@@ -4156,6 +4165,13 @@ fn before_229_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
                 VALUES
                     ('{SESSION_229_ID}', '{SESSION_229_TOKEN}', now(), now(), '{SESSION_229_SILO_USER}');
 
+                INSERT INTO omicron.public.console_session
+                    (id, token, time_created, time_last_used, silo_user_id)
+                VALUES
+                    ('{SESSION_229_OLD_ID}', '{SESSION_229_OLD_TOKEN}',
+                     now() - INTERVAL '48 hours', now() - INTERVAL '48 hours',
+                     '{SESSION_229_OLD_SILO_USER}');
+
                 INSERT INTO omicron.public.device_access_token
                     (id, token, client_id, device_code, silo_user_id, time_requested, time_created)
                 VALUES
@@ -4172,7 +4188,7 @@ fn after_229_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
     Box::pin(async move {
         // Verify data was preserved after the column reordering migration.
 
-        // Check console_session
+        // Check that the recent console_session was kept
         let rows = ctx
             .client
             .query(
@@ -4184,7 +4200,11 @@ fn after_229_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
             )
             .await
             .expect("failed to query post-migration console_session");
-        assert_eq!(rows.len(), 1, "console_session row should still exist");
+        assert_eq!(
+            rows.len(),
+            1,
+            "recent console_session row should still exist"
+        );
 
         let id: Uuid = rows[0].get("id");
         assert_eq!(id, SESSION_229_ID);
@@ -4193,6 +4213,26 @@ fn after_229_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
         let silo_user_id: Uuid = rows[0].get("silo_user_id");
         assert_eq!(silo_user_id, SESSION_229_SILO_USER);
 
+        // Check that the old (>24h) console_session was dropped
+        let rows = ctx
+            .client
+            .query(
+                &format!(
+                    "SELECT id FROM omicron.public.console_session
+                     WHERE id = '{SESSION_229_OLD_ID}'"
+                ),
+                &[],
+            )
+            .await
+            .expect(
+                "failed to query post-migration console_session for old row",
+            );
+        assert_eq!(
+            rows.len(),
+            0,
+            "console_session row older than 24h should have been dropped"
+        );
+
         // Check device_access_token
         let rows = ctx
             .client

schema/crdb/fix-session-token-column-order/up02.sql

@@ -1,10 +1,37 @@
--- Copy data from console_session to console_session_new
--- Full table scan is unavoidable when copying all data
+-- This step used to copy everything from console_session to console_session_new
+-- (see https://github.com/oxidecomputer/omicron/pull/9816), but we discovered
+-- an exciting new class of bug when applying this migration on the colo rack.
+-- That bug is https://github.com/oxidecomputer/omicron/issues/9866.
+--
+-- CREATE INDEX in CRDB proceeds in two steps: one that creates the index
+-- metadata allowing it to be found by name, and another that actually makes
+-- the schema change and populates the index. Nexus instance A starts the
+-- CREATE INDEX step (up06, up07, up08) but because the table is so large (3
+-- million rows in this case) the backfill takes a while. While that's going,
+-- Nexus B also tries to pick up step 6, and it immediately succeeds because
+-- CREATE INDEX IF NOT EXISTS sees that the index exists. This marks the step
+-- as successful even though it's actually still running. Then the backfill runs
+-- out of memory and fails, and the index metadata is rolled back, so there is
+-- no index, but the migration system thinks it worked.
+--
+-- We are working on more reliable ways to avoid this (it could happen while
+-- creating an index on any large table) but in the meantime we can avoid
+-- this particular manifestation by making sure the console_session table is
+-- small when the CREATE INDEX runs. So, instead of copying all the rows from
+-- console_session, we only copy recent ones by filtering for
+--
+--   time_created >= now() - INTERVAL '24 hours'.
+-- 
+-- The absolute session timeout is 24 hours (session_absolute_timeout_minutes
+-- = 1440), so any session older than that guaranteed to be expired already.
+--
+-- Full table scan is unavoidable when copying data
 SET LOCAL disallow_full_table_scans = off;
 
 INSERT INTO omicron.public.console_session_new
     (id, token, time_created, time_last_used, silo_user_id)
 SELECT
     id, token, time_created, time_last_used, silo_user_id
 FROM omicron.public.console_session
+WHERE time_created >= now() - INTERVAL '24 hours'
 ON CONFLICT DO NOTHING;