Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nexus] allow reconfiguring auto-restart policies #6743

Merged
merged 14 commits into from
Oct 3, 2024

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Oct 1, 2024

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.

@hawkw hawkw requested review from iximeow and gjcolombo October 1, 2024 22:12
@hawkw hawkw marked this pull request as ready for review October 1, 2024 22:12
nexus/src/app/sagas/instance_create.rs Outdated Show resolved Hide resolved
nexus/tests/integration_tests/instances.rs Outdated Show resolved Hide resolved
@hawkw hawkw marked this pull request as draft October 1, 2024 22:49
@hawkw
Copy link
Member Author

hawkw commented Oct 1, 2024

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 Creating, and then sic_set_boot_disk clobbers it back to whatever's in the InstanceCreate params.

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.

@hawkw hawkw force-pushed the eliza/reconfigure-auto-restart branch from ab3c972 to fcdc758 Compare October 2, 2024 19:03
@hawkw hawkw marked this pull request as ready for review October 2, 2024 19:05
hawkw added 6 commits October 2, 2024 12:05
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.
@hawkw hawkw force-pushed the eliza/reconfigure-auto-restart branch from fcdc758 to d867ef4 Compare October 2, 2024 19:05
@hawkw hawkw requested a review from iximeow October 2, 2024 19:20
Co-authored-by: iximeow <[email protected]>
Copy link
Member

@iximeow iximeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! it's really good to be able to be able to update the auto-restart policy of running instances too.

Copy link
Contributor

@gjcolombo gjcolombo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hawkw hawkw enabled auto-merge (squash) October 2, 2024 21:01
@hawkw
Copy link
Member Author

hawkw commented Oct 2, 2024

@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.

@hawkw hawkw merged commit c52827d into main Oct 3, 2024
17 checks passed
@hawkw hawkw deleted the eliza/reconfigure-auto-restart branch October 3, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants