[nexus] Don't watch VMMs that are terminal or belong to deleted instances - #11100
Open
smklein wants to merge 5 commits into
Open
[nexus] Don't watch VMMs that are terminal or belong to deleted instances#11100smklein wants to merge 5 commits into
smklein wants to merge 5 commits into
Conversation
The unwinding tests delete the test instance directly rather than going through the normal deletion path, which requires its VMMs to be cleaned up first. This left live VMM rows behind pointing at a deleted instance, and the instance_watcher background task could then health check them and trip the debug assertions in InstanceStateComputer::compute_state_from on the (Destroyed instance, live VMM) state pair. Part of the fix for #8755.
The instance_watcher query excluded VMM states known not to exist on a sled, but still returned VMMs in terminal states (Failed, Destroyed). A terminal-state VMM will never transition again, so a health check can learn nothing further about it. Notably, this returned Failed VMMs belonging to instances that had already been deleted, since deletion is permitted while an instance is Failed and the Failed VMM row outlives the instance record briefly. Part of the fix for #8755.
The instance_watcher query joined the vmm table with the instance table without filtering out deleted instances. A live VMM row belonging to a deleted instance could therefore be health checked, and the resulting (Destroyed instance, live VMM state) pair trips debug assertions in InstanceStateComputer::compute_state_from, which expects a deleted instance to have no associated VMM. Part of the fix for #8755.
…ed-agents Marking the VMM rows deleted in the database stops the instance_watcher from health checking them, but left the VMM objects behind on the simulated sled-agents. Orphaned sim VMMs are latent state: if anything ever pokes one, the sim agent's Nexus notification fails (the VMM row is gone) and the notify unwrap panics the sim agent mid-request. Unregister them as well, which removes the objects without notifying Nexus.
Collaborator
Author
|
Also just added 67181cc as an extra commit - it isn't necessary to fix the flake, but feels like "correct" clean-up |
…ive VMM The instance_watcher joins vmm rows to instances on instance_id alone, so the VMM it checks may be a migration target or no longer linked to the instance. InstanceStateComputer assumes it is given the active VMM's state, and debug asserts on instance/VMM state pairs that cannot occur for an active VMM, such as an instance in NoVmm paired with a leftover Migrating migration target (reproduced in #11101). The watcher only needs the external instance state corresponding to the observed VMM state, so pass InstanceState::Vmm unconditionally instead of the instance's actual state. Part of the fix for #8755.
smklein
changed the title
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The
instance_watcherbackground task's query could return a live VMM row paired with a deleted instance record. Health checking such a VMM trips debug assertions inInstanceStateComputer::compute_state_from, which expects a deleted instance to have no VMM state, causing the test flake in #8755.These fixes include:
test_migration_source_failed_destroyed_can_unwind.Failed,Destroyed), which will never transition again, so a health check can learn nothing further about them. This commit extendstest_instance_and_vmm_list_by_sled_agentwith an instance in each terminal VMM state and asserts that neither is listed.vmmtable with theinstancetable. This commit extends the same test with a deleted instance whose VMM row still exists, again asserting it is not listed. Additionally, it always passesInstanceState::Vmmto theInstanceStateComputer, to avoid src/dst issues during migration that can cause theInstanceStatemachine to panic. This avoids panicking the test demonstrated in reproducer for #8755 #11101Fixes #8755.