From a3dd93cb9cbd32a2724b2fafc6400406c3f309de Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 24 Oct 2022 20:58:44 -0500 Subject: [PATCH 01/24] Start wiring up disk-create in instance-create --- nexus/src/app/sagas/instance_create.rs | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 7f87c5e1e2c..d5718c695d0 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -709,10 +709,13 @@ async fn sic_allocate_instance_external_ip_undo( async fn sic_create_disks_for_instance( sagactx: NexusActionContext, ) -> Result, ActionError> { + let osagactx: &Arc = sagactx.user_data(); let disk_params = sagactx.saga_params::()?; let saga_params = disk_params.saga_params; let disk_index = disk_params.which; let saga_disks = &saga_params.create_params.disks; + let opctx = + OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); if disk_index >= saga_disks.len() { return Ok(None); @@ -720,14 +723,26 @@ async fn sic_create_disks_for_instance( let disk = &saga_disks[disk_index]; + // TODO-correctness TODO-security It's not correct to re-resolve the + // organization and project names now. See oxidecomputer/omicron#1536. + let organization_name: db::model::Name = + saga_params.organization_name.clone().into(); + let project_name: db::model::Name = saga_params.project_name.clone().into(); + match disk { - params::InstanceDiskAttachment::Create(_create_params) => { - return Err(ActionError::action_failed( - "Creating disk during instance create unsupported!".to_string(), - )); + params::InstanceDiskAttachment::Create(create_params) => { + osagactx + .nexus() + .project_create_disk( + &opctx, + &organization_name, + &project_name, + &create_params, + ) + .await } - _ => {} + _ => Ok(None), } Ok(None) From 09e6d877c759a41fbb6bf1c768edc2ca1c8cd03e Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 25 Oct 2022 13:52:30 -0500 Subject: [PATCH 02/24] Attach disk after its created --- nexus/src/app/sagas/instance_create.rs | 32 ++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index d5718c695d0..28314525c79 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -796,11 +796,33 @@ async fn ensure_instance_disk_attach_state( let project_name: db::model::Name = saga_params.project_name.clone().into(); match disk { - params::InstanceDiskAttachment::Create(_) => { - // TODO grab disks created in sic_create_disks_for_instance - return Err(ActionError::action_failed(Error::invalid_request( - "creating disks while creating an instance not supported", - ))); + params::InstanceDiskAttachment::Create(create_params) => { + let disk_name = db::model::Name(create_params.name.clone()); + + if attached { + osagactx + .nexus() + .instance_attach_disk( + &opctx, + &organization_name, + &project_name, + &instance_name, + &disk_name, + ) + .await + } else { + osagactx + .nexus() + .instance_detach_disk( + &opctx, + &organization_name, + &project_name, + &instance_name, + &disk_name, + ) + .await + } + .map_err(ActionError::action_failed)?; } params::InstanceDiskAttachment::Attach(instance_disk_attach) => { let disk_name: db::model::Name = From 0b0466d4ef23237a060ad19e1bd399252d978630 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 25 Oct 2022 14:04:08 -0500 Subject: [PATCH 03/24] Flatten attach --- nexus/src/app/sagas/instance_create.rs | 84 +++++++++----------------- 1 file changed, 29 insertions(+), 55 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 28314525c79..633ef9434a8 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -795,65 +795,39 @@ async fn ensure_instance_disk_attach_state( saga_params.organization_name.clone().into(); let project_name: db::model::Name = saga_params.project_name.clone().into(); - match disk { + let disk_name = match disk { params::InstanceDiskAttachment::Create(create_params) => { - let disk_name = db::model::Name(create_params.name.clone()); - - if attached { - osagactx - .nexus() - .instance_attach_disk( - &opctx, - &organization_name, - &project_name, - &instance_name, - &disk_name, - ) - .await - } else { - osagactx - .nexus() - .instance_detach_disk( - &opctx, - &organization_name, - &project_name, - &instance_name, - &disk_name, - ) - .await - } - .map_err(ActionError::action_failed)?; + db::model::Name(create_params.identity.name.clone()) } - params::InstanceDiskAttachment::Attach(instance_disk_attach) => { - let disk_name: db::model::Name = - instance_disk_attach.name.clone().into(); - - if attached { - osagactx - .nexus() - .instance_attach_disk( - &opctx, - &organization_name, - &project_name, - &instance_name, - &disk_name, - ) - .await - } else { - osagactx - .nexus() - .instance_detach_disk( - &opctx, - &organization_name, - &project_name, - &instance_name, - &disk_name, - ) - .await - } - .map_err(ActionError::action_failed)?; + params::InstanceDiskAttachment::Existing { name } => { + db::model::Name(name.clone()) } + }; + + if attached { + osagactx + .nexus() + .instance_attach_disk( + &opctx, + &organization_name, + &project_name, + &instance_name, + &disk_name, + ) + .await + } else { + osagactx + .nexus() + .instance_detach_disk( + &opctx, + &organization_name, + &project_name, + &instance_name, + &disk_name, + ) + .await } + .map_err(ActionError::action_failed)?; Ok(()) } From 0085424764d7b89ed8d54208b5b0edd97d751760 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 25 Oct 2022 14:32:31 -0500 Subject: [PATCH 04/24] Maybe fix create enum --- nexus/src/app/sagas/instance_create.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 633ef9434a8..00825f669c6 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -709,7 +709,7 @@ async fn sic_allocate_instance_external_ip_undo( async fn sic_create_disks_for_instance( sagactx: NexusActionContext, ) -> Result, ActionError> { - let osagactx: &Arc = sagactx.user_data(); + let osagactx = sagactx.user_data(); let disk_params = sagactx.saga_params::()?; let saga_params = disk_params.saga_params; let disk_index = disk_params.which; @@ -740,10 +740,11 @@ async fn sic_create_disks_for_instance( &create_params, ) .await + .map_err(ActionError::action_failed)?; } - _ => Ok(None), - } + _ => {} + }; Ok(None) } @@ -799,8 +800,8 @@ async fn ensure_instance_disk_attach_state( params::InstanceDiskAttachment::Create(create_params) => { db::model::Name(create_params.identity.name.clone()) } - params::InstanceDiskAttachment::Existing { name } => { - db::model::Name(name.clone()) + params::InstanceDiskAttachment::Attach(attach_params) => { + db::model::Name(attach_params.name.clone()) } }; From 59a7dc6cee9a2cc521f296ce5999e4bce259a00e Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 25 Oct 2022 18:16:12 -0500 Subject: [PATCH 05/24] Implement undo --- nexus/src/app/sagas/instance_create.rs | 41 ++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 00825f669c6..dba92d6c80e 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -705,7 +705,6 @@ async fn sic_allocate_instance_external_ip_undo( } /// Create disks during instance creation, and return a list of disk names -// TODO implement async fn sic_create_disks_for_instance( sagactx: NexusActionContext, ) -> Result, ActionError> { @@ -750,10 +749,46 @@ async fn sic_create_disks_for_instance( } /// Undo disks created during instance creation -// TODO implement async fn sic_create_disks_for_instance_undo( - _sagactx: NexusActionContext, + sagactx: NexusActionContext, ) -> Result<(), anyhow::Error> { + let osagactx = sagactx.user_data(); + let disk_params = sagactx.saga_params::()?; + let saga_params = disk_params.saga_params; + let disk_index = disk_params.which; + let saga_disks = &saga_params.create_params.disks; + let opctx = + OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); + + if disk_index >= saga_disks.len() { + return Ok(()); + } + + let disk = &saga_disks[disk_index]; + + // TODO-correctness TODO-security It's not correct to re-resolve the + // organization and project names now. See oxidecomputer/omicron#1536. + let organization_name: db::model::Name = + saga_params.organization_name.clone().into(); + let project_name: db::model::Name = saga_params.project_name.clone().into(); + + match disk { + params::InstanceDiskAttachment::Create(disk_create) => { + osagactx + .nexus() + .project_delete_disk( + &opctx, + &organization_name, + &project_name, + &disk_create.identity.name.clone().into(), + ) + .await + .map_err(ActionError::action_failed)?; + } + + _ => {} + }; + Ok(()) } From f8c0a5ad06341960fc5c17a644194edc09a04d05 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Wed, 26 Oct 2022 19:52:45 -0500 Subject: [PATCH 06/24] Update subsaga_append to take a dag instead of builder --- nexus/src/app/sagas/instance_create.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index dba92d6c80e..33d09329566 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -179,7 +179,7 @@ impl NexusSaga for SagaInstanceCreate { // Helper function for appending subsagas to our parent saga. fn subsaga_append( node_basename: &'static str, - subsaga_builder: steno::DagBuilder, + subsaga_dag: steno::Dag, parent_builder: &mut steno::DagBuilder, params: S, which: usize, @@ -198,7 +198,7 @@ impl NexusSaga for SagaInstanceCreate { let output_name = format!("{}{}", node_basename, which); parent_builder.append(Node::subsaga( output_name.as_str(), - subsaga_builder.build()?, + subsaga_dag, params_node_name, )); Ok(()) @@ -243,7 +243,7 @@ impl NexusSaga for SagaInstanceCreate { )); subsaga_append( "network_interface", - subsaga_builder, + subsaga_builder.build()?, &mut builder, repeat_params, i, @@ -281,7 +281,7 @@ impl NexusSaga for SagaInstanceCreate { )); subsaga_append( "external_ip", - subsaga_builder, + subsaga_builder.build()?, &mut builder, repeat_params, i, @@ -308,7 +308,7 @@ impl NexusSaga for SagaInstanceCreate { )); subsaga_append( "disk", - subsaga_builder, + subsaga_builder.build()?, &mut builder, disk_params, i, From 09447c20716761f762d00f0cf71dab1d4486e3e7 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Wed, 26 Oct 2022 22:05:46 -0500 Subject: [PATCH 07/24] Wire up disk create subsaga --- nexus/src/app/sagas/instance_create.rs | 124 +++++-------------------- 1 file changed, 24 insertions(+), 100 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 33d09329566..67c739d0762 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -3,6 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use super::{NexusActionContext, NexusSaga, SagaInitError, ACTION_GENERATE_ID}; +use crate::app::sagas::disk_create::{self, SagaDiskCreate}; use crate::app::sagas::NexusAction; use crate::app::{ MAX_DISKS_PER_INSTANCE, MAX_EXTERNAL_IPS_PER_INSTANCE, @@ -103,11 +104,6 @@ lazy_static! { sic_allocate_instance_external_ip, sic_allocate_instance_external_ip_undo, ); - static ref CREATE_DISKS_FOR_INSTANCE: NexusAction = ActionFunc::new_action( - "instance-create.create-disks-for-instance", - sic_create_disks_for_instance, - sic_create_disks_for_instance_undo, - ); static ref ATTACH_DISKS_TO_INSTANCE: NexusAction = ActionFunc::new_action( "instance-create.attach-disks-to-instance", sic_attach_disks_to_instance, @@ -134,7 +130,6 @@ impl NexusSaga for SagaInstanceCreate { registry.register(Arc::clone(&*CREATE_NETWORK_INTERFACE)); registry.register(Arc::clone(&*CREATE_SNAT_IP)); registry.register(Arc::clone(&*CREATE_EXTERNAL_IP)); - registry.register(Arc::clone(&*CREATE_DISKS_FOR_INSTANCE)); registry.register(Arc::clone(&*ATTACH_DISKS_TO_INSTANCE)); registry.register(Arc::clone(&*INSTANCE_ENSURE)); } @@ -288,19 +283,36 @@ impl NexusSaga for SagaInstanceCreate { )?; } + for (i, disk) in params.create_params.disks.iter().enumerate() { + if let params::InstanceDiskAttachment::Create(create_disk) = disk { + let subsaga_name = + SagaName::new(&format!("instance-create-disk-{i}")); + let subsaga_builder = DagBuilder::new(subsaga_name); + subsaga_append( + "create-disk", + SagaDiskCreate::make_saga_dag( + &disk_create::Params { + serialized_authn: params.serialized_authn.clone(), + project_id: params.project_id.clone(), + create_params: create_disk.clone(), + }, + subsaga_builder, + )?, + &mut builder, + create_disk, + i, + )?; + } + } + // See the comment above where we add nodes for creating NICs. We use // the same pattern here. for i in 0..(MAX_DISKS_PER_INSTANCE as usize) { let disk_params = DiskParams { saga_params: params.clone(), which: i }; let subsaga_name = - SagaName::new(&format!("instance-create-disk{i}")); + SagaName::new(&format!("instance-attach-disk-{i}")); let mut subsaga_builder = DagBuilder::new(subsaga_name); - subsaga_builder.append(Node::action( - "create_disk_output", - format!("CreateDisksForInstance-{i}").as_str(), - CREATE_DISKS_FOR_INSTANCE.as_ref(), - )); subsaga_builder.append(Node::action( "attach_disk_output", format!("AttachDisksToInstance-{i}").as_str(), @@ -704,94 +716,6 @@ async fn sic_allocate_instance_external_ip_undo( Ok(()) } -/// Create disks during instance creation, and return a list of disk names -async fn sic_create_disks_for_instance( - sagactx: NexusActionContext, -) -> Result, ActionError> { - let osagactx = sagactx.user_data(); - let disk_params = sagactx.saga_params::()?; - let saga_params = disk_params.saga_params; - let disk_index = disk_params.which; - let saga_disks = &saga_params.create_params.disks; - let opctx = - OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); - - if disk_index >= saga_disks.len() { - return Ok(None); - } - - let disk = &saga_disks[disk_index]; - - // TODO-correctness TODO-security It's not correct to re-resolve the - // organization and project names now. See oxidecomputer/omicron#1536. - let organization_name: db::model::Name = - saga_params.organization_name.clone().into(); - let project_name: db::model::Name = saga_params.project_name.clone().into(); - - match disk { - params::InstanceDiskAttachment::Create(create_params) => { - osagactx - .nexus() - .project_create_disk( - &opctx, - &organization_name, - &project_name, - &create_params, - ) - .await - .map_err(ActionError::action_failed)?; - } - - _ => {} - }; - - Ok(None) -} - -/// Undo disks created during instance creation -async fn sic_create_disks_for_instance_undo( - sagactx: NexusActionContext, -) -> Result<(), anyhow::Error> { - let osagactx = sagactx.user_data(); - let disk_params = sagactx.saga_params::()?; - let saga_params = disk_params.saga_params; - let disk_index = disk_params.which; - let saga_disks = &saga_params.create_params.disks; - let opctx = - OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); - - if disk_index >= saga_disks.len() { - return Ok(()); - } - - let disk = &saga_disks[disk_index]; - - // TODO-correctness TODO-security It's not correct to re-resolve the - // organization and project names now. See oxidecomputer/omicron#1536. - let organization_name: db::model::Name = - saga_params.organization_name.clone().into(); - let project_name: db::model::Name = saga_params.project_name.clone().into(); - - match disk { - params::InstanceDiskAttachment::Create(disk_create) => { - osagactx - .nexus() - .project_delete_disk( - &opctx, - &organization_name, - &project_name, - &disk_create.identity.name.clone().into(), - ) - .await - .map_err(ActionError::action_failed)?; - } - - _ => {} - }; - - Ok(()) -} - async fn sic_attach_disks_to_instance( sagactx: NexusActionContext, ) -> Result<(), ActionError> { From cf1fc5b98ca5f2c3a12b535d2df739373e047c76 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Wed, 26 Oct 2022 22:19:31 -0500 Subject: [PATCH 08/24] Fix lint error --- nexus/src/app/sagas/instance_create.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 67c739d0762..30f761a67e7 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -293,7 +293,7 @@ impl NexusSaga for SagaInstanceCreate { SagaDiskCreate::make_saga_dag( &disk_create::Params { serialized_authn: params.serialized_authn.clone(), - project_id: params.project_id.clone(), + project_id: params.project_id, create_params: create_disk.clone(), }, subsaga_builder, From c1bf2da8c0c7687f5f3919ac067421d75fa7a694 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 27 Oct 2022 10:55:33 -0500 Subject: [PATCH 09/24] Update fn name to be singular to clarify it attaches 1 disk --- nexus/src/app/sagas/instance_create.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 30f761a67e7..48a7c869344 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -106,8 +106,8 @@ lazy_static! { ); static ref ATTACH_DISKS_TO_INSTANCE: NexusAction = ActionFunc::new_action( "instance-create.attach-disks-to-instance", - sic_attach_disks_to_instance, - sic_attach_disks_to_instance_undo, + sic_attach_disk_to_instance, + sic_attach_disk_to_instance_undo, ); static ref INSTANCE_ENSURE: NexusAction = new_action_noop_undo( "instance-create.instance-ensure", @@ -716,13 +716,13 @@ async fn sic_allocate_instance_external_ip_undo( Ok(()) } -async fn sic_attach_disks_to_instance( +async fn sic_attach_disk_to_instance( sagactx: NexusActionContext, ) -> Result<(), ActionError> { ensure_instance_disk_attach_state(sagactx, true).await } -async fn sic_attach_disks_to_instance_undo( +async fn sic_attach_disk_to_instance_undo( sagactx: NexusActionContext, ) -> Result<(), anyhow::Error> { Ok(ensure_instance_disk_attach_state(sagactx, false).await?) From 91262236e707bea0c8c7ed53e84c58db80f36a33 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 27 Oct 2022 12:00:38 -0500 Subject: [PATCH 10/24] Improve attach/detaching to not create a saga as a node --- nexus/src/app/sagas/instance_create.rs | 58 ++++++++++++-------------- 1 file changed, 26 insertions(+), 32 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 48a7c869344..1325945acee 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -735,25 +735,13 @@ async fn ensure_instance_disk_attach_state( let osagactx = sagactx.user_data(); let disk_params = sagactx.saga_params::()?; let saga_params = disk_params.saga_params; - let disk_index = disk_params.which; + let datastore = osagactx.datastore(); let opctx = OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); + let instance_id = sagactx.lookup::("instance_id")?; + let project_id = saga_params.project_id; - let saga_disks = &saga_params.create_params.disks; - let instance_name = - db::model::Name(saga_params.create_params.identity.name); - - if disk_index >= saga_disks.len() { - return Ok(()); - } - - let disk = &saga_disks[disk_index]; - - // TODO-correctness TODO-security It's not correct to re-resolve the - // organization and project names now. See oxidecomputer/omicron#1536. - let organization_name: db::model::Name = - saga_params.organization_name.clone().into(); - let project_name: db::model::Name = saga_params.project_name.clone().into(); + let disk = &saga_params.create_params.disks[disk_params.which]; let disk_name = match disk { params::InstanceDiskAttachment::Create(create_params) => { @@ -764,30 +752,36 @@ async fn ensure_instance_disk_attach_state( } }; + let (.., authz_instance, _db_instance) = + LookupPath::new(&opctx, &datastore) + .instance_id(instance_id) + .fetch() + .await + .map_err(ActionError::action_failed)?; + + let (.., authz_disk, _db_disk) = LookupPath::new(&opctx, &datastore) + .project_id(project_id) + .disk_name(&disk_name) + .fetch() + .await + .map_err(ActionError::action_failed)?; + if attached { - osagactx - .nexus() + datastore .instance_attach_disk( &opctx, - &organization_name, - &project_name, - &instance_name, - &disk_name, + &authz_instance, + &authz_disk, + MAX_DISKS_PER_INSTANCE, ) .await + .map_err(ActionError::action_failed)?; } else { - osagactx - .nexus() - .instance_detach_disk( - &opctx, - &organization_name, - &project_name, - &instance_name, - &disk_name, - ) + datastore + .instance_detach_disk(&opctx, &authz_instance, &authz_disk) .await + .map_err(ActionError::action_failed)?; } - .map_err(ActionError::action_failed)?; Ok(()) } From d66965cc715e6e23def5d2bece4dc4f1872af502 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 27 Oct 2022 12:09:26 -0500 Subject: [PATCH 11/24] Clean up attach iteration logic --- nexus/src/app/sagas/instance_create.rs | 44 ++++++++++++++------------ 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 1325945acee..68996376700 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -283,13 +283,14 @@ impl NexusSaga for SagaInstanceCreate { )?; } + // Create any specified disks for (i, disk) in params.create_params.disks.iter().enumerate() { if let params::InstanceDiskAttachment::Create(create_disk) = disk { let subsaga_name = SagaName::new(&format!("instance-create-disk-{i}")); let subsaga_builder = DagBuilder::new(subsaga_name); subsaga_append( - "create-disk", + "create_disk", SagaDiskCreate::make_saga_dag( &disk_create::Params { serialized_authn: params.serialized_authn.clone(), @@ -305,26 +306,27 @@ impl NexusSaga for SagaInstanceCreate { } } - // See the comment above where we add nodes for creating NICs. We use - // the same pattern here. - for i in 0..(MAX_DISKS_PER_INSTANCE as usize) { - let disk_params = - DiskParams { saga_params: params.clone(), which: i }; - let subsaga_name = - SagaName::new(&format!("instance-attach-disk-{i}")); - let mut subsaga_builder = DagBuilder::new(subsaga_name); - subsaga_builder.append(Node::action( - "attach_disk_output", - format!("AttachDisksToInstance-{i}").as_str(), - ATTACH_DISKS_TO_INSTANCE.as_ref(), - )); - subsaga_append( - "disk", - subsaga_builder.build()?, - &mut builder, - disk_params, - i, - )?; + // Attach any specified disks, including those previously created + for (i, disk) in params.create_params.disks.iter().enumerate() { + if let params::InstanceDiskAttachment::Attach(_) = disk { + let disk_params = + DiskParams { saga_params: params.clone(), which: i }; + let subsaga_name = + SagaName::new(&format!("instance-attach-disk-{i}")); + let mut subsaga_builder = DagBuilder::new(subsaga_name); + subsaga_builder.append(Node::action( + "attach_disk_output", + format!("AttachDisksToInstance-{i}").as_str(), + ATTACH_DISKS_TO_INSTANCE.as_ref(), + )); + subsaga_append( + "attach_disk", + subsaga_builder.build()?, + &mut builder, + disk_params, + i, + )?; + } } builder.append(Node::action( From 5da701453e5e1756d15415bbff26663c80c0a47d Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 27 Oct 2022 12:21:15 -0500 Subject: [PATCH 12/24] Pass direct references down instead of disk array --- nexus/src/app/sagas/instance_create.rs | 62 ++++++++++---------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 68996376700..8f466cfe864 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -18,6 +18,7 @@ use crate::{authn, authz, db}; use chrono::Utc; use lazy_static::lazy_static; use nexus_defaults::DEFAULT_PRIMARY_NIC_NAME; +use nexus_types::external_api::params::InstanceDiskAttachment; use omicron_common::api::external::Error; use omicron_common::api::external::Generation; use omicron_common::api::external::IdentityMetadataCreateParams; @@ -62,14 +63,6 @@ struct NetParams { new_id: Uuid, } -// The disk-related nodes get a similar treatment, but the data they need are -// different. -#[derive(Debug, Deserialize, Serialize)] -struct DiskParams { - saga_params: Params, - which: usize, -} - // instance create saga: actions lazy_static! { @@ -285,7 +278,7 @@ impl NexusSaga for SagaInstanceCreate { // Create any specified disks for (i, disk) in params.create_params.disks.iter().enumerate() { - if let params::InstanceDiskAttachment::Create(create_disk) = disk { + if let InstanceDiskAttachment::Create(create_disk) = disk { let subsaga_name = SagaName::new(&format!("instance-create-disk-{i}")); let subsaga_builder = DagBuilder::new(subsaga_name); @@ -308,25 +301,21 @@ impl NexusSaga for SagaInstanceCreate { // Attach any specified disks, including those previously created for (i, disk) in params.create_params.disks.iter().enumerate() { - if let params::InstanceDiskAttachment::Attach(_) = disk { - let disk_params = - DiskParams { saga_params: params.clone(), which: i }; - let subsaga_name = - SagaName::new(&format!("instance-attach-disk-{i}")); - let mut subsaga_builder = DagBuilder::new(subsaga_name); - subsaga_builder.append(Node::action( - "attach_disk_output", - format!("AttachDisksToInstance-{i}").as_str(), - ATTACH_DISKS_TO_INSTANCE.as_ref(), - )); - subsaga_append( - "attach_disk", - subsaga_builder.build()?, - &mut builder, - disk_params, - i, - )?; - } + let subsaga_name = + SagaName::new(&format!("instance-attach-disk-{i}")); + let mut subsaga_builder = DagBuilder::new(subsaga_name); + subsaga_builder.append(Node::action( + "attach_disk_output", + format!("AttachDisksToInstance-{i}").as_str(), + ATTACH_DISKS_TO_INSTANCE.as_ref(), + )); + subsaga_append( + "attach_disk", + subsaga_builder.build()?, + &mut builder, + disk, + i, + )?; } builder.append(Node::action( @@ -735,21 +724,18 @@ async fn ensure_instance_disk_attach_state( attached: bool, ) -> Result<(), ActionError> { let osagactx = sagactx.user_data(); - let disk_params = sagactx.saga_params::()?; - let saga_params = disk_params.saga_params; + let disk_attachment = sagactx.saga_params::()?; + let params = sagactx.saga_params::()?; let datastore = osagactx.datastore(); - let opctx = - OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); + let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); let instance_id = sagactx.lookup::("instance_id")?; - let project_id = saga_params.project_id; - - let disk = &saga_params.create_params.disks[disk_params.which]; + let project_id = params.project_id; - let disk_name = match disk { - params::InstanceDiskAttachment::Create(create_params) => { + let disk_name = match disk_attachment { + InstanceDiskAttachment::Create(create_params) => { db::model::Name(create_params.identity.name.clone()) } - params::InstanceDiskAttachment::Attach(attach_params) => { + InstanceDiskAttachment::Attach(attach_params) => { db::model::Name(attach_params.name.clone()) } }; From 6dcaa2732e2018cf84dd2d8a8956a75e10637e04 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 27 Oct 2022 12:38:49 -0500 Subject: [PATCH 13/24] Fix disk create subsaga params; clean up comments --- nexus/src/app/sagas/instance_create.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 8f466cfe864..a6568df98de 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -276,31 +276,30 @@ impl NexusSaga for SagaInstanceCreate { )?; } - // Create any specified disks + // Appends the disk create saga as a subsaga directly to the instance create builder. for (i, disk) in params.create_params.disks.iter().enumerate() { if let InstanceDiskAttachment::Create(create_disk) = disk { let subsaga_name = SagaName::new(&format!("instance-create-disk-{i}")); let subsaga_builder = DagBuilder::new(subsaga_name); + let params = &disk_create::Params { + serialized_authn: params.serialized_authn.clone(), + project_id: params.project_id, + create_params: create_disk.clone(), + }; subsaga_append( "create_disk", - SagaDiskCreate::make_saga_dag( - &disk_create::Params { - serialized_authn: params.serialized_authn.clone(), - project_id: params.project_id, - create_params: create_disk.clone(), - }, - subsaga_builder, - )?, + SagaDiskCreate::make_saga_dag(params, subsaga_builder)?, &mut builder, - create_disk, + params, i, )?; } } - // Attach any specified disks, including those previously created - for (i, disk) in params.create_params.disks.iter().enumerate() { + // Attaches all disks included in the instance create request, including those which were previously created + // by the disk create subsagas. + for (i, disk_attach) in params.create_params.disks.iter().enumerate() { let subsaga_name = SagaName::new(&format!("instance-attach-disk-{i}")); let mut subsaga_builder = DagBuilder::new(subsaga_name); @@ -313,7 +312,7 @@ impl NexusSaga for SagaInstanceCreate { "attach_disk", subsaga_builder.build()?, &mut builder, - disk, + disk_attach, i, )?; } From 8d0c3ce8106444a81f26609657460a0c070db6ea Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 27 Oct 2022 12:45:43 -0500 Subject: [PATCH 14/24] Appease ye almighty clippy --- nexus/src/app/sagas/instance_create.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index a6568df98de..f8cf997325b 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -732,10 +732,10 @@ async fn ensure_instance_disk_attach_state( let disk_name = match disk_attachment { InstanceDiskAttachment::Create(create_params) => { - db::model::Name(create_params.identity.name.clone()) + db::model::Name(create_params.identity.name) } InstanceDiskAttachment::Attach(attach_params) => { - db::model::Name(attach_params.name.clone()) + db::model::Name(attach_params.name) } }; From 3927efedfa8e89c41460ef33697bdbdefb34ad4d Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 27 Oct 2022 14:14:46 -0500 Subject: [PATCH 15/24] Update attach params shape to include serialized_athn; project_id --- nexus/src/app/sagas/instance_create.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index f8cf997325b..d80ab2ccfad 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -63,6 +63,13 @@ struct NetParams { new_id: Uuid, } +#[derive(Debug, Deserialize, Serialize)] +struct DiskAttachParams { + serialized_authn: authn::saga::Serialized, + project_id: Uuid, + attach_params: InstanceDiskAttachment, +} + // instance create saga: actions lazy_static! { @@ -308,11 +315,16 @@ impl NexusSaga for SagaInstanceCreate { format!("AttachDisksToInstance-{i}").as_str(), ATTACH_DISKS_TO_INSTANCE.as_ref(), )); + let params = DiskAttachParams { + serialized_authn: params.serialized_authn.clone(), + project_id: params.project_id, + attach_params: disk_attach.clone(), + }; subsaga_append( "attach_disk", subsaga_builder.build()?, &mut builder, - disk_attach, + params, i, )?; } @@ -723,14 +735,13 @@ async fn ensure_instance_disk_attach_state( attached: bool, ) -> Result<(), ActionError> { let osagactx = sagactx.user_data(); - let disk_attachment = sagactx.saga_params::()?; - let params = sagactx.saga_params::()?; + let params = sagactx.saga_params::()?; let datastore = osagactx.datastore(); let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); let instance_id = sagactx.lookup::("instance_id")?; let project_id = params.project_id; - let disk_name = match disk_attachment { + let disk_name = match params.attach_params { InstanceDiskAttachment::Create(create_params) => { db::model::Name(create_params.identity.name) } From 033f5839a49559ee17f2b180e87b69f85c1b4647 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 27 Oct 2022 23:45:05 -0500 Subject: [PATCH 16/24] Address nit --- nexus/src/app/sagas/instance_create.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index d80ab2ccfad..06be9d78b2d 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -289,14 +289,14 @@ impl NexusSaga for SagaInstanceCreate { let subsaga_name = SagaName::new(&format!("instance-create-disk-{i}")); let subsaga_builder = DagBuilder::new(subsaga_name); - let params = &disk_create::Params { + let params = disk_create::Params { serialized_authn: params.serialized_authn.clone(), project_id: params.project_id, create_params: create_disk.clone(), }; subsaga_append( "create_disk", - SagaDiskCreate::make_saga_dag(params, subsaga_builder)?, + SagaDiskCreate::make_saga_dag(¶ms, subsaga_builder)?, &mut builder, params, i, From be534b9606d01dca026e89398b177730d9a8133b Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 28 Oct 2022 16:17:08 -0500 Subject: [PATCH 17/24] Add TODO on correctness of disk lookup by name --- nexus/src/app/sagas/instance_create.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 06be9d78b2d..865028dfc7f 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -757,6 +757,8 @@ async fn ensure_instance_disk_attach_state( .await .map_err(ActionError::action_failed)?; + // TODO-correctness TODO-security It's not correct to re-resolve the + // disk name now. See oxidecomputer/omicron#1536. let (.., authz_disk, _db_disk) = LookupPath::new(&opctx, &datastore) .project_id(project_id) .disk_name(&disk_name) From d21ce3e2463fd1167dd7a0e743201fcc76877ce9 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 31 Oct 2022 11:49:31 -0500 Subject: [PATCH 18/24] instance_id isn't in the subsaga context, pass it down instead --- nexus/src/app/sagas/instance_create.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 865028dfc7f..d8fef02349c 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -67,6 +67,7 @@ struct NetParams { struct DiskAttachParams { serialized_authn: authn::saga::Serialized, project_id: Uuid, + instance_id: Uuid, attach_params: InstanceDiskAttachment, } @@ -318,6 +319,7 @@ impl NexusSaga for SagaInstanceCreate { let params = DiskAttachParams { serialized_authn: params.serialized_authn.clone(), project_id: params.project_id, + instance_id: instance_id.clone(), attach_params: disk_attach.clone(), }; subsaga_append( @@ -738,7 +740,7 @@ async fn ensure_instance_disk_attach_state( let params = sagactx.saga_params::()?; let datastore = osagactx.datastore(); let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); - let instance_id = sagactx.lookup::("instance_id")?; + let instance_id = params.instance_id; let project_id = params.project_id; let disk_name = match params.attach_params { From d68e99b94e2e3bd02b233e492d382f0f61c452f8 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 31 Oct 2022 12:16:37 -0500 Subject: [PATCH 19/24] Fix clippy failures --- nexus/src/app/sagas/instance_create.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index d8fef02349c..91f74de1fd1 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -319,7 +319,7 @@ impl NexusSaga for SagaInstanceCreate { let params = DiskAttachParams { serialized_authn: params.serialized_authn.clone(), project_id: params.project_id, - instance_id: instance_id.clone(), + instance_id, attach_params: disk_attach.clone(), }; subsaga_append( From 1a673a432ddc24b95e9efd6335cf185e31fecba6 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 31 Oct 2022 12:54:53 -0500 Subject: [PATCH 20/24] Add integration test to create and attach disks via instance create --- nexus/tests/integration_tests/instances.rs | 89 ++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 170e03cfe15..87890fa548f 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -1901,6 +1901,95 @@ async fn test_instance_fails_to_boot_with_disk( assert_eq!(disks[0].state, DiskState::Detached); } +#[nexus_test] +async fn test_instance_create_attach_disks( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + const POOL_NAME: &str = "p0"; + const ORGANIZATION_NAME: &str = "bobs-barrel-of-bytes"; + const PROJECT_NAME: &str = "bit-barrel"; + + // Test pre-reqs + DiskTest::new(&cptestctx).await; + create_ip_pool(&client, POOL_NAME, None, None).await; + create_organization(&client, ORGANIZATION_NAME).await; + create_project(client, ORGANIZATION_NAME, PROJECT_NAME).await; + create_disk(&client, ORGANIZATION_NAME, PROJECT_NAME, "probablydata2") + .await; + + let instance_params = params::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("nfs")).unwrap(), + description: String::from("probably serving data"), + }, + ncpus: InstanceCpuCount::try_from(2).unwrap(), + memory: ByteCount::from_gibibytes_u32(4), + hostname: String::from("nfs"), + user_data: vec![], + network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, + external_ips: vec![], + disks: vec![ + params::InstanceDiskAttachment::Create(params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("probablydata")).unwrap(), + description: String::from("probably data"), + }, + size: ByteCount::from_gibibytes_u32(10), + disk_source: params::DiskSource::Blank { + block_size: params::BlockSize::try_from(512).unwrap(), + }, + }), + params::InstanceDiskAttachment::Attach( + params::InstanceDiskAttach { + name: Name::try_from(String::from("probablydata2")) + .unwrap(), + }, + ), + ], + start: true, + }; + + let url_instances = format!( + "/organizations/{}/projects/{}/instances", + ORGANIZATION_NAME, PROJECT_NAME + ); + + let builder = + RequestBuilder::new(client, http::Method::POST, &url_instances) + .body(Some(&instance_params)) + .expect_status(Some(http::StatusCode::CREATED)); + + let response = NexusRequest::new(builder) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Expected instance creation!"); + + let instance = response.parsed_body::().unwrap(); + + // Assert disks are created and attached + let url_project_disks = format!( + "/organizations/{}/projects/{}/disks", + ORGANIZATION_NAME, PROJECT_NAME, + ); + let disks: Vec = NexusRequest::iter_collection_authn( + client, + &url_project_disks, + "", + None, + ) + .await + .expect("failed to list disks") + .all_items; + assert_eq!(disks.len(), 2); + + for disk in disks { + assert_eq!(disk.state, DiskState::Attached(instance.identity.id)); + } +} + // Test that 8 disks is supported #[nexus_test] async fn test_attach_eight_disks_to_instance( From d3bbae0f8758e5b4efe4e10a49ee4d020279105b Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 31 Oct 2022 13:45:38 -0500 Subject: [PATCH 21/24] Add test for undos of creates/attaches for failed instance create saga --- nexus/tests/integration_tests/instances.rs | 129 +++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 87890fa548f..a5d692b9e8d 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -1990,6 +1990,135 @@ async fn test_instance_create_attach_disks( } } +#[nexus_test] +async fn test_instance_create_attach_disks_undo( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + const POOL_NAME: &str = "p0"; + const ORGANIZATION_NAME: &str = "bobs-barrel-of-bytes"; + const PROJECT_NAME: &str = "bit-barrel"; + + // Test pre-reqs + DiskTest::new(&cptestctx).await; + create_ip_pool(&client, POOL_NAME, None, None).await; + create_organization(&client, ORGANIZATION_NAME).await; + create_project(client, ORGANIZATION_NAME, PROJECT_NAME).await; + create_disk(&client, ORGANIZATION_NAME, PROJECT_NAME, "probablydata2") + .await; + let faulted_disk = + create_disk(&client, ORGANIZATION_NAME, PROJECT_NAME, "probablydata3") + .await; + + let url_project_disks = format!( + "/organizations/{}/projects/{}/disks", + ORGANIZATION_NAME, PROJECT_NAME, + ); + + // fault probablydata3 + let apictx = &cptestctx.server.apictx; + let nexus = &apictx.nexus; + assert!(nexus + .set_disk_as_faulted(&faulted_disk.identity.id) + .await + .unwrap()); + + // Assert FAULTED + let disks: Vec = NexusRequest::iter_collection_authn( + client, + &url_project_disks, + "", + None, + ) + .await + .expect("failed to list disks") + .all_items; + assert_eq!(disks.len(), 2); + + for (i, disk) in disks.iter().enumerate() { + if i == 0 { + assert_eq!(disk.state, DiskState::Detached); + } else { + assert_eq!(disk.state, DiskState::Faulted); + } + } + + let instance_params = params::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("nfs")).unwrap(), + description: String::from("probably serving data"), + }, + ncpus: InstanceCpuCount::try_from(2).unwrap(), + memory: ByteCount::from_gibibytes_u32(4), + hostname: String::from("nfs"), + user_data: vec![], + network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, + external_ips: vec![], + disks: vec![ + params::InstanceDiskAttachment::Create(params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("probablydata")).unwrap(), + description: String::from("probably data"), + }, + size: ByteCount::from_gibibytes_u32(5), + disk_source: params::DiskSource::Blank { + block_size: params::BlockSize::try_from(512).unwrap(), + }, + }), + params::InstanceDiskAttachment::Attach( + params::InstanceDiskAttach { + name: Name::try_from(String::from("probablydata2")) + .unwrap(), + }, + ), + params::InstanceDiskAttachment::Attach( + params::InstanceDiskAttach { + name: Name::try_from(String::from("probablydata3")) + .unwrap(), + }, + ), + ], + start: true, + }; + + let url_instances = format!( + "/organizations/{}/projects/{}/instances", + ORGANIZATION_NAME, PROJECT_NAME + ); + + let builder = + RequestBuilder::new(client, http::Method::POST, &url_instances) + .body(Some(&instance_params)) + .expect_status(Some(http::StatusCode::BAD_REQUEST)); + + let _response = NexusRequest::new(builder) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Expected instance creation to fail!"); + + // Assert disks are created and attached + let disks: Vec = NexusRequest::iter_collection_authn( + client, + &url_project_disks, + "", + None, + ) + .await + .expect("failed to list disks") + .all_items; + assert_eq!(disks.len(), 2); + + for (i, disk) in disks.iter().enumerate() { + if i == 0 { + assert_eq!(disk.state, DiskState::Detached); + } else { + assert_eq!(disk.state, DiskState::Faulted); + } + } +} + // Test that 8 disks is supported #[nexus_test] async fn test_attach_eight_disks_to_instance( From 445cda7d9ad5cfca3fd3ce8df0038c94d7edcfe3 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 31 Oct 2022 15:09:32 -0500 Subject: [PATCH 22/24] Reduce disk sizes to hopefully resolve zpool failures --- nexus/tests/integration_tests/instances.rs | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index a5d692b9e8d..fb4d4d5098d 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -1907,15 +1907,9 @@ async fn test_instance_create_attach_disks( ) { let client = &cptestctx.external_client; - const POOL_NAME: &str = "p0"; - const ORGANIZATION_NAME: &str = "bobs-barrel-of-bytes"; - const PROJECT_NAME: &str = "bit-barrel"; - // Test pre-reqs DiskTest::new(&cptestctx).await; - create_ip_pool(&client, POOL_NAME, None, None).await; - create_organization(&client, ORGANIZATION_NAME).await; - create_project(client, ORGANIZATION_NAME, PROJECT_NAME).await; + create_org_and_project(&client).await; create_disk(&client, ORGANIZATION_NAME, PROJECT_NAME, "probablydata2") .await; @@ -1925,7 +1919,7 @@ async fn test_instance_create_attach_disks( description: String::from("probably serving data"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_gibibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(3), hostname: String::from("nfs"), user_data: vec![], network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, @@ -1936,7 +1930,7 @@ async fn test_instance_create_attach_disks( name: Name::try_from(String::from("probablydata")).unwrap(), description: String::from("probably data"), }, - size: ByteCount::from_gibibytes_u32(10), + size: ByteCount::from_gibibytes_u32(4), disk_source: params::DiskSource::Blank { block_size: params::BlockSize::try_from(512).unwrap(), }, @@ -1996,15 +1990,9 @@ async fn test_instance_create_attach_disks_undo( ) { let client = &cptestctx.external_client; - const POOL_NAME: &str = "p0"; - const ORGANIZATION_NAME: &str = "bobs-barrel-of-bytes"; - const PROJECT_NAME: &str = "bit-barrel"; - // Test pre-reqs DiskTest::new(&cptestctx).await; - create_ip_pool(&client, POOL_NAME, None, None).await; - create_organization(&client, ORGANIZATION_NAME).await; - create_project(client, ORGANIZATION_NAME, PROJECT_NAME).await; + create_org_and_project(&client).await; create_disk(&client, ORGANIZATION_NAME, PROJECT_NAME, "probablydata2") .await; let faulted_disk = @@ -2061,7 +2049,7 @@ async fn test_instance_create_attach_disks_undo( name: Name::try_from(String::from("probablydata")).unwrap(), description: String::from("probably data"), }, - size: ByteCount::from_gibibytes_u32(5), + size: ByteCount::from_gibibytes_u32(4), disk_source: params::DiskSource::Blank { block_size: params::BlockSize::try_from(512).unwrap(), }, From 5f618ef320b0828020ad189fce27e469e8c1584c Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 1 Nov 2022 14:36:56 -0500 Subject: [PATCH 23/24] Address PR feedback on tests --- nexus/tests/integration_tests/instances.rs | 65 +++++++++++----------- 1 file changed, 31 insertions(+), 34 deletions(-) diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index fb4d4d5098d..4ad235516ab 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -1910,8 +1910,13 @@ async fn test_instance_create_attach_disks( // Test pre-reqs DiskTest::new(&cptestctx).await; create_org_and_project(&client).await; - create_disk(&client, ORGANIZATION_NAME, PROJECT_NAME, "probablydata2") - .await; + let attachable_disk = create_disk( + &client, + ORGANIZATION_NAME, + PROJECT_NAME, + "attachable-disk", + ) + .await; let instance_params = params::InstanceCreate { identity: IdentityMetadataCreateParams { @@ -1927,8 +1932,10 @@ async fn test_instance_create_attach_disks( disks: vec![ params::InstanceDiskAttachment::Create(params::DiskCreate { identity: IdentityMetadataCreateParams { - name: Name::try_from(String::from("probablydata")).unwrap(), - description: String::from("probably data"), + name: Name::try_from(String::from("created-disk")).unwrap(), + description: String::from( + "A disk that was created by instance create", + ), }, size: ByteCount::from_gibibytes_u32(4), disk_source: params::DiskSource::Blank { @@ -1937,8 +1944,7 @@ async fn test_instance_create_attach_disks( }), params::InstanceDiskAttachment::Attach( params::InstanceDiskAttach { - name: Name::try_from(String::from("probablydata2")) - .unwrap(), + name: attachable_disk.identity.name, }, ), ], @@ -1993,10 +1999,11 @@ async fn test_instance_create_attach_disks_undo( // Test pre-reqs DiskTest::new(&cptestctx).await; create_org_and_project(&client).await; - create_disk(&client, ORGANIZATION_NAME, PROJECT_NAME, "probablydata2") - .await; + let regular_disk = + create_disk(&client, ORGANIZATION_NAME, PROJECT_NAME, "a-reg-disk") + .await; let faulted_disk = - create_disk(&client, ORGANIZATION_NAME, PROJECT_NAME, "probablydata3") + create_disk(&client, ORGANIZATION_NAME, PROJECT_NAME, "faulted-disk") .await; let url_project_disks = format!( @@ -2004,7 +2011,7 @@ async fn test_instance_create_attach_disks_undo( ORGANIZATION_NAME, PROJECT_NAME, ); - // fault probablydata3 + // set `faulted_disk` to the faulted state let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; assert!(nexus @@ -2012,7 +2019,7 @@ async fn test_instance_create_attach_disks_undo( .await .unwrap()); - // Assert FAULTED + // Assert regular and faulted disks were created let disks: Vec = NexusRequest::iter_collection_authn( client, &url_project_disks, @@ -2024,13 +2031,11 @@ async fn test_instance_create_attach_disks_undo( .all_items; assert_eq!(disks.len(), 2); - for (i, disk) in disks.iter().enumerate() { - if i == 0 { - assert_eq!(disk.state, DiskState::Detached); - } else { - assert_eq!(disk.state, DiskState::Faulted); - } - } + assert_eq!(disks[0].identity.id, regular_disk.identity.id); + assert_eq!(disks[0].state, DiskState::Detached); + + assert_eq!(disks[1].identity.id, faulted_disk.identity.id); + assert_eq!(disks[1].state, DiskState::Faulted); let instance_params = params::InstanceCreate { identity: IdentityMetadataCreateParams { @@ -2055,16 +2060,10 @@ async fn test_instance_create_attach_disks_undo( }, }), params::InstanceDiskAttachment::Attach( - params::InstanceDiskAttach { - name: Name::try_from(String::from("probablydata2")) - .unwrap(), - }, + params::InstanceDiskAttach { name: regular_disk.identity.name }, ), params::InstanceDiskAttachment::Attach( - params::InstanceDiskAttach { - name: Name::try_from(String::from("probablydata3")) - .unwrap(), - }, + params::InstanceDiskAttach { name: faulted_disk.identity.name }, ), ], start: true, @@ -2086,7 +2085,7 @@ async fn test_instance_create_attach_disks_undo( .await .expect("Expected instance creation to fail!"); - // Assert disks are created and attached + // Assert disks are in the same state as before the instance creation began let disks: Vec = NexusRequest::iter_collection_authn( client, &url_project_disks, @@ -2098,13 +2097,11 @@ async fn test_instance_create_attach_disks_undo( .all_items; assert_eq!(disks.len(), 2); - for (i, disk) in disks.iter().enumerate() { - if i == 0 { - assert_eq!(disk.state, DiskState::Detached); - } else { - assert_eq!(disk.state, DiskState::Faulted); - } - } + assert_eq!(disks[0].identity.id, regular_disk.identity.id); + assert_eq!(disks[0].state, DiskState::Detached); + + assert_eq!(disks[1].identity.id, faulted_disk.identity.id); + assert_eq!(disks[1].state, DiskState::Faulted); } // Test that 8 disks is supported From 3636fd1ee7b53e86f70355996eaefcc81db027fe Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Wed, 2 Nov 2022 00:06:49 -0500 Subject: [PATCH 24/24] Comment what test is validating --- nexus/tests/integration_tests/instances.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 4ad235516ab..fa803e7ab26 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -1990,6 +1990,9 @@ async fn test_instance_create_attach_disks( } } +/// Tests to ensure that when an error occurs in the instance create saga after +/// some disks are succesfully created and attached, those disks are detached +/// and deleted. #[nexus_test] async fn test_instance_create_attach_disks_undo( cptestctx: &ControlPlaneTestContext,