Repository navigation
kvm: support Host HA on Ceph RBD primary storage - #14320
weizhouapache wants to merge 28 commits into
Conversation
Extend the existing KVM Host-HA heartbeat/VM-activity-check framework (currently limited to NetworkFilesystem and SharedMountPoint pools) to also cover RBD primary storage, based on the approach from the old PR apache#5862, adapted to the current HA architecture and reusing the multi-monitor Ceph support already added in apache#6792. - Add StoragePoolType.RBD to LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT, which is the single switch that makes pool registration, KVMHAMonitor, and the CheckOnHostCommand/CheckVMActivityOnStoragePoolCommand wrappers treat RBD pools as HA-capable. - LibvirtStoragePool: build rbd/rados connection args (--mon-host, pool, and cephx --id/--key when set) from the pool's existing sourceHost/ sourceDir/authUsername/authSecret fields for the heartbeat and VM-activity checks, mirroring the conventions KVMPhysicalDisk already uses to talk to RBD. - Add kvmheartbeat_rbd.sh and kvmvmactivity_rbd.sh: RBD has no shared mount point to write a heartbeat file to, so the heartbeat timestamp is stored as a small RADOS object per host instead, and VM activity is detected via RBD watchers (rbd status) rather than file mtimes. - Minor: fix a stale "NFS storage pool" log message in KVMHAMonitor now that this path also runs for RBD. - Add LibvirtStoragePoolTest#testIsPoolSupportHA covering the new RBD case (no HA/heartbeat tests existed previously for any pool type).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14320 +/- ##
============================================
+ Coverage 20.75% 20.82% +0.06%
+ Complexity 21901 20924 -977
============================================
Files 6496 6054 -442
Lines 591602 548368 -43234
Branches 72583 73259 +676
============================================
- Hits 122814 114194 -8620
+ Misses 455456 420816 -34640
- Partials 13332 13358 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19426 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-17079)
|
There was a problem hiding this comment.
🟡 Changes recommended
Critical RBD monitor-port and stale-heartbeat handling issues remain, along with incomplete HA cleanup.
3 open findings
What changed in this PR
Extends KVM Host HA heartbeat and VM-activity checks to Ceph RBD primary storage.
Changes:
- Adds RBD HA support and connection handling.
- Adds RADOS heartbeat and RBD watcher activity scripts.
- Updates logging and adds HA eligibility tests.
| File | Summary |
|---|---|
scripts/vm/hypervisor/kvm/kvmvmactivity_rbd.sh |
Adds RBD watcher-based VM activity detection. |
scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.sh |
Adds RADOS-backed heartbeat handling; stale delays over 255 seconds require bounded status handling. |
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java |
Tests RBD HA support eligibility. |
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java |
Selects RBD scripts and builds Ceph arguments; monitor ports must be propagated, and missing-script errors should be generic. |
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java |
Generalizes the storage-pool log message. |
engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java |
Enables RBD HA support; corresponding pool deletion paths must also clean up HA state. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- pass the pool's monitor port to the RBD heartbeat/activity scripts, for every monitor, instead of relying on the default port - kvmheartbeat_rbd.sh: keep the heartbeat age out of the function return status, which wraps above 255 and made a long-stale heartbeat look ALIVE - remove pools from the HA monitor on deletion for all storage pool types with HA support, without trying to umount an empty mount path - make the missing-script error message generic - add unit tests for the RBD monitor list (IPv4, IPv6, mixed, with/without port)
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
11 open findings
kvmheartbeat_rbd.shuses-hto derive the per-host RADOS object name… · New-hwas provided; ifHostIPis empty, the heartbeat object… · Newpool.getMountDestPath()directly into a shell command string can break if the path… · NewsourcePort <= 0, this returnssourceHostwithout normalization. IfsourceHostcontains… · NewauthUsername != nullis not sufficient to safely add cephx args:authUsernamecould be… · New-thelp text says 'time on ms', but the caller (LibvirtStoragePool) passes seconds… · New-thelp text says 'time on ms', but the caller (LibvirtStoragePool) passes seconds… · New-thelp text says 'time on ms', but the caller (LibvirtStoragePool) passes seconds… · New
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19467 |
…of the pool Follow-up to the review of the Host HA on Ceph RBD support: - kvmheartbeat_rbd.sh and kvmvmactivity_rbd.sh: exit with an error when the host IP, which names the RADOS objects, is missing (the self-fencing -c of the heartbeat script does not need it), or when a Ceph user is given without its key - LibvirtStoragePool: trim the Ceph monitors and skip empty entries, with or without a port; pass the Ceph user and key only if both are set and fail with a clear error if only one of them is - KVMHAMonitor: run umount without a shell when removing a pool - kvmvmactivity_rbd.sh: fix the -t help text and drop the unused field from the stored activity state - add unit tests for the monitor list (multiple IPv4 and IPv6 monitors, with and without a port, with spaces) and for the Ceph user and key handling
|
@blueorangutan package |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
6 open findings
:as an IPv6 literal and will wrap it in brackets, which… · NewgetRbdMonitors()currently branches on the presence of:(which can also appear in… · New--keyexposes it in the process list (both for this script’s args and… · Newintervalis used in a numeric comparison but is never validated when running with-r. If-r… · New${UUIDList//,/ }expansion relies on shell word-splitting and can mis-handle… · Newrados ... statoutput content to detect existence is brittle (output formats can vary, and… · New
11 resolved since last review
-hwas provided; ifHostIPis empty, the heartbeat object…kvmheartbeat_rbd.shuses-hto derive the per-host RADOS object name…-thelp text says 'time on ms', but the caller (LibvirtStoragePool) passes seconds…-thelp text says 'time on ms', but the caller (LibvirtStoragePool) passes seconds…-thelp text says 'time on ms', but the caller (LibvirtStoragePool) passes seconds…authUsername != nullis not sufficient to safely add cephx args:authUsernamecould be…sourcePort <= 0, this returnssourceHostwithout normalization. IfsourceHostcontains…pool.getMountDestPath()directly into a shell command string can break if the path…
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
…st HA checks, and harden the parsing Follow-up to the review of the Host HA on Ceph RBD support: - LibvirtStoragePool: do not add a port to Ceph monitors which have one already, and only wrap bare IPv6 addresses in square brackets; mask the Ceph key in the logged command - kvmheartbeat_rbd.sh, kvmvmactivity_rbd.sh: give the Ceph key to rados and rbd in a temporary keyfile instead of --key - kvmheartbeat_rbd.sh: require -t to be a number when checking a heartbeat - kvmvmactivity_rbd.sh: read the list of volumes without word splitting or globbing, skipping empty entries, and check if the activity object exists by the exit status of rados stat - add unit tests for monitors with a port and for the masked key
|
@RosiKyu a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-211)
|
|
[SF] Trillian test result (tid-17555)
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
7 open issues
time.time()returns a float, but%dformatting requires an integer in Python 3, which will… · NewCEPH_KEYto the script environment wheneverauthSecretis set, even if… · Newelseof `if [[…
2 resolved since last review
Review effort: Lite
A pool is removed from the HA monitor before it is deleted, so that no heartbeat is written while the pool is removed: the monitor holds its lock while it writes the heartbeats, so none is on going when the pool is removed from it, and a write which fails while the pool is unmounted could fence the host. If the delete does not succeed or throws, the pool is still there, but it was no longer monitored, so the host stopped writing its heartbeat on a pool which its VMs may still use. It is added to the monitor again.
addRbdConnectionArgs requires the Ceph user and its key to be either both set or both unset. The key in the environment of the scripts is now given under the same condition, and not whenever it is set.
If the cluster could not be connected, or the pool could not be opened, the error was dropped, and the operations failed with the name of an object, so that the output of the scripts did not say what was wrong. The reason of librados is now kept, and the scripts print it when a heartbeat can not be read or written, or when the activity of the VMs can not be checked. The exit status does not change: a failed read says that it is not known whether the host is alive (2), a failed write of the heartbeat is a failure (1). kvmvmactivity_rbd.py exits with 2 for an argument which is not valid, as it does for the other arguments and as its header says. The heartbeat is written as an integer timestamp explicitly.
|
Thanks for the review. I pushed three commits that address it: 392ce05, 4398ac1 and 183c5a9. One comment at a time:
Older thread on 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 |
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
6 open issues
deleteStatusisfalse, the pool is re-added to HA monitoring but is still unconditionally… · Newfd00::1:3300) will be… · NewgetHeartBeatHostId(...), `createHeartBeatCommand(..., String… · Newelseof `if [[…
6 resolved since last review
time.time()returns a float, but%dformatting requires an integer in Python 3, which will…CEPH_KEYto the script environment wheneverauthSecretis set, even if…
Review effort: Lite
| synchronized (_storagePools) { | ||
| _storagePools.remove(uuid); |
| volumeListFile = File.createTempFile("kvmha-volumes-", ".txt"); | ||
| Files.write(volumeListFile.toPath(), volumes.getBytes(StandardCharsets.UTF_8)); | ||
| cmd.add("-U", volumeListFile.getAbsolutePath()); | ||
| return volumeListFile; | ||
| } catch (IOException e) { | ||
| logger.warn("Unable to write the list of volumes to a file, giving it to the script as an argument: {}", e.getMessage()); | ||
| if (volumeListFile != null && !volumeListFile.delete()) { | ||
| logger.debug("Unable to delete the file {}", volumeListFile); | ||
| } |
bhouse-nexthop
left a comment
bhouse-nexthop
left a comment
There was a problem hiding this comment.
Thanks @weizhouapache, both follow-ups look good to me.
- 7. RBD and SharedMountPoint pools destroyed on removal: resolved in 9e50a47. 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.
KVMStoragePoolManagerTestnow splits NFS from RBD/SharedMountPoint and verifiesunregisterStoragePool, which matches the code. - Heartbeat age slack: resolved in b5a699d. The readers get
heartbeat.update.timeoutthrough-w, which is the same timeout the agent uses to kill a write increateHeartBeatCommand. The fallback is2 * TIMEOUT, which covers connect plus write. The newLibvirtStoragePoolTestcases check that-wgoes 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 inkvmvmactivity_rbd.pydoesn't match the header's "2 if the arguments are not valid". It makes no difference in practice, sincehasVmActivitytreats 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.
A pool of a type with HA support (a libvirt storage pool) which is not deleted is monitored for HA again, as it is still there, but it was still removed from the information which the manager keeps to create the pool again. It now stays known as well. The other types keep being forgotten when their delete does not succeed: their adaptors return false if they have no such pool. The tests of the delete are reduced to the cases which differ.
The agent writes the heartbeats of its pools one after another, so with several RBD pools on the same slow cluster the gap between two heartbeats of a pool can be longer than the margin, which covers one slow write. It is an edge case, which is now said next to the margin.
thanks @bhouse-nexthop for review updated as suggested |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
6 open issues
deleteStatusisfalse, the pool is re-added to HA monitoring but is still unconditionally…addRbdConnectionArgsvalidates monitors but does not validatesourceDir(the RBD pool name). If… · NewgetRbdMonitorswill throw aNullPointerExceptionwhensourceHostisnulldue to… · NewRbdUtils.getRbdMonitorsis a new public helper, consider adding test coverage fornull… · Newelseof `if [[…
3 resolved since last review
Review effort: Lite
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 20049 |
|
[SF] Trillian test result (tid-219)
|
Extend the KVM HA heartbeat and VM-activity check, until now limited to NetworkFilesystem and SharedMountPoint pools, to Ceph RBD primary storage. Each host writes its heartbeat to a RADOS object in the pool named after its GUID, through Python scripts which use the librados and librbd bindings (python3-rados and python3-rbd, now suggested by the agent packages). A pool whose heartbeat can not be used is not monitored for HA, and a pool which is not deleted is monitored again. Backport of apache#14320 (head f0e5e68) Conflict resolutions: - KVMStoragePoolManagerTest: both sides add the class. Merge the tests of upstream into our existing class (getPhysicalDisk retries and pool lock tests), sharing one setUp. Signed-off-by: Brad House <[email protected]>
Extend the KVM HA heartbeat and VM-activity check, until now limited to NetworkFilesystem and SharedMountPoint pools, to Ceph RBD primary storage. Each host writes its heartbeat to a RADOS object in the pool named after its GUID, through Python scripts which use the librados and librbd bindings (python3-rados and python3-rbd, now suggested by the agent packages). A pool whose heartbeat can not be used is not monitored for HA, and a pool which is not deleted is monitored again. Backport of apache#14320 (head f0e5e68) Conflict resolutions on 4.22: - KVMHAMonitor: add unregisterStoragePool(), which main got from apache#13897 and the pool deletion of this change needs (it drops the pool from the monitor without the umount of removeStoragePool()). - KVMStoragePoolManager: take the deletion of upstream as is, so the delete with details uses the delete without details for NetworkFilesystem, as on main, and the details one (which does not destroy the libvirt pool or secret) for RBD and the other types. Keep our unsynchronized deleteStoragePool(type, uuid) of the per-pool locks. - KVMStoragePoolManagerTest: merge the tests of upstream into our existing class (getPhysicalDisk retries and pool lock tests), sharing one setUp. - LibvirtStoragePoolTest: drop the resolvePhysicalDiskFile tests which came along from main and test code 4.22 does not have. - LibvirtComputingResourceTest: take the new testCheckOnHostCommand assertions and import CheckOnHostAnswer. - el8 spec: add only the Suggests, not the Requires of main. Signed-off-by: Brad House <[email protected]>
Description
Extend the existing KVM HA heartbeat and VM-activity-check framework (until now limited to NetworkFilesystem and SharedMountPoint pools) to Ceph RBD primary storage. It is based on the approach of the old PR #5862, adapted to the current HA architecture, and it reuses the multi-monitor Ceph support added in #6792.
How it works
StoragePoolType.RBDis added toLIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT, the single switch which makes pool registration,KVMHAMonitorand theCheckOnHostCommand/CheckVMActivityOnStoragePoolCommandwrappers treat RBD pools as HA-capable.KVMHA-hb-<host GUID>, and the state of the VM-activity check of Host HA toKVMHA-ac-<host GUID>. The objects are named after the GUID of the host as the management server knows it (the one ofhost.guid), not after the private IP. NFS and SharedMountPoint pools keep using the private IP.python3-rados,python3-rbd):kvmha_rbd.pyis the shared access to Ceph: one connection, operations which time out after 30 seconds, and nothing written to the standard error output.kvmheartbeat_rbd.pywrites the heartbeat, reads it with-r, and reboots the host with-c.kvmvmactivity_rbd.pyis the VM-activity check of Host HA: a fresh heartbeat means the host is alive, else the RBD watchers of the volumes of the host are looked at.CEPH_KEY, and only together with the Ceph user. The monitors, the pool and the user are arguments; the monitor list is built withRbdUtils, which handles IPv6.kvm.heartbeat.update.frequency) plusheartbeat.update.timeout, the time which the agent lets a write take. A missing or malformed object, or a cluster which can not be reached, is "unable to tell" (exit status 2), never dead. With several pools,KVMHACheckerreports a host as dead if its heartbeat stopped on all of them, or on one of them ifkvm.ha.fence.on.storage.heartbeat.failureis set. A host which monitors no pool can not tell, and does not report another host as dead.Suggestsindebian/controland in the el8 and suse15 specs), as they are not available on all the supported distributions. When a pool is registered, the agent runskvmheartbeat_rbd.py --check-bindings. If the bindings are missing, orpython3-rbdis too old to list the watchers of a volume, the pool is not monitored for HA on that host and an error naming the missing package is logged. That host then writes no heartbeat, so it can not fence itself, but it can not tell either how the other hosts are doing.KVMInvestigatorsendsCheckOnHostCommand, whichKVMHACheckeranswers from the age of the heartbeat, as it does for NFS. On an RBD-only cluster without OOBM this is all that decides. Host HA, which needs OOBM, also runs the VM-activity check on the watchers of the volumes before the host is fenced.KVMHAMonitorkeeps going if the heartbeat of one pool can not be started: that pool fails alone, and the heartbeats of the other pools go on.KVMStoragePoolManagerremoves a pool from the HA monitor before it deletes it, so that no heartbeat is written while the pool is removed, and puts it back if the delete does not succeed. RBD and SharedMountPoint pools are still deleted with the delete which takes the details, as onmain, so that the libvirt pool is not destroyed.cwiki: https://cwiki.apache.org/confluence/spaces/CLOUDSTACK/pages/451979203/Host+HA+support+for+Ceph+RBD+primary+storage+KVM
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
KVMHACheckerTest,KVMHAMonitorTest,KVMStoragePoolManagerTestandLibvirtCheckVMActivityOnStoragePoolCommandWrapperTest, and more cases inLibvirtStoragePoolTestandRbdUtilsTest.radosandrbdmodules, for the cases which are hard to produce on a real cluster: missing bindings, a cluster which can not be reached, a missing, malformed or stale heartbeat, and the age limits.test/integration/smoke/test_hostha_kvm.pypasses (9 of 9).force.haset, a KVM host was force-stopped: it was markedDownafter about 3.5 minutes, and its two Instances were started on the other host.How did you try to break this feature and the system with this change?