graceful shutdown: rework API types / compatibility

end-to-end-tests/src/instance_launch.rs

@@ -9,9 +9,8 @@ use anyhow::{Context as _, Result, ensure};
 use omicron_test_utils::dev::poll::{CondCheckError, wait_for_condition};
 use oxide_client::types::{
     ByteCount, DiskBackend, DiskCreate, DiskSource, ExternalIp,
-    ExternalIpCreate, InstanceCpuCount, InstanceCreate, InstanceDiskAttachment,
-    InstanceNetworkInterfaceAttachment, InstanceState, IpVersion, PoolSelector,
-    SshKeyCreate,
+    ExternalIpCreate, InstanceDiskAttachment, InstanceState, IpVersion,
+    PoolSelector, SshKeyCreate,
 };
 use oxide_client::{ClientCurrentUserExt, ClientDisksExt, ClientInstancesExt};
 use russh::keys::{
@@ -64,38 +63,30 @@ async fn instance_launch() -> Result<()> {
         .clone();
 
     eprintln!("create instance");
+    let instance_name = generate_name("instance")?;
     let instance = ctx
         .client
         .instance_create()
         .project(ctx.project_name.clone())
-        .body(InstanceCreate {
-            name: generate_name("instance")?,
-            description: String::new(),
-            hostname: "localshark".parse().unwrap(), // 🦈
-            memory: ByteCount(1024 * 1024 * 1024),
-            ncpus: InstanceCpuCount(2),
-            boot_disk: Some(InstanceDiskAttachment::Attach {
-                name: disk_name.clone(),
-            }),
-            disks: Vec::new(),
-            network_interfaces:
-                InstanceNetworkInterfaceAttachment::DefaultDualStack,
-            external_ips: vec![ExternalIpCreate::Ephemeral {
-                pool_selector: PoolSelector::Auto {
-                    ip_version: Some(IpVersion::V4),
-                },
-            }],
-            user_data: String::new(),
-            ssh_public_keys: Some(vec![oxide_client::types::NameOrId::Name(
-                ssh_key_name.clone(),
-            )]),
-            start: true,
-            auto_restart_policy: Default::default(),
-            anti_affinity_groups: Vec::new(),
-            cpu_platform: None,
-            multicast_groups: Vec::new(),
-            enable_jumbo_frames: false,
-            shutdown_policy: None,
+        .body_map(|builder| {
+            builder
+                .name(instance_name)
+                .description("")
+                .hostname("localshark")
+                .memory(ByteCount(1024 * 1024 * 1024))
+                .ncpus(2)
+                .boot_disk(Some(InstanceDiskAttachment::Attach {
+                    name: disk_name.clone(),
+                }))
+                .external_ips(vec![ExternalIpCreate::Ephemeral {
+                    pool_selector: PoolSelector::Auto {
+                        ip_version: Some(IpVersion::V4),
+                    },
+                }])
+                .ssh_public_keys(Some(vec![
+                    oxide_client::types::NameOrId::Name(ssh_key_name.clone()),
+                ]))
+                .start(true)
         })
         .send()
         .await?;

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;
 use omicron_common::api::external::Error;
 use omicron_generation_kinds::{
     InstanceStateGeneration, InstanceStateGenerationKind,
@@ -30,6 +31,7 @@ use omicron_uuid_kinds::ProjectUuid;
 use omicron_uuid_kinds::{GenericUuid, InstanceUuid};
 use serde::Deserialize;
 use serde::Serialize;
+use slog_error_chain::InlineErrorChain;
 use uuid::Uuid;
 
 /// An Instance (VM).
@@ -191,26 +193,8 @@ impl Instance {
             IntendedState::Stopped
         };
 
-        use instance_types::InstanceShutdownPolicy;
-        let (shutdown_policy_action, shutdown_policy_timeout) = match params
-            .shutdown_policy
-        {
-            // RFD 622: "A user-configurable timeout for the instance to
-            // finish stopping itself should be present, bounded between
-            // 1 and 3600 seconds, inclusive." This is enforced at the API
-            // level and as a crdb constraint (expect `timeout_secs as i64`
-            // to be handled as an error elsewhere if somehow in the range
-            // where the cast would be problematic)
-            Some(InstanceShutdownPolicy::PowerButton { timeout_secs }) => (
-                InstanceShutdownAction::PowerButton,
-                Some(TimeDelta::seconds(timeout_secs as i64)),
-            ),
-            // None: Default to hard-off, preserving the existing behavior of
-            // our system prior to ACPI shutdown support.
-            None | Some(InstanceShutdownPolicy::HardOff) => {
-                (InstanceShutdownAction::HardOff, None)
-            }
-        };
+        let (shutdown_policy_action, shutdown_policy_timeout) =
+            shutdown_policy_to_db_columns(params.shutdown_policy);
 
         Self {
             identity,
@@ -397,6 +381,7 @@ impl InstanceShutdownPolicyColumns {
     pub fn shutdown_policy(
         &self,
     ) -> Result<instance_types::InstanceShutdownPolicy, Error> {
+        use instance_types::AcpiTimeoutSecs;
         use instance_types::InstanceShutdownPolicy;
         match self.shutdown_policy_action {
             InstanceShutdownAction::HardOff => {
@@ -423,6 +408,16 @@ impl InstanceShutdownPolicyColumns {
                             self.id
                         ))
                     })?;
+                let timeout_secs = AcpiTimeoutSecs::try_from(timeout_secs)
+                    .map_err(|err| {
+                        Error::internal_error(format!(
+                            "db constraint violated or out of sync with API: \
+                             db timeout of {timeout_secs} is invalid: {} \
+                             (instance {})",
+                            InlineErrorChain::new(&*err),
+                            self.id,
+                        ))
+                    })?;
                 Ok(InstanceShutdownPolicy::PowerButton { timeout_secs })
             }
         }
@@ -735,15 +730,67 @@ pub struct InstanceUpdate {
 
     /// If this is `None`, the instance's shutdown policy will be unchanged by
     /// this update request.
+    ///
+    /// Unlike `Instance` above, these fields are `pub` because we need direct
+    /// access to them in the datastore crate. Consumers outside that crate
+    /// should *not* access these; construction should go through the stronger
+    /// types available via [`InstanceUpdate::new()`].
     #[diesel(treat_none_as_null = false)]
     pub shutdown_policy_action: Option<InstanceShutdownAction>,
     pub shutdown_policy_timeout: Option<TimeDelta>,
 }
 
+impl InstanceUpdate {
+    pub fn new(
+        boot_disk_id: Option<Uuid>,
+        auto_restart_policy: Option<instance_types::InstanceAutoRestartPolicy>,
+        ncpus: instance_types::InstanceCpuCount,
+        memory: external::ByteCount,
+        cpu_platform: Option<instance_types::InstanceCpuPlatform>,
+        enable_jumbo_frames: bool,
+        shutdown_policy: Option<instance_types::InstanceShutdownPolicy>,
+    ) -> Self {
+        let (shutdown_policy_action, shutdown_policy_timeout) =
+            if let Some(shutdown_policy) = shutdown_policy {
+                let (action, timeout) =
+                    shutdown_policy_to_db_columns(shutdown_policy);
+                (Some(action), timeout)
+            } else {
+                (None, None)
+            };
+        Self {
+            boot_disk_id,
+            auto_restart_policy: auto_restart_policy.map(From::from),
+            ncpus: ncpus.into(),
+            memory: memory.into(),
+            cpu_platform: cpu_platform.map(From::from),
+            enable_jumbo_frames,
+            shutdown_policy_action,
+            shutdown_policy_timeout,
+        }
+    }
+}
+
+// convert the API `InstanceShutdownPolicy` into its raw db column
+// representation
+fn shutdown_policy_to_db_columns(
+    shutdown_policy: instance_types::InstanceShutdownPolicy,
+) -> (InstanceShutdownAction, Option<TimeDelta>) {
+    use instance_types::InstanceShutdownPolicy;
+    match shutdown_policy {
+        InstanceShutdownPolicy::HardOff => {
+            (InstanceShutdownAction::HardOff, None)
+        }
+        InstanceShutdownPolicy::PowerButton { timeout_secs } => (
+            InstanceShutdownAction::PowerButton,
+            Some(TimeDelta::seconds(i64::from(timeout_secs))),
+        ),
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
-    use omicron_common::api::external;
 
     #[test]
     fn test_instance_shutdown_policy() {
@@ -769,34 +816,25 @@ mod tests {
             anti_affinity_groups: Vec::new(),
             multicast_groups: Vec::new(),
             enable_jumbo_frames: false,
-            shutdown_policy: None,
+            // explicitly set hard-off
+            shutdown_policy: instance_types::InstanceShutdownPolicy::HardOff,
         };
         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 HardOff
+        // confirm expected columns for hard-off
         let db_instance = Instance::new(instance_id, project_id, &params);
         assert_eq!(
             db_instance.shutdown_policy_action,
             InstanceShutdownAction::HardOff
         );
         assert_eq!(db_instance.shutdown_policy_timeout, None);
 
-        // explicitly set hard-off
+        // set power button with timeout
         params.shutdown_policy =
-            Some(instance_types::InstanceShutdownPolicy::HardOff);
-        let db_instance = Instance::new(instance_id, project_id, &params);
-        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,
-            });
+            instance_types::InstanceShutdownPolicy::PowerButton {
+                timeout_secs: 42.try_into().expect("42 is a valid timeout"),
+            };
         let db_instance = Instance::new(instance_id, project_id, &params);
         assert_eq!(
             db_instance.shutdown_policy_action,
@@ -806,21 +844,5 @@ mod tests {
             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, &params);
-        assert_eq!(
-            db_instance.shutdown_policy_action,
-            InstanceShutdownAction::PowerButton
-        );
-        assert_eq!(
-            db_instance.shutdown_policy_timeout.unwrap().num_seconds(),
-            999000
-        );
     }
 }

nexus/db-queries/src/db/datastore/instance.rs

@@ -186,7 +186,7 @@ impl TryFrom<InstanceAndActiveVmm> for instance_types::Instance {
             },
             auto_restart_status,
             enable_jumbo_frames: value.instance.enable_jumbo_frames,
-            shutdown_policy: Some(shutdown_policy),
+            shutdown_policy,
         })
     }
 }
@@ -2384,7 +2384,8 @@ mod tests {
                         anti_affinity_groups: Vec::new(),
                         multicast_groups: Vec::new(),
                         enable_jumbo_frames: false,
-                        shutdown_policy: None,
+                        shutdown_policy:
+                            instance_types::InstanceShutdownPolicy::default(),
                     },
                 ),
             )

