GeezerVison

🟡 Changes recommended

Critical RBD monitor-port and stale-heartbeat handling issues remain, along with incomplete HA cleanup.

🧠 Review effort: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot review overview

🧠 Review effort: Lite

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot review overview

🧠 Review effort: Lite

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

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.

Thanks for the quick and thorough turnaround, and for the point-by-point reply. I went through fe48966 against each item and ran the unit tests for the changed code. Nearly everything is resolved. One fix from the reply isn't in the code, though, and its new tests fail.

Resolved

• 2. Missing heartbeat object. kvmheartbeat_rbd.py now exits 2 for a missing or malformed object. It reports DEAD only when the object exists and is stale.
• 3. Neighbour with no HA pools. KVMHAChecker returns "alive / can't tell" for an empty pool list, and the old test expectation is updated.
• 4. GUID source. I accept the reasoning. Without a GUID the agent refuses to start outside developer mode, and the installer writes the same guid that host.guid is built from. With the fix for 2, a mismatched GUID now reads as "unable to tell", not DEAD, so the dangerous half of this item is gone.
• 5. A bad pool stops the monitor. Per-pool errors are caught in executePoolHeartBeatCommand, run() survives unexpected errors, and KVMHAMonitorTest covers both.
• 6. Verdict from the last pool. The any/all rule is correct for both settings of the flag, and KVMHACheckerTest covers it.
• 9. Thresholds. Both scripts now share heartbeat_max_age(), and the activity script gets the real interval through -i. I left one small suggestion inline.
• 10. RBD special cases. getHeartBeatHostId, getVmActivityCheckPath and getHeartBeatPrerequisiteError now live on the pool. Neither the monitor nor the wrapper tests for RBD any more.
• 12. Licence header. Added.

Reasonably declined or documented

• 1. VM HA uses only the heartbeat. Documented in KVMHAChecker, and it matches NFS. I'm fine with handling it in a follow-up. Please also say it in the PR description or the cwiki page, since RBD-only users without OOBM will rely on that path.
• 8. Bindings stay Suggests. The EL8 argument holds. The --check-bindings gate at registration fails clearly, and I checked that it exits 1 with a clear message when the bindings are missing. One consequence is worth documenting. A host without the bindings (or with a python3-rbd too old for watchers_list) monitors no RBD pool, so as a neighbour it answers "alive" for any host it checks. KVMHostActivityChecker.checkHostStatusWithNeighbourHosts returns on the first "alive" answer, so on an RBD-only cluster that one host stops VM HA from marking any host Down. This fails safe, so it isn't blocking, but the only signal is one line in that host's agent log. I left an optional follow-up idea inline.
• 11. composeOptionForMonHosts. Fine, since #14402 adds the callers.

Remaining

• 7. RBD pools are still destroyed on removal (blocking). The reply says RBD and SharedMountPoint keep the delete that takes the details, but KVMStoragePoolManager.deleteStoragePool(type, uuid, details) still sends every HA-capable type, now including RBD and SharedMountPoint, to adaptor.deleteStoragePool(uuid). fe48966 doesn't touch that method. The new KVMStoragePoolManagerTest expects the intended behaviour, and 2 of its 5 tests fail against the current code (details inline). It looks like a hunk got lost in the push.

Minor

• The PR description still describes kvmheartbeat_rbd.sh / kvmvmactivity_rbd.sh, rbd status and passing --id/--key on the command line. It would help reviewers to refresh it for the Python/librados scripts, the key in CEPH_KEY, the GUID-named objects, and the Suggests + --check-bindings behaviour.

Unit tests I ran on fe48966: KVMHACheckerTest (7), KVMHAMonitorTest (4), LibvirtCheckVMActivityOnStoragePoolCommandWrapperTest (6), LibvirtStoragePoolTest (29), LibvirtComputingResourceTest (323) and RbdUtilsTest (17) all pass. KVMStoragePoolManagerTest has 2 failures out of 5: testPoolsAreDeletedWithDetailsAsBefore and testPoolsWithHaSupportAreRemovedFromTheMonitorWhenDeleted.

Once 7 and its tests are fixed, I'm happy to approve.