fix: resolve the node the connection landed on, not the first cluster member #2

Merged
christianmanivong merged 1 commits from fix/cluster-node-resolution into master 2026-09-29 08:44:25 +00:00
3 changed files with 110 additions and 19 deletions
+33 -19
View File
@@ -183,30 +183,44 @@ class ProxmoxDriver(
) from exc
def _resolve_node(self) -> str:
"""Resolve the node name from the hostname or optional_args."""
"""Resolve the PVE node this connection talks to.
In a cluster, ``GET /nodes`` lists every member, so its first entry is
just some node, not necessarily the one at ``self.hostname``. That used
to be taken blindly, and a device then reported another node's name and
VMs. ``GET /cluster/status`` marks the node the session landed on with
``local: 1``; failing that, the node is matched by IP or name.
API errors propagate so that ``open()`` fails with the real cause (e.g.
a TLS verification error) rather than succeeding on a guessed name.
"""
if self._node:
self._node_name = self._node
return self._node_name
# Try the node's hostname via API cluster/resources
try:
nodes = self._api.nodes.get()
# Match by hostname or IP
for n in nodes:
n_node = n.get("node", "")
if n_node:
# First match: the node exists in the cluster
self._node_name = n_node
return self._node_name
except Exception:
pass
members = [
s for s in (self._api.cluster.status.get() or []) if s.get("type") == "node"
]
short_host = self.hostname.split(".")[0]
for pick in (
lambda s: s.get("local"),
lambda s: s.get("ip") == self.hostname,
lambda s: s.get("name") in (self.hostname, short_host),
):
match = next((s for s in members if pick(s)), None)
if match:
self._node_name = match["name"]
return self._node_name
# Fallback: use the configured hostname as node name
# (may not match the PVE node name — SSH-based methods will fail,
# but API methods that target a specific node name require correct
# resolution)
self._node_name = self.hostname
return self._node_name
names = [n.get("node") for n in (self._api.nodes.get() or []) if n.get("node")]
if len(names) == 1:
self._node_name = names[0]
return self._node_name
raise ConnectionException(
f"Cannot tell which PVE node {self.hostname} is among {names}; "
f"set the 'node' driver argument"
)
def close(self) -> None:
"""Close the connection."""
+2
View File
@@ -138,6 +138,7 @@ def _build_mock_api(
pve_users=None,
exec_return="",
sensors=None,
cluster_status=None,
):
"""Build a MagicMock ProxmoxAPI with pre-configured return values."""
api = MagicMock()
@@ -165,6 +166,7 @@ def _build_mock_api(
# SDN
cluster = MagicMock()
api.cluster = cluster
cluster.status.get.return_value = cluster_status or []
cluster.sdn.zones.get.return_value = sdn_zones or SDN_ZONES
cluster.sdn.vnets.get.return_value = sdn_vnets or SDN_VNETS
+75
View File
@@ -76,3 +76,78 @@ class TestIsAlive:
def test_not_alive_when_api_fails(self, driver):
driver._api.version.get.side_effect = Exception("timeout")
assert driver.is_alive() == {"is_alive": False}
# A four-node cluster as GET /cluster/status reports it. The node the API
# session landed on carries local=1 — here the third one, so taking the first
# entry of /nodes (the old behaviour) picks the wrong host.
CLUSTER_NODES = [
{"type": "node", "name": "pve-01", "ip": "172.22.8.101", "local": 0},
{"type": "node", "name": "pve-02", "ip": "172.22.8.102", "local": 0},
{"type": "node", "name": "pve-dual", "ip": "172.22.8.120", "local": 1},
{"type": "node", "name": "pve-garden", "ip": "172.22.8.5", "local": 0},
]
CLUSTER_STATUS = [{"type": "cluster", "name": "home", "nodes": 4}, *CLUSTER_NODES]
CLUSTER_NODES_LIST = [{"node": n["name"], "status": "online"} for n in CLUSTER_NODES]
def _open_with(hostname, **api_kwargs):
drv = ProxmoxDriver(hostname, "root", "secret")
mock_api = _build_mock_api(**api_kwargs)
with patch("napalm_proxmox.driver.ProxmoxAPI", return_value=mock_api):
drv.open()
return drv
class TestClusterNodeResolution:
def test_local_node_wins_over_first_listed(self):
drv = _open_with(
"172.22.8.120", nodes=CLUSTER_NODES_LIST, cluster_status=CLUSTER_STATUS
)
assert drv._node_name == "pve-dual"
def test_matches_by_ip_without_local_flag(self):
status = [{**n, "local": 0} for n in CLUSTER_STATUS if n["type"] == "node"]
drv = _open_with("172.22.8.102", nodes=CLUSTER_NODES_LIST, cluster_status=status)
assert drv._node_name == "pve-02"
def test_matches_by_name_without_local_flag(self):
status = [{**n, "local": 0} for n in CLUSTER_STATUS if n["type"] == "node"]
drv = _open_with("pve-garden", nodes=CLUSTER_NODES_LIST, cluster_status=status)
assert drv._node_name == "pve-garden"
def test_matches_short_name_of_fqdn(self):
status = [{**n, "local": 0} for n in CLUSTER_STATUS if n["type"] == "node"]
drv = _open_with(
"pve-01.mgmt.example.com", nodes=CLUSTER_NODES_LIST, cluster_status=status
)
assert drv._node_name == "pve-01"
def test_single_node_without_cluster_status(self):
drv = _open_with("10.0.0.9", nodes=[{"node": "solo", "status": "online"}])
assert drv._node_name == "solo"
def test_ambiguous_cluster_refuses_to_guess(self):
status = [{**n, "local": 0} for n in CLUSTER_STATUS if n["type"] == "node"]
with pytest.raises(ConnectionException, match="node"):
_open_with("10.9.9.9", nodes=CLUSTER_NODES_LIST, cluster_status=status)
def test_api_error_fails_open_instead_of_guessing(self):
# A TLS failure used to be swallowed here: the IP became the node
# name, open() succeeded and every later call failed quietly.
drv = ProxmoxDriver("172.22.8.120", "root", "secret")
mock_api = _build_mock_api()
mock_api.cluster.status.get.side_effect = Exception("CERTIFICATE_VERIFY_FAILED")
mock_api.nodes.get.side_effect = Exception("CERTIFICATE_VERIFY_FAILED")
with patch("napalm_proxmox.driver.ProxmoxAPI", return_value=mock_api):
with pytest.raises(ConnectionException, match="CERTIFICATE_VERIFY_FAILED"):
drv.open()
def test_explicit_node_skips_lookup(self):
drv = ProxmoxDriver("172.22.8.120", "root", "secret", optional_args={"node": "pve-dual"})
mock_api = _build_mock_api()
with patch("napalm_proxmox.driver.ProxmoxAPI", return_value=mock_api):
drv.open()
assert drv._node_name == "pve-dual"
mock_api.cluster.status.get.assert_not_called()
mock_api.nodes.get.assert_not_called()