-
Notifications
You must be signed in to change notification settings - Fork 283
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
CP-49665: VM anti-affinity support for host evacuation - implemented with Psq #5652
CP-49665: VM anti-affinity support for host evacuation - implemented with Psq #5652
Conversation
1de8b31
to
ec11a05
Compare
ec11a05
to
dd9d98d
Compare
7910a4b
to
965098a
Compare
Add func host_to_vm_count_map to be used, rename RefMap to HostMap Signed-off-by: Gang Ji <gang.ji@citrix.com>
fixup review comments Signed-off-by: Gang Ji <gang.ji@citrix.com>
Signed-off-by: Gang Ji <gang.ji@citrix.com>
965098a
to
487b289
Compare
487b289
to
4529230
Compare
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.
I think this is a lot clearer than the previous PR, now that the state is tracked with PSQ and without the complicated state-update mechanism. Also thank you for adding elaborate comments in the code. I personally would have chosen different names for some of the values, but the comments make things clearer. It's worth considering Colin's comments, but otherwise I'm happy with this now.
| Some ((host, (_, h_size)), rest_hosts) -> ( | ||
match vm_size <= h_size with | ||
| true -> | ||
Some (host, hosts_psq) |
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.
The only time hosts_psq
is not equal to the pool_state
if the "minimum" host from the PSQ does not have enough memory to run the VM. In this case, that host is removed from the PSQ and this smaller PSQ is eventually returned to compute_spread_evenly_plan
. The latter then continues with the next VM in the list, which will never get a chance to get planned onto the host removed above.
This relies on the fact that the list of VMs is sorted from small to large, such that any following VM will certainly not fit on hosts that have been removed from the pool_state
. So this is an optimisation for performance, not necessary for correctness. Is that right?
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.
This relies on the fact that the list of VMs is sorted from small to large...
Yes. And the checking is only on the VM size. E.g. in previous PR, it would check the can_boot_on
, then the current improvement would not work, as even if the current VM couldn't boot on a host, a bigger VM might do.
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.
Yes, this is only an optimization: if a host is smaller than the current VM, it doesn't need to be considered for the next VM.
Added some comments to make it clear.
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.
I personally would have chosen different names for some of the values, but the comments make things clearer.
@robhoes , would you please point those names and a better way for them? I think not only for this time but also will be helpful for later PRs, thanks~
in | ||
|
||
let ( let* ) o f = match o with None -> f None | p -> p in | ||
(let* _no_plan = |
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.
This is quite confusing, as the syntax looks like the plan is just always thrown away...
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.
It's failed to plan
, and has to continue with the next step.
Otherwise, the plan will be returned directly.
This is the opposite of usual pattern succeeded and then continue
.
4529230
to
f01ac08
Compare
f01ac08
to
089c0fd
Compare
Host evacuation plan with anti-affinity support will be carried out in 3 phases: 1. Try to get a "spread evenly" plan for anti-affinity VMs, done if all the rest VMs got planned using binpack, otherwise continue. 2. Try to get a "no breach" plan for anti-affinity VMs, done if all the rest VMs got planned using binpack, otherwise continue. 3. Carry out a binpack plan ignoring VM anti-affinity. Signed-off-by: Gang Ji <gang.ji@citrix.com>
Add "groups" "power_state" for VM, vm set_resident_on host Signed-off-by: Gang Ji <gang.ji@citrix.com>
Signed-off-by: Gang Ji <gang.ji@citrix.com>
089c0fd
to
c626cdb
Compare
d9c6535
into
xapi-project:feature/vm-anti-affinity
Host evacuation plan with anti-affinity support will be carried out in 3 phases:
all the rest VMs got planned using binpack, otherwise continue.
got planned using binpack, otherwise continue.