CP-48625, CP-48752: VM anti-affinity support for host evacuation and related UT

[gangj]

Host evacuation plan with anti-affinity support will be carried out in 3 steps:

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.

[minglumlu]

If the data structure could be HostMap (instead of IntMap), then the update would be as simple as:

HostMap.update h (fun c -> Option.(value ~default:0 c |> succ |> some)) hosts

And the only cost is to generate the ranks (IntMap, required by select_host_from_ranked_groups) from the HostMap by:

     let h =
       HostMap.fold (fun h' vm_cnt acc ->
         IntMap.update vm_cnt (fun hs ->
           Option.value hs ~default:[] |> List.cons h' |> some
         ) acc
       ) hosts IntMap.empty
       |> IntMap.bindings
       |> List.map snd
       |> Xapi_vm_helpers.select_host_from_ranked_groups vm host_selector
     in

[gangj]

Thanks, I would prefer the current implementation:

1. Don't need to regenerate the whole ranked_list everytime a VM is planned on a host.
2. Reuse(share) the same code: Xapi_vm_helpers.rank_hosts_by_vm_cnt_in_group with VM start, this can make sure they behave the same as code changes.

[minglumlu]

I think updating the grp_ranked_hosts is really painful.

[minglumlu]

The time complexity of update_anti_affinity_grp_ranked_hosts is:
log N + (N * log N * log N) + log N * (N * log N + log N)

But after choosing a proper data structure, the time complexity is:
update: log N
re-generate: N * log N

[gangj]

In current implementation, I think the time complexity of update_anti_affinity_grp_ranked_hosts is O(N) (the number of hosts in the pool):

The functionlocate_host is N, as the total number of hosts to find from is N.

the operation to update ranked_grp for the host's original rank:

          ranked_hosts
          |> IntMap.update vm_cnt_on_host (fun hosts ->
                 match
                   get_hosts hosts |> List.filter (fun h -> fst h <> host)
                 with
                 | [] ->
                     None
                 | hs ->
                     Some hs
             )

IntMap to locate vm_cnt_on_host : log N
Filter it out from the original list: N

then add it to the next rank:

          |> IntMap.update (vm_cnt_on_host + 1) (fun hosts ->
                 Some
                   (Binpack.insert bigger_than (host, updated_host_size)
                      (get_hosts hosts)
                   )
             )

time complexity:
IntMap to locate vm_cnt_on_host +1 : log N
Insert it into the ordered list: N

For the other VM groups(at most 4 now)

          ranked_hosts
          |> IntMap.update vm_cnt_on_host (fun hosts ->
                 Some
                   (Binpack.insert bigger_than (host, updated_host_size)
                      (List.remove_assoc host (get_hosts hosts))
                   )
             )

IntMap to locate vm_cnt_on_host : log N
Update it in the ordered list: N

---

While for re-generating from a HostMap (host -> numOfVMs), I think the time complexity might be more than N * log N, for each rank, we still need to sort the hosts in terms of each host's free memory (we at least need log N to insert a host into a sorted list, but if insert with List, I think it might be N), then the overall time complexity will be: N * log N or even N * N.

I think it is simply because for re-generating, we have to "update" for each host, while for update_anti_affinity_grp_ranked_hosts, only need to update for 1 host.

[minglumlu]

The function is even larger than before. 😟
Wouldn't the difficulty of updating on such a data structure (2-layer map) imply that the choice on the data structure is not good?

[gangj]

I think updating the 2 layers map is not difficult. :)

I updated the comments as:

(** Generate 2 layers Map: the outer layer is a Map from `VM_group to ranked_hosts, while
    each ranked_hosts is a Map from vm_count to sorted host(`host, free_memory_size) List
    in increasing order in terms of the host's free memory, where every host in the list
    has the same number of running anti-affinity VMs resident on it: vm_count, for example:
    { grp1 ->
       { 0 -> [ (master, 500), (slave2, 508) ] }
    , grp2 ->
       { 0 -> [ (slave2, 508) ]
       , 1 -> [ (master, 500) ]
       }
    }
    indicates that there are 2 VM groups. For grp1, all the 2 hosts has 0 running VM
    from grp1, where master has 500 free memory, slave2 has 508 free memory, the 2 hosts are
    in increasing order in terms of the host's free memory. For grp2, slave2 has 0 running
    VM from grp2, and master has 1 running VM from grp2. *)

