weizhouapache commented on PR #14320: URL: https://github.com/apache/cloudstack/pull/14320#issuecomment-6740225525
Thanks for the thorough review. I've pushed [fe48966d52b](https://github.com/apache/cloudstack/pull/14320/commits/fe48966d52b893b7794b1903b56be2ee0c145107) addressing it. Per finding: 1. **VM HA does not consult the activity check.** Confirmed. The VM HA path (`KVMInvestigator` → `CheckOnHostCommand` → `KVMHAChecker`) decides on the heartbeat age only, as it does for NFS today. Only Host HA (OOBM) runs the activity check. I documented this in `KVMHAChecker` rather than changing VM HA, because that would also change the NFS behaviour. Happy to follow up separately if you want it. 2. **Missing heartbeat object read as DEAD.** `kvmheartbeat_rbd.py` now exits 2 ("unable to tell") when the object is missing or malformed. DEAD is only reported when the object exists and is stale. 3. **Neighbour with no HA pools answered "dead".** `KVMHAChecker` now answers "can't tell" when it has no pools to check, so a neighbour that doesn't monitor any pool can no longer mark a running host Down. `testCheckOnHostCommand` asserted the old behaviour, so I changed that expectation. 4. **Source of the heartbeat GUID.** Not changed in this push. The heartbeat id still uses the GUID from `agent.properties`, which matches `host.guid` for normal installs. A GUID that comes only from the command line, or a random one in developer mode, is not covered. 5. **A bad RBD pool stops the HA monitor.** A pool that can't run its heartbeat now fails for that pool only, is not retried, and the monitor thread no longer dies on unexpected errors. 6. **Verdict taken from the last pool.** The verdict now covers all pools. With "report if one pool fails" set, the host is dead if any pool is dead. Otherwise it is dead only if all pools are dead. 7. **RBD pools removed with the wrong delete.** RBD and SharedMountPoint pools keep the delete that takes the details, as before, so a libvirt pool that VMs still use isn't destroyed. HA registration is still removed. 8. **Python bindings are only `Suggests`.** I kept `Suggests` because the packages aren't available on some supported distributions, for example EL8. To fail clearly, the agent now runs `kvmheartbeat_rbd.py --check-bindings` when it registers the pool. If the bindings are missing, it logs an error naming the missing package and does not monitor that pool. A host without the bindings therefore doesn't take part in RBD HA. 9. **Different freshness thresholds.** Both scripts now use the same rule: heartbeat interval plus 30 s of slack. `kvmvmactivity_rbd.py` takes the interval with a new `-i` option instead of the fixed 61 s. 10. **RBD special cases in several places.** The storage pool now decides the heartbeat id and the activity script. `KVMHAMonitor` and the check wrapper no longer test for RBD. 11. **Duplicate helper.** I kept `RbdUtils.composeOptionForMonHosts` where it is and made no change to it. 12. **Licence header.** Added to the wrapper test. New tests cover `KVMHAChecker`, `KVMHAMonitor` and `KVMStoragePoolManager`, plus additions to `LibvirtStoragePoolTest` and the wrapper test. I also ran this on a nested environment with KVM hosts and Ceph RBD primary storage. The 9 tests in `test_hostha_kvm.py` pass. In a manual test I force-stopped a KVM host: it was marked `Down` after about 3 minutes and its two VMs restarted on the other host. A heartbeat read for a missing object returned "unable to tell" (exit 2) and a stale one returned DEAD. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
