KVM: make storage heartbeat fence action configurable (graceful-reboot / restart-agent / log-only) - #13090
KVM: make storage heartbeat fence action configurable (graceful-reboot / restart-agent / log-only)#13090jmsperu wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13090 +/- ##
============================================
+ Coverage 3.53% 19.78% +16.25%
- Complexity 0 19991 +19991
============================================
Files 487 6371 +5884
Lines 41865 575909 +534044
Branches 7913 70496 +62583
============================================
+ Hits 1479 113940 +112461
- Misses 40171 449540 +409369
- Partials 215 12429 +12214
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:
|
DaanHoogland
left a comment
There was a problem hiding this comment.
clgtm, thanks for this feature @jmsperu . I would suggest renaming “reboot” to “fence” or “hard-reboot” (but no -1 on that).
|
@jmsperu , would you consider this for older versions as well? |
There was a problem hiding this comment.
Pull request overview
Adds a configurable fencing action for KVM storage-heartbeat timeouts to avoid overly-destructive false-positive host fencing on replicated/local primary storage backends (e.g., LINSTOR/DRBD).
Changes:
- Introduces new agent property
kvm.heartbeat.fence.action(defaultreboot) to select fencing behavior. - Updates
kvmheartbeat.shandkvmspheartbeat.shto read the property from/etc/cloudstack/agent/agent.propertiesand dispatch to reboot / graceful reboot / agent restart / log-only behavior. - Documents the property in
agent.propertiesand adds aAgentPropertiesentry for discoverability.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/vm/hypervisor/kvm/kvmheartbeat.sh | Reads kvm.heartbeat.fence.action and selects fence behavior for heartbeat write failures. |
| scripts/vm/hypervisor/kvm/kvmspheartbeat.sh | Same fencing configurability for StorPool heartbeat script. |
| agent/src/main/java/com/cloud/agent/properties/AgentProperties.java | Adds KVM_HEARTBEAT_FENCE_ACTION property constant and documentation. |
| agent/conf/agent.properties | Documents kvm.heartbeat.fence.action for operators. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@jmsperu The current hard option is there because a stale NFS (v3?) mount will keep the OS from rebooting gracefully. |
…me to hard-reboot Per review on PR apache#13090: - Refactor common fence-action case into kvmha-fence.sh sourced by both kvmheartbeat.sh and kvmspheartbeat.sh (per @sureshanaparti). - Add 'custom' fence action that invokes an operator-supplied script (kvm.heartbeat.fence.custom.script, default /etc/cloudstack/agent/ heartbeat-fence-custom.sh) with the heartbeat script name as arg; falls back to hard-reboot if the script is missing/non-executable (per @NuxRo). - Rename canonical action 'reboot' -> 'hard-reboot' for clarity; keep 'reboot' accepted as alias so existing deployments don't break (per @DaanHoogland). Default behavior unchanged: sysrq-trigger reboot, required where a stale NFSv3 mount blocks systemctl reboot.
|
Pushed f5f3db5ea3 addressing all review comments: @sureshanaparti — extracted the shared fence-action case into a new . "$(dirname "$0")/kvmha-fence.sh"
fence_action "kvmheartbeat.sh" # script name passed for log taggingNet -40 lines of duplication. @NuxRo — added @DaanHoogland — renamed canonical action to Backport question — happy to open separate cherry-pick PRs against any active branch you'd like (4.21? 4.20?). Pure agent-side change, no DB schema, low risk. |
|
@blueorangutan package |
|
Two-week ping — this PR has been APPROVED and all GHA checks (codecov, CodeQL, packaging all five distros via Jenkins SL-JID 17799) have been green since 2026-05-09. Could @sureshanaparti / @DaanHoogland trigger |
…me to hard-reboot Per review on PR apache#13090: - Refactor common fence-action case into kvmha-fence.sh sourced by both kvmheartbeat.sh and kvmspheartbeat.sh (per @sureshanaparti). - Add 'custom' fence action that invokes an operator-supplied script (kvm.heartbeat.fence.custom.script, default /etc/cloudstack/agent/ heartbeat-fence-custom.sh) with the heartbeat script name as arg; falls back to hard-reboot if the script is missing/non-executable (per @NuxRo). - Rename canonical action 'reboot' -> 'hard-reboot' for clarity; keep 'reboot' accepted as alias so existing deployments don't break (per @DaanHoogland). Default behavior unchanged: sysrq-trigger reboot, required where a stale NFSv3 mount blocks systemctl reboot.
f5f3db5 to
10303db
Compare
|
@blueorangutan package |
|
Can you update kvmsmpheartbeat.sh as well? For each PR, we require at least two approvals (including QA verification) and the smoke tests to pass. |
|
Thanks @DaanHoogland 🙏 On the rename — already in (commit 10303db): the property value is now On older versions — yes, happy to open backport PRs to 4.20.x and 4.21.x once this lands in main. Will tag you on those for review. Just re-ran the one failing CI shard ( |
Per @weizhouapache's review on apache#13090, kvmsmpheartbeat.sh was still hard-coding the immediate-reboot fence action (echo b > /proc/sysrq-trigger) while kvmheartbeat.sh and kvmspheartbeat.sh delegate to kvmha-fence.sh. Source kvmha-fence.sh and call fence_action so this script picks up the same hard-reboot / graceful-reboot / restart-agent / log-only / custom behaviour from agent.properties. Default behaviour (no property set) remains hard-reboot via sysrq-trigger, so existing deployments are unaffected.
|
Thanks @weizhouapache 🙏 You're right — kvmsmpheartbeat.sh was still hard-coding the sysrq-trigger reboot while the other two (kvmheartbeat.sh + kvmspheartbeat.sh) delegate to the shared kvmha-fence.sh helper. Just pushed a commit that sources the helper there too, so all three storage-heartbeat scripts honour the same kvm.heartbeat.fence.action property. Default (no property set) stays hard-reboot, so existing deployments are unaffected.
Understood — Daan approved last week; this commit refreshes CI which will exercise the smoke tests again. Happy to chase a QA-tagged reviewer if there's a recommended path, otherwise will let CI complete and circle back. |
1143903 to
5f46b1f
Compare
…me to hard-reboot Per review on PR apache#13090: - Refactor common fence-action case into kvmha-fence.sh sourced by both kvmheartbeat.sh and kvmspheartbeat.sh (per @sureshanaparti). - Add 'custom' fence action that invokes an operator-supplied script (kvm.heartbeat.fence.custom.script, default /etc/cloudstack/agent/ heartbeat-fence-custom.sh) with the heartbeat script name as arg; falls back to hard-reboot if the script is missing/non-executable (per @NuxRo). - Rename canonical action 'reboot' -> 'hard-reboot' for clarity; keep 'reboot' accepted as alias so existing deployments don't break (per @DaanHoogland). Default behavior unchanged: sysrq-trigger reboot, required where a stale NFSv3 mount blocks systemctl reboot.
Per @weizhouapache's review on apache#13090, kvmsmpheartbeat.sh was still hard-coding the immediate-reboot fence action (echo b > /proc/sysrq-trigger) while kvmheartbeat.sh and kvmspheartbeat.sh delegate to kvmha-fence.sh. Source kvmha-fence.sh and call fence_action so this script picks up the same hard-reboot / graceful-reboot / restart-agent / log-only / custom behaviour from agent.properties. Default behaviour (no property set) remains hard-reboot via sysrq-trigger, so existing deployments are unaffected.
…agent.properties lookup, clarify precedence - kvm.heartbeat.fence.action accepts hard-reboot (default), graceful-reboot, restart-agent, log-only and custom; the misleading 'reboot' alias is gone. - kvmha-fence.sh reads the property with a plain grep on the key=value form. - agent.properties comment states that reboot.host.and.alert.management.on.heartbeat.timeout stays the global on/off switch and this setting only chooses the action. Signed-off-by: James Peru <jmsperu@gmail.com>
|
Thanks both. Pushed 3ef45c9 for the two open comments from @weizhouapache (the |
|
[SF] Trillian test result (tid-16916)
|
The KVM agent's storage heartbeat scripts (kvmheartbeat.sh and
kvmspheartbeat.sh) hard-code an immediate kernel-level reboot via
'echo b > /proc/sysrq-trigger' when a heartbeat write to primary storage
times out. This bypasses all OS-level shutdown protections, drops every
running VM on the host instantly, and triggers HA cascades onto
surviving hosts.
For NFS shared storage the binary "heartbeat-write-failed = host-is-dead"
heuristic is reasonable. For LINSTOR/DRBD or other replicated local
storage, the same disk serves application I/O, replication I/O and
heartbeat I/O simultaneously - so a transient I/O contention spike can
time out the heartbeat write without the host actually being unhealthy.
The result is false-positive sysrq fencing.
Adds a new agent.properties option:
kvm.heartbeat.fence.action = reboot | graceful-reboot
| restart-agent | log-only
Default value is "reboot" so existing deployments keep their current
behavior. Operators on replicated storage backends can choose a less
destructive action:
- graceful-reboot: 'systemctl reboot' instead of sysrq, allowing VMs
a chance to shut down cleanly
- restart-agent: restart cloudstack-agent only, preserving running VMs
- log-only: log + alert, no automatic action
The existing 'reboot.host.and.alert.management.on.heartbeat.timeout'
boolean continues to function as a complete Java-side bypass.
Refs: apache#13089
…me to hard-reboot Per review on PR apache#13090: - Refactor common fence-action case into kvmha-fence.sh sourced by both kvmheartbeat.sh and kvmspheartbeat.sh (per @sureshanaparti). - Add 'custom' fence action that invokes an operator-supplied script (kvm.heartbeat.fence.custom.script, default /etc/cloudstack/agent/ heartbeat-fence-custom.sh) with the heartbeat script name as arg; falls back to hard-reboot if the script is missing/non-executable (per @NuxRo). - Rename canonical action 'reboot' -> 'hard-reboot' for clarity; keep 'reboot' accepted as alias so existing deployments don't break (per @DaanHoogland). Default behavior unchanged: sysrq-trigger reboot, required where a stale NFSv3 mount blocks systemctl reboot.
Per @weizhouapache's review on apache#13090, kvmsmpheartbeat.sh was still hard-coding the immediate-reboot fence action (echo b > /proc/sysrq-trigger) while kvmheartbeat.sh and kvmspheartbeat.sh delegate to kvmha-fence.sh. Source kvmha-fence.sh and call fence_action so this script picks up the same hard-reboot / graceful-reboot / restart-agent / log-only / custom behaviour from agent.properties. Default behaviour (no property set) remains hard-reboot via sysrq-trigger, so existing deployments are unaffected.
…agent.properties lookup, clarify precedence - kvm.heartbeat.fence.action accepts hard-reboot (default), graceful-reboot, restart-agent, log-only and custom; the misleading 'reboot' alias is gone. - kvmha-fence.sh reads the property with a plain grep on the key=value form. - agent.properties comment states that reboot.host.and.alert.management.on.heartbeat.timeout stays the global on/off switch and this setting only chooses the action. Signed-off-by: James Peru <jmsperu@gmail.com>
3ef45c9 to
8fdf607
Compare
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19147 |
|
[SF] Trillian test result (tid-16943)
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain in alerting, restart behavior, configuration parsing, and documented scope.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
agent/conf/agent.properties:321
- The new action is also dispatched by
kvmsmpheartbeat.sh, but this operator-facing description lists onlykvmheartbeat.shandkvmspheartbeat.sh. Add the Shared Mount Point heartbeat script here so users know the property applies to all three fence paths.
# Action taken by kvmheartbeat.sh / kvmspheartbeat.sh when a storage heartbeat
agent/src/main/java/com/cloud/agent/properties/AgentProperties.java:627
- The property is consumed by
kvmsmpheartbeat.shtoo, but this Javadoc names onlykvmheartbeat.shandkvmspheartbeat.sh. Include the third script so the Java discoverability metadata matches the behavior added in this PR.
* Action taken by the KVM agent's storage heartbeat scripts (kvmheartbeat.sh / kvmspheartbeat.sh)
* when a heartbeat write fails persistently. Allowed values:
scripts/vm/hypervisor/kvm/kvmha-fence.sh:51
KVMHAMonitor.run()calls the fence script on every heartbeat cycle. If the storage failure persists, restartingcloudstack-agenthere starts a new monitor that hits the same path again, creating an unbounded service-restart loop and preventing stable agent operation. Add a one-shot/cooldown policy (with a persistent or externally coordinated guard), or otherwise stop retrying this action after the first restart.
sync &
sleep 2
systemctl restart cloudstack-agent
scripts/vm/hypervisor/kvm/kvmha-fence.sh:20
- This helper is sourced by
kvmsmpheartbeat.shas well as the two scripts named here (see itsfence_actioncall), so the header is incomplete and can mislead maintainers about the scope of the shared behavior. Include all three heartbeat scripts.
# Shared fence-action helper for kvmheartbeat.sh and kvmspheartbeat.sh.
# Sourced by both scripts; do not invoke directly.
scripts/vm/hypervisor/kvm/kvmha-fence.sh:36
- When the last duplicate entry is empty or whitespace-only,
valbecomes empty and this guard leaves an earlier action inFENCE_ACTION. That contradicts the documented last-entry-wins behavior and can preserve an earlierlog-only/restart-agentvalue instead of falling back tohard-reboot; assign the last parsed value unconditionally so an empty value reaches the existing fallback case.
[ -n "$val" ] && FENCE_ACTION="$val"
scripts/vm/hypervisor/kvm/kvmha-fence.sh:35
- This exact
key=grep does not accept the whitespace forms supported by the agent's Java.propertiesparser (for example,kvm.heartbeat.fence.action = log-only). In that valid configuration, the helper silently keepshard-reboot, so an operator's less-destructive setting can be ignored; use a whitespace-tolerant property parser or explicitly normalize the key/value separator.
val=$(grep "^kvm.heartbeat.fence.action=" "$AGENT_PROPS" | tail -n 1 | cut -d= -f2- | tr -d '[:space:]')
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
|
|
||
| case "$FENCE_ACTION" in | ||
| log-only) | ||
| /usr/bin/logger -t heartbeat "${source_script}: heartbeat write to storage failed; fence action 'log-only' selected — taking no automatic action. Operator must investigate." |
| custom) | ||
| if [ -x "$CUSTOM_SCRIPT" ]; then | ||
| /usr/bin/logger -t heartbeat "${source_script}: heartbeat write to storage failed; fence action 'custom' — running ${CUSTOM_SCRIPT}." | ||
| sync & | ||
| sleep 2 | ||
| "$CUSTOM_SCRIPT" "$source_script" |
| * <li>{@code hard-reboot} (default; {@code reboot} accepted as alias) — immediate | ||
| * sysrq-trigger reboot. Required default for setups where a stale NFSv3 mount can |
Description
Fixes #13089
The KVM agent's storage heartbeat scripts (
kvmheartbeat.shandkvmspheartbeat.sh) hard-code an immediate kernel-level reboot viaecho b > /proc/sysrq-triggerwhen a heartbeat write to primary storage times out.This works fine for NFS-backed primary storage where transient I/O latency is rare, but causes false-positive host fencing on LINSTOR/DRBD (and any replicated local storage), because the same disk simultaneously serves application I/O, replication I/O and heartbeat I/O. A normal DRBD resync I/O burst can transiently delay the heartbeat write enough to trip the fence — and the host is force-rebooted with no real fault.
We hit this in production on 4.22.0.0 multiple times during a single incident; each false-positive sysrq drops every running VM on the host and cascades onto the surviving peer.
Change
Adds a new agent property
kvm.heartbeat.fence.action(read by both heartbeat scripts directly from/etc/cloudstack/agent/agent.properties):hard-reboot(default)echo b > /proc/sysrq-triggergraceful-rebootsystemctl reboot— allows running VMs to stop cleanlyrestart-agentcloudstack-agentonly; running VMs preservedlog-onlyDefault is
hard-rebootso existing deployments keep current behavior. Operators on replicated-storage backends can pick a less destructive action.The existing
reboot.host.and.alert.management.on.heartbeat.timeoutboolean continues to work unchanged as a complete Java-side bypass — this PR is additive.Files changed
scripts/vm/hypervisor/kvm/kvmheartbeat.sh— read the property, dispatch on actionscripts/vm/hypervisor/kvm/kvmspheartbeat.sh— sameagent/conf/agent.properties— document the new propertyagent/src/main/java/com/cloud/agent/properties/AgentProperties.java— add Java-side property entry for tooling/discoverabilityBackward compatibility
hard-reboot, identical to current behaviortail -n 1so duplicate entries take the last valuehard-rebootreboot.host.and.alert.management.on.heartbeat.timeout) continues to work as beforeTesting
hard-reboot(default) — verified produces same output as before viabash -xtrace; sysrq path unchangedlog-only— verified the script exits 0 with logger entry, no reboot/agent-restart attemptedrestart-agent— verifiedsystemctl restart cloudstack-agentis invokedgraceful-reboot— verifiedsystemctl rebootis invoked instead of sysrqIn production we have been running with the fence path neutered (equivalent to
log-only) for several hours since the incident, with no impact on cluster health — the host stays up while DRBD resyncs background-complete normally, and the previous false-positive cascade has not recurred.Related