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

[Mike Pontillo]

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

[Mike Pontillo (mpontillo)]

Self-approve backport.

review: Approve

diff --git a/src/maasserver/models/signals/interfaces.py b/src/maasserver/models/signals/interfaces.py
index b370b05..6e9fb33 100644
--- a/src/maasserver/models/signals/interfaces.py
+++ b/src/maasserver/models/signals/interfaces.py
@@ -52,6 +52,28 @@ signals = SignalsManager()
 log = LegacyLogger()
 
 
+class InterfaceVisitingThreadLocal(threading.local):
+    """Since infinite recursion could occur in an arbitrary interface
+    hierarchy, use thread-local storage to ensure that each interface is only
+    visited once.
+    """
+    def __init__(self):
+        super().__init__()
+        self.visiting = set()
+
+enabled_or_disabled_thread_local = InterfaceVisitingThreadLocal()
+
+
+def ensure_link_up(interface):
+    visiting = enabled_or_disabled_thread_local.visiting
+    if interface.id not in visiting:
+        try:
+            visiting.add(interface.id)
+            interface.ensure_link_up()
+        finally:
+            visiting.discard(interface.id)
+
+
 def interface_enabled_or_disabled(instance, old_values, **kwargs):
     """When an interface is enabled be sure at minimum a LINK_UP is created.
     When an interface is disabled make sure that all its links are removed,
@@ -62,9 +84,10 @@ def interface_enabled_or_disabled(instance, old_values, **kwargs):
         log.msg("%s: Physical interface enabled; ensuring link-up." % (
             instance.get_log_string()))
         # Make sure it has a LINK_UP link, and for its children.
-        instance.ensure_link_up()
+        ensure_link_up(instance)
         for rel in instance.children_relationships.all():
-            rel.child.ensure_link_up()
+            ensure_link_up(rel.child)
+
     else:
         log.msg("%s: Physical interface disabled; removing links." % (
             instance.get_log_string()))
@@ -157,16 +180,7 @@ for klass in INTERFACE_CLASSES:
         klass, ['params'], delete=False)
 
 
-class InterfaceUpdateParentsThreadLocal(threading.local):
-    """Since infinite recursion could occur in an arbitrary interface
-    hierarchy, use thread-local stroage to ensure that each interface is only
-    visited once.
-    """
-    def __init__(self):
-        super().__init__()
-        self.visiting = set()
-
-update_parents_thread_local = InterfaceUpdateParentsThreadLocal()
+update_parents_thread_local = InterfaceVisitingThreadLocal()
 
 
 def update_interface_parents(sender, instance, created, **kwargs):
diff --git a/src/maasserver/models/signals/tests/test_interfaces.py b/src/maasserver/models/signals/tests/test_interfaces.py
index d15235b..55d7a26 100644
--- a/src/maasserver/models/signals/tests/test_interfaces.py
+++ b/src/maasserver/models/signals/tests/test_interfaces.py
@@ -14,12 +14,18 @@ from maasserver.enum import (
     IPADDRESS_TYPE,
     NODE_TYPE,
 )
-from maasserver.models import Controller
+from maasserver.models import (
+    Controller,
+    Interface,
+)
 from maasserver.models.config import (
     Config,
     NetworkDiscoveryConfig,
 )
-from maasserver.models.signals.interfaces import update_parents_thread_local
+from maasserver.models.signals.interfaces import (
+    ensure_link_up,
+    update_parents_thread_local,
+)
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils.orm import reload_object
@@ -30,6 +36,11 @@ from testtools.matchers import (
 )
 
 
+def _mock_ensure_link_up(self):
+    """Mock method to test the 'visited' pattern for recursion prevention."""
+    ensure_link_up(self)
+
+
 class TestEnableAndDisableInterface(MAASServerTestCase):
 
     def test__enable_interface_creates_link_up(self):
@@ -52,6 +63,14 @@ class TestEnableAndDisableInterface(MAASServerTestCase):
             alloc_type=IPADDRESS_TYPE.STICKY, ip=None)
         self.assertIsNotNone(link_ip)
 
+    def test__ensure_link_up_only_called_once_per_interface(self):
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, enabled=False)
+        interface.enabled = True
+        self.patch(Interface, 'ensure_link_up', _mock_ensure_link_up)
+        # This will cause a RecursionError if the code doesn't work.
+        interface.save()
+
     def test__disable_interface_removes_links(self):
         interface = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, enabled=True)