Repository navigation
[reconfigurator] background task for stopping marked VMMs #11380
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
Merged
Merged
Changes from 27 commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
310551c
[reconfigurator] background task for stopping marked VMMs
karencfv e1f14c3
let's do batches
karencfv 99e577d
fix some comments
karencfv cb3ea34
use IdOrdMap everywhere
karencfv b1a9a19
have nicer return types
karencfv 055b6b0
pondering...
karencfv 81b1f62
merge main
karencfv a8f3e4e
Do the stopping and add a bunch of random thoughts
karencfv 787853b
merge main
karencfv f4fea86
integrate with vmm marking bg task
karencfv a91d7d1
handle all possible scenarios attempting to stop a VMM
karencfv 56d116c
Do not change the intended state if we're stopping for an update
karencfv 267ed82
better logging
karencfv 5f3b05e
expectorate
karencfv bf73666
add test for task
karencfv 3a62920
organise test
karencfv 8c2fdae
clean up
karencfv 6196cb9
add db-query test
karencfv 9ef8b8e
clean up
karencfv 97e2afc
formatting
karencfv ce41e5e
call notify_instance_updated
karencfv f076ccb
add comment
karencfv 1a63679
change interval for running task
karencfv 26cb9ac
remove nexus
karencfv 26779a3
one reqwest client
karencfv bc3cb0f
typos and small fixes
karencfv 14a620f
use paginated_multicolumn
karencfv c0154d4
put the client where it should go
karencfv 9f418e3
parallel tasks
karencfv 6d41c21
use BTreeSet and leave TODOs for tomorrow
karencfv e42049b
omfg
karencfv 179a441
log the instance id
karencfv 29db8ef
handle vmm_gone
karencfv f56309c
optimise the optimisation
karencfv a603bfa
clean up
karencfv 80eafe6
improve documentation
karencfv a30797d
make field more explicit
karencfv 2533d11
cache the reqwest client
karencfv 73529c9
fix logs
karencfv 1e77105
fix typos
karencfv 7355d30
do not stop Creating VMMs
karencfv a0b22ca
only retrieve vmms in in-service sleds
karencfv 6e18f90
adapt tests
karencfv 63d3678
fix typos before John finds them
karencfv 54357d1
address comments
karencfv 304c2f8
improve testing on failures
karencfv File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts? My initial inclination is to respect whatever policy the user set. They did so for a reason, and we shouldn't override settings like these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My inclination is the opposite, actually, since I think that when we have talked about the auto-restart policy to users (i.e. in the API docs), we explicitly state that the auto-restart behavior is intended to describe what we should do if the VM fails (i.e. the guest OS panics, or the VMM panics, or the sled disappears for some reason). My thinking on why a user might want to disable automatic restarts is if you don't really trust the VM to not crash itself or the sled it's on, and want us to avoid restarting it automatically if it crashes, because it might just crash again. Whether or not we should restart the VM if we stopped it on purpose in order to update the sled is quite different than whether we should restart it because it crashed, IMO.
If anything, I think perhaps we might want to consider an
AutoRestartPolicy::UpdatesOnlyor something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Thinking also about the case of a VMM failing during a sled evacuation. In this case I think it still makes sense to follow whatever the user had previously set.
I can think of other scenarios where a user may want to set the auto-restart policy to
Never. Perhaps they are running their software on some sort of orchestrator like Kubernetes and do not wish for random nodes to pop up again after they have been replaced.Maybe? I'm not sure this is necessary yet. This may solve a problem no one has yet. It'd be good to wait and hear feedback from customers before implementing another policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's correct, if the instance winds up in the
Failedstate, the decision of whether or not to restart it should be based on the configured auto-restart policy (and the instance'sintended_state). I think I may have misinterpreted your comment as asking whether we should determine if instances in theStoppedstate1 due to sled evacuation would be restarted based on the auto-restart policy. In that case, when a VMM stops gracefully because the sled was being evacuated, we do want to restart it elsewhere even if the auto-restart policy isNever, since the auto-restart policy is intended to configure whether instances are restarted on unexpected failures.Yeah, I agree; what I meant was that if we do end up getting customer feedback that they want instances stopped for sled evacuation to not restart, I think that ought to be a separate knob, rather than making the
Neverauto-restart policy also control restarting evacuated instances.Footnotes
i.e. those which were told to stop for sled evacuation and which stopped cleanly ↩