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

CA-380580: cross-pool migration: no CPU checks for halted VMs #5132

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

robhoes
Copy link
Member

@robhoes robhoes commented Jul 27, 2023

CPU checks are needed only for running VMs that are being migrated, to check for compatibility with the remote-host's CPUs.

CPU checks are needed only for running VMs that are being migrated, to
check for compatibility with the remote-host's CPUs.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
@@ -1456,7 +1457,7 @@ let migrate_send' ~__context ~vm ~dest ~live:_ ~vdi_map ~vif_map ~vgpu_map
in
inter_pool_metadata_transfer ~__context ~remote ~vm ~vdi_map
~vif_map ~vgpu_map ~dry_run:false ~live:true ~copy
~check_cpu:(not force)
~check_cpu:((not force) && power_state <> `Halted)
Copy link
Contributor

@edwintorok edwintorok Jul 27, 2023

Choose a reason for hiding this comment

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

how about 'Suspended' and 'Paused'? In theory you can move that around freely until you resume (at which point you do the CPU check).
Should the condition here we power_state = 'Running instead? (assuming that 'unpause' would do the CPU check, if not then Running||Paused.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an odd use case... The checks take into account that one may want to start/resume/unpause/shutdown the VM while it is being migrated to another pool. This is why the checks are always done with ~live:true, which checks for available memory on the target host, even if the VM is halted. The CPU checks would be similar, except that you can't actually do it for a halted VM.

@lindig
Copy link
Contributor

lindig commented Jul 27, 2023

Is this risking moving a halted VM to a host where it can't be started later?

@robhoes
Copy link
Member Author

robhoes commented Jul 27, 2023

Is this risking moving a halted VM to a host where it can't be started later?

No, CPU feature checks are for running VMs, because those would have made assumptions about the available CPU features when they started.

@robhoes robhoes merged commit 43ba295 into xapi-project:master Jul 27, 2023
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2024
…CPU check to the target host (#6175)

Rebased the work from 2023 merged in #5111 and #5132, that caused issues
and was partially fixed in #5148, but was completely reverted in #5147.
I've integrated the fix from #5148 and additionally the fix suggested by
@minglumlu in CA-380715 that was not merged at the time due to time
constraints.

This series passed the tests that were originally failing: sxm-unres
(Job ID 4177739), vGPUSXMM60CrossPool (4177750), and also passed the
Ring3 BST+BVT (209341). I can run more migration tests if needed - I've
heard @Vincent-lau has requested for these to be separated into its own
suite instead of being only in Core and Distribution regression tests.
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.

4 participants