nexus/db-queries/src/db/datastore/migration.rs

@@ -245,7 +245,8 @@ mod tests {
                         anti_affinity_groups: Vec::new(),
                         multicast_groups: Vec::new(),
                         enable_jumbo_frames: false,
-                        shutdown_policy: None,
+                        shutdown_policy:
+                            instance_types::InstanceShutdownPolicy::default(),
                     },
                 ),
             )

nexus/db-queries/src/db/datastore/sled.rs

@@ -5802,7 +5802,8 @@ pub(in crate::db::datastore) mod test {
                         anti_affinity_groups: Vec::new(),
                         multicast_groups: Vec::new(),
                         enable_jumbo_frames: false,
-                        shutdown_policy: None,
+                        shutdown_policy:
+                            instance::InstanceShutdownPolicy::default(),
                     },
                 ),
             )

nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs

@@ -472,7 +472,8 @@ mod test {
                         anti_affinity_groups: Vec::new(),
                         multicast_groups: Vec::new(),
                         enable_jumbo_frames: false,
-                        shutdown_policy: None,
+                        shutdown_policy:
+                            instance_types::InstanceShutdownPolicy::default(),
                     },
                 ),
             )

nexus/db-queries/src/db/datastore/vmm.rs

@@ -1440,7 +1440,8 @@ mod tests {
         let mut shutdown_policy_iter = iter::once(
             InstanceShutdownPolicy::HardOff,
         )
-        .chain((1000..).step_by(100).map(|timeout_secs| {
+        .chain((1000_u64..).step_by(100).map(|n| {
+            let timeout_secs = n.try_into().expect("these are valid timeouts");
             InstanceShutdownPolicy::PowerButton { timeout_secs }
         }));
         let mut expected_shutdown_policies =
@@ -1559,7 +1560,7 @@ mod tests {
                                 anti_affinity_groups: Vec::new(),
                                 multicast_groups: Vec::new(),
                                 enable_jumbo_frames: false,
-                                shutdown_policy: Some(shutdown_policy),
+                                shutdown_policy,
                             },
                         ),
                     )

nexus/db-queries/src/db/datastore/vpc.rs

@@ -4044,7 +4044,8 @@ mod tests {
                         anti_affinity_groups: Vec::new(),
                         multicast_groups: Vec::new(),
                         enable_jumbo_frames: false,
-                        shutdown_policy: None,
+                        shutdown_policy:
+                            instance_types::InstanceShutdownPolicy::default(),
                     },
                 ),
             )

