From 0c561cd07c82d09ec5f6f1aa0a92ead403d0952b Mon Sep 17 00:00:00 2001
From: Simon Rettberg
Date: Tue, 31 May 2022 15:42:07 +0200
Subject: [remoteaccess] Avoid waking too many machines

If a location is assigned to multiple groups, we potentially wake too
many machines, as the number of clients that received a WOL in that
location when processing group A is not accounted for when peocessing
arrives at location B. Keep track of the number of WOLed machines per
location to be able to avoid this situation.
---
 .../remoteaccess/inc/remoteaccess.inc.php          | 50 +++++++++++++++++-----
 1 file changed, 39 insertions(+), 11 deletions(-)

(limited to 'modules-available/remoteaccess')

diff --git a/modules-available/remoteaccess/inc/remoteaccess.inc.php b/modules-available/remoteaccess/inc/remoteaccess.inc.php
index 1910c595..0e8b52b4 100644
--- a/modules-available/remoteaccess/inc/remoteaccess.inc.php
+++ b/modules-available/remoteaccess/inc/remoteaccess.inc.php
@@ -28,40 +28,63 @@ class RemoteAccess
 			return;
 		}
 
-		$res = Database::simpleQuery("SELECT rg.groupid, rg.groupname, rg.wolcount, GROUP_CONCAT(rxl.locationid) AS locs FROM remoteaccess_group rg
+		$res = Database::simpleQuery("SELECT rg.groupid, rg.groupname, rg.wolcount, GROUP_CONCAT(rxl.locationid) AS locs
+			FROM remoteaccess_group rg
 			INNER JOIN remoteaccess_x_location rxl USING (groupid)
 			WHERE rg.active = 1
 			GROUP BY groupid");
 
 		// Consider machines we tried to wake in the past 90 seconds as online
 		$wolDeadline = time() - 90;
+		// Keep track of locations where we sent a WOL to clients, so we don't wake too many machines in locations
+		// that appear in multiple groups
+		$locationDone = [];
 		foreach ($res as $row) {
-			if ($row['wolcount'] <= 0)
-				continue;
 			// This can't really be anything but a CSV list, but better be safe
 			$locs = preg_replace('/[^0-9,]/', '', $row['locs']);
 			if (empty($locs))
 				continue;
+			$wantedCount = $row['wolcount'];
+			// Account for machines in the same location(s) already woken in a previous loop iteration
+			foreach (explode(',', $locs) as $lid) {
+				$wantedCount -= ($locationDone[$lid] ?? 0);
+			}
+			// Nothing to do?
+			if ($wantedCount <= 0)
+				continue;
+			// Subtract number of idle machines in those locations, or machines that have
+			// received a WOL in the past $wolDeadline (90) seconds.
 			$active = Database::queryFirst("SELECT Count(*) AS cnt FROM machine m
 					INNER JOIN remoteaccess_machine rm USING (machineuuid)
 					WHERE m.locationid IN ($locs) AND (m.state = 'IDLE' OR rm.woltime > $wolDeadline)");
-			$active = (isset($active['cnt']) ? $active['cnt'] : 0);
-			$wantNum = $row['wolcount'] - $active;
-			if ($wantNum <= 0)
+			$active = ($active['cnt'] ?? 0);
+			$wantedCount -= $active;
+			if ($wantedCount <= 0)
 				continue;
-			self::tryWakeMachines($locs, $wantNum);
+			$wokenMachines = self::tryWakeMachines($locs, $wantedCount);
+			// Update our lookup table of locations and number of machines woken up in them for additional iterations
+			if (!empty($wokenMachines)) {
+				$locations = array_count_values($wokenMachines);
+				foreach ($locations as $lid => $count) {
+					$locationDone[$lid] = ($locationDone[$lid] ?? 0) + $count;
+				}
+			}
 		}
 	}
 
-	private static function tryWakeMachines($locs, $num)
+	private static function tryWakeMachines(string $locs, int $num): array
 	{
 		$res = Database::simpleQuery("SELECT m.machineuuid, m.macaddr, m.clientip, m.locationid FROM machine m
 				LEFT JOIN remoteaccess_machine rm USING (machineuuid)
 				WHERE m.locationid IN ($locs) AND m.state IN ('OFFLINE', 'STANDBY')
 				ORDER BY rm.woltime ASC");
 		$NOW = time();
+		// List of machines we did successfully try to send a WOL to
+		$retval = [];
+		// Loop while we still have machines to wake
 		while ($num > 0) {
 			$list = [];
+			// Try to gather as many machines as required
 			for ($i = 0; $i < $num && $row = $res->fetch(); ++$i) {
 				$list[] = $row;
 				Database::exec("INSERT INTO remoteaccess_machine (machineuuid, password, woltime)
@@ -72,6 +95,13 @@ class RemoteAccess
 			if (empty($list))
 				break; // No more clients in this location
 			RebootControl::wakeMachines($list, $fails);
+			$triedKv = array_column($list, 'locationid', 'machineuuid');
+			// Remove failed from list, flatten to list of machineuuid => location for return
+			foreach (array_column($fails, 'machineuuid') as $failMachineUuid) {
+				unset($triedKv[$failMachineUuid]);
+			}
+			$retval += $triedKv;
+			// Now reduce $num by how many machines were actually reachable, so we keep going in case there were fails
 			$num -= count($list) - count($fails);
 			if (!empty($fails)) {
 				$failIds = ArrayUtil::flattenByKey($fails, 'machineuuid');
@@ -80,9 +110,7 @@ class RemoteAccess
 					['faketime' => $NOW - 95, 'fails' => $failIds]);
 			}
 		}
-		if ($num > 0) {
-			error_log("Could not wake $num clients in ($locs)...");
-		}
+		return $retval;
 	}
 
 }
-- 
cgit v1.2.3-55-g7522