CP-47656 VM anti-affinity generate breach alert

[LunfanZhang]

This PR introduces the generation of breach alerts during operations on VMs or groups, triggered when all VMs within a group are breached (i.e., placed on the same host).

Test scenarios covered in this PR include:

• Starting and resuming a VM triggers a breach alert if it is breached.
• Stopping and suspending a VM dismisses the breach alert if it is no longer breached.
• Migrating a VM, whether causing or resolving a breach, generates or dismisses the alert respectively.
• Moving a VM from one group to another triggers a breach alert if either group is breached.
• Destroying a VM group dismisses all related breach alerts.
• Changing the license from Premium to Standard dismisses all breach alerts, while changing from Standard to Premium generates them.

Additionally, the pool_feature has been split into two parts, pool_feature and pool_feature_helpers, to address dependency cycles.
Error: dependency cycle between modules in _build/default/ocaml/xapi: Xha_statefile -> Sm -> Sm_exec -> Xapi_session -> Pool_features -> Xapi_vm_group_helpers -> Xapi_alert -> Xapi_http -> Xapi_session

[gangj]

The first commit is to resolve dependency cycling issue if we call Xapi_alerts inside pool_features:

Error: dependency cycle between modules in _build/default/ocaml/xapi:

   Xha_statefile
-> Sm
-> Sm_exec
-> Xapi_session
-> Pool_features
-> Xapi_vm_group_helpers
-> Xapi_alert
-> Xapi_http
-> Xapi_session

@LunfanZhang , would you please include such commit message in the first commit? Thanks~

And the title in the commit message is too long now.

[gangj]

@LunfanZhang For this code change, I think we should also make sure no alert is generated/dismissed when no change in the state of VM anti-affinity, like below scenarios should also be tested:

1. toolstack restart
2. vm state change not including the running state, like: vm paused -> shutdown

[LunfanZhang]

@LunfanZhang For this code change, I think we should also make sure no alert is generated/dismissed when no change in the state of VM anti-affinity, like below scenarios should also be tested:

1. toolstack restart
2. vm state change not including the running state, like: vm paused -> shutdown

toolstack restart and VM paused -> shutdown has been verified. there will no alerts generate

[gangj]

• Stopping and suspending a VM dismisses the breach alert if it is no longer breached.

Stopping or suspending a VM might also trigger an alert.

[gangj]

• Starting and resuming a VM triggers a breach alert if it is breached.

Starting or resuming a VM might also dismiss an alert.

[gangj]

• Moving a VM from one group to another triggers a breach alert if either group is breached.

Moving a VM from one anti-affinity group to another might also dismiss an alert.

[lindig]

What are these assoc lists representing and what strings are expected here?

[LunfanZhang]

The assoc list here is represent the (Feature, restriction of this feature) which get from Db.Pool.get_restrictions, we want to get the restriction change of the anti-affinity feature, especially from restriction true -> false or false -> true, the breach alert should generate or remove.

[lindig]

This needs to be documented in the MLI file; might want to use an example to make the syntax for strings obvious.

[LunfanZhang]

I have updated the MLI file with clearer parameter explanations

[lindig]

Did you push the change?

[LunfanZhang]

Yeah, I pushed the change

[LunfanZhang]

I just updated it again to include a simple example.

[lindig]

What is the purpose of this code? Please add a comment. It also looks not efficient.

[LunfanZhang]

This code snippet checks the breach state of a group and returns the host that is in breach. When there are no VMs or only one VM in the group, it is not a breach. and, when there are two or more VMs and all of them are on the same host, it is a breach, and the specific host is returned.
In this context, h represents the host of the first VM, and remaining represents the other hosts. We compare h with the remaining hosts to determine whether the group is breached or not.

[lindig]

Would it be easier to express this using sets?

[LunfanZhang]

I think both approaches are fine, and the current method is more easily extendable if the breach rule were to change in the future.

[LunfanZhang]

Hi @lindig, I have resolved all the comments above, if there are no more comments, could help me to approve this PR? :-)

[lindig]

I asked to add a code comment or some other documentation that explains this code but I don't see it.

[LunfanZhang]

Ah, sorry, I misunderstood the comment you mentioned. I assumed you were asking to add a comment in this conversion. I have added the comments for this function now. I think it's fine.

[gangj]

@LunfanZhang Would you please update the commit message of the first commit?

1. Shorten the current title
2. Add some detailed description about the resolved dependency cycling.

[LunfanZhang]

@LunfanZhang Would you please update the commit message of the first commit?

1. Shorten the current title
2. Add some detailed description about the resolved dependency cycling.

Updated

[lindig]

Is the value in the key/value always true/false? If so, why is it a string and not a bool?

[minglumlu]

Yes. It's always either "true" string or "false" string.
They are read from the XAPI DB. Converting them to true or false boolean would be more efficient?

[lindig]

The string-based API invites errors if the only possible values are true/false but strings are accepted instead of just boolean values. It does not matter where the client obtains the values.

[gangj]

@LunfanZhang Would you please update the commit message of the first commit?

1. Shorten the current title
2. Add some detailed description about the resolved dependency cycling.

Updated

Would you please add more description to the commit message to make it better:
add something like "resolve dependency cycling issue if we call Xapi_alerts inside pool_features:"

[gangj]

@LunfanZhang
I have another thought here: would you please also update the commit message of the 2nd commit, to add some description of how the alert will be triggered in what scenarios. This will make the commit history easy to understand without having to read the code, thank you!