allow instance_reconfigure to resize a stopped instance

common/src/api/external/mod.rs

@@ -1146,7 +1146,9 @@ impl InstanceState {
 }
 
 /// The number of CPUs in an Instance
-#[derive(Copy, Clone, Debug, Deserialize, Serialize, JsonSchema)]
+#[derive(
+    Copy, Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq,
+)]
 pub struct InstanceCpuCount(pub u16);
 
 impl TryFrom<i64> for InstanceCpuCount {

nexus/db-model/src/instance.rs

@@ -542,4 +542,8 @@ pub struct InstanceUpdate {
     /// set the instance's auto-restart policy to `NULL`.
     #[diesel(column_name = auto_restart_policy)]
     pub auto_restart_policy: Option<InstanceAutoRestartPolicy>,
+
+    pub ncpus: InstanceCpuCount,
+
+    pub memory: ByteCount,
 }

nexus/db-model/src/instance_cpu_count.rs

@@ -13,7 +13,15 @@ use serde::Serialize;
 use std::convert::TryFrom;
 
 #[derive(
-    Copy, Clone, Debug, AsExpression, FromSqlRow, Serialize, Deserialize,
+    Copy,
+    Clone,
+    Debug,
+    AsExpression,
+    FromSqlRow,
+    Serialize,
+    Deserialize,
+    PartialEq,
+    Eq,
 )]
 #[diesel(sql_type = sql_types::BigInt)]
 pub struct InstanceCpuCount(pub external::InstanceCpuCount);

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

@@ -18,10 +18,12 @@ use crate::db::error::public_error_from_diesel;
 use crate::db::error::ErrorHandler;
 use crate::db::identity::Resource;
 use crate::db::lookup::LookupPath;
+use crate::db::model::ByteCount;
 use crate::db::model::Generation;
 use crate::db::model::Instance;
 use crate::db::model::InstanceAutoRestart;
 use crate::db::model::InstanceAutoRestartPolicy;
+use crate::db::model::InstanceCpuCount;
 use crate::db::model::InstanceRuntimeState;
 use crate::db::model::InstanceState;
 use crate::db::model::InstanceUpdate;