nexus/db-queries/src/db/pub_test_utils/helpers.rs

@@ -260,7 +260,7 @@ pub async fn create_stopped_instance_record(
             anti_affinity_groups: Vec::new(),
             multicast_groups: Vec::new(),
             enable_jumbo_frames: false,
-            shutdown_policy: None,
+            shutdown_policy: Default::default(),
         },
     );
 

nexus/db-queries/src/db/queries/external_ip.rs

@@ -998,6 +998,7 @@ mod tests {
     use nexus_types::external_api::instance;
     use nexus_types::external_api::instance::InstanceCreate;
     use nexus_types::external_api::instance::InstanceNetworkInterfaceAttachment;
+    use nexus_types::external_api::instance::InstanceShutdownPolicy;
     use omicron_common::address::IpRange;
     use omicron_common::address::NUM_SOURCE_NAT_PORTS;
     use omicron_common::api::external::Error;
@@ -1139,7 +1140,7 @@ mod tests {
                 anti_affinity_groups: Vec::new(),
                 multicast_groups: Vec::new(),
                 enable_jumbo_frames: false,
-                shutdown_policy: None,
+                shutdown_policy: InstanceShutdownPolicy::default(),
             });
 
             let conn = self
@@ -2694,7 +2695,7 @@ mod tests {
             anti_affinity_groups: Vec::new(),
             multicast_groups: Vec::new(),
             enable_jumbo_frames: false,
-            shutdown_policy: None,
+            shutdown_policy: InstanceShutdownPolicy::default(),
         });
 
         let conn = context