[nexus] allow reconfiguring auto-restart policies#6743
Conversation
|
Turning this into a draft, since I think the approach of doing everything in one query feels kinda sketchy --- in particular there's a possible race if someone sets an auto-restart policy when an instance is in Gonna try and pull apart the queries for setting those fields and just have the API endpoint do all of them, and make the saga use a more focused query. |
ab3c972 to
fcdc758
Compare
This commit extends the `instance-reconfigure` API endpoint added in #6585 to also allow setting instance auto-restart policies (as added in #6503). I've also added the actual auto-restart policy to the external API instance view, along with the boolean `auto_restart_enabled` added in #6503. This way, it's possible to change just the boot disk by providing the current auto-restart policy in an instance POST.
fcdc758 to
d867ef4
Compare
Co-authored-by: iximeow <iximeow@oxide.computer>
iximeow
left a comment
There was a problem hiding this comment.
nice! it's really good to be able to be able to update the auto-restart policy of running instances too.
gjcolombo
left a comment
There was a problem hiding this comment.
This looks good to me.
I sort of wonder if we can get away without using a transaction to do the update. I suspect it might be possible to do this via CTE--something like: do a subquery to look up rows in disk that match the requested boot device and a subquery to look up the instance, then do a join on instance.id = disk.attach_instance_id to determine whether there's a valid instance row to update. The update function would probably also need to take a slice of expected prior instance states; the external API would set this to [NoVmm, Failed] and the instance-create path would set it to [Creating].
Anyway, not necessary to do any of this now, but perhaps worth keeping in mind if this transaction turns out to be too expensive/shows up in our transaction-retry metrics in the future.
|
@gjcolombo I also have been wondering about getting rid of the transaction, I think it's probably possible, perhaps even without having to do a whole CTE for it. I'm not sure whether it's worth spending a bunch of time on though, since my intuition is that changing instance configurations isn't going to be particularly hot --- this feels like an infrequent user action, rather than an automated operation that runs hundreds of times a second or whatever. |
This commit extends the
instance-reconfigureAPI endpoint added in#6585 to also allow setting instance auto-restart policies (as added in
#6503).
I've also added the actual auto-restart policy to the external API
instance view, along with the boolean
auto_restart_enabledadded in#6503. This way, it's possible to change just the boot disk by providing
the current auto-restart policy in an instance POST.