weizhouapache commented on PR #14320: URL: https://github.com/apache/cloudstack/pull/14320#issuecomment-6854442529
> Thanks @weizhouapache, both follow-ups look good to me. > > * **7. RBD and SharedMountPoint pools destroyed on removal: resolved** in [9e50a47](https://github.com/apache/cloudstack/commit/9e50a474668c4ea3946e14cdfbf65954eb54c573). Only the HA unregister depends on the HA type list now. NFS keeps the uuid-only delete, which unmounts it, and every other type, RBD and SharedMountPoint included, goes back to the delete that takes the details, as on main. `KVMStoragePoolManagerTest` now splits NFS from RBD/SharedMountPoint and verifies `unregisterStoragePool`, which matches the code. > * **Heartbeat age slack: resolved** in [b5a699d](https://github.com/apache/cloudstack/commit/b5a699dc81124b5f0c0a3c431b3dfb05716837b1). The readers get `heartbeat.update.timeout` through `-w`, which is the same timeout the agent uses to kill a write in `createHeartBeatCommand`. The fallback is `2 * TIMEOUT`, which covers connect plus write. The new `LibvirtStoragePoolTest` cases check that `-w` goes only to the RBD script. > > On the new Copilot comments: > > * `"%d" % time.time()` is fine in Python 3, as you showed. > * The `Parser.error()` exit status of 1 in `kvmvmactivity_rbd.py` doesn't match the header's "2 if the arguments are not valid". It makes no difference in practice, since `hasVmActivity` treats any non-zero exit as "not dead". It's a one-line fix if you want the header and the code to agree. > * Unregistering before the delete has the same ordering as main had for NFS, so I don't consider it a regression for this PR. > > One small note, not blocking. `runHeartBeat()` writes the pools one after another before it sleeps. So on a host with several RBD pools on the same slow cluster, the gap between two heartbeats of one pool can be the interval plus one slow write per pool, not just one. With the any/all rule and the default of a single pool per cluster this is an edge case. A comment next to `heartbeat_max_age` would be enough. > > Please also refresh the PR description, which still describes `kvmheartbeat_rbd.sh` / `kvmvmactivity_rbd.sh`, `rbd status` and `--id/--key` on the command line. It should cover the Python/librados scripts, the key in `CEPH_KEY`, the GUID-named objects, `Suggests` + `--check-bindings`, and the fact that VM HA relies on the heartbeat only for RBD-only clusters without OOBM. > > LGTM. thanks @bhouse-nexthop for review updated as suggested -- 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]
