weizhouapache commented on PR #14320: URL: https://github.com/apache/cloudstack/pull/14320#issuecomment-6734623816
> Thanks for working on this. RBD-only deployments really need it. I read it closely because we'd like to run it on our RBD-only KVM cluster. I've sorted the findings by severity. > > The main concern is that, as it stands, a host that is still running can be declared Down. Ceph's exclusive lock and blocklisting protect the data, but the running VM loses its disk and is restarted elsewhere. > > ## Blocking: a live host can be declared Down > 1. **The watcher check doesn't protect VM HA.** > > * `kvmvmactivity_rbd.py` is only reached through `KVMHAProvider.isActive`, which is Host HA and needs OOBM. > * Without OOBM, RBD in `LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT` turns on the VM-HA path: `KVMInvestigator` → `CheckOnHostCommand` → `KVMHAChecker` → `hasHeartBeat`. That path uses only the heartbeat age. VM I/O is never consulted before a restart. > * Either use the activity check there too, or document that only Host HA has it. > 2. **A missing heartbeat object reads as DEAD** (`kvmheartbeat_rbd.py` `check()`). An agent that never wrote one is declared dead: > > * not upgraded yet; > * no bindings; > * a GUID mismatch; > * the first cycle. > * Suggest: missing object → exit 2 (unknown). > 3. **A neighbour with no HA pools answers "dead".** `KVMHAChecker.hasHeartBeat()` starts at `false`, so an empty pool list (any not-yet-upgraded agent) answers alive=false. > > * Before this PR, RBD-only clusters never reached it, because `storageSupportsHA` was false. > * Now a rolling upgrade (management server first) plus one agent restart marks a running host Down. > 4. **The heartbeat GUID comes from different sources.** > > * The writer uses the agent.properties `guid` + `-` + name. > * The checker uses `HostTO.getGuid()`, i.e. `AgentShell.getGuid()`, which prefers the command line and is a random UUID in dev mode. > * A missing property gives `KVMHA-hb-null-LibvirtComputingResource`, shared by every such host. > 5. **One bad RBD pool stops the HA monitor thread for every pool.** `createHeartBeatCommand` can now throw: > > * a user/key mismatch; > * an NPE in `getRbdMonitors` on a null host; > * a missing script. > * `KVMHAMonitor.run()` catches nothing, so the thread dies and NFS/SMP heartbeats stop too. The host then looks dead. > 6. **The verdict is whatever pool comes last.** In `KVMHAChecker` the loop overwrites the result per pool, so the verdict comes from the last pool in HashMap order. With mixed NFS and RBD pools, a missing RBD object can outvote a fresh NFS heartbeat. > > ## Blocking: behaviour change outside HA > 7. **RBD pools are now really destroyed on removal.** > > * `KVMStoragePoolManager.deleteStoragePool(type, uuid, details)` now sends RBD (and SMP) to `adaptor.deleteStoragePool(uuid)`. Before, it went to the `details` variant, which `LibvirtStorageAdaptor` leaves as the default no-op. > * So the libvirt RBD pool is destroyed and its secret undefined, e.g. on `removeHeartbeatForHostsFromPool` during pool maintenance, while VMs still use it. > * Keep the `details` delete and only add the HA unregister. > > ## Should fix > 8. **Bindings are only Suggests.** > > * Without them, the write fails every cycle, and 5 retries × 10 s while holding the `haStoragePools` lock delay NFS heartbeats as well. > * With the self-reboot default on, every RBD host reboots. > * Neighbour checks exit 2, so a really dead host never becomes Down. > * Make the bindings Depends/Requires, or fail clearly at registration. > 9. **The freshness thresholds are inconsistent.** > > * `kvmvmactivity_rbd.py` hard-codes 61 s for "fresh", ignoring `kvm.heartbeat.update.frequency` (the heartbeat script gets `-t`). > * The DEAD threshold equals the update interval with no slack, which is tight under Ceph slow ops. > > ## Nits > 10. **RBD special cases are spread across files:** monitor, wrapper and pool. Let the pool own the host id and the activity script. > 11. **Duplicate helper:** `RbdUtils.composeOptionForMonHosts` has no caller and duplicates `KVMPhysicalDisk.composeOptionForMonHosts`. > 12. **Licence header:** `LibvirtCheckVMActivityOnStoragePoolCommandWrapperTest` lacks the ASF header. > > ## Looks right > * The key is passed in the environment, not on the command line. > * Volume paths are passed to the activity check. > * A neighbour that can't reach Ceph reports exit 2, not DEAD. > * The large volume lists go through a file. @bhouse-nexthop I will address, thanks for the review -- 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]
