Skip to content

Fix leaked bp_oximeter_read_policy rows #8577

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 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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 nexus/db-model/src/schema_versions.rs
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(162, 0, 0);
pub const SCHEMA_VERSION: Version = Version::new(163, 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(163, "fix-leaked-bp-oximeter-read-policy-rows"),
KnownVersion::new(162, "bundle-by-creation"),
KnownVersion::new(161, "inv_cockroachdb_status"),
KnownVersion::new(160, "tuf-trust-root"),
Expand Down
62 changes: 62 additions & 0 deletions nexus/tests/integration_tests/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2650,6 +2650,64 @@ mod migration_156 {
}
}

const BP_OXIMETER_READ_POLICY_ID_0: &str =
"5cb42909-d94a-4903-be72-330eea0325d9";
const BP_OXIMETER_READ_POLICY_ID_1: &str =
"142b62c2-9348-4530-9eed-7077351fb94b";
const BP_OXIMETER_READ_POLICY_ID_2: &str =
"3b5b7861-03aa-420a-a057-0a14347dc4c0";
const BP_OXIMETER_READ_POLICY_ID_3: &str =
"de7ab4c0-30d4-4e9e-b620-3a959a9d59dd";

// Insert two blueprints and 4 oximeter read policies, two of which do not have
// a corresponding blueprint
fn before_163_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
Box::pin(async move {
ctx.client
.batch_execute(
&format!("
INSERT INTO omicron.public.blueprint
(id, parent_blueprint_id, time_created, creator, comment, internal_dns_version, external_dns_version, cockroachdb_fingerprint, cockroachdb_setting_preserve_downgrade, target_release_minimum_generation)
VALUES
(
'{BP_OXIMETER_READ_POLICY_ID_0}', NULL, now(), 'bob', 'hi', 1, 1, 'fingerprint', NULL, 1
),
(
'{BP_OXIMETER_READ_POLICY_ID_1}', NULL, now(), 'bab', 'hi', 1, 1, 'fingerprint', NULL, 1
);

INSERT INTO omicron.public.bp_oximeter_read_policy
(blueprint_id, version, oximeter_read_mode)
VALUES
('{BP_OXIMETER_READ_POLICY_ID_0}', 1, 'cluster'),
('{BP_OXIMETER_READ_POLICY_ID_1}', 2, 'cluster'),
('{BP_OXIMETER_READ_POLICY_ID_2}', 3, 'cluster'),
('{BP_OXIMETER_READ_POLICY_ID_3}', 4, 'cluster')
"),
)
.await
.expect("failed to insert pre-migration rows for 163");
})
}

// Validate that rows that do not have a corresponding blueprint are gone
fn after_163_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
Box::pin(async move {
let rows = ctx
.client
.query("SELECT blueprint_id FROM omicron.public.bp_oximeter_read_policy;", &[])
.await
.expect("failed to query post-migration bp_oximeter_read_policy table");
assert_eq!(rows.len(), 2);

let id_1: Uuid = (&rows[0]).get::<&str, Uuid>("blueprint_id");
assert_eq!(id_1.to_string(), BP_OXIMETER_READ_POLICY_ID_1);

let id_2: Uuid = (&rows[1]).get::<&str, Uuid>("blueprint_id");
assert_eq!(id_2.to_string(), BP_OXIMETER_READ_POLICY_ID_0);
})
}

// Lazily initializes all migration checks. The combination of Rust function
// pointers and async makes defining a static table fairly painful, so we're
// using lazy initialization instead.
Expand Down Expand Up @@ -2736,6 +2794,10 @@ fn get_migration_checks() -> BTreeMap<Version, DataMigrationFns> {
.before(migration_156::before)
.after(migration_156::after),
);
map.insert(
Version::new(163, 0, 0),
DataMigrationFns::new().before(before_163_0_0).after(after_163_0_0),
);

map
}
Expand Down
2 changes: 1 addition & 1 deletion schema/crdb/dbinit.sql
Original file line number Diff line number Diff line change
Expand Up @@ -6202,7 +6202,7 @@ INSERT INTO omicron.public.db_metadata (
version,
target_version
) VALUES
(TRUE, NOW(), NOW(), '162.0.0', NULL)
(TRUE, NOW(), NOW(), '163.0.0', NULL)
ON CONFLICT DO NOTHING;

COMMIT;
3 changes: 3 additions & 0 deletions schema/crdb/fix-leaked-bp-oximeter-read-policy-rows/up1.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
set local disallow_full_table_scans = off;

DELETE FROM bp_oximeter_read_policy WHERE blueprint_id NOT IN (SELECT id FROM blueprint);
Loading