@@ -1038,8 +1040,12 @@ impl DataStore {
             .transaction_retry_wrapper("reconfigure_instance")
             .transaction(&conn, |conn| {
                 let err = err.clone();
-                let InstanceUpdate { boot_disk_id, auto_restart_policy } =
-                    update.clone();
+                let InstanceUpdate {
+                    boot_disk_id,
+                    auto_restart_policy,
+                    ncpus,
+                    memory,
+                } = update.clone();
                 async move {
                     // Set the auto-restart policy.
                     diesel::update(instance_dsl::instance)
@@ -1051,6 +1057,16 @@ impl DataStore {
                         .execute_async(&conn)
                         .await?;
 
+                    // Set vCPUs and memory size.
+                    self.instance_set_size_on_conn(
+                        &conn,
+                        &err,
+                        authz_instance,
+                        ncpus,
+                        memory,
+                    )
+                    .await?;
+
                     // Next, set the boot disk if needed.
                     self.instance_set_boot_disk_on_conn(
                         &conn,
@@ -1141,13 +1157,13 @@ impl DataStore {
             InstanceState::Creating,
         ];
 
-        let maybe_instance = instance_dsl::instance
+        let maybe_old_boot_disk_id = instance_dsl::instance
             .filter(instance_dsl::id.eq(authz_instance.id()))
             .filter(instance_dsl::time_deleted.is_null())
-            .select(Instance::as_select())
-            .first_async::<Instance>(conn)
+            .select(instance_dsl::boot_disk_id)
+            .first_async::<Option<Uuid>>(conn)
             .await;
-        let instance = match maybe_instance {
+        let old_boot_disk_id = match maybe_old_boot_disk_id {
             Ok(i) => i,
             Err(diesel::NotFound) => {
                 // If the instance simply doesn't exist, we
@@ -1163,7 +1179,7 @@ impl DataStore {
         // If the desired boot disk is already set, we're good here, and can
         // elide the check that the instance is in an acceptable state to change
         // the boot disk.
-        if instance.boot_disk_id == boot_disk_id {
+        if old_boot_disk_id == boot_disk_id {
             return Ok(());
         }
 
@@ -1215,6 +1231,79 @@ impl DataStore {
         }
     }
 
+    /// Set an instance's CPU count and memory size to the provided values,
+    /// within an existing transaction.
+    async fn instance_set_size_on_conn(
+        &self,
+        conn: &async_bb8_diesel::Connection<DbConnection>,
+        err: &OptionalError<Error>,
+        authz_instance: &authz::Instance,
+        ncpus: InstanceCpuCount,
+        memory: ByteCount,
+    ) -> Result<(), diesel::result::Error> {
+        use db::schema::instance::dsl as instance_dsl;
+
+        // Do not allow setting sizes of running instances to ensure that if an
+        // instance is running, its resource usage matches what we record in the
+        // database.
+        const OK_TO_SET_SIZE_STATES: &'static [InstanceState] = &[
+            InstanceState::NoVmm,
+            InstanceState::Failed,
+            InstanceState::Creating,
+        ];
+
+        let maybe_old_sizes = instance_dsl::instance
+            .filter(instance_dsl::id.eq(authz_instance.id()))
+            .filter(instance_dsl::time_deleted.is_null())
+            .select((instance_dsl::ncpus, instance_dsl::memory))
+            .first_async::<(InstanceCpuCount, ByteCount)>(conn)
+            .await;
+
+        // Might be nice to extract `old_sizes` into an `InstanceDimensions` or
+        // something? Not processing the tuple `(InstanceCpuCount, ByteCount)`
+        // elsewhere yet, so maybe that would be premature...
+        let old_sizes = match maybe_old_sizes {
+            Ok(i) => i,
+            Err(diesel::NotFound) => {
+                // If the instance simply doesn't exist, we
+                // shouldn't retry. Bail with a useful error.
+                return Err(err.bail(Error::not_found_by_id(
+                    ResourceType::Instance,
+                    &authz_instance.id(),
+                )));
+            }
+            Err(e) => return Err(e),
+        };
+
+        // Bail out early if the extant vCPU and memory settings match the
+        // "new values".
+        if old_sizes == (ncpus, memory) {
+            return Ok(());
+        }
+
+        let r = diesel::update(instance_dsl::instance)
+            .filter(instance_dsl::id.eq(authz_instance.id()))
+            .filter(instance_dsl::state.eq_any(OK_TO_SET_SIZE_STATES))
+            .set((
+                instance_dsl::ncpus.eq(ncpus),
+                instance_dsl::memory.eq(memory),
+            ))
+            .check_if_exists::<Instance>(authz_instance.id())
+            .execute_and_check(&conn)
+            .await?;
+        match r.status {
+            UpdateStatus::NotUpdatedButExists => {
+                // This should be the only reason the query would fail...
+                debug_assert!(!OK_TO_SET_SIZE_STATES
+                    .contains(&r.found.runtime().nexus_state));
+                Err(err.bail(Error::conflict(
+                    "instance must be stopped to be resized",
+                )))
+            }
+            UpdateStatus::Updated => Ok(()),
+        }
+    }
+
     pub async fn project_delete_instance(
         &self,
         opctx: &OpContext,

nexus/src/app/instance.rs

@@ -41,6 +41,7 @@ use omicron_common::api::external::CreateResult;
 use omicron_common::api::external::DataPageParams;
 use omicron_common::api::external::DeleteResult;
 use omicron_common::api::external::Error;
+use omicron_common::api::external::InstanceCpuCount;
 use omicron_common::api::external::InstanceState;
 use omicron_common::api::external::InternalContext;
 use omicron_common::api::external::ListResultVec;
@@ -311,6 +312,8 @@ impl super::Nexus {
         let (.., authz_project, authz_instance) =
             instance_lookup.lookup_for(authz::Action::Modify).await?;
 
+        check_instance_cpu_memory_sizes(params.ncpus, params.memory)?;
+
         let boot_disk_id = match params.boot_disk.clone() {
             Some(disk) => {
                 let selector = params::DiskSelector {
@@ -331,8 +334,11 @@ impl super::Nexus {
         };
 
         let auto_restart_policy = params.auto_restart_policy.map(Into::into);
+        let ncpus = params.ncpus.into();
+        let memory = params.memory.into();
 
-        let update = InstanceUpdate { boot_disk_id, auto_restart_policy };
+        let update =
+            InstanceUpdate { boot_disk_id, auto_restart_policy, ncpus, memory };
         self.datastore()
             .instance_reconfigure(opctx, &authz_instance, update)
             .await
@@ -347,6 +353,8 @@ impl super::Nexus {
         let (.., authz_project) =
             project_lookup.lookup_for(authz::Action::CreateChild).await?;
 
+        check_instance_cpu_memory_sizes(params.ncpus, params.memory)?;
+
         let all_disks: Vec<&params::InstanceDiskAttachment> =
             params.boot_disk.iter().chain(params.disks.iter()).collect();
 
@@ -363,12 +371,6 @@ impl super::Nexus {
                     .await?;
             }
         }
-        if params.ncpus.0 > MAX_VCPU_PER_INSTANCE {
-            return Err(Error::invalid_request(&format!(
-                "cannot have more than {} vCPUs per instance",
-                MAX_VCPU_PER_INSTANCE
-            )));
-        }
         if params.external_ips.len() > MAX_EXTERNAL_IPS_PER_INSTANCE {
             return Err(Error::invalid_request(&format!(
                 "An instance may not have more than {} external IP addresses",
@@ -413,43 +415,6 @@ impl super::Nexus {
             }
         }
 
-        // Reject instances where the memory is not at least
-        // MIN_MEMORY_BYTES_PER_INSTANCE
-        if params.memory.to_bytes() < u64::from(MIN_MEMORY_BYTES_PER_INSTANCE) {
-            return Err(Error::invalid_value(
-                "size",
-                format!(
-                    "memory must be at least {}",
-                    ByteCount::from(MIN_MEMORY_BYTES_PER_INSTANCE)
-                ),
-            ));
-        }
-
-        // Reject instances where the memory is not divisible by
-        // MIN_MEMORY_BYTES_PER_INSTANCE
-        if (params.memory.to_bytes() % u64::from(MIN_MEMORY_BYTES_PER_INSTANCE))
-            != 0
-        {
-            return Err(Error::invalid_value(
-                "size",
-                format!(
-                    "memory must be divisible by {}",
-                    ByteCount::from(MIN_MEMORY_BYTES_PER_INSTANCE)
-                ),
-            ));
-        }
-
-        // Reject instances where the memory is greater than the limit
-        if params.memory.to_bytes() > MAX_MEMORY_BYTES_PER_INSTANCE {
-            return Err(Error::invalid_value(
-                "size",
-                format!(
-                    "memory must be less than or equal to {}",
-                    ByteCount::try_from(MAX_MEMORY_BYTES_PER_INSTANCE).unwrap()
-                ),
-            ));
-        }
-
         let actor = opctx.authn.actor_required().internal_context(
             "loading current user's ssh keys for new Instance",
         )?;
@@ -2187,6 +2152,57 @@ pub(crate) async fn process_vmm_update(
     }
 }
 
+/// Determines whether the supplied instance sizes (CPU count and memory size)
+/// are acceptable.
+fn check_instance_cpu_memory_sizes(
+    ncpus: InstanceCpuCount,
+    memory: ByteCount,
+) -> Result<(), Error> {
+    if ncpus.0 > MAX_VCPU_PER_INSTANCE {
+        return Err(Error::invalid_request(&format!(
+            "cannot have more than {} vCPUs per instance",
+            MAX_VCPU_PER_INSTANCE
+        )));
+    }
+
+    // Reject instances where the memory is not at least
+    // MIN_MEMORY_BYTES_PER_INSTANCE
+    if memory.to_bytes() < u64::from(MIN_MEMORY_BYTES_PER_INSTANCE) {
+        return Err(Error::invalid_value(
+            "size",
+            format!(
+                "memory must be at least {}",
+                ByteCount::from(MIN_MEMORY_BYTES_PER_INSTANCE)
+            ),
+        ));
+    }
+
+    // Reject instances where the memory is not divisible by
+    // MIN_MEMORY_BYTES_PER_INSTANCE
+    if (memory.to_bytes() % u64::from(MIN_MEMORY_BYTES_PER_INSTANCE)) != 0 {
+        return Err(Error::invalid_value(
+            "size",
+            format!(
+                "memory must be divisible by {}",
+                ByteCount::from(MIN_MEMORY_BYTES_PER_INSTANCE)
+            ),
+        ));
+    }
+
+    // Reject instances where the memory is greater than the limit
+    if memory.to_bytes() > MAX_MEMORY_BYTES_PER_INSTANCE {
+        return Err(Error::invalid_value(
+            "size",
+            format!(
+                "memory must be less than or equal to {}",
+                ByteCount::try_from(MAX_MEMORY_BYTES_PER_INSTANCE).unwrap()
+            ),
+        ));
+    }
+
+    Ok(())
+}
+
 /// Determines the disposition of a request to start an instance given its state
 /// (and its current VMM's state, if it has one) in the database.
 fn instance_start_allowed(

nexus/tests/integration_tests/endpoints.rs

@@ -437,6 +437,8 @@ pub static DEMO_INSTANCE_UPDATE: Lazy<params::InstanceUpdate> =
     Lazy::new(|| params::InstanceUpdate {
         boot_disk: None,
         auto_restart_policy: None,
+        ncpus: InstanceCpuCount(1),
+        memory: ByteCount::from_gibibytes_u32(16),
     });
 
 // The instance needs a network interface, too.