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,