Merge ~mpontillo/maas:interface-up-down-visited-recursion-prevent--2.4 into maas:2.4

Proposed by Mike Pontillo on 2018-07-05
Status: Merged
Approved by: Mike Pontillo on 2018-07-05
Approved revision: 9ae8d895f877b8888596e11613499c241336ea52
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~mpontillo/maas:interface-up-down-visited-recursion-prevent--2.4
Merge into: maas:2.4
Diff against target: 116 lines (+47/-14)
2 files modified
src/maasserver/models/signals/interfaces.py (+26/-12)
src/maasserver/models/signals/tests/test_interfaces.py (+21/-2)
Reviewer Review Type Date Requested Status
Mike Pontillo (community) Approve on 2018-07-05
Review via email: mp+349017@code.launchpad.net

Commit message

LP: #1622105 - Make sure ensure_link_up does not cause infinite recursion.

Add visited pattern to interface_enabled_or_disabled post-save signal handler for Interface model objects, to fix an infinite recursion bug seen during end-to-end testing while interfaces were being enabled and disabled on a rack controller.

Backports: 732f0300549322a49a02c4331d6cf2cb1f86af0d

To post a comment you must log in.
Mike Pontillo (mpontillo) wrote :

Self-approve backport.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/models/signals/interfaces.py b/src/maasserver/models/signals/interfaces.py
2index b370b05..6e9fb33 100644
3--- a/src/maasserver/models/signals/interfaces.py
4+++ b/src/maasserver/models/signals/interfaces.py
5@@ -52,6 +52,28 @@ signals = SignalsManager()
6 log = LegacyLogger()
7
8
9+class InterfaceVisitingThreadLocal(threading.local):
10+ """Since infinite recursion could occur in an arbitrary interface
11+ hierarchy, use thread-local storage to ensure that each interface is only
12+ visited once.
13+ """
14+ def __init__(self):
15+ super().__init__()
16+ self.visiting = set()
17+
18+enabled_or_disabled_thread_local = InterfaceVisitingThreadLocal()
19+
20+
21+def ensure_link_up(interface):
22+ visiting = enabled_or_disabled_thread_local.visiting
23+ if interface.id not in visiting:
24+ try:
25+ visiting.add(interface.id)
26+ interface.ensure_link_up()
27+ finally:
28+ visiting.discard(interface.id)
29+
30+
31 def interface_enabled_or_disabled(instance, old_values, **kwargs):
32 """When an interface is enabled be sure at minimum a LINK_UP is created.
33 When an interface is disabled make sure that all its links are removed,
34@@ -62,9 +84,10 @@ def interface_enabled_or_disabled(instance, old_values, **kwargs):
35 log.msg("%s: Physical interface enabled; ensuring link-up." % (
36 instance.get_log_string()))
37 # Make sure it has a LINK_UP link, and for its children.
38- instance.ensure_link_up()
39+ ensure_link_up(instance)
40 for rel in instance.children_relationships.all():
41- rel.child.ensure_link_up()
42+ ensure_link_up(rel.child)
43+
44 else:
45 log.msg("%s: Physical interface disabled; removing links." % (
46 instance.get_log_string()))
47@@ -157,16 +180,7 @@ for klass in INTERFACE_CLASSES:
48 klass, ['params'], delete=False)
49
50
51-class InterfaceUpdateParentsThreadLocal(threading.local):
52- """Since infinite recursion could occur in an arbitrary interface
53- hierarchy, use thread-local stroage to ensure that each interface is only
54- visited once.
55- """
56- def __init__(self):
57- super().__init__()
58- self.visiting = set()
59-
60-update_parents_thread_local = InterfaceUpdateParentsThreadLocal()
61+update_parents_thread_local = InterfaceVisitingThreadLocal()
62
63
64 def update_interface_parents(sender, instance, created, **kwargs):
65diff --git a/src/maasserver/models/signals/tests/test_interfaces.py b/src/maasserver/models/signals/tests/test_interfaces.py
66index d15235b..55d7a26 100644
67--- a/src/maasserver/models/signals/tests/test_interfaces.py
68+++ b/src/maasserver/models/signals/tests/test_interfaces.py
69@@ -14,12 +14,18 @@ from maasserver.enum import (
70 IPADDRESS_TYPE,
71 NODE_TYPE,
72 )
73-from maasserver.models import Controller
74+from maasserver.models import (
75+ Controller,
76+ Interface,
77+)
78 from maasserver.models.config import (
79 Config,
80 NetworkDiscoveryConfig,
81 )
82-from maasserver.models.signals.interfaces import update_parents_thread_local
83+from maasserver.models.signals.interfaces import (
84+ ensure_link_up,
85+ update_parents_thread_local,
86+)
87 from maasserver.testing.factory import factory
88 from maasserver.testing.testcase import MAASServerTestCase
89 from maasserver.utils.orm import reload_object
90@@ -30,6 +36,11 @@ from testtools.matchers import (
91 )
92
93
94+def _mock_ensure_link_up(self):
95+ """Mock method to test the 'visited' pattern for recursion prevention."""
96+ ensure_link_up(self)
97+
98+
99 class TestEnableAndDisableInterface(MAASServerTestCase):
100
101 def test__enable_interface_creates_link_up(self):
102@@ -52,6 +63,14 @@ class TestEnableAndDisableInterface(MAASServerTestCase):
103 alloc_type=IPADDRESS_TYPE.STICKY, ip=None)
104 self.assertIsNotNone(link_ip)
105
106+ def test__ensure_link_up_only_called_once_per_interface(self):
107+ interface = factory.make_Interface(
108+ INTERFACE_TYPE.PHYSICAL, enabled=False)
109+ interface.enabled = True
110+ self.patch(Interface, 'ensure_link_up', _mock_ensure_link_up)
111+ # This will cause a RecursionError if the code doesn't work.
112+ interface.save()
113+
114 def test__disable_interface_removes_links(self):
115 interface = factory.make_Interface(
116 INTERFACE_TYPE.PHYSICAL, enabled=True)

Subscribers

People subscribed via source and target branches

to all changes: