wido commented on PR #14320: URL: https://github.com/apache/cloudstack/pull/14320#issuecomment-6216027422
> > 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. > > thanks @wido ! > > > 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. > > yeah, it makes sense, I will update. We determine the host status from its neighbours, GUID will be used instead of host as well. > > > Note that `RbdUtils.java` and `RbdUtilsTest.java` are in both this PR and #14402, so whichever merges second needs a rebase. > > right, I will keep them synced in all PRs (#14402, #14320, #14386). I will create a PR for online/offline volume migration between RBD and RBD/NFS/Local/SMP soon, which also contains the RbdUtils.java, it will be synced too. Great! I think that if the last changes (IPv4 mainly) are made, we should be good to go with this one. -- 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]
