diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index c6b740d4df3..3a723c2db8b 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -20,6 +20,7 @@ use diesel::prelude::*; use diesel::sql_types::{Bool, Nullable}; use nexus_db_schema::schema::{disk, external_ip, external_subnet, instance}; use nexus_types::external_api::instance as instance_types; +use omicron_common::api::external::Error; use omicron_generation_kinds::{ InstanceStateGeneration, InstanceStateGenerationKind, InstanceUpdaterGeneration, InstanceUpdaterGenerationKind, @@ -152,14 +153,17 @@ pub struct Instance { /// Changes to this field only take effect on the next instance restart. pub enable_jumbo_frames: bool, + // The `shutdown_policy_*` fields are private to avoid leaking the db + // representation of a stronger type; callers should use `shutdown_policy()` + // to get the reconstructed value from these db columns. /// When `shutdown_policy_action` is `PowerButton`, this represents the time /// propolis will wait for the guest to power itself down after sending it /// the power button event, before forcefully terminating it. /// Must be None if the action is `HardOff`. - pub shutdown_policy_timeout: Option, + shutdown_policy_timeout: Option, /// The means by which to stop the instance. - pub shutdown_policy_action: InstanceShutdownAction, + shutdown_policy_action: InstanceShutdownAction, } impl Instance { @@ -322,24 +326,23 @@ impl Instance { } } + /// Reconstruct the [`instance_types::InstanceShutdownPolicy`] of this + /// instance from its database representation. + /// + /// # Errors + /// + /// Fails only if reconstruction fails, which should be impossible as long + /// as this conversion stays consistent with the constraints on the table in + /// CRDB. pub fn shutdown_policy( &self, - ) -> Option { - use instance_types::InstanceShutdownPolicy; - match self.shutdown_policy_action { - InstanceShutdownAction::HardOff => { - Some(InstanceShutdownPolicy::HardOff) - } - InstanceShutdownAction::PowerButton => { - // return None if timeout NULL or negative (enforced by sql-side check) - let timeout_secs: u64 = self - .shutdown_policy_timeout? - .num_seconds() - .try_into() - .ok()?; - Some(InstanceShutdownPolicy::PowerButton { timeout_secs }) - } + ) -> Result { + InstanceShutdownPolicyColumns { + id: self.identity.id, + shutdown_policy_timeout: self.shutdown_policy_timeout, + shutdown_policy_action: self.shutdown_policy_action, } + .shutdown_policy() } } @@ -378,6 +381,62 @@ impl DatastoreAttachTargetConfig for Instance { type ResourceTimeDeletedColumn = external_subnet::dsl::time_deleted; } +/// Just the columns needed to decode an instance's shutdown policy. +/// +/// Used by queries that join against `instance` but only care about the +/// shutdown policy. +#[derive(Queryable, Selectable, Clone, Debug)] +#[diesel(table_name = instance)] +pub struct InstanceShutdownPolicyColumns { + id: Uuid, + shutdown_policy_timeout: Option, + shutdown_policy_action: InstanceShutdownAction, +} + +impl InstanceShutdownPolicyColumns { + /// Reconstruct the [`instance_types::InstanceShutdownPolicy`] of this + /// instance from its database representation. + /// + /// # Errors + /// + /// Fails only if reconstruction fails, which should be impossible as long + /// as this conversion stays consistent with the constraints on the table in + /// CRDB. + pub fn shutdown_policy( + &self, + ) -> Result { + use instance_types::InstanceShutdownPolicy; + match self.shutdown_policy_action { + InstanceShutdownAction::HardOff => { + Ok(InstanceShutdownPolicy::HardOff) + } + InstanceShutdownAction::PowerButton => { + // DB constraints require `shutdown_policy_timeout` to be + // present and in a valid range if the action is `PowerButton`. + let timeout = + self.shutdown_policy_timeout.ok_or_else(|| { + Error::internal_error(format!( + "db constraint violated: shutdown_policy_timeout \ + is NULL when action is PowerButton \ + (instance {})", + self.id + )) + })?; + let timeout_secs: u64 = + timeout.num_seconds().try_into().map_err(|_| { + Error::internal_error(format!( + "db constraint violated: shutdown_policy_timeout \ + has invalid value {timeout} \ + (instance {})", + self.id + )) + })?; + Ok(InstanceShutdownPolicy::PowerButton { timeout_secs }) + } + } + } +} + /// Runtime state of the Instance, including the actual running state and minimal /// metadata /// @@ -688,3 +747,91 @@ pub struct InstanceUpdate { pub shutdown_policy_action: Option, pub shutdown_policy_timeout: Option, } + +#[cfg(test)] +mod tests { + use super::*; + use omicron_common::api::external; + + #[test] + fn test_instance_shutdown_policy() { + let mut params = instance_types::InstanceCreate { + identity: external::IdentityMetadataCreateParams { + name: external::Name::try_from("acpi".to_owned()).unwrap(), + description: "instance under acpi shutdown policy test" + .to_owned(), + }, + ncpus: instance_types::InstanceCpuCount(1), + memory: external::ByteCount::from_gibibytes_u32(1), + hostname: external::Hostname::try_from("jumbo").unwrap(), + user_data: vec![], + network_interfaces: + instance_types::InstanceNetworkInterfaceAttachment::None, + external_ips: vec![], + disks: vec![], + boot_disk: None, + cpu_platform: None, + ssh_public_keys: None, + start: false, + auto_restart_policy: Default::default(), + anti_affinity_groups: Vec::new(), + multicast_groups: Vec::new(), + enable_jumbo_frames: false, + shutdown_policy: None, + }; + let instance_id = InstanceUuid::from_untyped_uuid(Uuid::new_v4()); + let project_id = ProjectUuid::from_untyped_uuid(Uuid::new_v4()); + + // when shutdown policy omitted at creation, default to 600 seconds + let db_instance = Instance::new(instance_id, project_id, ¶ms); + assert_eq!( + db_instance.shutdown_policy_action, + InstanceShutdownAction::PowerButton + ); + assert_eq!( + db_instance.shutdown_policy_timeout.unwrap().num_seconds(), + 600 + ); + + // explicitly set hard-off + params.shutdown_policy = + Some(instance_types::InstanceShutdownPolicy::HardOff); + let db_instance = Instance::new(instance_id, project_id, ¶ms); + assert_eq!( + db_instance.shutdown_policy_action, + InstanceShutdownAction::HardOff + ); + assert_eq!(db_instance.shutdown_policy_timeout, None); + + // explicitly set power button with timeout + params.shutdown_policy = + Some(instance_types::InstanceShutdownPolicy::PowerButton { + timeout_secs: 42, + }); + let db_instance = Instance::new(instance_id, project_id, ¶ms); + assert_eq!( + db_instance.shutdown_policy_action, + InstanceShutdownAction::PowerButton + ); + assert_eq!( + db_instance.shutdown_policy_timeout.unwrap().num_seconds(), + 42 + ); + + // Instance::new doesn't clamp an invalid timeout + // (enforced instead by Nexus and crdb constraint) + params.shutdown_policy = + Some(instance_types::InstanceShutdownPolicy::PowerButton { + timeout_secs: 999000, + }); + let db_instance = Instance::new(instance_id, project_id, ¶ms); + assert_eq!( + db_instance.shutdown_policy_action, + InstanceShutdownAction::PowerButton + ); + assert_eq!( + db_instance.shutdown_policy_timeout.unwrap().num_seconds(), + 999000 + ); + } +} diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index 59001d494d2..1ecd6120048 100644 --- a/nexus/db-queries/src/db/datastore/instance.rs +++ b/nexus/db-queries/src/db/datastore/instance.rs @@ -115,8 +115,10 @@ impl From<(Instance, Option)> for InstanceAndActiveVmm { } } -impl From for instance_types::Instance { - fn from(value: InstanceAndActiveVmm) -> Self { +impl TryFrom for instance_types::Instance { + type Error = Error; + + fn try_from(value: InstanceAndActiveVmm) -> Result { let time_run_state_updated = value .vmm .as_ref() @@ -162,7 +164,8 @@ impl From for instance_types::Instance { } }; - Self { + let shutdown_policy = value.instance.shutdown_policy()?; + Ok(Self { identity: value.instance.identity(), project_id: value.instance.project_id().into_untyped_uuid(), ncpus: value.instance.ncpus.into(), @@ -183,8 +186,8 @@ impl From for instance_types::Instance { }, auto_restart_status, enable_jumbo_frames: value.instance.enable_jumbo_frames, - shutdown_policy: value.instance.shutdown_policy(), - } + shutdown_policy: Some(shutdown_policy), + }) } } diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index dab039ea852..dd2a1d80256 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -219,6 +219,7 @@ pub use user_data_export::*; pub use virtual_provisioning_collection::StorageType; pub use vmm::VmmBulkMarkStopForUpdateResult; pub use vmm::VmmStateUpdateResult; +pub use vmm::VmmStoppableForUpdate; pub use volume::*; pub use webhook_delivery::WebhookDeliveryFilters; diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index 8efe06e8a8e..32aaafe66e4 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -26,11 +26,13 @@ use nexus_db_errors::OptionalError; use nexus_db_errors::public_error_from_diesel; use nexus_db_lookup::DbConnection; use nexus_db_model::ApplySledFilterExt; +use nexus_db_model::InstanceShutdownPolicyColumns; use nexus_db_schema::enums::SledBpAvailabilityEnum; use nexus_db_schema::enums::VmmStateEnum; use nexus_db_schema::schema::vmm; use nexus_db_schema::schema::vmm::dsl; use nexus_types::deployment::SledFilter; +use nexus_types::external_api::instance::InstanceShutdownPolicy; use nexus_types::instance::Migrations; use nexus_types::instance::VmmRuntimeState; use omicron_common::api::external::CreateResult; @@ -81,6 +83,19 @@ pub struct VmmBulkMarkStopForUpdateResult { pub batch_size: NonZeroU32, } +/// The combination of a VMM that should be stopped for its hosting sled to be +/// evacuated for an update, paired with its instance's shutdown policy. +/// +/// This is returned by [`DataStore::vmm_list_stoppable_for_update_by_sled_id`]. +#[derive(Debug, Clone)] +pub struct VmmStoppableForUpdate { + /// The stoppable VMM. + pub vmm: Vmm, + + /// The shutdown policy for this VMM's instance. + pub shutdown_policy: InstanceShutdownPolicy, +} + /// Changeset for a VMM's runtime state. #[derive(Clone, Debug, AsChangeset)] #[diesel(table_name = vmm)] @@ -574,7 +589,8 @@ impl DataStore { } /// Lists VMMs that have been marked as needing to be stopped for a sled - /// update, and that *can* be stopped. + /// update, and that *can* be stopped. Returns each such VMM with its + /// instance's shutdown policy. /// /// The query performed by this function is paginated by the sled and /// VMM UUIDs, in that order. @@ -582,19 +598,42 @@ impl DataStore { &self, opctx: &OpContext, pagparams: &DataPageParams<'_, (Uuid, Uuid)>, - ) -> ListResultVec { + ) -> ListResultVec { + use nexus_db_schema::schema::instance::dsl as instance_dsl; use nexus_db_schema::schema::sled::dsl as sled_dsl; - paginated_multicolumn(dsl::vmm, (dsl::sled_id, dsl::id), pagparams) - .inner_join(sled_dsl::sled.on(sled_dsl::id.eq(dsl::sled_id))) - .filter(dsl::time_deleted.is_null()) - .filter(dsl::stop_for_update_disposition_generation.is_not_null()) - .filter(dsl::state.eq_any(DbVmmState::SHOULD_STOP_FOR_EVACUATION)) - .sled_filter(SledFilter::InService) - .select(Vmm::as_select()) - .load_async(&*self.pool_connection_authorized(opctx).await?) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + let vmms_and_instances = + paginated_multicolumn(dsl::vmm, (dsl::sled_id, dsl::id), pagparams) + .inner_join(sled_dsl::sled.on(sled_dsl::id.eq(dsl::sled_id))) + .inner_join( + instance_dsl::instance + .on(instance_dsl::id.eq(dsl::instance_id)), + ) + .filter(dsl::time_deleted.is_null()) + .filter( + dsl::stop_for_update_disposition_generation.is_not_null(), + ) + .filter( + dsl::state.eq_any(DbVmmState::SHOULD_STOP_FOR_EVACUATION), + ) + .sled_filter(SledFilter::InService) + .select(( + Vmm::as_select(), + InstanceShutdownPolicyColumns::as_select(), + )) + .load_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + vmms_and_instances + .into_iter() + .map(|(vmm, instance)| { + let shutdown_policy = instance.shutdown_policy()?; + Ok(VmmStoppableForUpdate { vmm, shutdown_policy }) + }) + .collect() } } @@ -611,19 +650,30 @@ mod tests { use iddqd::IdOrdMap; use nexus_db_lookup::LookupPath; use nexus_db_model::ActiveSledBpAvailability; + use nexus_db_model::Instance; + use nexus_db_model::Project; use nexus_db_model::SledBlueprintAvailabilityInput; use nexus_db_model::SledBpAvailabilityState; use nexus_db_model::VmmCpuPlatform; + use nexus_types::external_api::instance::InstanceCreate; + use nexus_types::external_api::instance::InstanceNetworkInterfaceAttachment; + use nexus_types::external_api::project::ProjectCreate; use nexus_types::instance::VmmFailureReason; use nexus_types::instance::VmmState; + use nexus_types::silo::DEFAULT_SILO_ID; + use omicron_common::api::external::ByteCount; + use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_generation_kinds::UpdateDispositionGeneration; use omicron_test_utils::dev; use omicron_uuid_kinds::BlueprintUuid; use omicron_uuid_kinds::InstanceUuid; + use omicron_uuid_kinds::ProjectUuid; use omicron_uuid_kinds::SledUuid; use sled_agent_types::instance::MigrationState; + use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashMap; + use std::iter; #[tokio::test] async fn test_vmm_and_migration_update_runtime() { @@ -1366,25 +1416,63 @@ mod tests { let (opctx, datastore) = (db.opctx(), db.datastore()); let conn = datastore.pool_connection_for_tests().await.unwrap(); - // The states from which a VMM can still be stopped for an update. Only - // marked VMMs in these states should be listed by - // `vmm_list_stoppable_for_update_by_sled_id`. - let is_stoppable = |state: DbVmmState| { - matches!( - state, - DbVmmState::Starting - | DbVmmState::Running - | DbVmmState::Rebooting + // Create a test project to host all our fake instances. + let (authz_project, _) = datastore + .project_create( + &opctx, + Project::new_with_id( + ProjectUuid::new_v4(), + DEFAULT_SILO_ID, + ProjectCreate { + identity: IdentityMetadataCreateParams { + name: "test-project".parse().unwrap(), + description: "".into(), + }, + defaults: None, + }, + ), ) - }; + .await + .expect("created project"); + + // For each stoppable VMM we insert, pick a different shutdown policy + // and record the choice to check below. + let mut shutdown_policy_iter = iter::once( + InstanceShutdownPolicy::HardOff, + ) + .chain((1000..).step_by(100).map(|timeout_secs| { + InstanceShutdownPolicy::PowerButton { timeout_secs } + })); + let mut expected_shutdown_policies = + BTreeMap::::new(); // Insert 2 VMMs (one marked to stop, one not) in each state per sled - // (2 in-service and 1 expunged). + // (2 in-service and 1 expunged). Each VMM also gets an associated + // instance, required by `vmm_list_stoppable_for_update_by_sled_id`'s + // join against the instance table to look up the shutdown policy. let generation = UpdateDispositionGeneration::from(1); let sled_a = SledUuid::new_v4(); let sled_b = SledUuid::new_v4(); let sled_expunged = SledUuid::new_v4(); let mut vmms = Vec::new(); + + // Do we expect a given VMM to be stoppable (i.e., listed by + // `vmm_list_stoppable_for_update_by_sled_id()`)? That requires: + // + // 1. It has been marked to be stopped + // 2. It's not on the sled we're marking expunged + // 3. Its state is stoppable + let is_stoppable = |vmm: &Vmm| { + vmm.stop_for_update_disposition_generation.is_some() + && vmm.sled_id() != sled_expunged + && matches!( + vmm.state, + DbVmmState::Starting + | DbVmmState::Running + | DbVmmState::Rebooting + ) + }; + for sled_id in [sled_a, sled_b, sled_expunged] { // The query only returns VMMs on in-service sleds. We store the // three sleds in the db. @@ -1409,28 +1497,77 @@ mod tests { for &state in DbVmmState::ALL_STATES { for marker in [Some(generation), None] { + let instance_id = InstanceUuid::new_v4(); let failure_reason = (state == DbVmmState::Failed) .then_some(db::model::VmmFailureReason::FromSledAgent); - let vmm = datastore - .vmm_insert( - &opctx, - Vmm { - id: Uuid::new_v4(), - time_created: Utc::now(), - time_deleted: None, - instance_id: Uuid::new_v4(), - sled_id: sled_id.into(), - propolis_ip: "10.1.9.32".parse().unwrap(), - propolis_port: 420.into(), - cpu_platform: VmmCpuPlatform::SledDefault, - time_state_updated: Utc::now(), - generation: Generation::new(), - state, - failure_reason, - stop_for_update_disposition_generation: marker - .map(Into::into), + let vmm = Vmm { + id: Uuid::new_v4(), + time_created: Utc::now(), + time_deleted: None, + instance_id: instance_id.into_untyped_uuid(), + sled_id: sled_id.into(), + propolis_ip: "10.1.9.32".parse().unwrap(), + propolis_port: 420.into(), + cpu_platform: VmmCpuPlatform::SledDefault, + time_state_updated: Utc::now(), + generation: Generation::new(), + state, + failure_reason, + stop_for_update_disposition_generation: marker + .map(Into::into), + }; + + // If this VMM is stoppable, pick a unique shutdown policy + // from our iterator and record the choice for our assertion + // later. Otherwise, pick an arbitrary policy. + let shutdown_policy = if is_stoppable(&vmm) { + let policy = shutdown_policy_iter + .next() + .expect("iter is effectively infinite"); + expected_shutdown_policies.insert(vmm.id, policy); + policy + } else { + InstanceShutdownPolicy::HardOff + }; + + datastore.project_create_instance( + &opctx, + &authz_project, + Instance::new( + instance_id, + authz_project.id(), + &InstanceCreate { + identity: IdentityMetadataCreateParams { + name: format!("instance-{}", vmm.id) + .parse() + .unwrap(), + description: "".into(), + }, + ncpus: 2i64.try_into().unwrap(), + memory: ByteCount::from_gibibytes_u32(16), + hostname: "myhostname".try_into().unwrap(), + user_data: Vec::new(), + network_interfaces: + InstanceNetworkInterfaceAttachment::None, + external_ips: Vec::new(), + disks: Vec::new(), + boot_disk: None, + cpu_platform: None, + ssh_public_keys: None, + start: false, + auto_restart_policy: Default::default(), + anti_affinity_groups: Vec::new(), + multicast_groups: Vec::new(), + enable_jumbo_frames: false, + shutdown_policy: Some(shutdown_policy), }, - ) + ), + ) + .await + .expect("instance created successfully"); + + let vmm = datastore + .vmm_insert(&opctx, vmm) .await .expect("VMM should be inserted successfully"); vmms.push(vmm); @@ -1450,15 +1587,8 @@ mod tests { assert_eq!(row_count, 60); // Check there are 6 stoppable VMMs (3 per in-service sled). - let expected_ids: BTreeSet = vmms - .iter() - .filter(|vmm| { - vmm.sled_id() != sled_expunged - && vmm.stop_for_update_disposition_generation.is_some() - && is_stoppable(vmm.state) - }) - .map(|vmm| vmm.id) - .collect(); + let expected_ids: BTreeSet = + expected_shutdown_policies.keys().copied().collect(); assert_eq!(expected_ids.len(), 6); // Check the retrieved ids for stoppable VMMs are the same as the @@ -1472,8 +1602,16 @@ mod tests { .expect("marked VMMs should be listed"); assert_eq!(listed.len(), expected_ids.len()); let listed_ids: BTreeSet = - listed.iter().map(|vmm| vmm.id).collect(); + listed.iter().map(|stoppable_vmm| stoppable_vmm.vmm.id).collect(); assert_eq!(listed_ids, expected_ids); + let listed_shutdown_policies: BTreeMap = + listed + .iter() + .map(|stoppable_vmm| { + (stoppable_vmm.vmm.id, stoppable_vmm.shutdown_policy) + }) + .collect(); + assert_eq!(listed_shutdown_policies, expected_shutdown_policies); // Clean up. db.terminate().await; diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 961f7060c8b..52ff75f9860 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -15,7 +15,9 @@ use nexus_db_model::Vmm; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; use nexus_db_queries::db::datastore::SQL_BATCH_SIZE; +use nexus_db_queries::db::datastore::VmmStoppableForUpdate; use nexus_db_queries::db::pagination::Paginator; +use nexus_types::external_api::instance::InstanceShutdownPolicy; use nexus_types::instance::SledVmmState; use nexus_types::instance::VmmFailureReason; use nexus_types::instance::VmmRuntimeState; @@ -152,7 +154,7 @@ impl VmmStopForUpdate { dropshot::PaginationOrder::Ascending, ); while let Some(p) = paginator.next() { - let vmms = match self + let stoppable_vmms = match self .datastore .vmm_list_stoppable_for_update_by_sled_id( opctx, @@ -166,12 +168,13 @@ impl VmmStopForUpdate { break; } }; - paginator = p.found_batch(&vmms, &|vmm| { + paginator = p.found_batch(&stoppable_vmms, &|stoppable_vmm| { + let vmm = &stoppable_vmm.vmm; (vmm.sled_id().into_untyped_uuid(), vmm.id) }); self.stop_batch( - &vmms, + &stoppable_vmms, &reqwest_client, &task_opctx, &mut tasks, @@ -190,26 +193,32 @@ impl VmmStopForUpdate { /// Stops a batch of VMMs async fn stop_batch( &self, - vmms: &[Vmm], + stoppable_vmms: &[VmmStoppableForUpdate], reqwest_client: &ReqwestClient, opctx: &Arc, tasks: &mut ParallelTaskSet, result: &mut VmmStopForUpdateResult, ) { - for vmms_per_sled in vmms.chunk_by(|a, b| a.sled_id() == b.sled_id()) { + for stoppable_vmms_per_sled in + stoppable_vmms.chunk_by(|a, b| a.vmm.sled_id() == b.vmm.sled_id()) + { // We only retrieve the first VMM in order to get the sled id. // The slice is constructed by VMMs with the same sled id, so it's // fine to just check the first one - let Some(first_vmm) = vmms_per_sled.first() else { + let Some(sled_id) = stoppable_vmms_per_sled + .first() + .map(|stoppable_vmm| stoppable_vmm.vmm.sled_id()) + else { // `chunk_by` never returns an empty slice, so this should never // be `None`. We handle the case regardless continue; }; - let sled_id = first_vmm.sled_id(); - let vmm_ids: Vec = vmms_per_sled + let vmm_ids: Vec = stoppable_vmms_per_sled .iter() - .map(|vmm| PropolisUuid::from_untyped_uuid(vmm.id)) + .map(|stoppable_vmm| { + PropolisUuid::from_untyped_uuid(stoppable_vmm.vmm.id) + }) .collect(); slog::info!( @@ -246,9 +255,13 @@ impl VmmStopForUpdate { } }; - for vmm in vmms_per_sled { - let stop = - self.stop_vmm(opctx, sled_client.clone(), vmm.clone()); + for stoppable_vmm in stoppable_vmms_per_sled { + let stop = self.stop_vmm( + opctx, + sled_client.clone(), + stoppable_vmm.vmm.clone(), + stoppable_vmm.shutdown_policy, + ); if let Some(vmm_result) = tasks.spawn(stop).await { result.merge(vmm_result); } @@ -262,6 +275,7 @@ impl VmmStopForUpdate { opctx: &Arc, sled_client: sled_agent_client::Client, vmm: Vmm, + shutdown_policy: InstanceShutdownPolicy, ) -> impl Future + Send + 'static + use<> { let datastore = self.datastore.clone(); @@ -278,8 +292,14 @@ impl VmmStopForUpdate { &vmm_id, &VmmPutStateBody { state: VmmStateRequested::Stopped, - // FIXME(lif) before undraft PR: add shutdown policy data! - acpi_timeout_secs: None, + // TODO can sled-agent take the shutdown policy directly + // instead of an option? + acpi_timeout_secs: match shutdown_policy { + InstanceShutdownPolicy::HardOff => None, + InstanceShutdownPolicy::PowerButton { + timeout_secs, + } => Some(timeout_secs), + }, }, ) .await diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index ff1cee8f181..27db6558260 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -1480,16 +1480,21 @@ impl super::Nexus { let acpi_timeout_secs = match requested { InstanceStateChangeRequest::Reboot { skip_os_shutdown } | InstanceStateChangeRequest::Stop { skip_os_shutdown } - if !skip_os_shutdown => match prev_instance_state.shutdown_policy_action { - nexus_db_model::InstanceShutdownAction::HardOff => None, - nexus_db_model::InstanceShutdownAction::PowerButton => { - prev_instance_state - .shutdown_policy_timeout - // non-neg enforced by api impl & crdb constraint - .and_then(|d| d.num_seconds().try_into().ok()) + if !skip_os_shutdown => + { + // TODO can sled-agent take the shutdown policy directly + // instead of an option? + match prev_instance_state + .shutdown_policy() + .map_err(InstanceStateChangeError::Other)? + { + instance::InstanceShutdownPolicy::HardOff => None, + instance::InstanceShutdownPolicy::PowerButton { + timeout_secs, + } => Some(timeout_secs), } - }, - _ => None + } + _ => None, }; let instance_put_result = sa .vmm_put_state( @@ -3132,9 +3137,8 @@ mod tests { use futures::{SinkExt, StreamExt}; use instance::InstanceNetworkInterfaceAttachment; use nexus_db_model::{ - Instance as DbInstance, InstanceShutdownAction, - InstanceState as DbInstanceState, VmmCpuPlatform, - VmmState as DbVmmState, + Instance as DbInstance, InstanceState as DbInstanceState, + VmmCpuPlatform, VmmState as DbVmmState, }; use nexus_types::external_api::instance; use omicron_common::api::external::{ @@ -3450,86 +3454,4 @@ mod tests { Some(omicron_common::address::EXTERNAL_JUMBO_FRAMES_MTU), ); } - - #[test] - fn test_instance_shutdown_policy() { - let mut params = instance::InstanceCreate { - identity: IdentityMetadataCreateParams { - name: Name::try_from("acpi".to_owned()).unwrap(), - description: "instance under acpi shutdown policy test" - .to_owned(), - }, - ncpus: instance::InstanceCpuCount(1), - memory: ByteCount::from_gibibytes_u32(1), - hostname: Hostname::try_from("jumbo").unwrap(), - user_data: vec![], - network_interfaces: InstanceNetworkInterfaceAttachment::None, - external_ips: vec![], - disks: vec![], - boot_disk: None, - cpu_platform: None, - ssh_public_keys: None, - start: false, - auto_restart_policy: Default::default(), - anti_affinity_groups: Vec::new(), - multicast_groups: Vec::new(), - enable_jumbo_frames: false, - shutdown_policy: None, - }; - let instance_id = InstanceUuid::from_untyped_uuid(Uuid::new_v4()); - let project_id = ProjectUuid::from_untyped_uuid(Uuid::new_v4()); - - // when shutdown policy omitted at creation, default to 600 seconds - let db_instance = DbInstance::new(instance_id, project_id, ¶ms); - assert_eq!( - db_instance.shutdown_policy_action, - InstanceShutdownAction::PowerButton - ); - assert_eq!( - db_instance.shutdown_policy_timeout.unwrap().num_seconds(), - 600 - ); - - // explicitly set hard-off - params.shutdown_policy = - Some(instance::InstanceShutdownPolicy::HardOff); - let db_instance = DbInstance::new(instance_id, project_id, ¶ms); - assert_eq!( - db_instance.shutdown_policy_action, - InstanceShutdownAction::HardOff - ); - assert_eq!(db_instance.shutdown_policy_timeout, None); - - // explicitly set power button with timeout - params.shutdown_policy = - Some(instance::InstanceShutdownPolicy::PowerButton { - timeout_secs: 42, - }); - let db_instance = DbInstance::new(instance_id, project_id, ¶ms); - assert_eq!( - db_instance.shutdown_policy_action, - InstanceShutdownAction::PowerButton - ); - assert_eq!( - db_instance.shutdown_policy_timeout.unwrap().num_seconds(), - 42 - ); - - // DbInstance::new doesn't clamp an invalid timeout - // (enforced instead by check_instance_shutdown_policy_valid - // and crdb constraint) - params.shutdown_policy = - Some(instance::InstanceShutdownPolicy::PowerButton { - timeout_secs: 999000, - }); - let db_instance = DbInstance::new(instance_id, project_id, ¶ms); - assert_eq!( - db_instance.shutdown_policy_action, - InstanceShutdownAction::PowerButton - ); - assert_eq!( - db_instance.shutdown_policy_timeout.unwrap().num_seconds(), - 999000 - ); - } } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 7aaba42d87f..dfcb66e83ac 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -2810,8 +2810,8 @@ impl NexusExternalApi for NexusExternalApiImpl { .instance_list(&opctx, &project_lookup, &paginated_by) .await? .into_iter() - .map(|i| i.into()) - .collect(); + .map(|i| i.try_into()) + .collect::>()?; Ok(HttpResponseOk(ScanByNameOrId::results_page( &query, instances, @@ -2842,7 +2842,7 @@ impl NexusExternalApi for NexusExternalApiImpl { &new_instance_params, ) .await?; - Ok(HttpResponseCreated(instance.into())) + Ok(HttpResponseCreated(instance.try_into()?)) }) .await } @@ -2871,7 +2871,7 @@ impl NexusExternalApi for NexusExternalApiImpl { .datastore() .instance_fetch_with_vmm(&opctx, &authz_instance) .await?; - Ok(HttpResponseOk(instance_and_vmm.into())) + Ok(HttpResponseOk(instance_and_vmm.try_into()?)) }; apictx .context @@ -2923,7 +2923,7 @@ impl NexusExternalApi for NexusExternalApiImpl { &instance_config, ) .await?; - Ok(HttpResponseOk(instance.into())) + Ok(HttpResponseOk(instance.try_into()?)) }) .await } @@ -2949,7 +2949,7 @@ impl NexusExternalApi for NexusExternalApiImpl { query.skip_os_shutdown.unwrap_or(false), ) .await?; - Ok(HttpResponseAccepted(instance.into())) + Ok(HttpResponseAccepted(instance.try_into()?)) }) .await } @@ -2975,7 +2975,7 @@ impl NexusExternalApi for NexusExternalApiImpl { crate::app::sagas::instance_start::Reason::User, ) .await?; - Ok(HttpResponseAccepted(instance.into())) + Ok(HttpResponseAccepted(instance.try_into()?)) }) .await } @@ -3001,7 +3001,7 @@ impl NexusExternalApi for NexusExternalApiImpl { query.skip_os_shutdown.unwrap_or(false), ) .await?; - Ok(HttpResponseAccepted(instance.into())) + Ok(HttpResponseAccepted(instance.try_into()?)) }) .await } diff --git a/nexus/src/lockstep_api/http_entrypoints.rs b/nexus/src/lockstep_api/http_entrypoints.rs index a25f3347f7e..30ea8b1ee46 100644 --- a/nexus/src/lockstep_api/http_entrypoints.rs +++ b/nexus/src/lockstep_api/http_entrypoints.rs @@ -154,7 +154,7 @@ impl NexusLockstepApi for NexusLockstepApiImpl { let instance = nexus .instance_migrate(&opctx, path.instance_id, migrate) .await?; - Ok(HttpResponseOk(instance.into())) + Ok(HttpResponseOk(instance.try_into()?)) }; apictx .internal_latencies diff --git a/nexus/types/versions/src/instance_acpi_shutdown/instance.rs b/nexus/types/versions/src/instance_acpi_shutdown/instance.rs index 77d764fb6f2..a365803206e 100644 --- a/nexus/types/versions/src/instance_acpi_shutdown/instance.rs +++ b/nexus/types/versions/src/instance_acpi_shutdown/instance.rs @@ -23,7 +23,9 @@ use crate::v2026_06_08_00; use crate::v2026_06_08_00::instance::InstanceCpuPlatform; /// Instance-stopping method choices -#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize, JsonSchema)] +#[derive( + Clone, Copy, Debug, Eq, PartialEq, Deserialize, Serialize, JsonSchema, +)] #[serde(tag = "action", rename_all = "snake_case")] pub enum InstanceShutdownPolicy { /// Instance's VM is forcefully terminated immediately. diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs index 7e22eb34b5c..989222caa93 100644 --- a/sled-agent/src/instance.rs +++ b/sled-agent/src/instance.rs @@ -4004,8 +4004,10 @@ mod tests { ), ) .await - .expect_err("Should have timed out waiting for VmmState::Destroyed \ - in FakeNexus (after canceled stop timeout)"); + .expect_err( + "Should have timed out waiting for VmmState::Destroyed \ + in FakeNexus (after canceled stop timeout)", + ); logctx.cleanup_successful(); }