Instance auto restart #2644
Instance auto restart #2644
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| // DocsPopoverPanel needed for enter animation | ||
| className="DocsPopoverPanel z-10 w-96 rounded-lg border bg-raise border-secondary elevation-2" | ||
| // popover-panel needed for enter animation | ||
| className="popover-panel z-10 w-96 rounded-lg border bg-raise border-secondary elevation-2" |
There was a problem hiding this comment.
This casing feels like a carry over from radix, and perhaps inconsistent?
| className="max-w-none" | ||
| /> | ||
| <FormMeta label="Enabled" helpText="Help"> | ||
| {instance.autoRestartEnabled ? 'True' : 'False'}{' '} |
There was a problem hiding this comment.
With our mock instance, it's possible for the policy to be set to never but autoRestartEnabled to be true. In case of the actual API it'll automatically set autoRestartEnabled to be false.
We might want to modify the mock InstanceView handler to match this behaviour.
There was a problem hiding this comment.
Yeah, definitely modify the mock handler and link to the API code in a comment.
| body: { | ||
| ncpus: instance.ncpus, | ||
| memory: instance.memory, | ||
| bootDisk: instance.bootDiskId, |
There was a problem hiding this comment.
This strikes me as an awkward API design. Let's say the user just wants to update the autoRestartPolicy and does not include the bootDisk, it would then unset the instances boot disk. It also makes updating this in the CLI cumbersome.
Obviously it's outside of the scope of this PR, but thought I'd mention.
There was a problem hiding this comment.
We've talked about this a lot on the API side (see oxidecomputer/omicron#6406). Most of our PUTs don't work this way, but it makes things simpler and more consistent if they do, so we are trying to move in this direction. Having to specify all the fields every time is of course a downside for clients. However, the problem with making the fields optional is that if leaving a field out doesn't unset, you still need to be able to unset one if you want, which means you have to have different semantics for "bootDisk not present" and bootDisk: null, and that's both confusing and not spec compliant.
In the CLI, we can deal with this by letting the user update one field but behind the scenes it supplies all the others.
There was a problem hiding this comment.
And I suppose having dedicated instance resize and update instance restart policy endpoints adds a bunch of extra code on the API? I was think less optionality and more something that would facilitate (CLI) instance auto-restart policy update.
There was a problem hiding this comment.
Yeah, we don't really want to proliferate endpoints like that. My inclination would be to write that command by hand on the CLI side without it being a dedicated endpoint, or better yet come up with a pattern (a flag to instance update?) that lets you update one field while telling it to handle the rest for you.
| ) | ||
| } | ||
|
|
||
| const AutoRestartIcon12 = ({ className }: { className?: string }) => ( |
There was a problem hiding this comment.
todo: Add to design system and import from there
| useEffect(() => { | ||
| const timer = setInterval(() => setNow(new Date()), 1000) | ||
| return () => clearInterval(timer) | ||
| }, []) |
There was a problem hiding this comment.
I'll see about making this less scary
| description="If unconfigured this instance uses the default auto-restart policy, which may or may not allow it to be restarted." | ||
| placeholder="Default" | ||
| items={[ | ||
| { value: '', label: 'None (Default)' }, |
There was a problem hiding this comment.
I have a slight preference to just say "Default" here, rather than "None (Default)". I don't want to give the incorrect impression that not setting a policy is equivalent to disabling auto-restart when currently, we always default to "Best Effort".
There was a problem hiding this comment.
Sounds good – I'd included None because it makes the API more intuitive to a user that has used the console, but default is clearer.
I suppose the argument for enforcing auto_restart_policy not null, and adding a default enum on the API is that it means the user is forced to include it at create time, when for most users they shouldn't need to touch this?
Co-authored-by: Eliza Weisman <[email protected]>
Fixes #2469
autoRestartEnabledto match actual API behaviourI'm sure someone will talk me down from the slow spinning icon when auto restart is enabled.