wido commented on PR #14320: URL: https://github.com/apache/cloudstack/pull/14320#issuecomment-6213530251
Thanks @weizhouapache, I went through the update manually and then used Claude Fable to check it again. The four points from my review are all addressed. RbdUtilsTest (17), LibvirtStoragePoolTest (15) and the activity wrapper test (5) pass here, and the scripts behave as expected against stubbed rados/rbd modules: a deleted volume is skipped, a watcher on another volume gives ALIVE, no watchers gives DEAD, and an unreachable cluster gives exit 2 for both the heartbeat check and the activity check. One request before this goes in. The RADOS objects are named after the private IPv4 address of the host (`KVMHA-hb-<ip>`, `KVMHA-ac-<ip>`). That copies the NFS convention, but it is IPv4 thinking: `NetUtils.getNetworkParams` only ever returns a 4-byte address, and a host that gets a new private IP loses its heartbeat identity. Please use the host GUID instead. It is already available on both sides: the `HostTO` the checker receives carries `getGuid()`, and the agent has its own GUID in agent.properties. The scripts treat `-h` as an opaque string, so only `KVMHAMonitor` and the three RBD spots in `LibvirtStoragePool` change. The NFS path should stay as it is, since renaming `hb-<ip>` files would break a rolling upgrade, but the RBD naming is new and this is the moment to get it right. A GUID also makes cleaning up the objects of a removed host possible later, since the management server knows the GUID of a host it removed. Note that `RbdUtils.java` and `RbdUtilsTest.java` are in both this PR and #14402, so whichever merges second needs a rebase. -- 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]
