InstanceUpdate body: change Option fields to Nullable so they're required to be present (#9046)

Optional fields on a request body can be left out completely from the
JSON. In the case of instance update (and probably many other endpoints)
this is a problem because passing `null` and passing nothing have the
same effect, namely unsetting that field if it is already set in the DB.
And because the fields are optional, the generated client types do not
enforce that they are present (at least when creating the JSON by hand
and also in TypeScript, which distinguishes between optional and
nullable at the type level). It is easy to do this by accident: while
working on [this](oxidecomputer/console#2900), I
[discovered a
bug](oxidecomputer/console#2900 (comment))
in the console where we accidentally unset the auto restart policy on an
instance when you resize an instance.

This PR changes `Option` to `Nullable`, which I added in #8214 to fix
this problem: `null` remains a valid value, but the client is no longer
allowed to leave that field out of the body. If we don't want to do
this, I can live with it because in the console, I have [made a
helper](oxidecomputer/console@60ca75e)
that enforces that all fields are explicitly present. But I do think the
status quo is very error-prone.


https://github.com/oxidecomputer/omicron/blob/c6989117c72ecf88d4e3dc8a597d9fe703152a93/common/src/api/external/mod.rs#L3564-L3570

Author: david-crespo

nexus/src/app/instance.rs

@@ -367,7 +367,7 @@ impl super::Nexus {
 
         check_instance_cpu_memory_sizes(*ncpus, *memory)?;
 
-        let boot_disk_id = match boot_disk {
+        let boot_disk_id = match boot_disk.as_ref() {
             Some(disk) => {
                 let selector = params::DiskSelector {
                     project: match &disk {

nexus/tests/integration_tests/endpoints.rs

@@ -688,9 +688,9 @@ pub static DEMO_STOPPED_INSTANCE_CREATE: LazyLock<params::InstanceCreate> =
     });
 pub static DEMO_INSTANCE_UPDATE: LazyLock<params::InstanceUpdate> =
     LazyLock::new(|| params::InstanceUpdate {
-        boot_disk: None,
-        cpu_platform: None,
-        auto_restart_policy: None,
+        boot_disk: Nullable(None),
+        cpu_platform: Nullable(None),
+        auto_restart_policy: Nullable(None),
         ncpus: InstanceCpuCount(1),
         memory: ByteCount::from_gibibytes_u32(16),
     });

nexus/tests/integration_tests/instances.rs

@@ -68,6 +68,7 @@ use omicron_common::api::external::InstanceNetworkInterface;
 use omicron_common::api::external::InstanceState;
 use omicron_common::api::external::Name;
 use omicron_common::api::external::NameOrId;
+use omicron_common::api::external::Nullable;
 use omicron_common::api::external::Vni;
 use omicron_common::api::internal::shared::ResolvedVpcRoute;
 use omicron_common::api::internal::shared::RouterId;
@@ -4920,9 +4921,9 @@ async fn test_cannot_detach_boot_disk(cptestctx: &ControlPlaneTestContext) {
         &client,
         &instance.identity.id,
         params::InstanceUpdate {
-            boot_disk: None,
-            auto_restart_policy: None,
-            cpu_platform: None,
+            boot_disk: Nullable(None),
+            auto_restart_policy: Nullable(None),
+            cpu_platform: Nullable(None),
             ncpus: InstanceCpuCount::try_from(2).unwrap(),
             memory: ByteCount::from_gibibytes_u32(4),
         },
@@ -5025,9 +5026,9 @@ async fn test_updating_running_instance_boot_disk_is_conflict(
         &client,
         &instance_id.into_untyped_uuid(),
         params::InstanceUpdate {
-            boot_disk: Some(alsodata.clone().into()),
-            auto_restart_policy: None,
-            cpu_platform: None,
+            boot_disk: Nullable(Some(alsodata.clone().into())),
+            auto_restart_policy: Nullable(None),
+            cpu_platform: Nullable(None),
             ncpus: InstanceCpuCount::try_from(2).unwrap(),
             memory: ByteCount::from_gibibytes_u32(4),
         },
@@ -5044,9 +5045,11 @@ async fn test_updating_running_instance_boot_disk_is_conflict(
         params::InstanceUpdate {
             // Leave the boot disk the same as the one with which the instance
             // was created.
-            boot_disk: Some(probablydata.clone().into()),
-            auto_restart_policy: Some(InstanceAutoRestartPolicy::BestEffort),
-            cpu_platform: None,
+            boot_disk: Nullable(Some(probablydata.clone().into())),
+            auto_restart_policy: Nullable(Some(
+                InstanceAutoRestartPolicy::BestEffort,
+            )),
+            cpu_platform: Nullable(None),
             ncpus: InstanceCpuCount::try_from(2).unwrap(),
             memory: ByteCount::from_gibibytes_u32(4),
         },
@@ -5067,9 +5070,9 @@ async fn test_updating_missing_instance_is_not_found(
         &client,
         &UUID_THAT_DOESNT_EXIST,
         params::InstanceUpdate {
-            boot_disk: None,
-            auto_restart_policy: None,
-            cpu_platform: None,
+            boot_disk: Nullable(None),
+            auto_restart_policy: Nullable(None),
+            cpu_platform: Nullable(None),
             ncpus: InstanceCpuCount::try_from(0).unwrap(),
             memory: ByteCount::from_gibibytes_u32(0),
         },
@@ -5185,16 +5188,22 @@ async fn test_size_can_be_changed(cptestctx: &ControlPlaneTestContext) {
     let new_ncpus = InstanceCpuCount::try_from(4).unwrap();
     let new_memory = ByteCount::from_gibibytes_u32(8);
 
+    let base_update = params::InstanceUpdate {
+        auto_restart_policy: Nullable(auto_restart_policy),
+        boot_disk: Nullable(boot_disk_nameorid.clone()),
+        cpu_platform: Nullable(None),
+        ncpus: initial_ncpus,
+        memory: initial_memory,
+    };
+
     // Resizing the instance immediately will error; the instance is running.
     let err = expect_instance_reconfigure_err(
         client,
         &instance.identity.id,
         params::InstanceUpdate {
-            auto_restart_policy,
-            boot_disk: boot_disk_nameorid.clone(),
-            cpu_platform: None,
             ncpus: new_ncpus,
             memory: new_memory,
+            ..base_update.clone()
         },
         StatusCode::CONFLICT,
     )
@@ -5213,11 +5222,9 @@ async fn test_size_can_be_changed(cptestctx: &ControlPlaneTestContext) {
         client,
         &instance.identity.id,
         params::InstanceUpdate {
-            auto_restart_policy,
-            boot_disk: boot_disk_nameorid.clone(),
-            cpu_platform: None,
             ncpus: new_ncpus,
             memory: new_memory,
+            ..base_update.clone()
         },
     )
     .await;
@@ -5229,11 +5236,9 @@ async fn test_size_can_be_changed(cptestctx: &ControlPlaneTestContext) {
         client,
         &instance.identity.id,
         params::InstanceUpdate {
-            auto_restart_policy,
-            boot_disk: boot_disk_nameorid.clone(),
-            cpu_platform: None,
             ncpus: initial_ncpus,
             memory: new_memory,
+            ..base_update.clone()
         },
     )
     .await;
@@ -5244,11 +5249,9 @@ async fn test_size_can_be_changed(cptestctx: &ControlPlaneTestContext) {
         client,
         &instance.identity.id,
         params::InstanceUpdate {
-            auto_restart_policy,
-            boot_disk: boot_disk_nameorid.clone(),
-            cpu_platform: None,
             ncpus: initial_ncpus,
             memory: initial_memory,
+            ..base_update.clone()
         },
     )
     .await;
@@ -5263,11 +5266,9 @@ async fn test_size_can_be_changed(cptestctx: &ControlPlaneTestContext) {
         client,
         &instance.identity.id,
         params::InstanceUpdate {
-            auto_restart_policy,
-            boot_disk: boot_disk_nameorid.clone(),
-            cpu_platform: None,
             ncpus: InstanceCpuCount(MAX_VCPU_PER_INSTANCE + 1),
             memory: instance.memory,
+            ..base_update.clone()
         },
         StatusCode::BAD_REQUEST,
     )
@@ -5285,11 +5286,9 @@ async fn test_size_can_be_changed(cptestctx: &ControlPlaneTestContext) {
         client,
         &instance.identity.id,
         params::InstanceUpdate {
-            auto_restart_policy,
-            boot_disk: boot_disk_nameorid.clone(),
-            cpu_platform: None,
             ncpus: instance.ncpus,
             memory: ByteCount::from_mebibytes_u32(0),
+            ..base_update.clone()
         },
         StatusCode::BAD_REQUEST,
     )
@@ -5301,12 +5300,10 @@ async fn test_size_can_be_changed(cptestctx: &ControlPlaneTestContext) {
         client,
         &instance.identity.id,
         params::InstanceUpdate {
-            auto_restart_policy,
-            boot_disk: boot_disk_nameorid.clone(),
-            cpu_platform: None,
             ncpus: instance.ncpus,
             memory: ByteCount::try_from(MAX_MEMORY_BYTES_PER_INSTANCE - 1)
                 .unwrap(),
+            ..base_update.clone()
         },
         StatusCode::BAD_REQUEST,
     )
@@ -5319,13 +5316,11 @@ async fn test_size_can_be_changed(cptestctx: &ControlPlaneTestContext) {
         client,
         &instance.identity.id,
         params::InstanceUpdate {
-            auto_restart_policy,
-            boot_disk: boot_disk_nameorid.clone(),
-            cpu_platform: None,
             ncpus: instance.ncpus,
             memory: ByteCount::from_mebibytes_u32(
                 (max_mib + 1024).try_into().unwrap(),
             ),
+            ..base_update.clone()
         },
         StatusCode::BAD_REQUEST,
     )
@@ -5342,11 +5337,9 @@ async fn test_size_can_be_changed(cptestctx: &ControlPlaneTestContext) {
         client,
         &instance.identity.id,
         params::InstanceUpdate {
-            auto_restart_policy,
-            boot_disk: boot_disk_nameorid.clone(),
-            cpu_platform: None,
             ncpus: new_ncpus,
             memory: new_memory,
+            ..base_update.clone()
         },
         StatusCode::NOT_FOUND,
     )
@@ -5404,9 +5397,9 @@ async fn test_auto_restart_policy_can_be_changed(
             client,
             &instance.identity.id,
             dbg!(params::InstanceUpdate {
-                auto_restart_policy,
-                boot_disk: None,
-                cpu_platform: None,
+                auto_restart_policy: Nullable(auto_restart_policy),
+                boot_disk: Nullable(None),
+                cpu_platform: Nullable(None),
                 ncpus: InstanceCpuCount::try_from(2).unwrap(),
                 memory: ByteCount::from_gibibytes_u32(4),
             }),
@@ -5477,9 +5470,9 @@ async fn test_cpu_platform_can_be_changed(cptestctx: &ControlPlaneTestContext) {
             client,
             &instance.identity.id,
             dbg!(params::InstanceUpdate {
-                auto_restart_policy: None,
-                boot_disk: None,
-                cpu_platform,
+                auto_restart_policy: Nullable(None),
+                boot_disk: Nullable(None),
+                cpu_platform: Nullable(cpu_platform),
                 ncpus: InstanceCpuCount::try_from(2).unwrap(),
                 memory: ByteCount::from_gibibytes_u32(4),
             }),
@@ -5572,9 +5565,9 @@ async fn test_boot_disk_can_be_changed(cptestctx: &ControlPlaneTestContext) {
         &client,
         &instance.identity.id,
         params::InstanceUpdate {
-            boot_disk: Some(disks[1].identity.id.into()),
-            auto_restart_policy: None,
-            cpu_platform: None,
+            boot_disk: Nullable(Some(disks[1].identity.id.into())),
+            auto_restart_policy: Nullable(None),
+            cpu_platform: Nullable(None),
             ncpus: InstanceCpuCount::try_from(2).unwrap(),
             memory: ByteCount::from_gibibytes_u32(4),
         },
@@ -5641,9 +5634,9 @@ async fn test_boot_disk_must_be_attached(cptestctx: &ControlPlaneTestContext) {
         &client,
         &instance.identity.id,
         params::InstanceUpdate {
-            boot_disk: Some(disks[0].identity.id.into()),
-            auto_restart_policy: None,
-            cpu_platform: None,
+            boot_disk: Nullable(Some(disks[0].identity.id.into())),
+            auto_restart_policy: Nullable(None),
+            cpu_platform: Nullable(None),
             ncpus: InstanceCpuCount::try_from(2).unwrap(),
             memory: ByteCount::from_gibibytes_u32(4),
         },
@@ -5675,9 +5668,9 @@ async fn test_boot_disk_must_be_attached(cptestctx: &ControlPlaneTestContext) {
         &client,
         &instance.identity.id,
         params::InstanceUpdate {
-            boot_disk: Some(disks[0].identity.id.into()),
-            auto_restart_policy: None,
-            cpu_platform: None,
+            boot_disk: Nullable(Some(disks[0].identity.id.into())),
+            auto_restart_policy: Nullable(None),
+            cpu_platform: Nullable(None),
             ncpus: InstanceCpuCount::try_from(2).unwrap(),
             memory: ByteCount::from_gibibytes_u32(4),
         },
@@ -6654,9 +6647,9 @@ async fn test_can_start_instance_with_cpu_platform(
         &client,
         &instance.identity.id,
         params::InstanceUpdate {
-            boot_disk: None,
-            auto_restart_policy: None,
-            cpu_platform: Some(InstanceCpuPlatform::AmdTurin),
+            boot_disk: Nullable(None),
+            auto_restart_policy: Nullable(None),
+            cpu_platform: Nullable(Some(InstanceCpuPlatform::AmdTurin)),
             ncpus: InstanceCpuCount::try_from(1).unwrap(),
             memory: ByteCount::from_gibibytes_u32(4),
         },

nexus/types/src/external_api/params.rs

@@ -1306,8 +1306,8 @@ pub struct InstanceUpdate {
 
     /// Name or ID of the disk the instance should be instructed to boot from.
     ///
-    /// If not provided, unset the instance's boot disk.
-    pub boot_disk: Option<NameOrId>,
+    /// A null value unsets the boot disk.
+    pub boot_disk: Nullable<NameOrId>,
 
     /// Sets the auto-restart policy for this instance.
     ///
@@ -1323,11 +1323,11 @@ pub struct InstanceUpdate {
     /// configurable through other mechanisms, such as on a per-project basis.
     /// In that case, any configured default policy will be used if this is
     /// `null`.
-    pub auto_restart_policy: Option<InstanceAutoRestartPolicy>,
+    pub auto_restart_policy: Nullable<InstanceAutoRestartPolicy>,
 
     /// The CPU platform to be used for this instance. If this is `null`, the
     /// instance requires no particular CPU platform.
-    pub cpu_platform: Option<InstanceCpuPlatform>,
+    pub cpu_platform: Nullable<InstanceCpuPlatform>,
 }
 
 #[inline]

openapi/nexus.json

@@ -20842,7 +20842,7 @@
           },
           "boot_disk": {
             "nullable": true,
-            "description": "Name or ID of the disk the instance should be instructed to boot from.\n\nIf not provided, unset the instance's boot disk.",
+            "description": "Name or ID of the disk the instance should be instructed to boot from.\n\nA null value unsets the boot disk.",
             "allOf": [
               {
                 "$ref": "#/components/schemas/NameOrId"
@@ -20876,6 +20876,9 @@
           }
         },
         "required": [
+          "auto_restart_policy",
+          "boot_disk",
+          "cpu_platform",
           "memory",
           "ncpus"
         ]