Skip to content

Commit 64462f6

Browse files
committed
fix(kea driver): fix broken retry logic in _make_request
The success return was outside the try/except but inside the attempt loop, so it always returned (or raised IndexError) after the first attempt regardless of success. Retries never actually ran, silently dropping reservations for ports that hit a transient request failure.
1 parent f47fbc4 commit 64462f6

2 files changed

Lines changed: 85 additions & 7 deletions

File tree

python/ironic-understack/ironic_understack/dhcp/kea.py

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ def _make_request(self, command, arguments, services=None):
4343
"arguments": arguments,
4444
}
4545

46+
last_exception = None
4647
for attempt in range(self.max_retries):
4748
results = []
4849
try:
@@ -58,26 +59,33 @@ def _make_request(self, command, arguments, services=None):
5859
)
5960
response.raise_for_status()
6061
results.append(response)
61-
except requests.exceptions.Timeout:
62+
return results[0].json()
63+
except requests.exceptions.Timeout as e:
64+
last_exception = e
6265
LOG.warning(
6366
"Timeout on attempt %d/%d for command %s",
6467
attempt + 1,
6568
self.max_retries,
6669
command,
6770
)
6871
except requests.exceptions.RequestException as e:
69-
if attempt == self.max_retries - 1:
70-
LOG.error("Failed to execute command %s: %s", command, e)
71-
raise DHCPConfigurationError(
72-
f"Failed to execute {command}: {e}"
73-
) from e
72+
last_exception = e
7473
LOG.warning(
7574
"Request failed on attempt %d/%d: %s",
7675
attempt + 1,
7776
self.max_retries,
7877
e,
7978
)
80-
return results[0].json()
79+
80+
LOG.error(
81+
"Failed to execute command %s after %d attempts: %s",
82+
command,
83+
self.max_retries,
84+
last_exception,
85+
)
86+
raise DHCPConfigurationError(
87+
f"Failed to execute {command}: {last_exception}"
88+
) from last_exception
8189

8290
def get_config(self):
8391
"""Retrieve current Kea configuration."""
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
from unittest.mock import Mock
2+
3+
import pytest
4+
import requests
5+
6+
from ironic_understack.dhcp.kea import DHCPConfigurationError
7+
from ironic_understack.dhcp.kea import KeaDHCPApi
8+
9+
10+
@pytest.fixture
11+
def kea(mocker):
12+
api = KeaDHCPApi()
13+
api.max_retries = 3
14+
mocker.patch.object(api, "_lookup_api_urls", return_value=["http://kea"])
15+
return api
16+
17+
18+
def _response(payload):
19+
response = Mock()
20+
response.raise_for_status.return_value = None
21+
response.json.return_value = payload
22+
return response
23+
24+
25+
def test_make_request_retries_after_transient_timeout(kea, mocker):
26+
post = mocker.patch(
27+
"ironic_understack.dhcp.kea.requests.post",
28+
side_effect=[requests.exceptions.Timeout(), _response({"result": 0})],
29+
)
30+
31+
result = kea._make_request("config-get", {})
32+
33+
assert result == {"result": 0}
34+
assert post.call_count == 2
35+
36+
37+
def test_make_request_raises_after_exhausting_retries(kea, mocker):
38+
post = mocker.patch(
39+
"ironic_understack.dhcp.kea.requests.post",
40+
side_effect=requests.exceptions.ConnectionError("boom"),
41+
)
42+
43+
with pytest.raises(DHCPConfigurationError):
44+
kea._make_request("config-get", {})
45+
46+
assert post.call_count == kea.max_retries
47+
48+
49+
def test_make_request_raises_after_exhausting_timeouts(kea, mocker):
50+
post = mocker.patch(
51+
"ironic_understack.dhcp.kea.requests.post",
52+
side_effect=requests.exceptions.Timeout(),
53+
)
54+
55+
with pytest.raises(DHCPConfigurationError):
56+
kea._make_request("config-get", {})
57+
58+
assert post.call_count == kea.max_retries
59+
60+
61+
def test_make_request_succeeds_on_first_attempt(kea, mocker):
62+
post = mocker.patch(
63+
"ironic_understack.dhcp.kea.requests.post",
64+
return_value=_response({"result": 0}),
65+
)
66+
67+
result = kea._make_request("config-get", {})
68+
69+
assert result == {"result": 0}
70+
assert post.call_count == 1

0 commit comments

Comments
 (0)