From acbeb27672e24cff2540bfd5c203deadd97a84bd Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 29 May 2024 18:38:49 -0700 Subject: [PATCH] [nexus] Add virtual provisioning idempotency tests, prevent underflow (#5830) Builds on https://github.com/oxidecomputer/omicron/pull/5081 and https://github.com/oxidecomputer/omicron/pull/5089 , but more out of convenience than necessity. # Summary This PR attempts to validate that, for the "virtual provisioning collection {insert, delete}" operations, they are idempotent. Currently, our usage of `max_instance_gen` only **partially** prevents updates during instance provisioning deletions: - If `max_instance_gen` is smaller than the observed instance generation number... - ... we avoid deleting the `virtual_provisioning_resource` record (which is great) - ... but we still decrement the `virtual_provisioning_collection` values (which is really not great). This basically means that we can "only cause the project/silo/fleet usage values to decrement arbitrarily, with no other changes". This has been, mechanically, the root cause of our observed underflows (e.g, https://github.com/oxidecomputer/omicron/issues/5525). # Details of this change - All the changes in `nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs` are tests validating idempotency of these operations. - All the changes in `nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs` are actually changes to the query which change functionality. The objective of these changes is to preserve idempotency of the newly added tests, and to prevent undercounting of virtual provisioning resources. If these changes are reverted, the newly added tests start failing, showing a lack of coverage. --- .../virtual_provisioning_collection.rs | 505 ++++++++++++++++++ .../virtual_provisioning_collection_update.rs | 99 ++-- ...ning_collection_update_delete_instance.sql | 16 +- ...oning_collection_update_delete_storage.sql | 2 +- 4 files changed, 556 insertions(+), 66 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs b/nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs index 348d277ddf..3630231b63 100644 --- a/nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs +++ b/nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs @@ -325,3 +325,508 @@ impl DataStore { Ok(()) } } + +#[cfg(test)] +mod test { + use super::*; + + use crate::db::datastore::test_utils::datastore_test; + use crate::db::fixed_data; + use crate::db::lookup::LookupPath; + use nexus_db_model::Instance; + use nexus_db_model::Project; + use nexus_db_model::SiloQuotasUpdate; + use nexus_test_utils::db::test_setup_database; + use nexus_types::external_api::params; + use omicron_common::api::external::IdentityMetadataCreateParams; + use omicron_test_utils::dev; + use uuid::Uuid; + + async fn verify_collection_usage( + datastore: &DataStore, + opctx: &OpContext, + id: Uuid, + expected_cpus: i64, + expected_memory: i64, + expected_storage: i64, + ) { + let collection = datastore + .virtual_provisioning_collection_get(opctx, id) + .await + .expect("Could not lookup collection"); + + assert_eq!(collection.cpus_provisioned, expected_cpus); + assert_eq!( + collection.ram_provisioned.0.to_bytes(), + expected_memory as u64 + ); + assert_eq!( + collection.virtual_disk_bytes_provisioned.0.to_bytes(), + expected_storage as u64 + ); + } + + struct TestData { + project_id: Uuid, + silo_id: Uuid, + fleet_id: Uuid, + authz_project: crate::authz::Project, + } + + impl TestData { + fn ids(&self) -> [Uuid; 3] { + [self.project_id, self.silo_id, self.fleet_id] + } + } + + // Use the default fleet and silo, but create a new project. + async fn setup_collections( + datastore: &DataStore, + opctx: &OpContext, + ) -> TestData { + let fleet_id = *fixed_data::FLEET_ID; + let silo_id = *fixed_data::silo::DEFAULT_SILO_ID; + let project_id = Uuid::new_v4(); + + let (authz_project, _project) = datastore + .project_create( + &opctx, + Project::new_with_id( + project_id, + silo_id, + params::ProjectCreate { + identity: IdentityMetadataCreateParams { + name: "myproject".parse().unwrap(), + description: "It's a project".into(), + }, + }, + ), + ) + .await + .unwrap(); + + // Ensure the silo has a quota that can fit our requested instance. + // + // This also acts as a guard against a change in the default silo quota + // -- we overwrite it for the test unconditionally. + + let quotas_update = SiloQuotasUpdate { + cpus: Some(24), + memory: Some(1 << 40), + storage: Some(1 << 50), + time_modified: chrono::Utc::now(), + }; + let authz_silo = LookupPath::new(&opctx, &datastore) + .silo_id(silo_id) + .lookup_for(crate::authz::Action::Modify) + .await + .unwrap() + .0; + datastore + .silo_update_quota(&opctx, &authz_silo, quotas_update) + .await + .unwrap(); + + TestData { fleet_id, silo_id, project_id, authz_project } + } + + async fn create_instance_record( + datastore: &DataStore, + opctx: &OpContext, + authz_project: &crate::authz::Project, + instance_id: Uuid, + project_id: Uuid, + cpus: i64, + memory: ByteCount, + ) { + datastore + .project_create_instance( + &opctx, + &authz_project, + Instance::new( + instance_id, + project_id, + ¶ms::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: "myinstance".parse().unwrap(), + description: "It's an instance".into(), + }, + ncpus: cpus.try_into().unwrap(), + memory: memory.try_into().unwrap(), + hostname: "myhostname".try_into().unwrap(), + user_data: Vec::new(), + network_interfaces: + params::InstanceNetworkInterfaceAttachment::None, + external_ips: Vec::new(), + disks: Vec::new(), + ssh_public_keys: None, + start: false, + }, + ), + ) + .await + .unwrap(); + } + + #[tokio::test] + async fn test_instance_create_and_delete() { + let logctx = dev::test_setup_log("test_instance_create_and_delete"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + let test_data = setup_collections(&datastore, &opctx).await; + let ids = test_data.ids(); + let project_id = test_data.project_id; + let authz_project = test_data.authz_project; + + // Actually provision the instance + + let instance_id = Uuid::new_v4(); + let cpus = 12; + let ram = ByteCount::try_from(1 << 30).unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 0).await; + } + + create_instance_record( + &datastore, + &opctx, + &authz_project, + instance_id, + project_id, + cpus, + ram, + ) + .await; + + datastore + .virtual_provisioning_collection_insert_instance( + &opctx, + instance_id, + project_id, + cpus, + ram, + ) + .await + .unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 12, 1 << 30, 0) + .await; + } + + // Delete the instance + + // Make this value outrageously high, so that as a "max" it is ignored. + let max_instance_gen: i64 = 1000; + datastore + .virtual_provisioning_collection_delete_instance( + &opctx, + instance_id, + project_id, + cpus, + ram, + max_instance_gen, + ) + .await + .unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 0).await; + } + + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn test_instance_create_and_delete_twice() { + let logctx = + dev::test_setup_log("test_instance_create_and_delete_twice"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + let test_data = setup_collections(&datastore, &opctx).await; + let ids = test_data.ids(); + let project_id = test_data.project_id; + let authz_project = test_data.authz_project; + + // Actually provision the instance + + let instance_id = Uuid::new_v4(); + let cpus = 12; + let ram = ByteCount::try_from(1 << 30).unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 0).await; + } + + create_instance_record( + &datastore, + &opctx, + &authz_project, + instance_id, + project_id, + cpus, + ram, + ) + .await; + + datastore + .virtual_provisioning_collection_insert_instance( + &opctx, + instance_id, + project_id, + cpus, + ram, + ) + .await + .unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 12, 1 << 30, 0) + .await; + } + + // Attempt to provision that same instance once more. + // + // The "virtual_provisioning_collection_insert" call should succeed for + // idempotency reasons, but we should not be double-dipping on the + // instance object's provisioning accounting. + + datastore + .virtual_provisioning_collection_insert_instance( + &opctx, + instance_id, + project_id, + cpus, + ram, + ) + .await + .unwrap(); + + // Verify that the usage is the same as before the call + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 12, 1 << 30, 0) + .await; + } + + // Delete the instance + + // If the "instance gen" is too low, the delete operation should be + // dropped. This mimics circumstances where an instance update arrives + // late to the query. + let max_instance_gen = 0; + datastore + .virtual_provisioning_collection_delete_instance( + &opctx, + instance_id, + project_id, + cpus, + ram, + max_instance_gen, + ) + .await + .unwrap(); + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 12, 1 << 30, 0) + .await; + } + + // Make this value outrageously high, so that as a "max" it is ignored. + let max_instance_gen = 1000; + datastore + .virtual_provisioning_collection_delete_instance( + &opctx, + instance_id, + project_id, + cpus, + ram, + max_instance_gen, + ) + .await + .unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 0).await; + } + + // Attempt to delete the same instance once more. + // + // Just like "double-adding", double deletion should be an idempotent + // operation. + + datastore + .virtual_provisioning_collection_delete_instance( + &opctx, + instance_id, + project_id, + cpus, + ram, + max_instance_gen, + ) + .await + .unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 0).await; + } + + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn test_storage_create_and_delete() { + let logctx = dev::test_setup_log("test_storage_create_and_delete"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + let test_data = setup_collections(&datastore, &opctx).await; + let ids = test_data.ids(); + let project_id = test_data.project_id; + + // Actually provision storage + + let disk_id = Uuid::new_v4(); + let disk_byte_diff = ByteCount::try_from(1 << 30).unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 0).await; + } + + datastore + .virtual_provisioning_collection_insert_storage( + &opctx, + disk_id, + project_id, + disk_byte_diff, + StorageType::Disk, + ) + .await + .unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 1 << 30) + .await; + } + + // Delete the disk + + datastore + .virtual_provisioning_collection_delete_storage( + &opctx, + disk_id, + project_id, + disk_byte_diff, + ) + .await + .unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 0).await; + } + + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn test_storage_create_and_delete_twice() { + let logctx = + dev::test_setup_log("test_storage_create_and_delete_twice"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + let test_data = setup_collections(&datastore, &opctx).await; + let ids = test_data.ids(); + let project_id = test_data.project_id; + + // Actually provision the disk + + let disk_id = Uuid::new_v4(); + let disk_byte_diff = ByteCount::try_from(1 << 30).unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 0).await; + } + + datastore + .virtual_provisioning_collection_insert_storage( + &opctx, + disk_id, + project_id, + disk_byte_diff, + StorageType::Disk, + ) + .await + .unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 1 << 30) + .await; + } + + // Attempt to provision that same disk once more. + // + // The "virtual_provisioning_collection_insert" call should succeed for + // idempotency reasons, but we should not be double-dipping on the + // disk object's provisioning accounting. + + datastore + .virtual_provisioning_collection_insert_storage( + &opctx, + disk_id, + project_id, + disk_byte_diff, + StorageType::Disk, + ) + .await + .unwrap(); + + // Verify that the usage is the same as before the call + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 1 << 30) + .await; + } + + // Delete the disk + + datastore + .virtual_provisioning_collection_delete_storage( + &opctx, + disk_id, + project_id, + disk_byte_diff, + ) + .await + .unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 0).await; + } + + // Attempt to delete the same disk once more. + // + // Just like "double-adding", double deletion should be an idempotent + // operation. + + datastore + .virtual_provisioning_collection_delete_storage( + &opctx, + disk_id, + project_id, + disk_byte_diff, + ) + .await + .unwrap(); + + for id in ids { + verify_collection_usage(&datastore, &opctx, id, 0, 0, 0).await; + } + + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } +} diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index 156691866e..895fee2092 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -225,7 +225,7 @@ WITH .bind::(resource.virtual_disk_bytes_provisioned) .bind::(resource.virtual_disk_bytes_provisioned) }, - UpdateKind::DeleteInstance { id, .. } | UpdateKind::DeleteStorage { id, .. } => { + UpdateKind::DeleteStorage { id, .. } => { query.sql(" do_update AS ( @@ -239,11 +239,52 @@ WITH virtual_provisioning_resource.id = ").param().sql(" LIMIT 1 + ) = 1 + AS update + ),") + .bind::(id) + }, + UpdateKind::DeleteInstance { id, max_instance_gen, .. } => { + // The filter condition here ensures that the provisioning record is + // only deleted if the corresponding instance has a generation + // number less than the supplied `max_instance_gen`. This allows a + // caller that is about to apply an instance update that will stop + // the instance and that bears generation G to avoid deleting + // resources if the instance generation was already advanced to or + // past G. + // + // If the relevant instance ID is not in the database, then some + // other operation must have ensured the instance was previously + // stopped (because that's the only way it could have been deleted), + // and that operation should have cleaned up the resources already, + // in which case there's nothing to do here. + query.sql(" + do_update + AS ( + SELECT + ( + SELECT + count(*) + FROM + virtual_provisioning_resource + WHERE + virtual_provisioning_resource.id = ").param().sql(" + LIMIT + 1 + ) = 1 AND + EXISTS ( + SELECT 1 + FROM + instance + WHERE + instance.id = ").param().sql(" AND instance.state_generation < ").param().sql(" + LIMIT 1 ) - = 1 AS update ),") .bind::(id) + .bind::(id) + .bind::(max_instance_gen) }, }; @@ -295,57 +336,8 @@ WITH ) .bind::(resource.cpus_provisioned) .bind::(resource.ram_provisioned), - UpdateKind::DeleteInstance { id, max_instance_gen, .. } => { - // The filter condition here ensures that the provisioning record is - // only deleted if the corresponding instance has a generation - // number less than the supplied `max_instance_gen`. This allows a - // caller that is about to apply an instance update that will stop - // the instance and that bears generation G to avoid deleting - // resources if the instance generation was already advanced to or - // past G. - // - // If the relevant instance ID is not in the database, then some - // other operation must have ensured the instance was previously - // stopped (because that's the only way it could have been deleted), - // and that operation should have cleaned up the resources already, - // in which case there's nothing to do here. - query - .sql( - " - unused_cte_arm - AS ( - DELETE FROM - virtual_provisioning_resource - WHERE - virtual_provisioning_resource.id = ", - ) - .param() - .sql( - " - AND - virtual_provisioning_resource.id = ( - SELECT instance.id FROM instance WHERE - instance.id = ", - ) - .param() - .sql( - " AND - instance.state_generation < ", - ) - .param() - .sql( - " LIMIT 1) - RETURNING ", - ) - .sql(AllColumnsOfVirtualResource::with_prefix( - "virtual_provisioning_resource", - )) - .sql("),") - .bind::(id) - .bind::(id) - .bind::(max_instance_gen) - } - UpdateKind::DeleteStorage { id, .. } => query + UpdateKind::DeleteInstance { id, .. } + | UpdateKind::DeleteStorage { id, .. } => query .sql( " unused_cte_arm @@ -358,6 +350,7 @@ WITH .param() .sql( " + AND (SELECT do_update.update FROM do_update LIMIT 1) RETURNING ", ) .sql(AllColumnsOfVirtualResource::with_prefix( diff --git a/nexus/db-queries/tests/output/virtual_provisioning_collection_update_delete_instance.sql b/nexus/db-queries/tests/output/virtual_provisioning_collection_update_delete_instance.sql index 48094a8371..3c97b7efc7 100644 --- a/nexus/db-queries/tests/output/virtual_provisioning_collection_update_delete_instance.sql +++ b/nexus/db-queries/tests/output/virtual_provisioning_collection_update_delete_instance.sql @@ -40,6 +40,9 @@ WITH 1 ) = 1 + AND EXISTS( + SELECT 1 FROM instance WHERE instance.id = $5 AND instance.state_generation < $6 LIMIT 1 + ) AS update ), unused_cte_arm @@ -47,18 +50,7 @@ WITH DELETE FROM virtual_provisioning_resource WHERE - virtual_provisioning_resource.id = $5 - AND virtual_provisioning_resource.id - = ( - SELECT - instance.id - FROM - instance - WHERE - instance.id = $6 AND instance.state_generation < $7 - LIMIT - 1 - ) + virtual_provisioning_resource.id = $7 AND (SELECT do_update.update FROM do_update LIMIT 1) RETURNING virtual_provisioning_resource.id, virtual_provisioning_resource.time_modified, diff --git a/nexus/db-queries/tests/output/virtual_provisioning_collection_update_delete_storage.sql b/nexus/db-queries/tests/output/virtual_provisioning_collection_update_delete_storage.sql index b607ac4185..b372a62003 100644 --- a/nexus/db-queries/tests/output/virtual_provisioning_collection_update_delete_storage.sql +++ b/nexus/db-queries/tests/output/virtual_provisioning_collection_update_delete_storage.sql @@ -47,7 +47,7 @@ WITH DELETE FROM virtual_provisioning_resource WHERE - virtual_provisioning_resource.id = $5 + virtual_provisioning_resource.id = $5 AND (SELECT do_update.update FROM do_update LIMIT 1) RETURNING virtual_provisioning_resource.id, virtual_provisioning_resource.time_modified,