Skip to content

Commit 7c3ff67

Browse files
committed
address review (#13090): exact property parsing, safe fallbacks, restart-agent loop guard
- read_agent_property parses agent.properties the way java.util.Properties does: the key is compared as a plain string (dots are no longer regex wildcards), whitespace around '=' or ':' (or a whitespace separator) is accepted, the value is trimmed, '#' and '!' lines are comments and the last definition wins. - kvmheartbeat.sh, kvmspheartbeat.sh and kvmsmpheartbeat.sh fall back to the original sysrq hard-reboot, with an ERROR in syslog, if kvmha-fence.sh cannot be sourced or does not define fence_action. - restart-agent and graceful-reboot check for systemctl before acting and hard-reboot with an ERROR if it is missing or the systemctl call fails. - restart-agent records its time under /run (kept across agent restarts, cleared by a reboot). A new restart-agent fence within kvm.heartbeat.fence.restart.agent.cooldown seconds (default 600, 0 turns the guard off) escalates to hard-reboot, so a host whose storage stays down cannot loop on agent restarts. Documented in agent.properties and AgentProperties. - KvmHaFenceScriptTest: graceful-reboot (systemctl reboot, no VM stop by the fence), failing and missing systemctl, whitespace/colon separators, last definition wins, dotted key exact match, the cooldown guard, and the heartbeat scripts' fallback when the helper is missing or empty. Claude-Session: https://claude.ai/code/session_01EUU5skTaj95LfpkSbrWhPN
1 parent 8fa1937 commit 7c3ff67

7 files changed

Lines changed: 362 additions & 27 deletions

File tree

‎agent/conf/agent.properties‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -330,12 +330,14 @@ iscsi.session.cleanup.enabled=false
330330
# graceful-reboot - 'systemctl reboot' instead of sysrq; allows VMs to stop cleanly.
331331
# Use only if a stale storage mount cannot block shutdown.
332332
# restart-agent - stop every VM on the host, then restart cloudstack-agent; the
333-
# host itself stays up.
333+
# host itself stays up. If it already fired within
334+
# kvm.heartbeat.fence.restart.agent.cooldown, hard-reboot instead.
334335
# log-only - stop every VM on the host, then only write a message to the
335336
# host's syslog (no reboot, no agent restart, no management server
336337
# alert). The operator must watch syslog and investigate.
337338
#
338-
# Any other value falls back to hard-reboot.
339+
# Any other value falls back to hard-reboot. restart-agent and graceful-reboot need
340+
# systemctl; if it is missing or the systemctl call fails, the fence hard-reboots.
339341
#
340342
# restart-agent and log-only stop the VMs because, once the heartbeat is stale, the
341343
# management server may declare the host Down and HA may start the same VMs on another
@@ -381,6 +383,15 @@ iscsi.session.cleanup.enabled=false
381383
# kvm.heartbeat.fence.keep.vms.running is true.
382384
#kvm.heartbeat.fence.stop.vms.timeout=30
383385

386+
# Loop guard for restart-agent, in seconds. While storage keeps failing, the restarted
387+
# agent runs the heartbeat again, it fails again and restart-agent would restart the
388+
# agent again, over and over. Each restart-agent fence records its time under /run
389+
# (kept across agent restarts, cleared by a reboot); if restart-agent already fired less
390+
# than this many seconds ago, the fence hard-reboots the host instead. 0 turns the guard
391+
# off. A value that is not a whole number of seconds means 600.
392+
# Has no effect on hard-reboot, graceful-reboot and log-only.
393+
#kvm.heartbeat.fence.restart.agent.cooldown=600
394+
384395
# Enables manually setting CPU's topology on KVM's VM.
385396
#enable.manually.setting.cpu.topology.on.kvm.vm=true
386397

‎agent/src/main/java/com/cloud/agent/properties/AgentProperties.java‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -632,11 +632,13 @@ public class AgentProperties{
632632
* where a stale NFSv3 mount can prevent a graceful shutdown from completing.</li>
633633
* <li>{@code graceful-reboot}: {@code systemctl reboot} instead of sysrq; allows VMs
634634
* to stop cleanly. Use only if a stale storage mount cannot block shutdown.</li>
635-
* <li>{@code restart-agent}: stop every VM on the host, then restart cloudstack-agent; the host stays up.</li>
635+
* <li>{@code restart-agent}: stop every VM on the host, then restart cloudstack-agent; the host stays up. If it
636+
* already fired within {@link #KVM_HEARTBEAT_FENCE_RESTART_AGENT_COOLDOWN}, hard-reboot instead.</li>
636637
* <li>{@code log-only}: stop every VM on the host, then only write a message to the host's syslog (no
637638
* reboot, no agent restart, no management server alert); the operator must investigate.</li>
638639
* </ul>
639-
* Any other value falls back to {@code hard-reboot}.<br>
640+
* Any other value falls back to {@code hard-reboot}. {@code restart-agent} and {@code graceful-reboot} need
641+
* systemctl; if it is missing or the systemctl call fails, the fence hard-reboots.<br>
640642
* {@code restart-agent} and {@code log-only} stop the VMs because, once the heartbeat is stale, the management
641643
* server may declare the host Down and HA may start the same VMs on another host, where both copies would write
642644
* to the same disks. If any VM cannot be stopped the fence falls back to {@code hard-reboot}. See
@@ -696,6 +698,20 @@ public class AgentProperties{
696698
public static final Property<Integer> KVM_HEARTBEAT_FENCE_STOP_VMS_TIMEOUT
697699
= new Property<>("kvm.heartbeat.fence.stop.vms.timeout", 30);
698700

701+
/**
702+
* Loop guard for the {@code restart-agent} fence action, in seconds. While storage keeps failing, the restarted
703+
* agent runs the heartbeat again, it fails again and {@code restart-agent} would restart the agent again, over and
704+
* over. Each {@code restart-agent} fence records its time under /run (kept across agent restarts, cleared by a
705+
* reboot); if {@code restart-agent} already fired less than this many seconds ago, the fence hard-reboots the host
706+
* instead. 0 turns the guard off. A value that is not a whole number of seconds falls back to the default. No
707+
* effect on {@code hard-reboot}, {@code graceful-reboot} and {@code log-only}.<br>
708+
* Read by the heartbeat shell scripts directly from agent.properties.<br>
709+
* Data type: Integer.<br>
710+
* Default value: <code>600</code>
711+
*/
712+
public static final Property<Integer> KVM_HEARTBEAT_FENCE_RESTART_AGENT_COOLDOWN
713+
= new Property<>("kvm.heartbeat.fence.restart.agent.cooldown", 600);
714+
699715
/**
700716
* Enables manually setting CPU's topology on KVM's VM. <br>
701717
* Data type: Boolean.<br>

‎agent/src/test/java/com/cloud/agent/KvmHaFenceScriptTest.java‎

Lines changed: 171 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,11 @@
2222
import java.nio.file.Files;
2323
import java.nio.file.Path;
2424
import java.util.ArrayList;
25+
import java.util.Arrays;
2526
import java.util.Comparator;
27+
import java.util.HashMap;
2628
import java.util.List;
29+
import java.util.Map;
2730
import java.util.concurrent.TimeUnit;
2831
import java.util.regex.Matcher;
2932
import java.util.regex.Pattern;
@@ -42,6 +45,7 @@
4245
public class KvmHaFenceScriptTest {
4346

4447
private static final File SCRIPT = new File("../scripts/vm/hypervisor/kvm/kvmha-fence.sh");
48+
private static final File SCRIPT_DIR = SCRIPT.getParentFile();
4549

4650
private Path dir;
4751
private Path running;
@@ -51,6 +55,8 @@ public class KvmHaFenceScriptTest {
5155
private Path props;
5256
private Path deadmanDir;
5357
private Path hangPids;
58+
private Path marker;
59+
private final Map<String, String> extraEnv = new HashMap<>();
5460

5561
@Before
5662
public void setUp() throws IOException {
@@ -64,6 +70,7 @@ public void setUp() throws IOException {
6470
props = dir.resolve("agent.properties");
6571
deadmanDir = dir.resolve("deadman");
6672
hangPids = dir.resolve("hang-pids");
73+
marker = deadmanDir.resolve("restart-agent.last");
6774
Files.createFile(calls);
6875
Files.createFile(syslog);
6976
stub("virsh", "echo \"virsh $*\" >> '" + calls + "'\n"
@@ -113,10 +120,15 @@ private int fence(final String properties, final String... runningVms) throws Ex
113120
}
114121

115122
private Process start(final String properties, final String... runningVms) throws IOException {
123+
return start(Arrays.asList("/bin/bash", "-c", "source \"$1\"; fence_action test.sh", "fence", SCRIPT.getAbsolutePath()),
124+
properties, runningVms);
125+
}
126+
127+
private Process start(final List<String> command, final String properties, final String... runningVms) throws IOException {
116128
Files.write(props, properties.getBytes(StandardCharsets.UTF_8));
117129
Files.write(running, (String.join("\n", runningVms) + "\n").getBytes(StandardCharsets.UTF_8));
118-
final ProcessBuilder pb = new ProcessBuilder("/bin/bash", "-c", "source \"$1\"; fence_action test.sh", "fence",
119-
SCRIPT.getAbsolutePath());
130+
final ProcessBuilder pb = new ProcessBuilder(command);
131+
pb.environment().putAll(extraEnv);
120132
pb.environment().put("PATH", dir + File.pathSeparator + System.getenv("PATH"));
121133
pb.environment().put("AGENT_PROPS", props.toString());
122134
pb.environment().put("FENCE_LOGGER", dir.resolve("logger").toString());
@@ -127,6 +139,26 @@ private Process start(final String properties, final String... runningVms) throw
127139
return pb.start();
128140
}
129141

142+
/** Runs a copy of the heartbeat script {@code name} with -c from a directory where kvmha-fence.sh is {@code helper}. */
143+
private int heartbeatCleanup(final String name, final String helper, final String... args) throws Exception {
144+
final Path callerDir = Files.createDirectories(dir.resolve("caller"));
145+
final Path caller = callerDir.resolve(name);
146+
Files.copy(new File(SCRIPT_DIR, name).toPath(), caller);
147+
if (helper != null) {
148+
Files.write(callerDir.resolve("kvmha-fence.sh"), helper.getBytes(StandardCharsets.UTF_8));
149+
}
150+
final List<String> command = new ArrayList<>(Arrays.asList("/bin/bash", caller.toString()));
151+
command.addAll(Arrays.asList(args));
152+
final Process process = start(command, "", "i-2-10-VM");
153+
Assert.assertTrue("heartbeat script did not finish", process.waitFor(60, TimeUnit.SECONDS));
154+
return process.exitValue();
155+
}
156+
157+
private void writeMarker(final long secondsAgo) throws IOException {
158+
Files.createDirectories(deadmanDir);
159+
Files.write(marker, (System.currentTimeMillis() / 1000 - secondsAgo + "\n").getBytes(StandardCharsets.UTF_8));
160+
}
161+
130162
/** Pids of every deadman the script started, as each one logs its own pid when it starts. */
131163
private List<Long> deadmanPids() throws IOException {
132164
final List<Long> pids = new ArrayList<>();
@@ -314,4 +346,141 @@ public void unsetStopBudgetUsesTheDefaultWithoutAWarning() throws Exception {
314346
Assert.assertTrue(log.contains("with a budget of 30 s"));
315347
Assert.assertFalse(log.contains("is not a positive number"));
316348
}
349+
350+
@Test
351+
public void gracefulRebootCallsSystemctlRebootAndLeavesStoppingVmsToTheShutdown() throws Exception {
352+
Assert.assertEquals(0, fence("kvm.heartbeat.fence.action=graceful-reboot\n", "i-2-10-VM", "r-4-VM"));
353+
final String log = read(calls);
354+
Assert.assertTrue(log.contains("systemctl reboot"));
355+
// The orderly shutdown stops the VMs; the fence itself neither destroys them nor lists them.
356+
Assert.assertFalse(log.contains("virsh"));
357+
Assert.assertEquals("i-2-10-VM\nr-4-VM", read(running).trim());
358+
Assert.assertFalse(rebooted());
359+
}
360+
361+
@Test
362+
public void failingSystemctlRebootFallsBackToHardReboot() throws Exception {
363+
stub("systemctl", "echo \"systemctl $*\" >> '" + calls + "'\nexit 1\n");
364+
fence("kvm.heartbeat.fence.action=graceful-reboot\n", "i-2-10-VM");
365+
Assert.assertTrue(read(calls).contains("systemctl reboot"));
366+
Assert.assertTrue(read(syslog).contains("ERROR 'systemctl reboot' failed; falling back to hard-reboot"));
367+
Assert.assertTrue(rebooted());
368+
}
369+
370+
@Test
371+
public void failingAgentRestartFallsBackToHardReboot() throws Exception {
372+
stub("systemctl", "echo \"systemctl $*\" >> '" + calls + "'\nexit 1\n");
373+
fence("kvm.heartbeat.fence.action=restart-agent\n", "i-2-10-VM");
374+
Assert.assertTrue(read(calls).contains("destroy i-2-10-VM"));
375+
Assert.assertTrue(read(calls).contains("systemctl restart cloudstack-agent"));
376+
Assert.assertTrue(read(syslog).contains("ERROR 'systemctl restart cloudstack-agent' failed; falling back to hard-reboot"));
377+
Assert.assertTrue(rebooted());
378+
}
379+
380+
@Test
381+
public void missingSystemctlFallsBackToHardReboot() throws Exception {
382+
extraEnv.put("FENCE_SYSTEMCTL", "no-such-systemctl");
383+
for (final String action : new String[] {"restart-agent", "graceful-reboot"}) {
384+
Files.write(sysrq, new byte[0]);
385+
Files.write(syslog, new byte[0]);
386+
fence("kvm.heartbeat.fence.action=" + action + "\n", "i-2-10-VM");
387+
Assert.assertTrue(action + " without systemctl must hard-reboot", rebooted());
388+
Assert.assertTrue(read(syslog).contains("ERROR fence action '" + action + "' needs systemctl"));
389+
}
390+
// The host reboots anyway, so no VM is stopped first.
391+
Assert.assertFalse(read(calls).contains("destroy"));
392+
}
393+
394+
@Test
395+
public void whitespaceAndColonAroundTheSeparatorAreAccepted() throws Exception {
396+
Assert.assertEquals(0, fence(" kvm.heartbeat.fence.action = log-only \nkvm.heartbeat.fence.keep.vms.running\t=\ttrue\n",
397+
"i-2-10-VM"));
398+
Assert.assertFalse("keep.vms.running = true must be honoured", read(calls).contains("destroy"));
399+
Assert.assertFalse(rebooted());
400+
401+
Assert.assertEquals(0, fence("kvm.heartbeat.fence.action: log-only\nkvm.heartbeat.fence.keep.vms.running true\n", "i-2-10-VM"));
402+
Assert.assertFalse(read(calls).contains("destroy"));
403+
Assert.assertFalse(rebooted());
404+
}
405+
406+
@Test
407+
public void lastDefinitionWinsAndCommentsAreIgnored() throws Exception {
408+
Assert.assertEquals(0, fence("kvm.heartbeat.fence.action=restart-agent\nkvm.heartbeat.fence.action = log-only\n"
409+
+ "# kvm.heartbeat.fence.action=restart-agent\n! kvm.heartbeat.fence.action=restart-agent\n", "i-2-10-VM"));
410+
Assert.assertFalse(read(calls).contains("systemctl"));
411+
Assert.assertFalse(rebooted());
412+
}
413+
414+
@Test
415+
public void dotsInTheKeyAreNotWildcards() throws Exception {
416+
fence("kvmXheartbeatXfenceXaction=log-only\nkvm.heartbeat.fence.action.old=log-only\n", "i-2-10-VM");
417+
Assert.assertTrue("no exact kvm.heartbeat.fence.action key, so the default hard-reboot applies", rebooted());
418+
Assert.assertFalse(read(calls).contains("destroy"));
419+
}
420+
421+
@Test
422+
public void restartAgentRecordsWhenItRestartedTheAgent() throws Exception {
423+
final long before = System.currentTimeMillis() / 1000;
424+
Assert.assertEquals(0, fence("kvm.heartbeat.fence.action=restart-agent\n", "i-2-10-VM"));
425+
Assert.assertTrue(read(calls).contains("systemctl restart cloudstack-agent"));
426+
final long recorded = Long.parseLong(read(marker).trim());
427+
Assert.assertTrue(recorded >= before && recorded <= System.currentTimeMillis() / 1000);
428+
}
429+
430+
@Test
431+
public void restartAgentWithinTheCooldownEscalatesToHardReboot() throws Exception {
432+
writeMarker(30);
433+
fence("kvm.heartbeat.fence.action=restart-agent\n", "i-2-10-VM");
434+
Assert.assertTrue("a second restart-agent within the default 600 s must hard-reboot", rebooted());
435+
Assert.assertFalse(read(calls).contains("systemctl restart cloudstack-agent"));
436+
Assert.assertFalse("the host reboots anyway, so no VM is stopped first", read(calls).contains("destroy"));
437+
Assert.assertTrue(read(syslog).contains("within kvm.heartbeat.fence.restart.agent.cooldown (600 s)"));
438+
Assert.assertTrue(read(syslog).contains("escalating to hard-reboot"));
439+
}
440+
441+
@Test
442+
public void restartAgentAfterTheCooldownRestartsTheAgentAgain() throws Exception {
443+
writeMarker(120);
444+
Assert.assertEquals(0, fence("kvm.heartbeat.fence.action=restart-agent\nkvm.heartbeat.fence.restart.agent.cooldown = 60\n",
445+
"i-2-10-VM"));
446+
Assert.assertTrue(read(calls).contains("systemctl restart cloudstack-agent"));
447+
Assert.assertFalse(rebooted());
448+
Assert.assertTrue("the marker must be refreshed", Long.parseLong(read(marker).trim()) >= System.currentTimeMillis() / 1000 - 5);
449+
}
450+
451+
@Test
452+
public void restartAgentCooldownZeroTurnsTheGuardOff() throws Exception {
453+
writeMarker(1);
454+
Assert.assertEquals(0, fence("kvm.heartbeat.fence.action=restart-agent\nkvm.heartbeat.fence.restart.agent.cooldown=0\n",
455+
"i-2-10-VM"));
456+
Assert.assertTrue(read(calls).contains("systemctl restart cloudstack-agent"));
457+
Assert.assertFalse(rebooted());
458+
}
459+
460+
@Test
461+
public void invalidRestartAgentCooldownFallsBackToTheDefault() throws Exception {
462+
writeMarker(300);
463+
fence("kvm.heartbeat.fence.action=restart-agent\nkvm.heartbeat.fence.restart.agent.cooldown=ten\n", "i-2-10-VM");
464+
Assert.assertTrue(read(syslog).contains("kvm.heartbeat.fence.restart.agent.cooldown='ten'"));
465+
Assert.assertTrue("300 s ago is within the default 600 s", rebooted());
466+
}
467+
468+
@Test
469+
public void heartbeatScriptsHardRebootWhenTheFenceHelperIsMissing() throws Exception {
470+
heartbeatCleanup("kvmspheartbeat.sh", null, "-c");
471+
Assert.assertTrue(rebooted());
472+
Assert.assertTrue(read(syslog).contains("kvmspheartbeat.sh: ERROR could not load kvmha-fence.sh; falling back to hard-reboot."));
473+
Assert.assertTrue(read(syslog).contains("kvmspheartbeat.sh will reboot system"));
474+
}
475+
476+
@Test
477+
public void heartbeatScriptsHardRebootWhenTheFenceHelperDoesNotDefineTheFence() throws Exception {
478+
// An empty or truncated helper (partial upgrade) loads fine but leaves fence_action undefined.
479+
stub("mount", "exit 0\n");
480+
stub("ps", "exit 0\n");
481+
heartbeatCleanup("kvmheartbeat.sh", "", "-i", "192.0.2.1", "-p", "/export", "-m", dir.resolve("mnt").toString(),
482+
"-h", "192.0.2.2", "-c");
483+
Assert.assertTrue(rebooted());
484+
Assert.assertTrue(read(syslog).contains("kvmheartbeat.sh: ERROR could not load kvmha-fence.sh; falling back to hard-reboot."));
485+
}
317486
}

0 commit comments

Comments
 (0)