Wish it will be helpful in understanding the data structure.

[minglumlu]

The compare could be based on a two-dimension key (vm_cnt, free_mem). Then the data structure will be a sorted list (or a Set) still. Creating and updating the list would be easier?
The host free memory could be in a Hashtbl to gain O(1) in comparison. - (This is wrong)

[gangj]

PR updated this way: choose host from HostsSet, the min_elt one in the Set is the most suit host for the VM.

[gangj]

@gangj, there have been so many fixup commits now so that the only way to review this PR is to look at the overall diff, which is very large and includes refactoring and code re-organisation. This makes it very hard to verify that what has changed relative to the original code is correct (especially in xapi_ha_vm_failover.ml). Could you please squash the fixups and create a set of commits that can be reviewed one by one?

@robhoes Sure, the commits are squashed.

[minglumlu]

Useless module?

[minglumlu]

Could get rid of the nested match:

match (Int.compare x1 x2, Int.compare y1 y2, Ref.compare z1 z2) with
| n, _, _ when n <> 0 -> n
| _, n, _ when n <> 0 -> n
| _, _, n -> n

[minglumlu]

Useless function?

[minglumlu]

This error is for almost impossible case. Either using monad or catching exceptions outside to get the code compact.

[minglumlu]

Maybe we can use a module to wrap all of these VM anti-affinity functions and types.
So that in the module, we don't need to add anti_affinity to the names of functions and variables.

[minglumlu]

If the impossible Not_found being caught outside, how about the following?

let update_grp_host_vm_cnt group host grp_host_vm_cnt_map =
  let M = Xapi_vm_helpers.HostMap in
  let hs =
    VMGroupMap.find group grp_host_vm_cnt_map in
    |> M.update host (fun c -> value ~default:0 c |> succ |> some)
  in
  VMGroupMap.update group (fun _ -> Some hs) grp_host_vm_cnt_map

[minglumlu]

Again, with catching the impossible Not_found, this could as simple as:

let vm_cnt =
  VMGroupMap.find group grp_host_vm_cnt_map
  |> Xapi_vm_helpers.HostMap.find host
in

[gangj]

There is no entry for host in the map when there is no VM from the same group running on it.

[minglumlu]

There are some unnecessary parameters for this function. Say,
Initially, we have

A: HostMap: host -> free_mem
B: GroupMap: group -> (HostMap : host -> vm_cnt)

The plan_for_spead_evenly requires:

A: HostMap: host -> free_mem
B: GroupMap: group -> (HostMap : host -> vm_cnt)
B -> C: GroupMap: group -> (vm_cnt, free_mem, h)

The plan_for_no_breach requires:

A: HostMap: host -> free_mem
B -> D: GroupMap: group -> (hosts, inhabited_hosts)

And the B -> D is as simple as:

  GroupMap.map (fun hosts ->
    let m1, m2 = HostMap.partition (fun _ vm_cnt -> vm_cnt = 0) hosts in
    (List.map fst HostMap.bindings, HostMap.cardinal m2)
  ) 

[minglumlu]

I think this performs the following mapping:

B: GroupMap: group -> (HostMap : host -> vm_cnt)
->
C: GroupMap: group -> (vm_cnt, free_mem, h)

So it could be just as the B is already a map, a simple map will do the trick.

GroupMap.map (fun hosts ->
  HostMap.to_seq
  |> Seq.map (fun (h, vm_cnt) -> (vm_cnt, (HostMap.find h free_mems), h))
  |> HostSet.of_seq
) GroupMap.empty

[gangj]

There is no entry for host in the map B when vm_cnt of the host is 0 .