weizhouapache commented on PR #14320: URL: https://github.com/apache/cloudstack/pull/14320#issuecomment-6853138371
Thanks for the review. I pushed three commits that address it: [392ce05](https://github.com/apache/cloudstack/pull/14320/commits/392ce05275575c70c9e9b61fb85944b9ac76d0c1), [4398ac1](https://github.com/apache/cloudstack/pull/14320/commits/4398ac1b970754b707558ed1132c5abe9e9bf1a6) and [183c5a9](https://github.com/apache/cloudstack/pull/14320/commits/183c5a94c9673233ea2568c224b30a0b949aa5cb). One comment at a time: **`kvmheartbeat_rbd.py`: `%d` with `time.time()`.** This does not raise in Python 3. `'%d' % 1794569667.3` gives `'1794569667'`, and only `'{:d}'.format(float)` raises. The heartbeat has been written this way on real hosts through all the tests. I made the conversion explicit with `int(time.time())` anyway ([183c5a9](https://github.com/apache/cloudstack/pull/14320/commits/183c5a94c9673233ea2568c224b30a0b949aa5cb)). **`KVMStoragePoolManager`: unregistering before the delete (two comments).** The order is deliberate and is the one on `main`. The monitor holds its lock for a whole heartbeat cycle, so once the pool is unregistered no heartbeat is running and none is written while the pool is unmounted or removed. A write that fails at that point could fence the host. The concern about a failed delete is valid, though: the pool stays but is no longer monitored. If the delete does not succeed or throws, the pool is now registered with the monitor again ([392ce05](https://github.com/apache/cloudstack/pull/14320/commits/392ce05275575c70c9e9b61fb85944b9ac76d0c1)). The unit tests cover a failed delete, an exception, a successful delete and a pool that was not monitored. **`kvmha_rbd.py`: the connection error is swallowed.** I kept the connection from raising in the constructor on purpose. The scripts need a failed connection to be a failed operation with the exit status they already have: a read says it is not known whether the host is alive (2), and a failed heartbeat write is a failure (1). What was missing was the reason. It is now kept and included in the `CephError` and `WatchersError`, and the scripts print it. For example: `Unable to read the RADOS object [KVMHA-hb-...] in pool [pool]: unable to connect to the cluster or to open the pool [pool]: <error of librados>` ([183c5a9](https://github.com/apache/cloudstack/pull/14320/commits/183c5a94c9673233ea2568c224b30a0b949aa5cb)). **`kvmvmactivity_rbd.py`: exit status for invalid arguments.** Fixed. The argument parser now exits with 2, as the header says and as the other argument checks do ([183c5a9](https://github.com/apache/cloudstack/pull/14320/commits/183c5a94c9673233ea2568c224b30a0b949aa5cb)). **`LibvirtStoragePool`: `CEPH_KEY` without a user.** Fixed. The key is passed to the scripts only together with its user, as `addRbdConnectionArgs` requires, and there is a test for it ([4398ac1](https://github.com/apache/cloudstack/pull/14320/commits/4398ac1b970754b707558ed1132c5abe9e9bf1a6)). **Older thread on `kvmvmactivity_rbd.sh` ("DEAD reason is misleading").** That `.sh` script no longer exists in this PR. The Python script gives the reason of the branch that is taken: in the `else` case the message says `suspectTimeDiff -lt 0` was not satisfied and `latestUpdateTime -gt lastUpdateTime` was not either. Nothing to change, so the thread can be resolved. I also ran the same changes (on the 4.23 branch) in a nested environment with KVM hosts and Ceph RBD primary storage. The 9 tests in `test_hostha_kvm.py` pass. When I force-stopped a KVM host, it was marked `Down` after about 3.5 minutes and its two VMs restarted on the other host. -- 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]
