Skip to content

Commit f45acb7

Browse files
committed
fix: handle None state in StateMachine and None args in DowntimeCommand
- StateMachine.setState: guard against self.state being None before accessing self.states[self.state], preventing KeyError when the machine transitions from a valid state to None and back. - DowntimeCommand._prepareCommand: change key-existence checks to value checks (if not self.args.get("name")) so that None values from decisionParams defaults are also caught. - Add unit tests for both fixes and a new StateMachine test suite.
1 parent 0d3b11d commit f45acb7

5 files changed

Lines changed: 174 additions & 6 deletions

File tree

dirac-common/src/DIRACCommon/Core/Utilities/StateMachine.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,11 +129,11 @@ def setState(self, candidateState, noWarn=False, *, logger_warn=None):
129129
if not candidateState:
130130
self.state = candidateState
131131
elif candidateState in self.states:
132-
if not self.states[self.state].stateMap:
132+
if self.state is not None and not self.states[self.state].stateMap:
133133
if not noWarn and logger_warn:
134134
logger_warn("Final state, won't move", f"({self.state}, asked to move to {candidateState})")
135135
return S_OK(self.state)
136-
if candidateState not in self.states[self.state].stateMap and logger_warn:
136+
if self.state is not None and candidateState not in self.states[self.state].stateMap and logger_warn:
137137
logger_warn(f"Can't move from {self.state} to {candidateState}, choosing a good one")
138138
result = self.getNextState(candidateState)
139139
if not result["OK"]:
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
import pytest
2+
3+
from DIRACCommon.Core.Utilities.StateMachine import State, StateMachine
4+
5+
6+
class TestState:
7+
"""Tests for the State class"""
8+
9+
def test_state_basic(self):
10+
state = State(100)
11+
assert state.level == 100
12+
assert state.stateMap == []
13+
assert state.default is None
14+
15+
def test_state_with_transitions(self):
16+
state = State(0, ["State1", "State2"], defState="State1")
17+
assert state.level == 0
18+
assert state.stateMap == ["State1", "State2"]
19+
assert state.default == "State1"
20+
21+
def test_transition_rule_in_map(self):
22+
state = State(0, ["State1", "State2"], defState="State1")
23+
assert state.transitionRule("State2") == "State2"
24+
25+
def test_transition_rule_not_in_map_with_default(self):
26+
state = State(0, ["State1", "State2"], defState="State1")
27+
assert state.transitionRule("UnknownState") == "State1"
28+
29+
def test_transition_rule_not_in_map_without_default(self):
30+
state = State(0, ["State1", "State2"])
31+
assert state.transitionRule("UnknownState") == "UnknownState"
32+
33+
34+
class TestStateMachine:
35+
"""Tests for the StateMachine class"""
36+
37+
def test_get_level_of_state(self):
38+
sm = StateMachine()
39+
assert sm.getLevelOfState("Nirvana") == 100
40+
assert sm.getLevelOfState("UnknownState") == -1
41+
42+
def test_get_states(self):
43+
sm = StateMachine()
44+
assert sm.getStates() == ["Nirvana"]
45+
46+
def test_set_state_none_candidate(self):
47+
sm = StateMachine(state="Nirvana")
48+
result = sm.setState(None)
49+
assert result["OK"] is True
50+
assert result["Value"] is None
51+
52+
def test_set_state_same_state(self):
53+
sm = StateMachine(state="Nirvana")
54+
result = sm.setState("Nirvana")
55+
assert result["OK"] is True
56+
assert result["Value"] == "Nirvana"
57+
58+
def test_set_state_invalid_candidate(self):
59+
sm = StateMachine(state="Nirvana")
60+
result = sm.setState("InvalidState")
61+
assert result["OK"] is False
62+
63+
def test_set_state_from_none_to_valid(self):
64+
"""Test transitioning from None state to a valid state"""
65+
sm = StateMachine(state=None)
66+
result = sm.setState("Nirvana")
67+
assert result["OK"] is True
68+
assert result["Value"] == "Nirvana"
69+
70+
def test_set_state_to_none_then_to_valid(self):
71+
"""Test setting state to None and then to a valid state - reproduces KeyError bug"""
72+
sm = StateMachine(state="Nirvana")
73+
# First transition to None
74+
result = sm.setState(None)
75+
assert result["OK"] is True
76+
assert result["Value"] is None
77+
# Then transition to a valid state - this should not raise KeyError
78+
result = sm.setState("Nirvana")
79+
assert result["OK"] is True
80+
assert result["Value"] == "Nirvana"
81+
82+
def test_get_next_state_with_none_current(self):
83+
sm = StateMachine(state=None)
84+
result = sm.getNextState("Nirvana")
85+
assert result["OK"] is True
86+
assert result["Value"] == "Nirvana"
87+
88+
def test_get_next_state_invalid(self):
89+
sm = StateMachine(state="Nirvana")
90+
result = sm.getNextState("InvalidState")
91+
assert result["OK"] is False

