Instance CPU types #2900
Merged
Instance CPU types #2900
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
david-crespo
force-pushed
the
bump-api-cpu-types
branch
from
8207fdb to
2c6de5d
Compare
david-crespo
commented
Sep 18, 2025
| bootDisk: instance.bootDiskId, | ||
| cpuPlatform: instance.cpuPlatform, | ||
| autoRestartPolicy: instance.autoRestartPolicy, | ||
| }, |
Collaborator
Author
There was a problem hiding this comment.
Pretty sure the fact that this was missing autoRestartPolicy was a bug.
Collaborator
Author
|
@iximeow do we want full UI support for the next release or can it wait until v18? Not that it's a big lift, really. |
Member
|
my 2c is that UI support isn't really helpful until someone's got Cosmos to work with, so I wouldn't sweat v17 for this |
Collaborator
Author
|
I might make a helper with a stricter type than the API that can help us avoid forgetting a key. Another thing we might consider is removing optional from those keys on the API side while keeping them nullable, if that’s possible. |
david-crespo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Sep 18, 2025
…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
david-crespo
force-pushed
the
bump-api-cpu-types
branch
from
934254c to
8b7536a
Compare
Collaborator
Author
I created this in 60ca75e and then removed it in 60ca75e because I did it on the API side. |
david-crespo
commented
Sep 19, 2025
| Currently, the global default auto-restart policy is "best-effort", so instances with `null` auto-restart policies will be automatically restarted. However, in the future, the default policy may be 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`. */ | ||
| autoRestartPolicy?: InstanceAutoRestartPolicy | null | ||
| autoRestartPolicy: InstanceAutoRestartPolicy | null |
Collaborator
Author
There was a problem hiding this comment.
no longer optional
david-crespo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Sep 19, 2025
charliepark
pushed a commit
to oxidecomputer/omicron
that referenced
this pull request
Sep 19, 2025
…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
david-crespo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Sep 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See oxidecomputer/omicron#8728. The API change is small and the default value means we don't actually have to change anything in the console yet except avoiding unsetting it in existing instance update calls. When we do add it to the UI, for create it would probably be down at the bottom of the form:
We'd also have to display the current value on instance detail and give a way to change it.