From c334e30fa974132a4d56d092019bad23561e14ab Mon Sep 17 00:00:00 2001 From: Florian Paul Azim Hoberg Date: Wed, 16 Sep 2026 14:27:53 +0200 Subject: [PATCH 1/4] fix: Reevaluate and restart the run when there is a connection related API issue. Fixes: #134 --- proxlb/__main__.py | 236 +++++++++++++++++++++++++-------------------- 1 file changed, 131 insertions(+), 105 deletions(-) diff --git a/proxlb/__main__.py b/proxlb/__main__.py index bc499e0..f38f087 100644 --- a/proxlb/__main__.py +++ b/proxlb/__main__.py @@ -14,6 +14,11 @@ import logging import signal +import time + +import proxmoxer +import proxmoxer.backends.https +import requests from proxlb_solver import shadow as _solver_shadow from proxlb_solver.models import MigrationPlan @@ -83,116 +88,137 @@ def reinstall_sigint() -> None: while True: - # Validate if HA mode is enabled and if this node is the HA manager. - if proxlb_config.service.enable_ha: - if not HaStatus.is_node_ha_manager(proxmox_api): - logger.info("This node is not the HA manager. Waiting for next run.") - Helper.get_daemon_mode(proxlb_config) - continue - else: - logger.debug("This node is the HA manager. Continuing.") - - # Validate if reload signal was sent during runtime - # and reload the ProxLB configuration and adjust log level - if Helper.proxlb_reload: - logger.info("Reloading ProxLB configuration.") - proxlb_config = config_parser.get_config() - logger.set_log_level(proxlb_config.service.log_level) - Helper.proxlb_reload = False - - # Get all required objects from the Proxmox cluster - Helper.apply_maintenance_nodes_schedule(proxlb_config) - nodes = Nodes.get_nodes(proxmox_api, proxlb_config) - meta = Features.validate_any_non_pve9_node(proxlb_config, nodes) - pools = Pools.get_pools(proxmox_api) - ha_rules = HaRules.get_ha_rules(proxmox_api, meta) - guests = Guests.get_guests(proxmox_api, pools, ha_rules, nodes, proxlb_config) - groups = Groups.get_groups(guests, nodes) - - # Merge obtained objects from the Proxmox cluster for further usage - proxlb_data = ProxLbData( - meta=meta, - nodes=nodes, - guests=guests, - pools=pools, - ha_rules=ha_rules, - groups=groups, - ) - Helper.log_node_metrics(proxlb_data) - - # Validate usable features by PVE versions - Features.validate_available_features(proxlb_data) - - # Update the initial node resource assignments - # by the previously created groups. - Calculations.set_node_assignments(proxlb_data) - Helper.log_node_metrics(proxlb_data, init=False) - Calculations.set_node_hot(proxlb_data) - Calculations.set_guest_hot(proxlb_data) - target = Calculations.get_most_free_node(proxlb_data, cli_args.best_node) - if target is None: - logger.warning("No suitable target node found for balancing. Skipping this run.") - else: - Calculations.validate_affinity_map(proxlb_data) - Calculations.relocate_guests_on_maintenance_nodes(proxlb_data) - Calculations.get_balanciness(proxlb_data) - Calculations.relocate_guests(proxlb_data) - - # CP-SAT solver (optional) — shadow (read-only) or active mode. - _solver_cfg = proxlb_config.solver - _run_file: str | None = None - _solver_plan: MigrationPlan | None = None - if _solver_cfg.enable: - _run_file, _solver_plan = _solver_shadow.run_shadow( - proxlb_data, _solver_cfg - ) - reinstall_sigint() - + try: + # Validate if HA mode is enabled and if this node is the HA manager. + if proxlb_config.service.enable_ha: + if not HaStatus.is_node_ha_manager(proxmox_api): + logger.info("This node is not the HA manager. Waiting for next run.") + Helper.get_daemon_mode(proxlb_config) + continue + else: + logger.debug("This node is the HA manager. Continuing.") + + # Validate if reload signal was sent during runtime + # and reload the ProxLB configuration and adjust log level + if Helper.proxlb_reload: + logger.info("Reloading ProxLB configuration.") + proxlb_config = config_parser.get_config() + logger.set_log_level(proxlb_config.service.log_level) + Helper.proxlb_reload = False + + # Get all required objects from the Proxmox cluster + Helper.apply_maintenance_nodes_schedule(proxlb_config) + nodes = Nodes.get_nodes(proxmox_api, proxlb_config) + meta = Features.validate_any_non_pve9_node(proxlb_config, nodes) + pools = Pools.get_pools(proxmox_api) + ha_rules = HaRules.get_ha_rules(proxmox_api, meta) + guests = Guests.get_guests(proxmox_api, pools, ha_rules, nodes, proxlb_config) + groups = Groups.get_groups(guests, nodes) + + # Merge obtained objects from the Proxmox cluster for further usage + proxlb_data = ProxLbData( + meta=meta, + nodes=nodes, + guests=guests, + pools=pools, + ha_rules=ha_rules, + groups=groups, + ) + Helper.log_node_metrics(proxlb_data) + + # Validate usable features by PVE versions + Features.validate_available_features(proxlb_data) + + # Update the initial node resource assignments + # by the previously created groups. + Calculations.set_node_assignments(proxlb_data) Helper.log_node_metrics(proxlb_data, init=False) + Calculations.set_node_hot(proxlb_data) + Calculations.set_guest_hot(proxlb_data) + target = Calculations.get_most_free_node(proxlb_data, cli_args.best_node) + if target is None: + logger.warning("No suitable target node found for balancing. Skipping this run.") + else: + Calculations.validate_affinity_map(proxlb_data) + Calculations.relocate_guests_on_maintenance_nodes(proxlb_data) + Calculations.get_balanciness(proxlb_data) + Calculations.relocate_guests(proxlb_data) + + # CP-SAT solver (optional) — shadow (read-only) or active mode. + _solver_cfg = proxlb_config.solver + _run_file: str | None = None + _solver_plan: MigrationPlan | None = None + if _solver_cfg.enable: + _run_file, _solver_plan = _solver_shadow.run_shadow( + proxlb_data, _solver_cfg + ) + reinstall_sigint() - # Perform balancing actions via Proxmox API - _skipped_greedy_balancing = False - if proxlb_data.meta.balancing.enable: - if not cli_args.dry_run: - if (_solver_cfg.enable - and _solver_cfg.mode == "active"): - if _solver_plan is not None: - try: - _solver_shadow.execute_solver_plan( - proxmox_api, proxlb_data, - _solver_plan, _solver_cfg, _run_file - ) - except Exception as exc: - if _solver_cfg.fallback_to_greedy: - logger.warning( - f"[solver] active execution failed, " - f"falling back to ProxLB plan: {exc}") - Balancing.balance(proxmox_api, proxlb_data) - else: - _skipped_greedy_balancing = True - logger.warning( - f"[solver] active execution failed, " - f"skipping greedy fallback " - f"(fallback_to_greedy=False): {exc}") - elif _solver_cfg.fallback_to_greedy: - Balancing.balance(proxmox_api, proxlb_data) + Helper.log_node_metrics(proxlb_data, init=False) + + # Perform balancing actions via Proxmox API + _skipped_greedy_balancing = False + if proxlb_data.meta.balancing.enable: + if not cli_args.dry_run: + if (_solver_cfg.enable + and _solver_cfg.mode == "active"): + if _solver_plan is not None: + try: + _solver_shadow.execute_solver_plan( + proxmox_api, proxlb_data, + _solver_plan, _solver_cfg, _run_file + ) + except Exception as exc: + if _solver_cfg.fallback_to_greedy: + logger.warning( + f"[solver] active execution failed, " + f"falling back to ProxLB plan: {exc}") + Balancing.balance(proxmox_api, proxlb_data) + else: + _skipped_greedy_balancing = True + logger.warning( + f"[solver] active execution failed, " + f"skipping greedy fallback " + f"(fallback_to_greedy=False): {exc}") + elif _solver_cfg.fallback_to_greedy: + Balancing.balance(proxmox_api, proxlb_data) + else: + _skipped_greedy_balancing = True + logger.warning( + "[solver] active: no feasible solver plan; " + "skipping greedy fallback " + "(fallback_to_greedy=False)") else: - _skipped_greedy_balancing = True - logger.warning( - "[solver] active: no feasible solver plan; " - "skipping greedy fallback " - "(fallback_to_greedy=False)") - else: - Balancing.balance(proxmox_api, proxlb_data) + Balancing.balance(proxmox_api, proxlb_data) + reinstall_sigint() + + # Record whether balancing was executed or skipped (dry-run). + if _run_file is not None and not _skipped_greedy_balancing: + try: + _solver_shadow.finalize_run(_run_file, dry_run=cli_args.dry_run) + except Exception as exc: + logger.warning(f"[solver] finalize_run failed: {exc}") reinstall_sigint() - # Record whether balancing was executed or skipped (dry-run). - if _run_file is not None and not _skipped_greedy_balancing: - try: - _solver_shadow.finalize_run(_run_file, dry_run=cli_args.dry_run) - except Exception as exc: - logger.warning(f"[solver] finalize_run failed: {exc}") - reinstall_sigint() + except ( + proxmoxer.core.ResourceException, + proxmoxer.backends.https.AuthenticationError, + requests.exceptions.RequestException, + ) as api_error: + logger.critical( + f"ProxLB: A Proxmox API call failed: {api_error}. " + "Reconnecting to the Proxmox API and restarting balancing run from scratch." + ) + time.sleep(proxlb_config.proxmox_api.wait_time) + + # Reload the config to recover the real credentials (masked below after + # every connect) and reconnect from scratch, re-running host failover. + proxlb_config = config_parser.get_config() + logger.set_log_level(proxlb_config.service.log_level) + proxmox_api = ProxmoxApi(proxlb_config) + proxlb_config.proxmox_api.password = "********" + proxlb_config.proxmox_api.token_secret = "********" + continue # Validate if the JSON output should be # printed to stdout From 7088163a76923ee2fb1e72da11bf71599f8e2538 Mon Sep 17 00:00:00 2001 From: Florian Paul Azim Hoberg Date: Wed, 16 Sep 2026 20:57:38 +0200 Subject: [PATCH 2/4] Add node availability check before migrations Fixes: #134 --- ...e-availability-check-before-migrations.yml | 2 + ...-fix_segfault_when_pve_api_unreachable.yml | 2 + .changelogs/2.4.0/release_meta.yml | 1 + proxlb/__main__.py | 5 +- proxlb/models/balancing.py | 15 ++++- .../tests/test_balancing_streaming_queue.py | 24 ++++++++ proxlb/utils/helper.py | 43 ++++++++++++++- tests/test_check_nodes_available.py | 55 +++++++++++++++++++ 8 files changed, 143 insertions(+), 4 deletions(-) create mode 100644 .changelogs/2.4.0/134-add-node-availability-check-before-migrations.yml create mode 100644 .changelogs/2.4.0/134-fix_segfault_when_pve_api_unreachable.yml create mode 100644 .changelogs/2.4.0/release_meta.yml create mode 100644 tests/test_check_nodes_available.py diff --git a/.changelogs/2.4.0/134-add-node-availability-check-before-migrations.yml b/.changelogs/2.4.0/134-add-node-availability-check-before-migrations.yml new file mode 100644 index 0000000..5a2b74b --- /dev/null +++ b/.changelogs/2.4.0/134-add-node-availability-check-before-migrations.yml @@ -0,0 +1,2 @@ +added: +- Check that all cluster nodes still respond to `nodes/{node}/status` before dispatching each migration during balancing; restarts ProxLB from scratch if a node has become unavailable. [#134] diff --git a/.changelogs/2.4.0/134-fix_segfault_when_pve_api_unreachable.yml b/.changelogs/2.4.0/134-fix_segfault_when_pve_api_unreachable.yml new file mode 100644 index 0000000..8d770a0 --- /dev/null +++ b/.changelogs/2.4.0/134-fix_segfault_when_pve_api_unreachable.yml @@ -0,0 +1,2 @@ +fix: +- Fix segmentation fault when PVE API is unreachable during runtime. [#134] diff --git a/.changelogs/2.4.0/release_meta.yml b/.changelogs/2.4.0/release_meta.yml new file mode 100644 index 0000000..8d44ed2 --- /dev/null +++ b/.changelogs/2.4.0/release_meta.yml @@ -0,0 +1 @@ +date: TBD \ No newline at end of file diff --git a/proxlb/__main__.py b/proxlb/__main__.py index f38f087..d2bd9f3 100644 --- a/proxlb/__main__.py +++ b/proxlb/__main__.py @@ -36,7 +36,7 @@ from proxlb.models.pools import Pools from proxlb.models.ha_rules import HaRules from proxlb.models.ha_status import HaStatus -from proxlb.utils.helper import Helper +from proxlb.utils.helper import Helper, NodeUnavailableError from proxlb.utils.proxlb_data import ProxLbData @@ -204,9 +204,10 @@ def reinstall_sigint() -> None: proxmoxer.core.ResourceException, proxmoxer.backends.https.AuthenticationError, requests.exceptions.RequestException, + NodeUnavailableError, ) as api_error: logger.critical( - f"ProxLB: A Proxmox API call failed: {api_error}. " + f"ProxLB: A Proxmox API call failed or a cluster node became unavailable: {api_error}. " "Reconnecting to the Proxmox API and restarting balancing run from scratch." ) time.sleep(proxlb_config.proxmox_api.wait_time) diff --git a/proxlb/models/balancing.py b/proxlb/models/balancing.py index 42b6bbe..9958b59 100644 --- a/proxlb/models/balancing.py +++ b/proxlb/models/balancing.py @@ -12,6 +12,7 @@ import proxmoxer import time +from proxlb.utils.helper import Helper, NodeUnavailableError from proxlb.utils.logger import SystemdLogger from proxlb.utils.proxmox_api import ProxmoxApi from proxlb.utils.config_parser import Config @@ -93,7 +94,9 @@ def balance(proxmox_api: ProxmoxApi, proxlb_data: ProxLbData) -> bool: Keeps up to parallel_job_limit migrations in flight at once and immediately submits the next guest as soon as a slot becomes free, rather than waiting - for an entire chunk to finish. + for an entire chunk to finish. Before dispatching each guest, all cluster + nodes are re-checked for availability so a node that disappeared mid-run + aborts the run instead of migrating guests onto a stale target. Args: proxmox_api (ProxmoxApi): The Proxmox API client instance. @@ -101,6 +104,9 @@ def balance(proxmox_api: ProxmoxApi, proxlb_data: ProxLbData) -> bool: Returns: bool: True if all migrations completed successfully, False otherwise. + + Raises: + NodeUnavailableError: If a cluster node no longer responds to a status check. """ logger.debug("Starting: balance.") parallel_job_limit = Balancing.get_parallel_job_limit(proxlb_data.meta.balancing) @@ -118,6 +124,13 @@ def balance(proxmox_api: ProxmoxApi, proxlb_data: ProxLbData) -> bool: if len(jobs_to_wait) >= parallel_job_limit: time.sleep(5) + if not Helper.check_nodes_available(proxmox_api, proxlb_data.nodes): + logger.critical( + "Balancing: At least one cluster node is no longer reachable via the " + "Proxmox API. Aborting the current balancing run.") + raise NodeUnavailableError( + "At least one cluster node became unavailable during balancing.") + job_id = Balancing._exec_rebalancing(proxmox_api, proxlb_data, guest_name) logger.debug(f"Balancing: job_id for {guest_name}: {job_id!r}, jobs_to_wait len: {len(jobs_to_wait)}") if job_id is not None: diff --git a/proxlb/models/tests/test_balancing_streaming_queue.py b/proxlb/models/tests/test_balancing_streaming_queue.py index 9903040..63f8c09 100644 --- a/proxlb/models/tests/test_balancing_streaming_queue.py +++ b/proxlb/models/tests/test_balancing_streaming_queue.py @@ -14,8 +14,11 @@ from unittest.mock import MagicMock, patch +import pytest + from proxlb.models.balancing import Balancing from proxlb.utils.config_parser import Config +from proxlb.utils.helper import Helper, NodeUnavailableError from proxlb.utils.proxmox_api import ProxmoxApi GuestType = Config.GuestType @@ -55,6 +58,7 @@ def _proxlb_data( data.meta.balancing.with_local_disks = True data.meta.balancing.with_conntrack_state = True data.guests = guests + data.nodes = {} return data @@ -247,3 +251,23 @@ def test_failed_migration_returns_false( result = Balancing.balance(MagicMock(), proxlb_data) assert result is False + + +@patch("proxlb.models.balancing.time.sleep") +@patch.object(Balancing, "_exec_rebalancing_vm") +@patch.object(Helper, "check_nodes_available") +def test_balance_aborts_when_a_node_becomes_unavailable( + mock_check_nodes: MagicMock, mock_exec_vm: MagicMock, mock_sleep: MagicMock, +) -> None: + """balance() must raise NodeUnavailableError instead of migrating once a node drops out.""" + proxlb_data = _proxlb_data({ + "vm1": _guest(101, "node1", "node2"), + }) + proxlb_data.nodes = {"node1": MagicMock(), "node2": MagicMock()} + + mock_check_nodes.return_value = False + + with pytest.raises(NodeUnavailableError): + Balancing.balance(MagicMock(), proxlb_data) + + mock_exec_vm.assert_not_called() diff --git a/proxlb/utils/helper.py b/proxlb/utils/helper.py index 4f41a04..ce40bf7 100644 --- a/proxlb/utils/helper.py +++ b/proxlb/utils/helper.py @@ -14,19 +14,32 @@ import socket import sys import time +import proxmoxer +import requests from datetime import datetime, timedelta, time as datetime_time from proxlb.utils import version from proxlb.utils.config_parser import Config from proxlb.utils.logger import SystemdLogger from proxlb.utils.proxlb_data import ProxLbData -from typing import Dict, Tuple, Optional +from typing import Dict, Tuple, Optional, TYPE_CHECKING from types import FrameType +if TYPE_CHECKING: + from proxlb.utils.proxmox_api import ProxmoxApi + BalancingResource = Config.Balancing.Resource logger = SystemdLogger() +class NodeUnavailableError(Exception): + """ + Raised when a node that was previously discovered via Nodes.get_nodes() + no longer responds to a Proxmox API status check, indicating the ProxLB + run should be aborted and restarted from scratch. + """ + + class Helper: """ The Helper class provides some basic helper functions to not mess up the code in other @@ -405,3 +418,31 @@ def tcp_connect_test(addr_family: int, host: str, port: int, timeout: int) -> tu return (rc == 0, rc if rc != 0 else None) finally: test_socket.close() + + @staticmethod + def check_nodes_available(proxmox_api: "ProxmoxApi", nodes: Dict[str, ProxLbData.Node]) -> bool: + """ + Validates that all previously discovered cluster nodes are still reachable + via the Proxmox API by querying each node's status endpoint (nodes/{node}/status). + + Args: + proxmox_api (ProxmoxApi): The Proxmox API client instance. + nodes (Dict[str, ProxLbData.Node]): The nodes collected at the start of the run. + + Returns: + bool: True if every node responded successfully, False if any node is unreachable. + """ + logger.debug("Starting: check_nodes_available.") + + for node_name in nodes: + try: + proxmox_api.nodes(node_name).status.get() + except (proxmoxer.core.ResourceException, requests.exceptions.RequestException) as node_error: + logger.warning( + f"Helper: Node {node_name} did not respond to a status check " + f"and may no longer be available: {node_error}") + logger.debug("Finished: check_nodes_available.") + return False + + logger.debug("Finished: check_nodes_available.") + return True diff --git a/tests/test_check_nodes_available.py b/tests/test_check_nodes_available.py new file mode 100644 index 0000000..6f70237 --- /dev/null +++ b/tests/test_check_nodes_available.py @@ -0,0 +1,55 @@ +from unittest.mock import MagicMock + +import proxmoxer +import requests + +from proxlb.utils.helper import Helper + + +def _proxmox_api(node_errors: dict[str, Exception] | None = None) -> MagicMock: + node_errors = node_errors or {} + api = MagicMock() + + def _node(node_name: str) -> MagicMock: + node = MagicMock() + error = node_errors.get(node_name) + if error is not None: + node.status.get.side_effect = error + return node + + api.nodes.side_effect = _node + return api + + +def test_check_nodes_available_returns_true_when_all_nodes_respond() -> None: + proxmox_api = _proxmox_api() + + result = Helper.check_nodes_available(proxmox_api, {"node1": MagicMock(), "node2": MagicMock()}) + + assert result is True + + +def test_check_nodes_available_returns_false_on_connection_error() -> None: + proxmox_api = _proxmox_api({"node2": requests.exceptions.ConnectionError("unreachable")}) + + result = Helper.check_nodes_available(proxmox_api, {"node1": MagicMock(), "node2": MagicMock()}) + + assert result is False + + +def test_check_nodes_available_returns_false_on_proxmox_resource_exception() -> None: + proxmox_api = _proxmox_api({ + "node1": proxmoxer.core.ResourceException(500, "Internal Server Error", "node gone"), + }) + + result = Helper.check_nodes_available(proxmox_api, {"node1": MagicMock()}) + + assert result is False + + +def test_check_nodes_available_returns_true_for_empty_node_set() -> None: + proxmox_api = _proxmox_api() + + result = Helper.check_nodes_available(proxmox_api, {}) + + assert result is True From 6b92d627dcf0794b04574e1be99bbf11c168899d Mon Sep 17 00:00:00 2001 From: Florian Paul Azim Hoberg Date: Thu, 17 Sep 2026 14:41:47 +0200 Subject: [PATCH 3/4] Adjust to review remarks --- proxlb/__main__.py | 1 + proxlb/models/balancing.py | 21 ++++-- .../tests/test_balancing_streaming_queue.py | 64 +++++++++++++++++++ proxlb/utils/helper.py | 38 ++++++++--- tests/test_check_nodes_available.py | 50 ++++++++++----- 5 files changed, 140 insertions(+), 34 deletions(-) diff --git a/proxlb/__main__.py b/proxlb/__main__.py index d2bd9f3..b307efb 100644 --- a/proxlb/__main__.py +++ b/proxlb/__main__.py @@ -219,6 +219,7 @@ def reinstall_sigint() -> None: proxmox_api = ProxmoxApi(proxlb_config) proxlb_config.proxmox_api.password = "********" proxlb_config.proxmox_api.token_secret = "********" + Helper.get_daemon_mode(proxlb_config) continue # Validate if the JSON output should be diff --git a/proxlb/models/balancing.py b/proxlb/models/balancing.py index 9958b59..a4e8f1a 100644 --- a/proxlb/models/balancing.py +++ b/proxlb/models/balancing.py @@ -94,9 +94,11 @@ def balance(proxmox_api: ProxmoxApi, proxlb_data: ProxLbData) -> bool: Keeps up to parallel_job_limit migrations in flight at once and immediately submits the next guest as soon as a slot becomes free, rather than waiting - for an entire chunk to finish. Before dispatching each guest, all cluster - nodes are re-checked for availability so a node that disappeared mid-run - aborts the run instead of migrating guests onto a stale target. + for an entire chunk to finish. Before dispatching a guest that will actually + be migrated, all cluster nodes are re-checked for availability so a node + that disappeared mid-run aborts the run instead of migrating guests onto a + stale target. Guests that would not be migrated anyway (already on their + target node, ignored, or of a type excluded from balancing) skip this check. Args: proxmox_api (ProxmoxApi): The Proxmox API client instance. @@ -106,7 +108,7 @@ def balance(proxmox_api: ProxmoxApi, proxlb_data: ProxLbData) -> bool: bool: True if all migrations completed successfully, False otherwise. Raises: - NodeUnavailableError: If a cluster node no longer responds to a status check. + NodeUnavailableError: If a cluster node is no longer online. """ logger.debug("Starting: balance.") parallel_job_limit = Balancing.get_parallel_job_limit(proxlb_data.meta.balancing) @@ -124,10 +126,15 @@ def balance(proxmox_api: ProxmoxApi, proxlb_data: ProxLbData) -> bool: if len(jobs_to_wait) >= parallel_job_limit: time.sleep(5) - if not Helper.check_nodes_available(proxmox_api, proxlb_data.nodes): + guest_will_move = ( + guest_meta.node_current != guest_meta.node_target + and not guest_meta.ignore + and guest_meta.type in proxlb_data.meta.balancing.balance_types) + + if guest_will_move and not Helper.check_nodes_available(proxmox_api, proxlb_data.nodes): logger.critical( - "Balancing: At least one cluster node is no longer reachable via the " - "Proxmox API. Aborting the current balancing run.") + "Balancing: At least one cluster node is no longer online. " + "Aborting the current balancing run.") raise NodeUnavailableError( "At least one cluster node became unavailable during balancing.") diff --git a/proxlb/models/tests/test_balancing_streaming_queue.py b/proxlb/models/tests/test_balancing_streaming_queue.py index 63f8c09..91fdb14 100644 --- a/proxlb/models/tests/test_balancing_streaming_queue.py +++ b/proxlb/models/tests/test_balancing_streaming_queue.py @@ -99,6 +99,70 @@ def test_no_migration_when_guests_are_ignored( mock_exec_vm.assert_not_called() +@patch("proxlb.models.balancing.time.sleep") +@patch.object(Balancing, "_get_rebalancing_job_status") +@patch.object(Balancing, "_exec_rebalancing_vm") +@patch.object(Helper, "check_nodes_available") +def test_check_nodes_available_skipped_when_guests_already_on_target( + mock_check_nodes: MagicMock, mock_exec_vm: MagicMock, mock_get_status: MagicMock, mock_sleep: MagicMock, +) -> None: + """Guests that are already on their target node must not trigger a node availability check.""" + proxlb_data = _proxlb_data({ + "vm1": _guest(101, "node1", "node1"), + "vm2": _guest(102, "node2", "node2"), + }) + proxlb_data.nodes = {"node1": MagicMock(), "node2": MagicMock()} + + result = Balancing.balance(MagicMock(), proxlb_data) + + assert result is True + mock_check_nodes.assert_not_called() + mock_exec_vm.assert_not_called() + + +@patch("proxlb.models.balancing.time.sleep") +@patch.object(Balancing, "_get_rebalancing_job_status") +@patch.object(Balancing, "_exec_rebalancing_vm") +@patch.object(Helper, "check_nodes_available") +def test_check_nodes_available_skipped_when_guests_are_ignored( + mock_check_nodes: MagicMock, mock_exec_vm: MagicMock, mock_get_status: MagicMock, mock_sleep: MagicMock, +) -> None: + """Ignored guests must not trigger a node availability check even when their target node differs.""" + proxlb_data = _proxlb_data({ + "vm1": _guest(101, "node1", "node2", ignore=True), + }) + proxlb_data.nodes = {"node1": MagicMock(), "node2": MagicMock()} + + result = Balancing.balance(MagicMock(), proxlb_data) + + assert result is True + mock_check_nodes.assert_not_called() + mock_exec_vm.assert_not_called() + + +@patch("proxlb.models.balancing.time.sleep") +@patch.object(Balancing, "_get_rebalancing_job_status") +@patch.object(Balancing, "_exec_rebalancing_vm") +@patch.object(Helper, "check_nodes_available") +def test_check_nodes_available_called_when_guest_will_move( + mock_check_nodes: MagicMock, mock_exec_vm: MagicMock, mock_get_status: MagicMock, mock_sleep: MagicMock, +) -> None: + """A guest that will actually be migrated must trigger a node availability check.""" + proxlb_data = _proxlb_data({ + "vm1": _guest(101, "node1", "node2"), + }) + proxlb_data.nodes = {"node1": MagicMock(), "node2": MagicMock()} + + mock_check_nodes.return_value = True + mock_exec_vm.return_value = "job-vm1" + mock_get_status.return_value = Balancing.BalancingStatus.FINISHED + + result = Balancing.balance(MagicMock(), proxlb_data) + + assert result is True + mock_check_nodes.assert_called_once_with(mock_check_nodes.call_args.args[0], proxlb_data.nodes) + + @patch("proxlb.models.balancing.time.sleep") @patch.object(Balancing, "_get_rebalancing_job_status") @patch.object(Balancing, "_exec_rebalancing_vm") diff --git a/proxlb/utils/helper.py b/proxlb/utils/helper.py index ce40bf7..bc1f932 100644 --- a/proxlb/utils/helper.py +++ b/proxlb/utils/helper.py @@ -35,8 +35,8 @@ class NodeUnavailableError(Exception): """ Raised when a node that was previously discovered via Nodes.get_nodes() - no longer responds to a Proxmox API status check, indicating the ProxLB - run should be aborted and restarted from scratch. + is no longer online in the cluster, indicating the ProxLB run should be + aborted and restarted from scratch. """ @@ -422,25 +422,43 @@ def tcp_connect_test(addr_family: int, host: str, port: int, timeout: int) -> tu @staticmethod def check_nodes_available(proxmox_api: "ProxmoxApi", nodes: Dict[str, ProxLbData.Node]) -> bool: """ - Validates that all previously discovered cluster nodes are still reachable - via the Proxmox API by querying each node's status endpoint (nodes/{node}/status). + Validates that a balancing decision made at the start of a run is still + useful by checking that all previously discovered cluster nodes are still + online, via a single call to the cluster status endpoint (cluster/status). + + API connection reachability itself is already handled by ProxmoxApi and + is not what this check is for. Args: proxmox_api (ProxmoxApi): The Proxmox API client instance. nodes (Dict[str, ProxLbData.Node]): The nodes collected at the start of the run. Returns: - bool: True if every node responded successfully, False if any node is unreachable. + bool: True if every previously discovered node is still online, False otherwise. """ logger.debug("Starting: check_nodes_available.") + if not nodes: + logger.debug("Finished: check_nodes_available.") + return True + + try: + cluster_status = proxmox_api.cluster.status.get() + except (proxmoxer.core.ResourceException, requests.exceptions.RequestException) as cluster_error: + logger.warning(f"Helper: Failed to query cluster status: {cluster_error}") + logger.debug("Finished: check_nodes_available.") + return False + + online_nodes = { + entry["name"] for entry in cluster_status + if entry.get("type") == "node" and entry.get("online") + } + for node_name in nodes: - try: - proxmox_api.nodes(node_name).status.get() - except (proxmoxer.core.ResourceException, requests.exceptions.RequestException) as node_error: + if node_name not in online_nodes: logger.warning( - f"Helper: Node {node_name} did not respond to a status check " - f"and may no longer be available: {node_error}") + f"Helper: Node {node_name} is no longer online in the cluster " + "and may no longer be available.") logger.debug("Finished: check_nodes_available.") return False diff --git a/tests/test_check_nodes_available.py b/tests/test_check_nodes_available.py index 6f70237..a4ceeb5 100644 --- a/tests/test_check_nodes_available.py +++ b/tests/test_check_nodes_available.py @@ -6,31 +6,49 @@ from proxlb.utils.helper import Helper -def _proxmox_api(node_errors: dict[str, Exception] | None = None) -> MagicMock: - node_errors = node_errors or {} +def _proxmox_api(cluster_status: list[dict] | None = None, error: Exception | None = None) -> MagicMock: api = MagicMock() - - def _node(node_name: str) -> MagicMock: - node = MagicMock() - error = node_errors.get(node_name) - if error is not None: - node.status.get.side_effect = error - return node - - api.nodes.side_effect = _node + if error is not None: + api.cluster.status.get.side_effect = error + else: + api.cluster.status.get.return_value = cluster_status or [] return api -def test_check_nodes_available_returns_true_when_all_nodes_respond() -> None: - proxmox_api = _proxmox_api() +def test_check_nodes_available_returns_true_when_all_nodes_online() -> None: + proxmox_api = _proxmox_api([ + {"type": "node", "name": "node1", "online": 1}, + {"type": "node", "name": "node2", "online": 1}, + ]) result = Helper.check_nodes_available(proxmox_api, {"node1": MagicMock(), "node2": MagicMock()}) assert result is True +def test_check_nodes_available_returns_false_when_node_offline() -> None: + proxmox_api = _proxmox_api([ + {"type": "node", "name": "node1", "online": 1}, + {"type": "node", "name": "node2", "online": 0}, + ]) + + result = Helper.check_nodes_available(proxmox_api, {"node1": MagicMock(), "node2": MagicMock()}) + + assert result is False + + +def test_check_nodes_available_returns_false_when_node_missing_from_cluster_status() -> None: + proxmox_api = _proxmox_api([ + {"type": "node", "name": "node1", "online": 1}, + ]) + + result = Helper.check_nodes_available(proxmox_api, {"node1": MagicMock(), "node2": MagicMock()}) + + assert result is False + + def test_check_nodes_available_returns_false_on_connection_error() -> None: - proxmox_api = _proxmox_api({"node2": requests.exceptions.ConnectionError("unreachable")}) + proxmox_api = _proxmox_api(error=requests.exceptions.ConnectionError("unreachable")) result = Helper.check_nodes_available(proxmox_api, {"node1": MagicMock(), "node2": MagicMock()}) @@ -38,9 +56,7 @@ def test_check_nodes_available_returns_false_on_connection_error() -> None: def test_check_nodes_available_returns_false_on_proxmox_resource_exception() -> None: - proxmox_api = _proxmox_api({ - "node1": proxmoxer.core.ResourceException(500, "Internal Server Error", "node gone"), - }) + proxmox_api = _proxmox_api(error=proxmoxer.core.ResourceException(500, "Internal Server Error", "cluster gone")) result = Helper.check_nodes_available(proxmox_api, {"node1": MagicMock()}) From ce5be95ac551086ea4bf623d42a8caeb0c41f8e6 Mon Sep 17 00:00:00 2001 From: Florian Paul Azim Hoberg Date: Thu, 17 Sep 2026 14:45:34 +0200 Subject: [PATCH 4/4] fix unit test --- tests/test_check_nodes_available.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_check_nodes_available.py b/tests/test_check_nodes_available.py index a4ceeb5..8a09f01 100644 --- a/tests/test_check_nodes_available.py +++ b/tests/test_check_nodes_available.py @@ -6,7 +6,7 @@ from proxlb.utils.helper import Helper -def _proxmox_api(cluster_status: list[dict] | None = None, error: Exception | None = None) -> MagicMock: +def _proxmox_api(cluster_status: list[dict[str, object]] | None = None, error: Exception | None = None) -> MagicMock: api = MagicMock() if error is not None: api.cluster.status.get.side_effect = error