src/DIRAC/ResourceStatusSystem/Command/DowntimeCommand.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -157,15 +157,15 @@ def _prepareCommand(self):
157157
:returns: S_OK tuple ``(element, elementName, hours, gOCDBServiceType)`` or S_ERROR.
158158
"""
159159

160-
if "name" not in self.args:
160+
if not self.args.get("name"):
161161
return S_ERROR('"name" not found in self.args')
162162
elementName = self.args["name"]
163163

164-
if "element" not in self.args:
164+
if not self.args.get("element"):
165165
return S_ERROR('"element" not found in self.args')
166166
element = self.args["element"]
167167

168-
if "elementType" not in self.args:
168+
if not self.args.get("elementType"):
169169
return S_ERROR('"elementType" not found in self.args')
170170
elementType = self.args["elementType"]
171171

src/DIRAC/ResourceStatusSystem/Command/test/Test_RSS_Command_GOCDBStatusCommand.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,9 @@ def test_doCache(mocker):
325325
[
326326
({"element": "X"}, None, False, None),
327327
({"element": "Site", "name": "aSite", "elementType": "Z"}, S_OK(), True, None),
328+
({"element": "Site", "name": None, "elementType": "Z"}, None, False, None),
329+
({"element": "Site", "name": "aSite", "elementType": None}, None, False, None),
330+
({"element": None, "name": "aSite", "elementType": "Z"}, None, False, None),
328331
(
329332
{"element": "Resource", "name": "669 devel.edu.mk", "elementType": "Z"},
330333
{

src/DIRAC/ResourceStatusSystem/PolicySystem/test/Test_PolicySystem.py

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
from unittest.mock import MagicMock
44

55
from DIRAC import gLogger
6+
from DIRAC.ResourceStatusSystem.PolicySystem.PDP import PDP
67
from DIRAC.ResourceStatusSystem.PolicySystem.PEP import PEP
78

89
# from DIRAC.ResourceStatusSystem.PolicySystem.PDP import PDP
@@ -274,20 +275,93 @@ def test_enforce(self):
274275
# # #############################################################################
275276
#
276277
# # class PolicyInvokerFailure(PolicySystemTestCase):
277-
#
278+
278279
# # def test_policyFail(self):
279280
# # for granularity in ValidRes:
280281
# # self.assertRaises(Exception, self.pi.evaluatePolicy)
281282

282283
#############################################################################
283284

285+
286+
class PDPCombinePoliciesTest(PolicySystemTestCase):
287+
"""Test cases for PDP._combineSinglePolicyResults, especially edge cases with None status"""
288+
289+
def test_combineSinglePolicyResults_with_none_status(self):
290+
"""Test that _combineSinglePolicyResults handles None status without raising KeyError.
291+
292+
This reproduces the bug where decisionParams['status'] is None, causing
293+
StateMachine.setState to raise KeyError when accessing self.states[None].
294+
"""
295+
pdp = PDP()
296+
pdp.setup(
297+
{
298+
"element": "Resource",
299+
"name": "testResource",
300+
"elementType": "ComputingElement",
301+
"statusType": "ReadAccess",
302+
"status": None, # This is the problematic case
303+
"reason": None,
304+
"tokenOwner": None,
305+
}
306+
)
307+
308+
singlePolicyResults = [{"Status": "Active", "Reason": "TestReason", "Policy": {"name": "TestPolicy"}}]
309+
310+
# This should not raise KeyError
311+
result = pdp._combineSinglePolicyResults(singlePolicyResults)
312+
self.assertTrue(result["OK"], f"Expected OK=True, got: {result}")
313+
314+
def test_combineSinglePolicyResults_with_valid_status(self):
315+
"""Test that _combineSinglePolicyResults works correctly with a valid status"""
316+
pdp = PDP()
317+
pdp.setup(
318+
{
319+
"element": "Resource",
320+
"name": "testResource",
321+
"elementType": "ComputingElement",
322+
"statusType": "ReadAccess",
323+
"status": "Active",
324+
"reason": None,
325+
"tokenOwner": None,
326+
}
327+
)
328+
329+
singlePolicyResults = [{"Status": "Active", "Reason": "TestReason", "Policy": {"name": "TestPolicy"}}]
330+
331+
result = pdp._combineSinglePolicyResults(singlePolicyResults)
332+
self.assertTrue(result["OK"], f"Expected OK=True, got: {result}")
333+
self.assertEqual(result["Value"]["Status"], "Active")
334+
335+
def test_combineSinglePolicyResults_empty_results(self):
336+
"""Test that _combineSinglePolicyResults handles empty results"""
337+
pdp = PDP()
338+
pdp.setup(
339+
{
340+
"element": "Resource",
341+
"name": "testResource",
342+
"elementType": "ComputingElement",
343+
"statusType": "ReadAccess",
344+
"status": None,
345+
"reason": None,
346+
"tokenOwner": None,
347+
}
348+
)
349+
350+
result = pdp._combineSinglePolicyResults([])
351+
self.assertTrue(result["OK"], f"Expected OK=True, got: {result}")
352+
self.assertEqual(result["Value"]["Status"], "Unknown")
353+
354+
355+
#############################################################################
356+
284357
if __name__ == "__main__":
285358
suite = unittest.defaultTestLoader.loadTestsFromTestCase(PolicySystemTestCase)
286359
# suite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(PolicyBaseSuccess))
287360
# suite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(PolicyBaseFailure))
288361
# suite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(PolicyInvokerSuccess))
289362
# suite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(PolicyInvokerFailure))
290363
suite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(PEPSuccess))
364+
suite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(PDPCombinePoliciesTest))
291365
# suite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(PEPFailure))
292366
# suite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(PDPSuccess))
293367
# suite.addTest(unittest.defaultTestLoader.loadTestsFromTestCase(PDPFailure))

0 commit comments

Comments
 (0)