From caac0b95b7378892d99d90feb87c1f4c9ca520ea Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Thu, 6 Oct 2022 16:28:33 +0000 Subject: [PATCH 01/18] WIP for volume remove read only parent --- nexus/src/app/sagas/mod.rs | 4 + nexus/src/app/sagas/volume_remove_rop.rs | 118 ++++ nexus/src/app/volume.rs | 21 + nexus/src/db/datastore/volume.rs | 128 ++++ nexus/src/internal_api/http_entrypoints.rs | 27 + .../integration_tests/volume_management.rs | 574 +++++++++++------- openapi/nexus-internal.json | 29 + package-manifest.toml | 5 +- smf/sled-agent/config-rss.toml | 6 +- smf/sled-agent/config.toml | 2 +- 10 files changed, 687 insertions(+), 227 deletions(-) create mode 100644 nexus/src/app/sagas/volume_remove_rop.rs diff --git a/nexus/src/app/sagas/mod.rs b/nexus/src/app/sagas/mod.rs index b73cf62c677..3fdba98c73a 100644 --- a/nexus/src/app/sagas/mod.rs +++ b/nexus/src/app/sagas/mod.rs @@ -25,6 +25,7 @@ pub mod instance_create; pub mod instance_migrate; pub mod snapshot_create; pub mod volume_delete; +pub mod volume_remove_rop; pub mod common_storage; @@ -103,6 +104,9 @@ fn make_action_registry() -> ActionRegistry { ::register_actions( &mut registry, ); + ::register_actions( + &mut registry, + ); registry } diff --git a/nexus/src/app/sagas/volume_remove_rop.rs b/nexus/src/app/sagas/volume_remove_rop.rs new file mode 100644 index 00000000000..b177db8eb29 --- /dev/null +++ b/nexus/src/app/sagas/volume_remove_rop.rs @@ -0,0 +1,118 @@ +// 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::{ + ActionRegistry, NexusActionContext, NexusSaga, SagaInitError, + ACTION_GENERATE_ID, +}; +use crate::app::sagas::NexusAction; +use lazy_static::lazy_static; +use serde::Deserialize; +use serde::Serialize; +use std::sync::Arc; +use steno::ActionError; +use steno::{new_action_noop_undo, Node}; +use uuid::Uuid; + +// Volume remove read only parent saga: input parameters + +#[derive(Debug, Deserialize, Serialize)] +pub struct Params { + pub volume_id: Uuid, +} + +// Volume remove_read_only_parent saga: actions + +lazy_static! { + // We remove a read_only parent by + // - Creating a temporary volume. + // - Remove the read_only_parent from our source volume and attach it + // to the temporary volume. + // - Delete the temporary volume. + + // remove the read_only_parent, attach it to the temp volume. + static ref REMOVE_READ_ONLY_PARENT: NexusAction = new_action_noop_undo( + "volume-remove-rop.remove-read-only-parent", + svr_remove_read_only_parent + ); + + // remove temp volume. + static ref REMOVE_NEW_VOLUME: NexusAction = new_action_noop_undo( + "volume-remove-rop.remove-new-volume", + svr_remove_new_volume + ); +} + +// volume remove read only parent saga: definition + +#[derive(Debug)] +pub struct SagaVolumeRemoveROP; +impl NexusSaga for SagaVolumeRemoveROP { + const NAME: &'static str = "volume-remove-read-only-parent"; + type Params = Params; + + fn register_actions(registry: &mut ActionRegistry) { + registry.register(Arc::clone(&*REMOVE_READ_ONLY_PARENT)); + registry.register(Arc::clone(&*REMOVE_NEW_VOLUME)); + } + + fn make_saga_dag( + _params: &Self::Params, + mut builder: steno::DagBuilder, + ) -> Result { + builder.append(Node::action( + "temp_volume_id", + "GenerateVolumeId", + ACTION_GENERATE_ID.as_ref(), + )); + builder.append(Node::action( + "no_result_1", + "RemoveReadOnlyParent", + REMOVE_READ_ONLY_PARENT.as_ref(), + )); + builder.append(Node::action( + "final_no_result", + "RemoveNewVolume", + REMOVE_NEW_VOLUME.as_ref(), + )); + + Ok(builder.build()?) + } +} + +// volume remove read only parent saga: action implementations +async fn svr_remove_read_only_parent( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let osagactx = sagactx.user_data(); + let params = sagactx.saga_params::()?; + + let temp_volume_id = + sagactx.lookup::("temp_volume_id")?; + + println!("svr_remove_read_only_parent nv:{}", temp_volume_id); + osagactx + .datastore() + .volume_remove_rop(params.volume_id, temp_volume_id) + .await + .map_err(ActionError::action_failed)?; + Ok(()) +} + +async fn svr_remove_new_volume( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let osagactx = sagactx.user_data(); + + let temp_volume_id = + sagactx.lookup::("temp_volume_id")?; + + println!("svr_remove_new_volume nv:{}", temp_volume_id); + osagactx + .nexus() + .volume_delete(temp_volume_id) + .await + .map_err(ActionError::action_failed)?; + Ok(()) +} diff --git a/nexus/src/app/volume.rs b/nexus/src/app/volume.rs index e8bfe362920..8c8f523c128 100644 --- a/nexus/src/app/volume.rs +++ b/nexus/src/app/volume.rs @@ -40,4 +40,25 @@ impl super::Nexus { Ok(volume_deleted) } + + /// Kick off a saga to remove a read only parent from a volume. + pub async fn volume_remove_read_only_parent( + self: &Arc, + volume_id: Uuid, + ) -> DeleteResult { + let saga_params = sagas::volume_remove_rop::Params { volume_id }; + + let saga_outputs = self + .execute_saga::( + saga_params + ) + .await?; + + let volume_res = + saga_outputs.lookup_node_output::<()>("final_no_result").map_err( + |e| Error::InternalError { internal_message: e.to_string() }, + )?; + + Ok(volume_res) + } } diff --git a/nexus/src/db/datastore/volume.rs b/nexus/src/db/datastore/volume.rs index 8be451592c0..e45cbc52dc2 100644 --- a/nexus/src/db/datastore/volume.rs +++ b/nexus/src/db/datastore/volume.rs @@ -447,6 +447,134 @@ impl DataStore { } }) } + + // Here we remove the read only parent from volume_id, and attach it + // to temp_volume_id. + // + // As this is part of a saga, it will be able to handle being replayed + // If we call this twice, any work done the first time through should + // not happen again, or be undone. + pub async fn volume_remove_rop( + &self, + volume_id: Uuid, + temp_volume_id: Uuid, + ) -> Result { + + // In this single transaction: + // - Get the given volume from the volume_id from the database + // - Extract the volume.data into a VolumeConstructionRequest (VCR) + // - Create a new VCR, copying over anything from the original VCR, + // but, replacing the read_only_parent with None. + // - Put the new VCR into volume.data, then update the volume in the + // database. + // - Get the given volume from temp_volume_id from the database + // - Extract the temp volume.data into a VCR + // - Create a new VCR, copying over anything from the original VCR, + // but, replacing the read_only_parent with the read_only_parent + // data from volume_id. + // - Put the new temp VCR into the temp volume.data, update the + // temp_volume in the database. + self.pool() + .transaction(move |conn| { + // Grab the volume in question. If the volume record was already + // delete the we can just return. + let volume = { + use db::schema::volume::dsl; + + let volume = dsl::volume + .filter(dsl::id.eq(volume_id)) + .select(Volume::as_select()) + .get_result(conn) + .optional()?; + + let volume = if let Some(v) = volume { + v + } else { + // the volume does not exist, nothing to do. + return Ok(false); + }; + + if volume.time_deleted.is_some() { + // this volume is deleted, so let whatever is deleting + // it clean it up. + return Ok(false); + } else { + // A volume record exists, and was not deleted, we + // can attempt to remove its read_only_parent. + volume + } + }; + + // If a read_only_parent exists, remove it from volume_id, and + // attach it to temp_volume_id. + let vcr: VolumeConstructionRequest = + serde_json::from_str(volume.data()).unwrap(); + + match vcr { + VolumeConstructionRequest::Volume { + id, + block_size, + sub_volumes, + read_only_parent, + } => { + if read_only_parent.is_none() { + // This volume has no read_only_parent + Ok(false) + } else { + // Create a new VCR and fill in the contents + // from what the original volume had. + let new_vcr = VolumeConstructionRequest::Volume { + id, + block_size, + sub_volumes, + read_only_parent: None, + }; + + let new_volume_data = + serde_json::to_string(&new_vcr).unwrap(); + + // Update the original volume_id with the new + // volume.data. + use db::schema::volume::dsl as volume_dsl; + diesel::update(volume_dsl::volume) + .filter(volume_dsl::id.eq(volume_id)) + .set(volume_dsl::data.eq(new_volume_data)) + .execute(conn)?; + + // Make a new VCR, with the information from + // our temp_volume_id, but the read_only_parent + // from the original volume. + let rop_vcr = VolumeConstructionRequest::Volume { + id: temp_volume_id, + block_size, + sub_volumes: vec![], + read_only_parent, + }; + let rop_volume_data = + serde_json::to_string(&rop_vcr).unwrap(); + // Update the temp_volume_id with the volume + // data that contains the read_only_parent. + diesel::update(volume_dsl::volume) + .filter(volume_dsl::id.eq(temp_volume_id)) + .filter(volume_dsl::time_deleted.is_null()) + .set(volume_dsl::data.eq(rop_volume_data)) + .execute(conn)?; + + Ok(true) + } + } + _ => { + // Volume has a format that does not contain ROPs + Ok(false) + } + } + }) + .await + .map_err(|e| public_error_from_diesel_pool( + e, + ErrorHandler::Server + )) + } } #[derive(Default)] diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index b75caddda13..9d6c7bc72b9 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -42,6 +42,7 @@ pub fn internal_api() -> NexusApiDescription { api.register(dataset_put)?; api.register(cpapi_instances_put)?; api.register(cpapi_disks_put)?; + api.register(cpapi_volume_remove_read_only_parent)?; api.register(cpapi_producers_post)?; api.register(cpapi_collectors_post)?; api.register(cpapi_metrics_collect)?; @@ -224,6 +225,32 @@ async fn cpapi_disks_put( apictx.internal_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// Path parameters for Volume requests (internal API) +#[derive(Deserialize, JsonSchema)] +struct VolumePathParam { + volume_id: Uuid, +} + +/// Request removal of a read_only_parent from a volume +#[endpoint { + method = POST, + path = "/volume/remove-read-only-parent/{volume_id}", + }] +async fn cpapi_volume_remove_read_only_parent( + rqctx: Arc>>, + path_params: Path, +) -> Result { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + + let handler = async { + nexus.volume_remove_read_only_parent(path.volume_id).await?; + Ok(HttpResponseUpdatedNoContent()) + }; + apictx.internal_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + /// Accept a registration from a new metric producer #[endpoint { method = POST, diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index 1943ec3e6c8..050ca6d78e3 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -22,11 +22,13 @@ use omicron_common::api::external::ByteCount; use omicron_common::api::external::Disk; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::Name; +use omicron_nexus::db::DataStore; use omicron_nexus::external_api::params; use omicron_nexus::external_api::views; use rand::prelude::SliceRandom; use rand::{rngs::StdRng, SeedableRng}; use sled_agent_client::types::VolumeConstructionRequest; +use std::sync::Arc; use uuid::Uuid; use httptest::{matchers::*, responders::*, Expectation, ServerBuilder}; @@ -48,21 +50,12 @@ async fn create_org_and_project(client: &ClientTestContext) -> Uuid { project.identity.id } -#[nexus_test] -async fn test_snapshot_then_delete_disk(cptestctx: &ControlPlaneTestContext) { - // Test that Nexus does not delete a region if there's a snapshot of that - // region: - // - // 1. Create a disk - // 2. Create a snapshot of that disk (creating running snapshots) - // 3. Delete the disk - // 4. Delete the snapshot +async fn create_global_image( + client: &ClientTestContext, +) -> views::GlobalImage { - let client = &cptestctx.external_client; - let disk_test = DiskTest::new(&cptestctx).await; create_ip_pool(&client, "p0", None, None).await; create_org_and_project(client).await; - let disks_url = get_disks_url(); // Define a global image let server = ServerBuilder::new().run().unwrap(); @@ -94,7 +87,7 @@ async fn test_snapshot_then_delete_disk(cptestctx: &ControlPlaneTestContext) { block_size: params::BlockSize::try_from(512).unwrap(), }; - let global_image: views::GlobalImage = NexusRequest::objects_post( + NexusRequest::objects_post( client, "/system/images", &image_create_params, @@ -104,11 +97,18 @@ async fn test_snapshot_then_delete_disk(cptestctx: &ControlPlaneTestContext) { .await .unwrap() .parsed_body() - .unwrap(); + .unwrap() +} - // Create a disk from this image +async fn create_base_disk( + client: &ClientTestContext, + global_image: &views::GlobalImage, + disks_url: &String, + base_disk_name: &Name, +) -> Disk { + + // let disk_size = ByteCount::try_from(2u64 * 1024 * 1024 * 1024).unwrap(); let disk_size = ByteCount::from_gibibytes_u32(2); - let base_disk_name: Name = "base-disk".parse().unwrap(); let base_disk = params::DiskCreate { identity: IdentityMetadataCreateParams { name: base_disk_name.clone(), @@ -120,7 +120,7 @@ async fn test_snapshot_then_delete_disk(cptestctx: &ControlPlaneTestContext) { size: disk_size, }; - let base_disk: Disk = NexusRequest::new( + NexusRequest::new( RequestBuilder::new(client, Method::POST, &disks_url) .body(Some(&base_disk)) .expect_status(Some(StatusCode::CREATED)), @@ -130,7 +130,32 @@ async fn test_snapshot_then_delete_disk(cptestctx: &ControlPlaneTestContext) { .await .unwrap() .parsed_body() - .unwrap(); + .unwrap() +} + +#[nexus_test] +async fn test_snapshot_then_delete_disk(cptestctx: &ControlPlaneTestContext) { + // Test that Nexus does not delete a region if there's a snapshot of that + // region: + // + // 1. Create a disk + // 2. Create a snapshot of that disk (creating running snapshots) + // 3. Delete the disk + // 4. Delete the snapshot + + let client = &cptestctx.external_client; + let disk_test = DiskTest::new(&cptestctx).await; + let disks_url = get_disks_url(); + let base_disk_name: Name = "base-disk".parse().unwrap(); + + let global_image = create_global_image(&client).await; + // Create a disk from this image + let base_disk = create_base_disk( + &client, + &global_image, + &disks_url, + &base_disk_name + ).await; // Issue snapshot request let snapshots_url = format!( @@ -192,77 +217,18 @@ async fn test_delete_snapshot_then_disk(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let disk_test = DiskTest::new(&cptestctx).await; - create_ip_pool(&client, "p0", None, None).await; - create_org_and_project(client).await; let disks_url = get_disks_url(); + let base_disk_name: Name = "base-disk".parse().unwrap(); // Define a global image - let server = ServerBuilder::new().run().unwrap(); - server.expect( - Expectation::matching(request::method_path("HEAD", "/image.raw")) - .times(1..) - .respond_with( - status_code(200).append_header( - "Content-Length", - format!("{}", 4096 * 1000), - ), - ), - ); - - let image_create_params = params::GlobalImageCreate { - identity: IdentityMetadataCreateParams { - name: "alpine-edge".parse().unwrap(), - description: String::from( - "you can boot any image, as long as it's alpine", - ), - }, - source: params::ImageSource::Url { - url: server.url("/image.raw").to_string(), - }, - distribution: params::Distribution { - name: "alpine".parse().unwrap(), - version: "edge".into(), - }, - block_size: params::BlockSize::try_from(512).unwrap(), - }; - - let global_image: views::GlobalImage = NexusRequest::objects_post( - client, - "/system/images", - &image_create_params, - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); - + let global_image = create_global_image(&client).await; // Create a disk from this image - let disk_size = ByteCount::from_gibibytes_u32(2); - let base_disk_name: Name = "base-disk".parse().unwrap(); - let base_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: base_disk_name.clone(), - description: String::from("sells rainsticks"), - }, - disk_source: params::DiskSource::GlobalImage { - image_id: global_image.identity.id, - }, - size: disk_size, - }; - - let base_disk: Disk = NexusRequest::new( - RequestBuilder::new(client, Method::POST, &disks_url) - .body(Some(&base_disk)) - .expect_status(Some(StatusCode::CREATED)), - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); + let base_disk = create_base_disk( + &client, + &global_image, + &disks_url, + &base_disk_name + ).await; // Issue snapshot request let snapshots_url = format!( @@ -324,77 +290,18 @@ async fn test_multiple_snapshots(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let disk_test = DiskTest::new(&cptestctx).await; - create_ip_pool(&client, "p0", None, None).await; - create_org_and_project(client).await; let disks_url = get_disks_url(); - - // Define a global image - let server = ServerBuilder::new().run().unwrap(); - server.expect( - Expectation::matching(request::method_path("HEAD", "/image.raw")) - .times(1..) - .respond_with( - status_code(200).append_header( - "Content-Length", - format!("{}", 4096 * 1000), - ), - ), - ); - - let image_create_params = params::GlobalImageCreate { - identity: IdentityMetadataCreateParams { - name: "alpine-edge".parse().unwrap(), - description: String::from( - "you can boot any image, as long as it's alpine", - ), - }, - source: params::ImageSource::Url { - url: server.url("/image.raw").to_string(), - }, - distribution: params::Distribution { - name: "alpine".parse().unwrap(), - version: "edge".into(), - }, - block_size: params::BlockSize::try_from(512).unwrap(), - }; - - let global_image: views::GlobalImage = NexusRequest::objects_post( - client, - "/system/images", - &image_create_params, - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); - - // Create a disk from this image - let disk_size = ByteCount::from_gibibytes_u32(1); let base_disk_name: Name = "base-disk".parse().unwrap(); - let base_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: base_disk_name.clone(), - description: String::from("sells rainsticks"), - }, - disk_source: params::DiskSource::GlobalImage { - image_id: global_image.identity.id, - }, - size: disk_size, - }; - let base_disk: Disk = NexusRequest::new( - RequestBuilder::new(client, Method::POST, &disks_url) - .body(Some(&base_disk)) - .expect_status(Some(StatusCode::CREATED)), - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); + let global_image = create_global_image(&client).await; + // Create a disk from this image + let disk_size = ByteCount::from_gibibytes_u32(1); // ZZZ + let base_disk = create_base_disk( + &client, + &global_image, + &disks_url, + &base_disk_name + ).await; // Issue snapshot requests let snapshots_url = format!( @@ -455,79 +362,20 @@ async fn test_snapshot_prevents_other_disk( ) { // Test that region remains if there is a snapshot, preventing further // allocation. + let client = &cptestctx.external_client; let disk_test = DiskTest::new(&cptestctx).await; - create_ip_pool(&client, "p0", None, None).await; - create_org_and_project(client).await; let disks_url = get_disks_url(); - - // Define a global image - let server = ServerBuilder::new().run().unwrap(); - server.expect( - Expectation::matching(request::method_path("HEAD", "/image.raw")) - .times(1..) - .respond_with( - status_code(200).append_header( - "Content-Length", - format!("{}", 4096 * 1000), - ), - ), - ); - - let image_create_params = params::GlobalImageCreate { - identity: IdentityMetadataCreateParams { - name: "alpine-edge".parse().unwrap(), - description: String::from( - "you can boot any image, as long as it's alpine", - ), - }, - source: params::ImageSource::Url { - url: server.url("/image.raw").to_string(), - }, - distribution: params::Distribution { - name: "alpine".parse().unwrap(), - version: "edge".into(), - }, - block_size: params::BlockSize::try_from(512).unwrap(), - }; - - let global_image: views::GlobalImage = NexusRequest::objects_post( - client, - "/system/images", - &image_create_params, - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); - - // Create a disk from this image - let disk_size = ByteCount::from_gibibytes_u32(2); let base_disk_name: Name = "base-disk".parse().unwrap(); - let base_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: base_disk_name.clone(), - description: String::from("sells rainsticks"), - }, - disk_source: params::DiskSource::GlobalImage { - image_id: global_image.identity.id, - }, - size: disk_size, - }; - let base_disk: Disk = NexusRequest::new( - RequestBuilder::new(client, Method::POST, &disks_url) - .body(Some(&base_disk)) - .expect_status(Some(StatusCode::CREATED)), - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); + let global_image = create_global_image(&client).await; + // Create a disk from this image + let base_disk = create_base_disk( + &client, + &global_image, + &disks_url, + &base_disk_name + ).await; // Issue snapshot request let snapshots_url = format!( @@ -1208,6 +1056,290 @@ async fn test_multiple_layers_of_snapshots_random_delete_order( assert!(disk_test.crucible_resources_deleted().await); } +async fn create_volume( + datastore: &Arc, + volume_id: Uuid, + rop_option: Option +) { + let block_size = 512; + + // Make the SubVolume + let sub_volume = VolumeConstructionRequest::File { + id: volume_id, + block_size, + path: "/lol".to_string(), + }; + let sub_volumes = vec![sub_volume]; + + let rop = match rop_option { + Some(x) => Some(Box::new(x)), + None => None, + }; + + // Create the volume from the parts above and insert into the database. + datastore + .volume_create(nexus_db_model::Volume::new( + volume_id, + serde_json::to_string(&VolumeConstructionRequest::Volume { + id: volume_id, + block_size, + sub_volumes, + read_only_parent: rop, + }) + .unwrap(), + )) + .await + .unwrap(); +} + +#[nexus_test] +async fn test_volume_remove_read_only_parent_base( + cptestctx: &ControlPlaneTestContext, +) { + // Test the removal of a volume with a read only parent. + // The ROP should end up on the t_vid volume. + let nexus = &cptestctx.server.apictx.nexus; + let datastore = nexus.datastore(); + + let volume_id = Uuid::new_v4(); + let t_vid = Uuid::new_v4(); + let block_size = 512; + + // Make our read_only_parent + let rop = VolumeConstructionRequest::Url { + id: Uuid::new_v4(), + block_size, + url: "http://oxide.computer/rop".to_string(), + }; + + // Create the Volume with a read_only_parent, and the temp volume that + // the saga would create for us. + create_volume(&datastore, volume_id, Some(rop)).await; + create_volume(&datastore, t_vid, None).await; + + // We should get Ok(true) back after removal of the ROP. + let res = datastore.volume_remove_rop(volume_id, t_vid).await.unwrap(); + assert!(res); + + // Go and get the volume from the database, verify it no longer + // has a read only parent. + let new_vol = datastore.volume_get(volume_id).await.unwrap(); + let vcr: VolumeConstructionRequest = + serde_json::from_str(new_vol.data()).unwrap(); + + match vcr { + VolumeConstructionRequest::Volume { + id: _, + block_size: _, + sub_volumes: _, + read_only_parent, + } => { + assert!(read_only_parent.is_none()); + }, + x => { + panic!("Unexpected volume type returned: {:?}", x); + } + } + + // Verify the t_vid now has a ROP. + let new_vol = datastore.volume_get(t_vid).await.unwrap(); + let vcr: VolumeConstructionRequest = + serde_json::from_str(new_vol.data()).unwrap(); + + match vcr { + VolumeConstructionRequest::Volume { + id: _, + block_size: _, + sub_volumes: _, + read_only_parent, + } => { + assert!(read_only_parent.is_some()); + }, + x => { + panic!("Unexpected volume type returned: {:?}", x); + } + } + + // Try to remove the read only parent a 2nd time, it should + // return Ok(false) as there is now no volume to remove. + let res = datastore.volume_remove_rop(volume_id, t_vid).await.unwrap(); + assert!(!res); + + // Verify the t_vid still has the read_only_parent. + // We want to verify we can call volume_remove_rop twice and the second + // time through it won't change what it did the first time. This is + // critical to supporting replay of the saga, should it be needed. + let new_vol = datastore.volume_get(t_vid).await.unwrap(); + let vcr: VolumeConstructionRequest = + serde_json::from_str(new_vol.data()).unwrap(); + + match vcr { + VolumeConstructionRequest::Volume { + id: _, + block_size: _, + sub_volumes: _, + read_only_parent, + } => { + assert!(read_only_parent.is_some()); + }, + x => { + panic!("Unexpected volume type returned: {:?}", x); + } + } +} + +#[nexus_test] +async fn test_volume_remove_read_only_parent_no_parent( + cptestctx: &ControlPlaneTestContext, +) { + // Test the removal of a read only parent from a volume + // without a read only parent. + let nexus = &cptestctx.server.apictx.nexus; + let datastore = nexus.datastore(); + + let volume_id = Uuid::new_v4(); + let t_vid = Uuid::new_v4(); + create_volume(&datastore, volume_id, None).await; + + // We will get Ok(false) back from this operation. + let res = datastore.volume_remove_rop(volume_id, t_vid).await.unwrap(); + assert!(!res); +} + +#[nexus_test] +async fn test_volume_remove_read_only_parent_volume_not_volume( + cptestctx: &ControlPlaneTestContext, +) { + // test removal of a read only volume for a volume that is not + // of a type to have a read only parent. + let nexus = &cptestctx.server.apictx.nexus; + let datastore = nexus.datastore(); + + let volume_id = Uuid::new_v4(); + let t_vid = Uuid::new_v4(); + + datastore + .volume_create(nexus_db_model::Volume::new( + volume_id, + serde_json::to_string(&VolumeConstructionRequest::File { + id: volume_id, + block_size: 512, + path: "/lol".to_string(), + }) + .unwrap(), + )) + .await + .unwrap(); + + let removed = datastore.volume_remove_rop(volume_id, t_vid).await.unwrap(); + assert!(!removed); +} + +#[nexus_test] +async fn test_volume_remove_read_only_parent_bad_volume( + cptestctx: &ControlPlaneTestContext, +) { + // Test the removal of a read only parent from a volume + // that does not exist + let nexus = &cptestctx.server.apictx.nexus; + let datastore = nexus.datastore(); + + let volume_id = Uuid::new_v4(); + let t_vid = Uuid::new_v4(); + + // Nothing should be removed, but we also don't return error. + let removed = datastore.volume_remove_rop(volume_id, t_vid).await.unwrap(); + assert!(!removed); +} + +#[nexus_test] +async fn test_volume_remove_read_only_parent_volume_deleted( + cptestctx: &ControlPlaneTestContext, +) { + // Test the removal of a read_only_parent from a deleted volume. + let nexus = &cptestctx.server.apictx.nexus; + let datastore = nexus.datastore(); + let volume_id = Uuid::new_v4(); + let block_size = 512; + + // Make our read_only_parent + let rop = VolumeConstructionRequest::Url { + id: Uuid::new_v4(), + block_size, + url: "http://oxide.computer/rop".to_string(), + }; + // Make the volume + create_volume(&datastore, volume_id, Some(rop)).await; + + // Soft delete the volume + let _cr = datastore + .decrease_crucible_resource_count_and_soft_delete_volume( + volume_id + ) + .await + .unwrap(); + + let t_vid = Uuid::new_v4(); + // Nothing should be removed, but we also don't return error. + let removed = datastore.volume_remove_rop(volume_id, t_vid).await.unwrap(); + assert!(!removed); +} + +#[nexus_test] +async fn test_volume_remove_rop_saga(cptestctx: &ControlPlaneTestContext) { + + // Test the saga for removal of a volume with a read only parent. + // We create a volume with a read only parent, then call the saga on it. + let nexus = &cptestctx.server.apictx.nexus; + let datastore = nexus.datastore(); + + let volume_id = Uuid::new_v4(); + let block_size = 512; + + // Make our read_only_parent + let rop = VolumeConstructionRequest::Url { + id: Uuid::new_v4(), + block_size, + url: "http://oxide.computer/rop".to_string(), + }; + + create_volume(&datastore, volume_id, Some(rop)).await; + + println!("Created this volume: {:?}", volume_id); + // disk to volume id, to then remove ROP? + let int_client = &cptestctx.internal_client; + let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); + + // Call the internal API endpoint + int_client + .make_request( + Method::POST, + &rop_url, + None as Option<&serde_json::Value>, + StatusCode::NO_CONTENT, + ) + .await + .unwrap(); + + let new_vol = datastore.volume_get(volume_id).await.unwrap(); + let vcr: VolumeConstructionRequest = + serde_json::from_str(new_vol.data()).unwrap(); + + match vcr { + VolumeConstructionRequest::Volume { + id: _, + block_size: _, + sub_volumes: _, + read_only_parent, + } => { + assert!(read_only_parent.is_none()); + }, + x => { + panic!("Unexpected volume type returned: {:?}", x); + } + } +} + // volume_delete saga node idempotency tests #[nexus_test] diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 9cfb220e9e8..b1208b0717b 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -373,6 +373,35 @@ } } }, + "/volume/remove-read-only-parent/{volume_id}": { + "post": { + "summary": "Request removal of a read_only_parent from a volume", + "operationId": "cpapi_volume_remove_read_only_parent", + "parameters": [ + { + "in": "path", + "name": "volume_id", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + }, + "style": "simple" + } + ], + "responses": { + "204": { + "description": "resource updated" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/zpools/{zpool_id}/dataset/{dataset_id}": { "put": { "summary": "Report that a dataset within a pool has come online.", diff --git a/package-manifest.toml b/package-manifest.toml index 80d1e20dbcf..28b18615764 100644 --- a/package-manifest.toml +++ b/package-manifest.toml @@ -98,7 +98,8 @@ zone = true # 1. Build the zone image manually # 2. Copy the output zone image from crucible/out to omicron/out # 3. Use type = "manual" instead of the "prebuilt" -type = "prebuilt" +#type = "prebuilt" +type = "manual" repo = "crucible" commit = "1d67a53042f19ff7ca30dd20a04da94b7715ed7c" # The SHA256 digest is automatically posted to: @@ -112,7 +113,7 @@ sha256 = "d43fcfabc3f6402cfdbe3a0d31d49ae903f76b5ddec955dcee63236e4a60fdb0" service_name = "propolis-server" zone = true [external_package.propolis-server.source] -type = "prebuilt" +type = "manual" repo = "propolis" commit = "c59b1ac246b19130bd489cdce217e40a4e51c094" # The SHA256 digest is automatically posted to: diff --git a/smf/sled-agent/config-rss.toml b/smf/sled-agent/config-rss.toml index 98b03cdcce0..8e6e5c54d1e 100644 --- a/smf/sled-agent/config-rss.toml +++ b/smf/sled-agent/config-rss.toml @@ -16,7 +16,7 @@ rack_secret_threshold = 1 # IP address of Internet gateway # # NOTE: In the lab, use "172.20.15.225" -# address = "192.168.1.1" +address = "172.20.11.1" # MAC address of the internet gateway in the local network, i.e., of the above # IP address. @@ -72,8 +72,8 @@ gz_addresses = [] [request.service.service_type] type = "nexus" internal_ip = "fd00:1122:3344:0101::3" -# NOTE: In the lab, use "172.20.15.226" -external_ip = "192.168.1.20" +# NOTE: In the lab, use "172.20.11.2" +external_ip = "172.20.11.2" # TODO(https://github.com/oxidecomputer/omicron/issues/732): Nexus # should allocate Oximeter services. diff --git a/smf/sled-agent/config.toml b/smf/sled-agent/config.toml index 9af1db6f2e2..23de0f45226 100644 --- a/smf/sled-agent/config.toml +++ b/smf/sled-agent/config.toml @@ -19,7 +19,7 @@ zpools = [ # # If empty, this will be equivalent to the first result from: # $ dladm show-phys -p -o LINK -# data_link = "igb0" +data_link = "cxgbe0" [log] level = "info" From e68394b95d530e5cf8522378b7879d54c8548171 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 6 Oct 2022 10:28:59 -0700 Subject: [PATCH 02/18] pair coding subsaga --- nexus/src/app/sagas/volume_remove_rop.rs | 76 ++++++++++++------------ nexus/src/db/datastore/volume.rs | 4 +- 2 files changed, 41 insertions(+), 39 deletions(-) diff --git a/nexus/src/app/sagas/volume_remove_rop.rs b/nexus/src/app/sagas/volume_remove_rop.rs index b177db8eb29..a397db73e23 100644 --- a/nexus/src/app/sagas/volume_remove_rop.rs +++ b/nexus/src/app/sagas/volume_remove_rop.rs @@ -2,10 +2,8 @@ // 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::{ - ActionRegistry, NexusActionContext, NexusSaga, SagaInitError, - ACTION_GENERATE_ID, -}; +use super::{ActionRegistry, NexusActionContext, NexusSaga, SagaInitError}; +use crate::app::sagas; use crate::app::sagas::NexusAction; use lazy_static::lazy_static; use serde::Deserialize; @@ -36,12 +34,6 @@ lazy_static! { "volume-remove-rop.remove-read-only-parent", svr_remove_read_only_parent ); - - // remove temp volume. - static ref REMOVE_NEW_VOLUME: NexusAction = new_action_noop_undo( - "volume-remove-rop.remove-new-volume", - svr_remove_new_volume - ); } // volume remove read only parent saga: definition @@ -54,27 +46,55 @@ impl NexusSaga for SagaVolumeRemoveROP { fn register_actions(registry: &mut ActionRegistry) { registry.register(Arc::clone(&*REMOVE_READ_ONLY_PARENT)); - registry.register(Arc::clone(&*REMOVE_NEW_VOLUME)); } fn make_saga_dag( _params: &Self::Params, mut builder: steno::DagBuilder, ) -> Result { - builder.append(Node::action( + let temp_volume_id = Uuid::new_v4(); + let subsaga_params = + sagas::volume_delete::Params { volume_id: temp_volume_id }; + let subsaga_dag = { + let subsaga_builder = + steno::DagBuilder::new(steno::SagaName::new( + sagas::volume_delete::SagaVolumeDelete::NAME, + )); + sagas::volume_delete::SagaVolumeDelete::make_saga_dag( + &subsaga_params, + subsaga_builder, + )? + }; + + builder.append(Node::constant( "temp_volume_id", - "GenerateVolumeId", - ACTION_GENERATE_ID.as_ref(), + serde_json::to_value(&temp_volume_id).map_err(|e| { + SagaInitError::SerializeError(String::from("temp_volume_id"), e) + })?, )); + + // XXX add a node that creates the volume with the given volume id + builder.append(Node::action( "no_result_1", "RemoveReadOnlyParent", REMOVE_READ_ONLY_PARENT.as_ref(), )); - builder.append(Node::action( + + builder.append(Node::constant( + "params_for_delete_subsaga", + serde_json::to_value(&subsaga_params).map_err(|e| { + SagaInitError::SerializeError( + String::from("params_for_delete_subsaga"), + e, + ) + })?, + )); + + builder.append(Node::subsaga( "final_no_result", - "RemoveNewVolume", - REMOVE_NEW_VOLUME.as_ref(), + subsaga_dag, + "params_for_delete_subsaga", )); Ok(builder.build()?) @@ -88,10 +108,9 @@ async fn svr_remove_read_only_parent( let osagactx = sagactx.user_data(); let params = sagactx.saga_params::()?; - let temp_volume_id = - sagactx.lookup::("temp_volume_id")?; + let temp_volume_id = sagactx.lookup::("temp_volume_id")?; - println!("svr_remove_read_only_parent nv:{}", temp_volume_id); + println!("svr_remove_read_only_parent nv:{}", temp_volume_id); osagactx .datastore() .volume_remove_rop(params.volume_id, temp_volume_id) @@ -99,20 +118,3 @@ async fn svr_remove_read_only_parent( .map_err(ActionError::action_failed)?; Ok(()) } - -async fn svr_remove_new_volume( - sagactx: NexusActionContext, -) -> Result<(), ActionError> { - let osagactx = sagactx.user_data(); - - let temp_volume_id = - sagactx.lookup::("temp_volume_id")?; - - println!("svr_remove_new_volume nv:{}", temp_volume_id); - osagactx - .nexus() - .volume_delete(temp_volume_id) - .await - .map_err(ActionError::action_failed)?; - Ok(()) -} diff --git a/nexus/src/db/datastore/volume.rs b/nexus/src/db/datastore/volume.rs index e45cbc52dc2..3f71fa6e221 100644 --- a/nexus/src/db/datastore/volume.rs +++ b/nexus/src/db/datastore/volume.rs @@ -554,12 +554,12 @@ impl DataStore { serde_json::to_string(&rop_vcr).unwrap(); // Update the temp_volume_id with the volume // data that contains the read_only_parent. - diesel::update(volume_dsl::volume) + let num_updated = diesel::update(volume_dsl::volume) .filter(volume_dsl::id.eq(temp_volume_id)) .filter(volume_dsl::time_deleted.is_null()) .set(volume_dsl::data.eq(rop_volume_data)) .execute(conn)?; - + println!("*** num_updated = {}", num_updated); Ok(true) } } From 2e8cb2e5528f7fa03846dd57ef21523297a67677 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Thu, 6 Oct 2022 22:24:09 +0000 Subject: [PATCH 03/18] Create the temp volume data and volume --- nexus/src/app/sagas/volume_remove_rop.rs | 74 ++++- nexus/src/app/volume.rs | 2 +- nexus/src/db/datastore/volume.rs | 46 ++- .../integration_tests/volume_management.rs | 276 ++++++++---------- 4 files changed, 229 insertions(+), 169 deletions(-) diff --git a/nexus/src/app/sagas/volume_remove_rop.rs b/nexus/src/app/sagas/volume_remove_rop.rs index a397db73e23..b1cb6c3b582 100644 --- a/nexus/src/app/sagas/volume_remove_rop.rs +++ b/nexus/src/app/sagas/volume_remove_rop.rs @@ -5,6 +5,7 @@ use super::{ActionRegistry, NexusActionContext, NexusSaga, SagaInitError}; use crate::app::sagas; use crate::app::sagas::NexusAction; +use crate::db; use lazy_static::lazy_static; use serde::Deserialize; use serde::Serialize; @@ -29,11 +30,23 @@ lazy_static! { // to the temporary volume. // - Delete the temporary volume. + // Create the empty data for the temp volume. + static ref CREATE_TEMP_DATA: NexusAction = new_action_noop_undo( + "volume-remove-rop.create-temp-data", + svr_create_temp_data + ); + static ref CREATE_TEMP_VOLUME: NexusAction = new_action_noop_undo( + "volume-remove-rop.create-temp-volume", + svr_create_temp_volume + ); + // remove the read_only_parent, attach it to the temp volume. static ref REMOVE_READ_ONLY_PARENT: NexusAction = new_action_noop_undo( "volume-remove-rop.remove-read-only-parent", svr_remove_read_only_parent ); + + // Remove temp volume } // volume remove read only parent saga: definition @@ -45,6 +58,8 @@ impl NexusSaga for SagaVolumeRemoveROP { type Params = Params; fn register_actions(registry: &mut ActionRegistry) { + registry.register(Arc::clone(&*CREATE_TEMP_DATA)); + registry.register(Arc::clone(&*CREATE_TEMP_VOLUME)); registry.register(Arc::clone(&*REMOVE_READ_ONLY_PARENT)); } @@ -52,20 +67,22 @@ impl NexusSaga for SagaVolumeRemoveROP { _params: &Self::Params, mut builder: steno::DagBuilder, ) -> Result { + // Generate the temp volume ID this saga will use. let temp_volume_id = Uuid::new_v4(); + // Generate the params for the subsaga called at the end. let subsaga_params = sagas::volume_delete::Params { volume_id: temp_volume_id }; let subsaga_dag = { - let subsaga_builder = - steno::DagBuilder::new(steno::SagaName::new( - sagas::volume_delete::SagaVolumeDelete::NAME, - )); + let subsaga_builder = steno::DagBuilder::new(steno::SagaName::new( + sagas::volume_delete::SagaVolumeDelete::NAME, + )); sagas::volume_delete::SagaVolumeDelete::make_saga_dag( &subsaga_params, subsaga_builder, )? }; + // Add the temp_volume_id to the saga. builder.append(Node::constant( "temp_volume_id", serde_json::to_value(&temp_volume_id).map_err(|e| { @@ -73,7 +90,19 @@ impl NexusSaga for SagaVolumeRemoveROP { })?, )); - // XXX add a node that creates the volume with the given volume id + // Create the temporary volume data + builder.append(Node::action( + "temp_volume_data", + "CreateTempData", + CREATE_TEMP_DATA.as_ref(), + )); + + // Create the temporary volume + builder.append(Node::action( + "temp_volume", + "CreateTempVolume", + CREATE_TEMP_VOLUME.as_ref(), + )); builder.append(Node::action( "no_result_1", @@ -102,6 +131,41 @@ impl NexusSaga for SagaVolumeRemoveROP { } // volume remove read only parent saga: action implementations + +async fn svr_create_temp_data( + sagactx: NexusActionContext, +) -> Result { + let osagactx = sagactx.user_data(); + + let temp_volume_id = sagactx.lookup::("temp_volume_id")?; + + let volume_created = osagactx + .datastore() + .volume_create_empty_data(temp_volume_id) + .await + .map_err(ActionError::action_failed)?; + + Ok(volume_created) +} + +async fn svr_create_temp_volume( + sagactx: NexusActionContext, +) -> Result { + let osagactx = sagactx.user_data(); + + let temp_volume_id = sagactx.lookup::("temp_volume_id")?; + let temp_volume_data = sagactx.lookup::("temp_volume_data")?; + + let volume = db::model::Volume::new(temp_volume_id, temp_volume_data); + let volume_created = osagactx + .datastore() + .volume_create(volume) + .await + .map_err(ActionError::action_failed)?; + + Ok(volume_created) +} + async fn svr_remove_read_only_parent( sagactx: NexusActionContext, ) -> Result<(), ActionError> { diff --git a/nexus/src/app/volume.rs b/nexus/src/app/volume.rs index 8c8f523c128..f0a63cc61ff 100644 --- a/nexus/src/app/volume.rs +++ b/nexus/src/app/volume.rs @@ -50,7 +50,7 @@ impl super::Nexus { let saga_outputs = self .execute_saga::( - saga_params + saga_params, ) .await?; diff --git a/nexus/src/db/datastore/volume.rs b/nexus/src/db/datastore/volume.rs index 3f71fa6e221..1d9e16e64b7 100644 --- a/nexus/src/db/datastore/volume.rs +++ b/nexus/src/db/datastore/volume.rs @@ -29,9 +29,29 @@ use omicron_common::api::external::ResourceType; use serde::Deserialize; use serde::Serialize; use sled_agent_client::types::VolumeConstructionRequest; +use steno::ActionError; use uuid::Uuid; impl DataStore { + // Create an empty VolumeConstructionRequest and convert it to a string. + pub async fn volume_create_empty_data( + &self, + volume_id: Uuid, + ) -> Result { + // Manufacture an empty volume construction request. + let volume_construction_request = VolumeConstructionRequest::Volume { + id: volume_id, + block_size: 512, + sub_volumes: vec![], + read_only_parent: None, + }; + let volume_data = serde_json::to_string(&volume_construction_request) + .map_err(|e| { + ActionError::action_failed(Error::internal_error(&e.to_string())) + })?; + Ok(volume_data) + } + pub async fn volume_create(&self, volume: Volume) -> CreateResult { use db::schema::volume::dsl; @@ -459,7 +479,6 @@ impl DataStore { volume_id: Uuid, temp_volume_id: Uuid, ) -> Result { - // In this single transaction: // - Get the given volume from the volume_id from the database // - Extract the volume.data into a VolumeConstructionRequest (VCR) @@ -477,7 +496,7 @@ impl DataStore { self.pool() .transaction(move |conn| { // Grab the volume in question. If the volume record was already - // delete the we can just return. + // deleted then we can just return. let volume = { use db::schema::volume::dsl; @@ -510,6 +529,7 @@ impl DataStore { let vcr: VolumeConstructionRequest = serde_json::from_str(volume.data()).unwrap(); + println!("Updating volume: {} {}", volume_id, temp_volume_id); match vcr { VolumeConstructionRequest::Volume { id, @@ -554,13 +574,16 @@ impl DataStore { serde_json::to_string(&rop_vcr).unwrap(); // Update the temp_volume_id with the volume // data that contains the read_only_parent. - let num_updated = diesel::update(volume_dsl::volume) - .filter(volume_dsl::id.eq(temp_volume_id)) - .filter(volume_dsl::time_deleted.is_null()) - .set(volume_dsl::data.eq(rop_volume_data)) - .execute(conn)?; - println!("*** num_updated = {}", num_updated); - Ok(true) + let num_updated = + diesel::update(volume_dsl::volume) + .filter(volume_dsl::id.eq(temp_volume_id)) + .filter(volume_dsl::time_deleted.is_null()) + .set(volume_dsl::data.eq(rop_volume_data)) + .execute(conn)?; + if num_updated != 1 { + println!("RETURN ERROR"); + } + Ok(true) } } _ => { @@ -570,10 +593,7 @@ impl DataStore { } }) .await - .map_err(|e| public_error_from_diesel_pool( - e, - ErrorHandler::Server - )) + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } } diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index 050ca6d78e3..7320db17978 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -50,10 +50,7 @@ async fn create_org_and_project(client: &ClientTestContext) -> Uuid { project.identity.id } -async fn create_global_image( - client: &ClientTestContext, -) -> views::GlobalImage { - +async fn create_global_image(client: &ClientTestContext) -> views::GlobalImage { create_ip_pool(&client, "p0", None, None).await; create_org_and_project(client).await; @@ -87,27 +84,21 @@ async fn create_global_image( block_size: params::BlockSize::try_from(512).unwrap(), }; - NexusRequest::objects_post( - client, - "/system/images", - &image_create_params, - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap() + NexusRequest::objects_post(client, "/system/images", &image_create_params) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap() } async fn create_base_disk( - client: &ClientTestContext, - global_image: &views::GlobalImage, + client: &ClientTestContext, + global_image: &views::GlobalImage, disks_url: &String, base_disk_name: &Name, ) -> Disk { - - // let disk_size = ByteCount::try_from(2u64 * 1024 * 1024 * 1024).unwrap(); let disk_size = ByteCount::from_gibibytes_u32(2); let base_disk = params::DiskCreate { identity: IdentityMetadataCreateParams { @@ -148,14 +139,11 @@ async fn test_snapshot_then_delete_disk(cptestctx: &ControlPlaneTestContext) { let disks_url = get_disks_url(); let base_disk_name: Name = "base-disk".parse().unwrap(); - let global_image = create_global_image(&client).await; + let global_image = create_global_image(&client).await; // Create a disk from this image - let base_disk = create_base_disk( - &client, - &global_image, - &disks_url, - &base_disk_name - ).await; + let base_disk = + create_base_disk(&client, &global_image, &disks_url, &base_disk_name) + .await; // Issue snapshot request let snapshots_url = format!( @@ -221,14 +209,11 @@ async fn test_delete_snapshot_then_disk(cptestctx: &ControlPlaneTestContext) { let base_disk_name: Name = "base-disk".parse().unwrap(); // Define a global image - let global_image = create_global_image(&client).await; + let global_image = create_global_image(&client).await; // Create a disk from this image - let base_disk = create_base_disk( - &client, - &global_image, - &disks_url, - &base_disk_name - ).await; + let base_disk = + create_base_disk(&client, &global_image, &disks_url, &base_disk_name) + .await; // Issue snapshot request let snapshots_url = format!( @@ -293,15 +278,12 @@ async fn test_multiple_snapshots(cptestctx: &ControlPlaneTestContext) { let disks_url = get_disks_url(); let base_disk_name: Name = "base-disk".parse().unwrap(); - let global_image = create_global_image(&client).await; + let global_image = create_global_image(&client).await; // Create a disk from this image let disk_size = ByteCount::from_gibibytes_u32(1); // ZZZ - let base_disk = create_base_disk( - &client, - &global_image, - &disks_url, - &base_disk_name - ).await; + let base_disk = + create_base_disk(&client, &global_image, &disks_url, &base_disk_name) + .await; // Issue snapshot requests let snapshots_url = format!( @@ -368,14 +350,11 @@ async fn test_snapshot_prevents_other_disk( let disks_url = get_disks_url(); let base_disk_name: Name = "base-disk".parse().unwrap(); - let global_image = create_global_image(&client).await; + let global_image = create_global_image(&client).await; // Create a disk from this image - let base_disk = create_base_disk( - &client, - &global_image, - &disks_url, - &base_disk_name - ).await; + let base_disk = + create_base_disk(&client, &global_image, &disks_url, &base_disk_name) + .await; // Issue snapshot request let snapshots_url = format!( @@ -1058,23 +1037,23 @@ async fn test_multiple_layers_of_snapshots_random_delete_order( async fn create_volume( datastore: &Arc, - volume_id: Uuid, - rop_option: Option + volume_id: Uuid, + rop_option: Option, ) { - let block_size = 512; + let block_size = 512; // Make the SubVolume let sub_volume = VolumeConstructionRequest::File { - id: volume_id, - block_size, - path: "/lol".to_string(), - }; - let sub_volumes = vec![sub_volume]; + id: volume_id, + block_size, + path: "/lol".to_string(), + }; + let sub_volumes = vec![sub_volume]; - let rop = match rop_option { - Some(x) => Some(Box::new(x)), - None => None, - }; + let rop = match rop_option { + Some(x) => Some(Box::new(x)), + None => None, + }; // Create the volume from the parts above and insert into the database. datastore @@ -1084,7 +1063,7 @@ async fn create_volume( id: volume_id, block_size, sub_volumes, - read_only_parent: rop, + read_only_parent: rop, }) .unwrap(), )) @@ -1103,19 +1082,19 @@ async fn test_volume_remove_read_only_parent_base( let volume_id = Uuid::new_v4(); let t_vid = Uuid::new_v4(); - let block_size = 512; + let block_size = 512; // Make our read_only_parent let rop = VolumeConstructionRequest::Url { - id: Uuid::new_v4(), - block_size, - url: "http://oxide.computer/rop".to_string(), - }; + id: Uuid::new_v4(), + block_size, + url: "http://oxide.computer/rop".to_string(), + }; // Create the Volume with a read_only_parent, and the temp volume that // the saga would create for us. - create_volume(&datastore, volume_id, Some(rop)).await; - create_volume(&datastore, t_vid, None).await; + create_volume(&datastore, volume_id, Some(rop)).await; + create_volume(&datastore, t_vid, None).await; // We should get Ok(true) back after removal of the ROP. let res = datastore.volume_remove_rop(volume_id, t_vid).await.unwrap(); @@ -1123,42 +1102,42 @@ async fn test_volume_remove_read_only_parent_base( // Go and get the volume from the database, verify it no longer // has a read only parent. - let new_vol = datastore.volume_get(volume_id).await.unwrap(); - let vcr: VolumeConstructionRequest = - serde_json::from_str(new_vol.data()).unwrap(); - - match vcr { - VolumeConstructionRequest::Volume { - id: _, - block_size: _, - sub_volumes: _, - read_only_parent, - } => { + let new_vol = datastore.volume_get(volume_id).await.unwrap(); + let vcr: VolumeConstructionRequest = + serde_json::from_str(new_vol.data()).unwrap(); + + match vcr { + VolumeConstructionRequest::Volume { + id: _, + block_size: _, + sub_volumes: _, + read_only_parent, + } => { assert!(read_only_parent.is_none()); - }, - x => { - panic!("Unexpected volume type returned: {:?}", x); - } - } + } + x => { + panic!("Unexpected volume type returned: {:?}", x); + } + } // Verify the t_vid now has a ROP. - let new_vol = datastore.volume_get(t_vid).await.unwrap(); - let vcr: VolumeConstructionRequest = - serde_json::from_str(new_vol.data()).unwrap(); - - match vcr { - VolumeConstructionRequest::Volume { - id: _, - block_size: _, - sub_volumes: _, - read_only_parent, - } => { + let new_vol = datastore.volume_get(t_vid).await.unwrap(); + let vcr: VolumeConstructionRequest = + serde_json::from_str(new_vol.data()).unwrap(); + + match vcr { + VolumeConstructionRequest::Volume { + id: _, + block_size: _, + sub_volumes: _, + read_only_parent, + } => { assert!(read_only_parent.is_some()); - }, - x => { - panic!("Unexpected volume type returned: {:?}", x); - } - } + } + x => { + panic!("Unexpected volume type returned: {:?}", x); + } + } // Try to remove the read only parent a 2nd time, it should // return Ok(false) as there is now no volume to remove. @@ -1169,23 +1148,23 @@ async fn test_volume_remove_read_only_parent_base( // We want to verify we can call volume_remove_rop twice and the second // time through it won't change what it did the first time. This is // critical to supporting replay of the saga, should it be needed. - let new_vol = datastore.volume_get(t_vid).await.unwrap(); - let vcr: VolumeConstructionRequest = - serde_json::from_str(new_vol.data()).unwrap(); - - match vcr { - VolumeConstructionRequest::Volume { - id: _, - block_size: _, - sub_volumes: _, - read_only_parent, - } => { + let new_vol = datastore.volume_get(t_vid).await.unwrap(); + let vcr: VolumeConstructionRequest = + serde_json::from_str(new_vol.data()).unwrap(); + + match vcr { + VolumeConstructionRequest::Volume { + id: _, + block_size: _, + sub_volumes: _, + read_only_parent, + } => { assert!(read_only_parent.is_some()); - }, - x => { - panic!("Unexpected volume type returned: {:?}", x); - } - } + } + x => { + panic!("Unexpected volume type returned: {:?}", x); + } + } } #[nexus_test] @@ -1199,7 +1178,7 @@ async fn test_volume_remove_read_only_parent_no_parent( let volume_id = Uuid::new_v4(); let t_vid = Uuid::new_v4(); - create_volume(&datastore, volume_id, None).await; + create_volume(&datastore, volume_id, None).await; // We will get Ok(false) back from this operation. let res = datastore.volume_remove_rop(volume_id, t_vid).await.unwrap(); @@ -1260,22 +1239,20 @@ async fn test_volume_remove_read_only_parent_volume_deleted( let nexus = &cptestctx.server.apictx.nexus; let datastore = nexus.datastore(); let volume_id = Uuid::new_v4(); - let block_size = 512; + let block_size = 512; // Make our read_only_parent let rop = VolumeConstructionRequest::Url { - id: Uuid::new_v4(), - block_size, - url: "http://oxide.computer/rop".to_string(), - }; + id: Uuid::new_v4(), + block_size, + url: "http://oxide.computer/rop".to_string(), + }; // Make the volume - create_volume(&datastore, volume_id, Some(rop)).await; + create_volume(&datastore, volume_id, Some(rop)).await; // Soft delete the volume let _cr = datastore - .decrease_crucible_resource_count_and_soft_delete_volume( - volume_id - ) + .decrease_crucible_resource_count_and_soft_delete_volume(volume_id) .await .unwrap(); @@ -1287,26 +1264,25 @@ async fn test_volume_remove_read_only_parent_volume_deleted( #[nexus_test] async fn test_volume_remove_rop_saga(cptestctx: &ControlPlaneTestContext) { - // Test the saga for removal of a volume with a read only parent. // We create a volume with a read only parent, then call the saga on it. let nexus = &cptestctx.server.apictx.nexus; let datastore = nexus.datastore(); let volume_id = Uuid::new_v4(); - let block_size = 512; + let block_size = 512; // Make our read_only_parent let rop = VolumeConstructionRequest::Url { - id: Uuid::new_v4(), - block_size, - url: "http://oxide.computer/rop".to_string(), - }; + id: Uuid::new_v4(), + block_size, + url: "http://oxide.computer/rop".to_string(), + }; - create_volume(&datastore, volume_id, Some(rop)).await; + create_volume(&datastore, volume_id, Some(rop)).await; - println!("Created this volume: {:?}", volume_id); - // disk to volume id, to then remove ROP? + println!("Created this volume: {:?}", volume_id); + // disk to volume id, to then remove ROP? let int_client = &cptestctx.internal_client; let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); @@ -1321,23 +1297,23 @@ async fn test_volume_remove_rop_saga(cptestctx: &ControlPlaneTestContext) { .await .unwrap(); - let new_vol = datastore.volume_get(volume_id).await.unwrap(); - let vcr: VolumeConstructionRequest = - serde_json::from_str(new_vol.data()).unwrap(); - - match vcr { - VolumeConstructionRequest::Volume { - id: _, - block_size: _, - sub_volumes: _, - read_only_parent, - } => { + let new_vol = datastore.volume_get(volume_id).await.unwrap(); + let vcr: VolumeConstructionRequest = + serde_json::from_str(new_vol.data()).unwrap(); + + match vcr { + VolumeConstructionRequest::Volume { + id: _, + block_size: _, + sub_volumes: _, + read_only_parent, + } => { assert!(read_only_parent.is_none()); - }, - x => { - panic!("Unexpected volume type returned: {:?}", x); - } - } + } + x => { + panic!("Unexpected volume type returned: {:?}", x); + } + } } // volume_delete saga node idempotency tests From d4a600c89bc8d962f9ea190b256482c2dcbaa6c0 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Fri, 7 Oct 2022 21:35:43 +0000 Subject: [PATCH 04/18] Return errors if transaction steps fail --- nexus/src/db/datastore/volume.rs | 75 +++++++++++++++++-- .../integration_tests/volume_management.rs | 2 + 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/nexus/src/db/datastore/volume.rs b/nexus/src/db/datastore/volume.rs index 1d9e16e64b7..7a68dde976f 100644 --- a/nexus/src/db/datastore/volume.rs +++ b/nexus/src/db/datastore/volume.rs @@ -479,6 +479,19 @@ impl DataStore { volume_id: Uuid, temp_volume_id: Uuid, ) -> Result { + #[derive(Debug, thiserror::Error)] + enum RemoveReadOnlyParentError { + #[error("Error removing read only parent: {0}")] + DieselError(#[from] diesel::result::Error), + + #[error("Serde error removing read only parent: {0}")] + SerdeError(#[from] serde_json::Error), + + #[error("Database in unexpected state")] + UnexpectedDatabaseState, + } + type TxnError = TransactionError; + // In this single transaction: // - Get the given volume from the volume_id from the database // - Extract the volume.data into a VolumeConstructionRequest (VCR) @@ -527,9 +540,17 @@ impl DataStore { // If a read_only_parent exists, remove it from volume_id, and // attach it to temp_volume_id. let vcr: VolumeConstructionRequest = - serde_json::from_str(volume.data()).unwrap(); + serde_json::from_str( + volume.data() + ) + .map_err(|e| { + TxnError::CustomError( + RemoveReadOnlyParentError::SerdeError( + e, + ), + ) + })?; - println!("Updating volume: {} {}", volume_id, temp_volume_id); match vcr { VolumeConstructionRequest::Volume { id, @@ -551,16 +572,34 @@ impl DataStore { }; let new_volume_data = - serde_json::to_string(&new_vcr).unwrap(); + serde_json::to_string( + &new_vcr + ) + .map_err(|e| { + TxnError::CustomError( + RemoveReadOnlyParentError::SerdeError( + e, + ), + ) + })?; // Update the original volume_id with the new // volume.data. use db::schema::volume::dsl as volume_dsl; - diesel::update(volume_dsl::volume) + let num_updated = diesel::update(volume_dsl::volume) .filter(volume_dsl::id.eq(volume_id)) .set(volume_dsl::data.eq(new_volume_data)) .execute(conn)?; + // This should update just one row. If it does + // not, then something is terribly wrong in the + // database. + if num_updated != 1 { + return Err(TxnError::CustomError( + RemoveReadOnlyParentError::UnexpectedDatabaseState, + )); + } + // Make a new VCR, with the information from // our temp_volume_id, but the read_only_parent // from the original volume. @@ -571,7 +610,16 @@ impl DataStore { read_only_parent, }; let rop_volume_data = - serde_json::to_string(&rop_vcr).unwrap(); + serde_json::to_string( + &rop_vcr + ) + .map_err(|e| { + TxnError::CustomError( + RemoveReadOnlyParentError::SerdeError( + e, + ), + ) + })?; // Update the temp_volume_id with the volume // data that contains the read_only_parent. let num_updated = @@ -581,7 +629,9 @@ impl DataStore { .set(volume_dsl::data.eq(rop_volume_data)) .execute(conn)?; if num_updated != 1 { - println!("RETURN ERROR"); + return Err(TxnError::CustomError( + RemoveReadOnlyParentError::UnexpectedDatabaseState, + )); } Ok(true) } @@ -593,7 +643,18 @@ impl DataStore { } }) .await - .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + .map_err(|e| match e { + TxnError::CustomError( + RemoveReadOnlyParentError::DieselError(e), + ) => public_error_from_diesel_pool( + e.into(), + ErrorHandler::Server, + ), + + _ => { + Error::internal_error(&format!("Transaction error: {}", e)) + } + }) } } diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index 7320db17978..fc6f2b9da0a 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -22,6 +22,7 @@ use omicron_common::api::external::ByteCount; use omicron_common::api::external::Disk; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::Name; +use omicron_nexus::context::OpContext; use omicron_nexus::db::DataStore; use omicron_nexus::external_api::params; use omicron_nexus::external_api::views; @@ -1035,6 +1036,7 @@ async fn test_multiple_layers_of_snapshots_random_delete_order( assert!(disk_test.crucible_resources_deleted().await); } +// A test function to create a volume with the provided read only parent. async fn create_volume( datastore: &Arc, volume_id: Uuid, From d041d4289e1ef5ef2e7fc9e4ee779616dd498ca9 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Fri, 7 Oct 2022 22:01:51 +0000 Subject: [PATCH 05/18] Put back original config files --- package-manifest.toml | 5 ++--- smf/sled-agent/config-rss.toml | 6 +++--- smf/sled-agent/config.toml | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/package-manifest.toml b/package-manifest.toml index 28b18615764..80d1e20dbcf 100644 --- a/package-manifest.toml +++ b/package-manifest.toml @@ -98,8 +98,7 @@ zone = true # 1. Build the zone image manually # 2. Copy the output zone image from crucible/out to omicron/out # 3. Use type = "manual" instead of the "prebuilt" -#type = "prebuilt" -type = "manual" +type = "prebuilt" repo = "crucible" commit = "1d67a53042f19ff7ca30dd20a04da94b7715ed7c" # The SHA256 digest is automatically posted to: @@ -113,7 +112,7 @@ sha256 = "d43fcfabc3f6402cfdbe3a0d31d49ae903f76b5ddec955dcee63236e4a60fdb0" service_name = "propolis-server" zone = true [external_package.propolis-server.source] -type = "manual" +type = "prebuilt" repo = "propolis" commit = "c59b1ac246b19130bd489cdce217e40a4e51c094" # The SHA256 digest is automatically posted to: diff --git a/smf/sled-agent/config-rss.toml b/smf/sled-agent/config-rss.toml index 8e6e5c54d1e..98b03cdcce0 100644 --- a/smf/sled-agent/config-rss.toml +++ b/smf/sled-agent/config-rss.toml @@ -16,7 +16,7 @@ rack_secret_threshold = 1 # IP address of Internet gateway # # NOTE: In the lab, use "172.20.15.225" -address = "172.20.11.1" +# address = "192.168.1.1" # MAC address of the internet gateway in the local network, i.e., of the above # IP address. @@ -72,8 +72,8 @@ gz_addresses = [] [request.service.service_type] type = "nexus" internal_ip = "fd00:1122:3344:0101::3" -# NOTE: In the lab, use "172.20.11.2" -external_ip = "172.20.11.2" +# NOTE: In the lab, use "172.20.15.226" +external_ip = "192.168.1.20" # TODO(https://github.com/oxidecomputer/omicron/issues/732): Nexus # should allocate Oximeter services. diff --git a/smf/sled-agent/config.toml b/smf/sled-agent/config.toml index 23de0f45226..9af1db6f2e2 100644 --- a/smf/sled-agent/config.toml +++ b/smf/sled-agent/config.toml @@ -19,7 +19,7 @@ zpools = [ # # If empty, this will be equivalent to the first result from: # $ dladm show-phys -p -o LINK -data_link = "cxgbe0" +# data_link = "igb0" [log] level = "info" From f7c8412a9acb851ce22e265d1e3b9adbfb1c0823 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Fri, 7 Oct 2022 22:13:52 +0000 Subject: [PATCH 06/18] remove unused variable --- nexus/tests/integration_tests/volume_management.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index fc6f2b9da0a..0a88a2261cb 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -281,7 +281,6 @@ async fn test_multiple_snapshots(cptestctx: &ControlPlaneTestContext) { let global_image = create_global_image(&client).await; // Create a disk from this image - let disk_size = ByteCount::from_gibibytes_u32(1); // ZZZ let base_disk = create_base_disk(&client, &global_image, &disks_url, &base_disk_name) .await; From 295ed8f1a105fa43f176c57ce0f2c092e145300d Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Fri, 7 Oct 2022 23:09:11 +0000 Subject: [PATCH 07/18] More comments --- nexus/src/app/sagas/volume_remove_rop.rs | 12 +++++++----- nexus/src/app/volume.rs | 2 +- nexus/src/db/datastore/volume.rs | 2 +- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/nexus/src/app/sagas/volume_remove_rop.rs b/nexus/src/app/sagas/volume_remove_rop.rs index b1cb6c3b582..ab1b9f35abe 100644 --- a/nexus/src/app/sagas/volume_remove_rop.rs +++ b/nexus/src/app/sagas/volume_remove_rop.rs @@ -24,10 +24,10 @@ pub struct Params { // Volume remove_read_only_parent saga: actions lazy_static! { - // We remove a read_only parent by + // We remove a read_only parent by: // - Creating a temporary volume. // - Remove the read_only_parent from our source volume and attach it - // to the temporary volume. + // to the temporary volume. // - Delete the temporary volume. // Create the empty data for the temp volume. @@ -45,8 +45,6 @@ lazy_static! { "volume-remove-rop.remove-read-only-parent", svr_remove_read_only_parent ); - - // Remove temp volume } // volume remove read only parent saga: definition @@ -104,12 +102,14 @@ impl NexusSaga for SagaVolumeRemoveROP { CREATE_TEMP_VOLUME.as_ref(), )); + // Remove the read only parent, attach to temp volume builder.append(Node::action( "no_result_1", "RemoveReadOnlyParent", REMOVE_READ_ONLY_PARENT.as_ref(), )); + // Build the params for the subsaga to delete the temp volume builder.append(Node::constant( "params_for_delete_subsaga", serde_json::to_value(&subsaga_params).map_err(|e| { @@ -120,6 +120,7 @@ impl NexusSaga for SagaVolumeRemoveROP { })?, )); + // Call the subsaga to delete the temp volume builder.append(Node::subsaga( "final_no_result", subsaga_dag, @@ -132,6 +133,8 @@ impl NexusSaga for SagaVolumeRemoveROP { // volume remove read only parent saga: action implementations +// To create a volume, we need a crucible volume construction request. +// We create that data here. async fn svr_create_temp_data( sagactx: NexusActionContext, ) -> Result { @@ -174,7 +177,6 @@ async fn svr_remove_read_only_parent( let temp_volume_id = sagactx.lookup::("temp_volume_id")?; - println!("svr_remove_read_only_parent nv:{}", temp_volume_id); osagactx .datastore() .volume_remove_rop(params.volume_id, temp_volume_id) diff --git a/nexus/src/app/volume.rs b/nexus/src/app/volume.rs index f0a63cc61ff..fd1a10ec29c 100644 --- a/nexus/src/app/volume.rs +++ b/nexus/src/app/volume.rs @@ -41,7 +41,7 @@ impl super::Nexus { Ok(volume_deleted) } - /// Kick off a saga to remove a read only parent from a volume. + /// Start a saga to remove a read only parent from a volume. pub async fn volume_remove_read_only_parent( self: &Arc, volume_id: Uuid, diff --git a/nexus/src/db/datastore/volume.rs b/nexus/src/db/datastore/volume.rs index 7a68dde976f..1a220103f99 100644 --- a/nexus/src/db/datastore/volume.rs +++ b/nexus/src/db/datastore/volume.rs @@ -503,7 +503,7 @@ impl DataStore { // - Extract the temp volume.data into a VCR // - Create a new VCR, copying over anything from the original VCR, // but, replacing the read_only_parent with the read_only_parent - // data from volume_id. + // data from original volume_id. // - Put the new temp VCR into the temp volume.data, update the // temp_volume in the database. self.pool() From bfe1fc94c04d29cb95bf9971a30d992bdcdbbc5e Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Sun, 9 Oct 2022 17:41:09 +0000 Subject: [PATCH 08/18] Remove unused deps --- nexus/tests/integration_tests/volume_management.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index 0a88a2261cb..af682ea7a5a 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -22,7 +22,6 @@ use omicron_common::api::external::ByteCount; use omicron_common::api::external::Disk; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::Name; -use omicron_nexus::context::OpContext; use omicron_nexus::db::DataStore; use omicron_nexus::external_api::params; use omicron_nexus::external_api::views; From 6a78370442ba448b3268ffd526eedeceffcf4b7f Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Mon, 10 Oct 2022 21:13:15 +0000 Subject: [PATCH 09/18] More tests --- .../integration_tests/volume_management.rs | 196 +++++++++++++++++- 1 file changed, 195 insertions(+), 1 deletion(-) diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index af682ea7a5a..a305ea15aef 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -1286,7 +1286,7 @@ async fn test_volume_remove_rop_saga(cptestctx: &ControlPlaneTestContext) { let int_client = &cptestctx.internal_client; let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); - // Call the internal API endpoint + // Call the internal API endpoint for removal of the read only parent int_client .make_request( Method::POST, @@ -1316,6 +1316,200 @@ async fn test_volume_remove_rop_saga(cptestctx: &ControlPlaneTestContext) { } } +#[nexus_test] +async fn test_volume_remove_rop_saga_twice( + cptestctx: &ControlPlaneTestContext, +) { + // Test calling the saga for removal of a volume with a read only parent + // two times, the first will remove the read_only_parent, the second will + // do nothing. + let nexus = &cptestctx.server.apictx.nexus; + let datastore = nexus.datastore(); + + let volume_id = Uuid::new_v4(); + let block_size = 512; + + // Make our read_only_parent + let rop = VolumeConstructionRequest::Url { + id: Uuid::new_v4(), + block_size, + url: "http://oxide.computer/rop".to_string(), + }; + + create_volume(&datastore, volume_id, Some(rop)).await; + + println!("Created this volume: {:?}", volume_id); + // disk to volume id, to then remove ROP? + let int_client = &cptestctx.internal_client; + let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); + + // Call the internal API endpoint for removal of the read only parent + let res = int_client + .make_request( + Method::POST, + &rop_url, + None as Option<&serde_json::Value>, + StatusCode::NO_CONTENT, + ) + .await + .unwrap(); + + println!("first returns {:?}", res); + let new_vol = datastore.volume_get(volume_id).await.unwrap(); + let vcr: VolumeConstructionRequest = + serde_json::from_str(new_vol.data()).unwrap(); + + match vcr { + VolumeConstructionRequest::Volume { + id: _, + block_size: _, + sub_volumes: _, + read_only_parent, + } => { + assert!(read_only_parent.is_none()); + } + x => { + panic!("Unexpected volume type returned: {:?}", x); + } + } + + // Call the internal API endpoint a second time. Should be okay. + let res = int_client + .make_request( + Method::POST, + &rop_url, + None as Option<&serde_json::Value>, + StatusCode::NO_CONTENT, + ) + .await + .unwrap(); + + println!("twice returns {:?}", res); +} + +#[nexus_test] +async fn test_volume_remove_rop_saga_no_volume( + cptestctx: &ControlPlaneTestContext, +) { + // Test calling the saga on a volume that does not exist. + let nexus = &cptestctx.server.apictx.nexus; + let volume_id = Uuid::new_v4(); + + println!("Non-existant volume: {:?}", volume_id); + let int_client = &cptestctx.internal_client; + let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); + + // Call the internal API endpoint for removal of the read only parent + let res = int_client + .make_request( + Method::POST, + &rop_url, + None as Option<&serde_json::Value>, + StatusCode::NO_CONTENT, + ) + .await + .unwrap(); +} + +#[nexus_test] +async fn test_volume_remove_rop_saga_volume_not_volume( + cptestctx: &ControlPlaneTestContext, +) { + // Test saga removal of a read only volume for a volume that is not + // of a type to have a read only parent. + let nexus = &cptestctx.server.apictx.nexus; + let volume_id = Uuid::new_v4(); + let datastore = nexus.datastore(); + + datastore + .volume_create(nexus_db_model::Volume::new( + volume_id, + serde_json::to_string(&VolumeConstructionRequest::File { + id: volume_id, + block_size: 512, + path: "/lol".to_string(), + }) + .unwrap(), + )) + .await + .unwrap(); + + let int_client = &cptestctx.internal_client; + // Call the saga on this volume + let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); + + // Call the internal API endpoint for removal of the read only parent + let res = int_client + .make_request( + Method::POST, + &rop_url, + None as Option<&serde_json::Value>, + StatusCode::NO_CONTENT, + ) + .await + .unwrap(); +} + +#[nexus_test] +async fn test_volume_remove_rop_saga_deleted_volume( + cptestctx: &ControlPlaneTestContext, +) { + // Test that a saga removal of a read_only_parent from a deleted volume + // takes no action on that deleted volume. + let nexus = &cptestctx.server.apictx.nexus; + let datastore = nexus.datastore(); + let volume_id = Uuid::new_v4(); + let block_size = 512; + + // Make our read_only_parent + let rop = VolumeConstructionRequest::Url { + id: Uuid::new_v4(), + block_size, + url: "http://oxide.computer/rop".to_string(), + }; + // Make the volume + create_volume(&datastore, volume_id, Some(rop)).await; + + // Soft delete the volume + let _cr = datastore + .decrease_crucible_resource_count_and_soft_delete_volume(volume_id) + .await + .unwrap(); + + let int_client = &cptestctx.internal_client; + let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); + + // Call the internal API endpoint for removal of the read only parent + let res = int_client + .make_request( + Method::POST, + &rop_url, + None as Option<&serde_json::Value>, + StatusCode::NO_CONTENT, + ) + .await + .unwrap(); + + let new_vol = datastore.volume_get(volume_id).await.unwrap(); + let vcr: VolumeConstructionRequest = + serde_json::from_str(new_vol.data()).unwrap(); + + // Volume should still have read only parent + match vcr { + VolumeConstructionRequest::Volume { + id: _, + block_size: _, + sub_volumes: _, + read_only_parent, + } => { + assert!(read_only_parent.is_some()); + } + x => { + panic!("Unexpected volume type returned: {:?}", x); + } + } +} + // volume_delete saga node idempotency tests #[nexus_test] From 37936bd0858efaa0a9222eb6f22954c38c69816f Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Mon, 10 Oct 2022 22:07:25 +0000 Subject: [PATCH 10/18] fix warning --- nexus/tests/integration_tests/volume_management.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index a305ea15aef..65e6abe958f 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -1480,7 +1480,7 @@ async fn test_volume_remove_rop_saga_deleted_volume( let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); // Call the internal API endpoint for removal of the read only parent - let res = int_client + int_client .make_request( Method::POST, &rop_url, From 3aecb529037bd6dd4b86a396cdc4959719ef3257 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Tue, 11 Oct 2022 00:59:10 +0000 Subject: [PATCH 11/18] Fix more warnings --- nexus/tests/integration_tests/volume_management.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index 65e6abe958f..12c40b8a6d1 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -1392,7 +1392,6 @@ async fn test_volume_remove_rop_saga_no_volume( cptestctx: &ControlPlaneTestContext, ) { // Test calling the saga on a volume that does not exist. - let nexus = &cptestctx.server.apictx.nexus; let volume_id = Uuid::new_v4(); println!("Non-existant volume: {:?}", volume_id); @@ -1400,7 +1399,7 @@ async fn test_volume_remove_rop_saga_no_volume( let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); // Call the internal API endpoint for removal of the read only parent - let res = int_client + int_client .make_request( Method::POST, &rop_url, @@ -1439,7 +1438,7 @@ async fn test_volume_remove_rop_saga_volume_not_volume( let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); // Call the internal API endpoint for removal of the read only parent - let res = int_client + int_client .make_request( Method::POST, &rop_url, From 79ce29db49e5d94d430df31e5c35d0bf1d18a76b Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Fri, 14 Oct 2022 01:27:17 +0000 Subject: [PATCH 12/18] Update comment --- nexus/src/app/sagas/volume_remove_rop.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/nexus/src/app/sagas/volume_remove_rop.rs b/nexus/src/app/sagas/volume_remove_rop.rs index ab1b9f35abe..d9b651c3642 100644 --- a/nexus/src/app/sagas/volume_remove_rop.rs +++ b/nexus/src/app/sagas/volume_remove_rop.rs @@ -24,11 +24,22 @@ pub struct Params { // Volume remove_read_only_parent saga: actions lazy_static! { - // We remove a read_only parent by: - // - Creating a temporary volume. - // - Remove the read_only_parent from our source volume and attach it - // to the temporary volume. - // - Delete the temporary volume. + // A read-only parent is a structure in a volume that indicates that the + // volume is logically created from this parent. The initial data for the + // volume (implicitly) comes from the parent volume. In the background, + // we'll copy data from the parent into the physical storage allocated + // for this volume and, when that copy has completed, it will no longer + // be necessary to maintain this link to the read-only parent. At that + // point, we execute this saga. + // If this volume was the only one referencing that parent, then it's time + // to free the underlying storage resources of the parent as well. We can + // do this with the volume-delete saga, which takes care of correctly + // identifying whether other volumes are still referencing this parent. + // But we don't actually want to delete this volume. Instead, we create a + // temporary volume, move the read-only parent information from this volume + // to the temporary volume (so that this volume is now independent of the + // parent, and the temporary volume appears to depend on the parent), and + // then delete that temporary volume. // Create the empty data for the temp volume. static ref CREATE_TEMP_DATA: NexusAction = new_action_noop_undo( From fac40d36f05e45ab83248b2a6be809637d22a565 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Mon, 17 Oct 2022 16:19:11 +0000 Subject: [PATCH 13/18] More PR comments. Update volume remove read only parent API endpoint path. Add more descriptive error message. --- nexus/src/db/datastore/volume.rs | 12 +++++++----- nexus/src/internal_api/http_entrypoints.rs | 2 +- nexus/tests/integration_tests/volume_management.rs | 10 +++++----- openapi/nexus-internal.json | 2 +- 4 files changed, 14 insertions(+), 12 deletions(-) diff --git a/nexus/src/db/datastore/volume.rs b/nexus/src/db/datastore/volume.rs index 1a220103f99..3e95a8d627a 100644 --- a/nexus/src/db/datastore/volume.rs +++ b/nexus/src/db/datastore/volume.rs @@ -487,8 +487,8 @@ impl DataStore { #[error("Serde error removing read only parent: {0}")] SerdeError(#[from] serde_json::Error), - #[error("Database in unexpected state")] - UnexpectedDatabaseState, + #[error("Updated {0} database rows, expected {1}")] + UnexpectedDatabaseUpdate(usize, usize), } type TxnError = TransactionError; @@ -596,7 +596,7 @@ impl DataStore { // database. if num_updated != 1 { return Err(TxnError::CustomError( - RemoveReadOnlyParentError::UnexpectedDatabaseState, + RemoveReadOnlyParentError::UnexpectedDatabaseUpdate(num_updated, 1), )); } @@ -630,13 +630,15 @@ impl DataStore { .execute(conn)?; if num_updated != 1 { return Err(TxnError::CustomError( - RemoveReadOnlyParentError::UnexpectedDatabaseState, + RemoveReadOnlyParentError::UnexpectedDatabaseUpdate(num_updated, 1), )); } Ok(true) } } - _ => { + VolumeConstructionRequest::File { id: _, block_size: _, path: _ } + | VolumeConstructionRequest::Region { block_size: _, opts: _, gen: _ } + | VolumeConstructionRequest::Url { id: _, block_size: _, url: _ } => { // Volume has a format that does not contain ROPs Ok(false) } diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index 9d6c7bc72b9..4188991baf3 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -234,7 +234,7 @@ struct VolumePathParam { /// Request removal of a read_only_parent from a volume #[endpoint { method = POST, - path = "/volume/remove-read-only-parent/{volume_id}", + path = "/volume/{volume_id}/remove-read-only-parent", }] async fn cpapi_volume_remove_read_only_parent( rqctx: Arc>>, diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index 12c40b8a6d1..469d35ffb2a 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -1284,7 +1284,7 @@ async fn test_volume_remove_rop_saga(cptestctx: &ControlPlaneTestContext) { println!("Created this volume: {:?}", volume_id); // disk to volume id, to then remove ROP? let int_client = &cptestctx.internal_client; - let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); + let rop_url = format!("/volume/{}/remove-read-only-parent", volume_id); // Call the internal API endpoint for removal of the read only parent int_client @@ -1341,7 +1341,7 @@ async fn test_volume_remove_rop_saga_twice( println!("Created this volume: {:?}", volume_id); // disk to volume id, to then remove ROP? let int_client = &cptestctx.internal_client; - let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); + let rop_url = format!("/volume/{}/remove-read-only-parent", volume_id); // Call the internal API endpoint for removal of the read only parent let res = int_client @@ -1396,7 +1396,7 @@ async fn test_volume_remove_rop_saga_no_volume( println!("Non-existant volume: {:?}", volume_id); let int_client = &cptestctx.internal_client; - let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); + let rop_url = format!("/volume/{}/remove-read-only-parent", volume_id); // Call the internal API endpoint for removal of the read only parent int_client @@ -1435,7 +1435,7 @@ async fn test_volume_remove_rop_saga_volume_not_volume( let int_client = &cptestctx.internal_client; // Call the saga on this volume - let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); + let rop_url = format!("/volume/{}/remove-read-only-parent", volume_id); // Call the internal API endpoint for removal of the read only parent int_client @@ -1476,7 +1476,7 @@ async fn test_volume_remove_rop_saga_deleted_volume( .unwrap(); let int_client = &cptestctx.internal_client; - let rop_url = format!("/volume/remove-read-only-parent/{}", volume_id); + let rop_url = format!("/volume/{}/remove-read-only-parent", volume_id); // Call the internal API endpoint for removal of the read only parent int_client diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index b1208b0717b..1be103418ab 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -373,7 +373,7 @@ } } }, - "/volume/remove-read-only-parent/{volume_id}": { + "/volume/{volume_id}/remove-read-only-parent": { "post": { "summary": "Request removal of a read_only_parent from a volume", "operationId": "cpapi_volume_remove_read_only_parent", From 0a39254048c67293ee2f59a532406dcc1759383b Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Mon, 17 Oct 2022 16:46:28 +0000 Subject: [PATCH 14/18] More comments --- nexus/src/internal_api/http_entrypoints.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index 4188991baf3..692dd82b9b6 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -232,6 +232,13 @@ struct VolumePathParam { } /// Request removal of a read_only_parent from a volume +/// A volume can be created with the source data for that volume being another +/// volume that attached as a "read_only_parent". In the background there +/// exists a scrubber that will copy the data from the read_only_parent +/// into the volume. When that scrubber has completed copying the data, this +/// endpoint can be called to update the database that the read_only_parent +/// is no longer needed for a volume and future attachments of this volume +/// should not include that read_only_parent. #[endpoint { method = POST, path = "/volume/{volume_id}/remove-read-only-parent", From 68035471bea58cff3a27ea66cebf7950b4b99fe5 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Mon, 17 Oct 2022 19:20:06 +0000 Subject: [PATCH 15/18] Update openapi description --- openapi/nexus-internal.json | 1 + 1 file changed, 1 insertion(+) diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 1be103418ab..c125ed92d89 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -376,6 +376,7 @@ "/volume/{volume_id}/remove-read-only-parent": { "post": { "summary": "Request removal of a read_only_parent from a volume", + "description": "A volume can be created with the source data for that volume being another volume that attached as a \"read_only_parent\". In the background there exists a scrubber that will copy the data from the read_only_parent into the volume. When that scrubber has completed copying the data, this endpoint can be called to update the database that the read_only_parent is no longer needed for a volume and future attachments of this volume should not include that read_only_parent.", "operationId": "cpapi_volume_remove_read_only_parent", "parameters": [ { From f63e901e7538d27013c4e5cffbbe3a9b38db12cb Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Tue, 18 Oct 2022 19:34:33 +0000 Subject: [PATCH 16/18] simplify return values --- nexus/src/app/volume.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/nexus/src/app/volume.rs b/nexus/src/app/volume.rs index fd1a10ec29c..86c01b19c2a 100644 --- a/nexus/src/app/volume.rs +++ b/nexus/src/app/volume.rs @@ -48,17 +48,11 @@ impl super::Nexus { ) -> DeleteResult { let saga_params = sagas::volume_remove_rop::Params { volume_id }; - let saga_outputs = self - .execute_saga::( - saga_params, - ) - .await?; - - let volume_res = - saga_outputs.lookup_node_output::<()>("final_no_result").map_err( - |e| Error::InternalError { internal_message: e.to_string() }, - )?; + self.execute_saga::( + saga_params, + ) + .await?; - Ok(volume_res) + Ok(()) } } From 1fd452d3ad9c8fd9e86495052d0b47138dd3bde0 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Wed, 19 Oct 2022 00:05:01 +0000 Subject: [PATCH 17/18] More PR comments --- nexus/src/app/sagas/volume_remove_rop.rs | 54 +++++++++--------------- nexus/src/db/datastore/volume.rs | 20 --------- 2 files changed, 20 insertions(+), 54 deletions(-) diff --git a/nexus/src/app/sagas/volume_remove_rop.rs b/nexus/src/app/sagas/volume_remove_rop.rs index d9b651c3642..04a5f784449 100644 --- a/nexus/src/app/sagas/volume_remove_rop.rs +++ b/nexus/src/app/sagas/volume_remove_rop.rs @@ -7,11 +7,12 @@ use crate::app::sagas; use crate::app::sagas::NexusAction; use crate::db; use lazy_static::lazy_static; +use omicron_common::api::external::Error; use serde::Deserialize; use serde::Serialize; +use sled_agent_client::types::VolumeConstructionRequest; use std::sync::Arc; -use steno::ActionError; -use steno::{new_action_noop_undo, Node}; +use steno::{new_action_noop_undo, ActionError, Node}; use uuid::Uuid; // Volume remove read only parent saga: input parameters @@ -41,11 +42,7 @@ lazy_static! { // parent, and the temporary volume appears to depend on the parent), and // then delete that temporary volume. - // Create the empty data for the temp volume. - static ref CREATE_TEMP_DATA: NexusAction = new_action_noop_undo( - "volume-remove-rop.create-temp-data", - svr_create_temp_data - ); + // Create the temporary volume static ref CREATE_TEMP_VOLUME: NexusAction = new_action_noop_undo( "volume-remove-rop.create-temp-volume", svr_create_temp_volume @@ -67,7 +64,6 @@ impl NexusSaga for SagaVolumeRemoveROP { type Params = Params; fn register_actions(registry: &mut ActionRegistry) { - registry.register(Arc::clone(&*CREATE_TEMP_DATA)); registry.register(Arc::clone(&*CREATE_TEMP_VOLUME)); registry.register(Arc::clone(&*REMOVE_READ_ONLY_PARENT)); } @@ -99,13 +95,6 @@ impl NexusSaga for SagaVolumeRemoveROP { })?, )); - // Create the temporary volume data - builder.append(Node::action( - "temp_volume_data", - "CreateTempData", - CREATE_TEMP_DATA.as_ref(), - )); - // Create the temporary volume builder.append(Node::action( "temp_volume", @@ -144,31 +133,28 @@ impl NexusSaga for SagaVolumeRemoveROP { // volume remove read only parent saga: action implementations -// To create a volume, we need a crucible volume construction request. -// We create that data here. -async fn svr_create_temp_data( - sagactx: NexusActionContext, -) -> Result { - let osagactx = sagactx.user_data(); - - let temp_volume_id = sagactx.lookup::("temp_volume_id")?; - - let volume_created = osagactx - .datastore() - .volume_create_empty_data(temp_volume_id) - .await - .map_err(ActionError::action_failed)?; - - Ok(volume_created) -} - async fn svr_create_temp_volume( sagactx: NexusActionContext, ) -> Result { let osagactx = sagactx.user_data(); let temp_volume_id = sagactx.lookup::("temp_volume_id")?; - let temp_volume_data = sagactx.lookup::("temp_volume_data")?; + + // Create the crucible VolumeConstructionRequest which we use + // for the temporary volume. + let volume_construction_request = VolumeConstructionRequest::Volume { + id: temp_volume_id, + block_size: 512, + sub_volumes: vec![], + read_only_parent: None, + }; + let temp_volume_data = serde_json::to_string(&volume_construction_request) + .map_err(|e| { + ActionError::action_failed(Error::internal_error(&format!( + "failed to deserialize volume data: {}", + e, + ))) + })?; let volume = db::model::Volume::new(temp_volume_id, temp_volume_data); let volume_created = osagactx diff --git a/nexus/src/db/datastore/volume.rs b/nexus/src/db/datastore/volume.rs index 3e95a8d627a..a4882ab5ef3 100644 --- a/nexus/src/db/datastore/volume.rs +++ b/nexus/src/db/datastore/volume.rs @@ -29,29 +29,9 @@ use omicron_common::api::external::ResourceType; use serde::Deserialize; use serde::Serialize; use sled_agent_client::types::VolumeConstructionRequest; -use steno::ActionError; use uuid::Uuid; impl DataStore { - // Create an empty VolumeConstructionRequest and convert it to a string. - pub async fn volume_create_empty_data( - &self, - volume_id: Uuid, - ) -> Result { - // Manufacture an empty volume construction request. - let volume_construction_request = VolumeConstructionRequest::Volume { - id: volume_id, - block_size: 512, - sub_volumes: vec![], - read_only_parent: None, - }; - let volume_data = serde_json::to_string(&volume_construction_request) - .map_err(|e| { - ActionError::action_failed(Error::internal_error(&e.to_string())) - })?; - Ok(volume_data) - } - pub async fn volume_create(&self, volume: Volume) -> CreateResult { use db::schema::volume::dsl; From 3cf4dccc7299caec5c3fa394923acfe075c8f1d4 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Wed, 19 Oct 2022 18:42:14 +0000 Subject: [PATCH 18/18] Add undo --- nexus/src/app/sagas/volume_remove_rop.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/nexus/src/app/sagas/volume_remove_rop.rs b/nexus/src/app/sagas/volume_remove_rop.rs index 04a5f784449..caaaa302da0 100644 --- a/nexus/src/app/sagas/volume_remove_rop.rs +++ b/nexus/src/app/sagas/volume_remove_rop.rs @@ -12,7 +12,7 @@ use serde::Deserialize; use serde::Serialize; use sled_agent_client::types::VolumeConstructionRequest; use std::sync::Arc; -use steno::{new_action_noop_undo, ActionError, Node}; +use steno::{new_action_noop_undo, ActionError, ActionFunc, Node}; use uuid::Uuid; // Volume remove read only parent saga: input parameters @@ -43,9 +43,10 @@ lazy_static! { // then delete that temporary volume. // Create the temporary volume - static ref CREATE_TEMP_VOLUME: NexusAction = new_action_noop_undo( + static ref CREATE_TEMP_VOLUME: NexusAction = ActionFunc::new_action( "volume-remove-rop.create-temp-volume", - svr_create_temp_volume + svr_create_temp_volume, + svr_create_temp_volume_undo ); // remove the read_only_parent, attach it to the temp volume. @@ -166,6 +167,21 @@ async fn svr_create_temp_volume( Ok(volume_created) } +async fn svr_create_temp_volume_undo( + sagactx: NexusActionContext, +) -> Result<(), anyhow::Error> { + let osagactx = sagactx.user_data(); + + let temp_volume_id = sagactx.lookup::("temp_volume_id")?; + + osagactx + .datastore() + .volume_hard_delete(temp_volume_id) + .await + .map_err(ActionError::action_failed)?; + Ok(()) +} + async fn svr_remove_read_only_parent( sagactx: NexusActionContext, ) -> Result<(), ActionError> {