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

david-crespo · 2025-09-18T17:51:15Z

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, I discovered a bug 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 that enforces that all fields are explicitly present. But I do think the status quo is very error-prone.

omicron/common/src/api/external/mod.rs

Lines 3564 to 3570 in c698911

    
           /// Use instead of Option in API request body structs to get a field that can 
        
           /// be null (parsed as `None`) but is not optional. Unlike Option, Nullable 
        
           /// will fail to parse if the key is not present. The JSON Schema in the 
        
           /// OpenAPI definition will also reflect that the field is required. See 
        
           /// <https://github.com/serde-rs/serde/issues/2753>. 
        
           #[derive(Clone, Debug, Serialize)] 
        
           pub struct Nullable<T>(pub Option<T>);

david-crespo · 2025-09-18T17:52:43Z

openapi/nexus.json

+          "boot_disk",
+          "cpu_platform",
          "memory",
          "ncpus"


This lets the TS client generator mark these fields as nullable but not optional.

nice, this rules.

FWIW, the Rust generator doesn't (currently) draw this distinction; I don't know about the go generator.

nexus/types/src/external_api/params.rs

iximeow

very good stuff

iximeow · 2025-09-18T18:00:48Z

openapi/nexus.json

+          "boot_disk",
+          "cpu_platform",
          "memory",
          "ncpus"


nice, this rules.

nexus/src/app/instance.rs

hawkw

tres bien

david-crespo · 2025-09-18T21:04:45Z

Issue for test flake: #9048

ahl

Is PUT the right interface? If I understand correctly, in order to use this interface I need to call instance_view, pull out some subset of those settings (e.g. not hostname), populate a PUT body and then call instance_update. This seems particularly cumbersome in the CLI.

Would we consider using PATCH here and distinguish the three states for a field:

value = set to that value
null = unset
absent = don't modify

ahl · 2025-09-18T23:01:21Z

nexus/types/src/external_api/params.rs

    ///
-    /// If not provided, unset the instance's boot disk.
-    pub boot_disk: Option<NameOrId>,
+    /// A null value unsets the boot disk.


What does it mean to have an unset boot disk? That the instance can't boot?

This is explained painstakingly in the create params. I will give this the same comment.

omicron/nexus/types/src/external_api/params.rs

Lines 1234 to 1250 in 3c97dce

/// The disk the instance is configured to boot from.

///

/// This disk can either be attached if it already exists or created along

/// with the instance.

///

/// Specifying a boot disk is optional but recommended to ensure predictable

/// boot behavior. The boot disk can be set during instance creation or

/// later if the instance is stopped. The boot disk counts against the disk

/// attachment limit.

///

/// An instance that does not have a boot disk set will use the boot

/// options specified in its UEFI settings, which are controlled by both the

/// instance's UEFI firmware and the guest operating system. Boot options

/// can change as disks are attached and detached, which may result in an

/// instance that only boots to the EFI shell until a boot disk is set.

#[serde(default)]

pub boot_disk: Option<InstanceDiskAttachment>,

(the main wrinkle being that not having a fixed boot disk is a meaningful and valid configuration, and that's the only way to let a VM manage its boot settings. it seems pretty frustrating to take that away by requiring an explicit boot disk, though it'd simplify some API questions like this one. it's all complicated and doesn't feel great.)

ahl · 2025-09-18T23:03:46Z

nexus/tests/integration_tests/instances.rs

+    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,
+    };


is this to avoid repeating code? For what it's worth, I found the prior structure easier to understand--especially when looking locally. Is your thought that this makes it easier to add new settings in the future? Perhaps this argues for a PATCH interface.

Yes, this was basically me grumbling about the fact that I would have had to fix a lot fewer lines of code if this had already been in place. But that is a narrow reason and it's easy to change it back.

ahl · 2025-09-18T23:04:28Z

openapi/nexus.json

+          "boot_disk",
+          "cpu_platform",
          "memory",
          "ncpus"


FWIW, the Rust generator doesn't (currently) draw this distinction; I don't know about the go generator.

ahl · 2025-09-18T23:05:12Z

nexus/src/app/instance.rs

        check_instance_cpu_memory_sizes(*ncpus, *memory)?;

-        let boot_disk_id = match boot_disk {
+        let boot_disk_id = match boot_disk.as_ref() {


it’s now a Nullable, so you have to pull out the option inside it, but .0 is not sufficient, the compile whines about that

see #9046 (comment)

david-crespo · 2025-09-18T23:48:51Z

PATCH would probably be more convenient, but we have the same serde/schema problem with the request body type — these would need to become Option<Nullable<T>> or some special type with custom serde and json schema in order to distinguish between null and not present. This would be our first PATCH endpoint.

#333 is a discussion of making our PUTs standards-compliant since we're not doing PATCH, but I can't find a direct discussion of doing PATCH. I vaguely remember there being one — maybe it was on an RFD.

iximeow · 2025-09-18T23:50:39Z

yes, I was just going to say RFD 4 (!) mentions a bit here. I think conceptually PATCH fits better than it might elsewhere specifically because of the difference between null and don't change this, but I'm still sympathetic to the argument that with PATCH two concurrent updates can race and produce a state neither expected, where racing PUT at least leaves you with something someone wanted. (though I also remember a more recent conversation about PUT/PATCH, which I also am struggling to find..)

ahl · 2025-09-19T00:00:35Z

yes, I was just going to say RFD 4 (!) mentions a bit here. I think conceptually PATCH fits better than it might elsewhere specifically because of the difference between null and don't change this, but I'm still sympathetic to the argument that with PATCH two concurrent updates can race and produce a state neither expected, where racing PUT at least leaves you with something someone wanted. (though I also remember a more recent conversation about PUT/PATCH, which I also am struggling to find..)

I think both provide the opportunity for different kinds of races. A CLI user--for example--will view ... ... ... .. update potentially blowing away subsequent updates unintentionally--and with no particular opinion about the consistency or intention of the other state, just blithely (and potentially inaccurately) copying it.

…e 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

) Followup to #9046

option -> nullable for instance update fields

2bbcbd5

david-crespo commented Sep 18, 2025

View reviewed changes

nexus/types/src/external_api/params.rs Outdated Show resolved Hide resolved

david-crespo requested review from ahl and iximeow September 18, 2025 17:53

iximeow approved these changes Sep 18, 2025

View reviewed changes

doc comment tweak, eschew &**

d44418d

david-crespo enabled auto-merge (squash) September 18, 2025 19:05

sudomateo mentioned this pull request Sep 18, 2025

Support IP pool API changes oxidecomputer/oxide.go#328

Open

hawkw approved these changes Sep 18, 2025

View reviewed changes

david-crespo mentioned this pull request Sep 18, 2025

test failed in CI: test_omdb_success_cases #9048

Closed

david-crespo mentioned this pull request Sep 18, 2025

test failed in CI: test_omdb_success_cases #8979

Closed

david-crespo merged commit 3c97dce into main Sep 18, 2025
17 checks passed

david-crespo deleted the nullable-instance-update branch September 18, 2025 22:53

ahl reviewed Sep 18, 2025

View reviewed changes

david-crespo mentioned this pull request Sep 19, 2025

Make doc comments on InstanceUpdate body match InstanceCreate #9055

Merged

david-crespo added a commit that referenced this pull request Sep 19, 2025

Make doc comments on InstanceUpdate body match InstanceCreate (#9055

ef00d62

) Followup to #9046

	/// Use instead of Option in API request body structs to get a field that can
	/// be null (parsed as `None`) but is not optional. Unlike Option, Nullable
	/// will fail to parse if the key is not present. The JSON Schema in the
	/// OpenAPI definition will also reflect that the field is required. See
	/// <https://github.com/serde-rs/serde/issues/2753>.
	#[derive(Clone, Debug, Serialize)]
	pub struct Nullable<T>(pub Option<T>);

	/// The disk the instance is configured to boot from.
	///
	/// This disk can either be attached if it already exists or created along
	/// with the instance.
	///
	/// Specifying a boot disk is optional but recommended to ensure predictable
	/// boot behavior. The boot disk can be set during instance creation or
	/// later if the instance is stopped. The boot disk counts against the disk
	/// attachment limit.
	///
	/// An instance that does not have a boot disk set will use the boot
	/// options specified in its UEFI settings, which are controlled by both the
	/// instance's UEFI firmware and the guest operating system. Boot options
	/// can change as disks are attached and detached, which may result in an
	/// instance that only boots to the EFI shell until a boot disk is set.
	#[serde(default)]
	pub boot_disk: Option<InstanceDiskAttachment>,

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

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

Uh oh!

Conversation

david-crespo commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iximeow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

david-crespo commented Sep 18, 2025

Uh oh!

Uh oh!

ahl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo commented Sep 18, 2025

Uh oh!

iximeow commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahl commented Sep 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

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

david-crespo commented Sep 18, 2025 •

edited

Loading

iximeow commented Sep 18, 2025 •

edited

Loading