Merge ~ack/maas:1902425-again-2.8 into maas:2.8

[Alberto Donato]

LP: #1902425 - always create neighbour entries for IPs in use.

Rework Interface.update_neighbour move check for neighbour discovery to call sites.

This also changes IP claim checks to always create a neighbour entry when the IP is found to be in use (even if the rack interface doesn't have neighbour discovery enabled).

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1902425-again-2.8 lp:~ack/maas/+git/maas into -b 2.8 lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/9590/console
COMMIT: be9256f379f35aa6ccc1512f0033ca2dce511d2c

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1902425-again-2.8 lp:~ack/maas/+git/maas into -b 2.8 lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 54a6395ec631e1b8612b27a986bc7f07436ea9b6

review: Approve

diff --git a/src/maasserver/models/interface.py b/src/maasserver/models/interface.py
index 6415344..de96c89 100644
--- a/src/maasserver/models/interface.py
+++ b/src/maasserver/models/interface.py
@@ -111,7 +111,7 @@ class InterfaceQueriesMixin(MAASQueriesMixin):
             specifiers,
             specifier_types=specifier_types,
             separator=separator,
-            **kwargs
+            **kwargs,
         )
 
     def _add_interface_id_query(self, current_q, op, item):
@@ -1522,20 +1522,10 @@ class Interface(CleanSave, TimestampedModel):
                     % (self.get_log_string(), vid, vlan.fabric.get_name())
                 )
 
-    def update_neighbour(self, neighbour_json: dict):
-        """Updates the neighbour table for this interface.
-
-        Input is expected to be the neighbour JSON from the controller.
-        """
-        # Circular imports
+    def update_neighbour(self, ip, mac, time, vid=None):
+        """Updates the neighbour table for this interface."""
         from maasserver.models.neighbour import Neighbour
 
-        if self.neighbour_discovery_state is False:
-            return None
-        ip = neighbour_json["ip"]
-        mac = neighbour_json["mac"]
-        time = neighbour_json["time"]
-        vid = neighbour_json.get("vid", None)
         deleted = Neighbour.objects.delete_and_log_obsolete_neighbours(
             ip, mac, interface=self, vid=vid
         )
@@ -1552,13 +1542,10 @@ class Interface(CleanSave, TimestampedModel):
             # generated a log statement about this neighbour.
             if not deleted:
                 maaslog.info(
-                    "%s: New MAC, IP binding observed%s: %s, %s"
-                    % (
-                        self.get_log_string(),
-                        Neighbour.objects.get_vid_log_snippet(vid),
-                        mac,
-                        ip,
-                    )
+                    f"{self.get_log_string()}: "
+                    "New MAC, IP binding "
+                    f"observed{Neighbour.objects.get_vid_log_snippet(vid)}: "
+                    f"{mac}, {ip}"
                 )
         else:
             neighbour.time = time
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 3c521be..c8df076 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -4413,11 +4413,9 @@ class Node(CleanSave, TimestampedModel):
                             rack_interface = rack_interface.order_by("id")
                             rack_interface = rack_interface.first()
                             rack_interface.update_neighbour(
-                                {
-                                    "ip": ip_obj.ip,
-                                    "mac": ip_result.get("mac_address"),
-                                    "time": time.time(),
-                                }
+                                ip_obj.ip,
+                                ip_result.get("mac_address"),
+                                time.time(),
                             )
                             ip_obj.ip = None
                             ip_obj.temp_expires_on = None
@@ -6728,8 +6726,14 @@ class Controller(Node):
         for neighbour in neighbours:
             interface = interfaces.get(neighbour["interface"], None)
             if interface is not None:
-                interface.update_neighbour(neighbour)
                 vid = neighbour.get("vid", None)
+                if interface.neighbour_discovery_state:
+                    interface.update_neighbour(
+                        neighbour["ip"],
+                        neighbour["mac"],
+                        neighbour["time"],
+                        vid=vid,
+                    )
                 if vid is not None:
                     interface.report_vid(vid)
 
diff --git a/src/maasserver/models/tests/test_interface.py b/src/maasserver/models/tests/test_interface.py
index 7c9ee39..71a5553 100644
--- a/src/maasserver/models/tests/test_interface.py
+++ b/src/maasserver/models/tests/test_interface.py
@@ -1364,7 +1364,7 @@ class InterfaceTest(MAASServerTestCase):
         self.assertEquals(0, interface.link_speed)
 
 
-class InterfaceUpdateNeighbourTest(MAASServerTestCase):
+class TestInterfaceUpdateNeighbour(MAASServerTestCase):
     """Tests for `Interface.update_neighbour`."""
 
     def make_neighbour_json(self, ip=None, mac=None, time=None, **kwargs):
@@ -1385,22 +1385,15 @@ class InterfaceUpdateNeighbourTest(MAASServerTestCase):
                 vid = None
         return {"ip": ip, "mac": mac, "time": time, "vid": vid}
 
-    def test_ignores_updates_if_neighbour_discovery_state_is_false(self):
+    def test_adds_new_neighbour(self):
         iface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        iface.update_neighbour(self.make_neighbour_json())
-        self.assertThat(Neighbour.objects.count(), Equals(0))
-
-    def test_adds_new_neighbour_if_neighbour_discovery_state_is_true(self):
-        iface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        iface.neighbour_discovery_state = True
-        iface.update_neighbour(self.make_neighbour_json())
+        iface.update_neighbour(**self.make_neighbour_json())
         self.assertThat(Neighbour.objects.count(), Equals(1))
 
     def test_updates_existing_neighbour(self):
         iface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        iface.neighbour_discovery_state = True
         json = self.make_neighbour_json()
-        iface.update_neighbour(json)
+        iface.update_neighbour(**json)
         neighbour = get_one(Neighbour.objects.all())
         # Pretend this was updated one day ago.
         yesterday = datetime.datetime.now() - datetime.timedelta(days=1)
@@ -1411,7 +1404,7 @@ class InterfaceUpdateNeighbourTest(MAASServerTestCase):
             Equals(int(yesterday.timestamp())),
         )
         json["time"] += 1
-        iface.update_neighbour(json)
+        iface.update_neighbour(**json)
         neighbour = reload_object(neighbour)
         self.assertThat(Neighbour.objects.count(), Equals(1))
         self.assertThat(neighbour.time, Equals(json["time"]))
@@ -1423,13 +1416,12 @@ class InterfaceUpdateNeighbourTest(MAASServerTestCase):
 
     def test_replaces_obsolete_neighbour(self):
         iface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        iface.neighbour_discovery_state = True
         json = self.make_neighbour_json()
-        iface.update_neighbour(json)
+        iface.update_neighbour(**json)
         # Have a different MAC address claim ownership of the IP.
         json["time"] += 1
         json["mac"] = factory.make_mac_address()
-        iface.update_neighbour(json)
+        iface.update_neighbour(**json)
         self.assertThat(Neighbour.objects.count(), Equals(1))
         self.assertThat(
             list(Neighbour.objects.all())[0].mac_address, Equals(json["mac"])
@@ -1440,10 +1432,8 @@ class InterfaceUpdateNeighbourTest(MAASServerTestCase):
 
     def test_logs_new_binding(self):
         iface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        iface.neighbour_discovery_state = True
-        json = self.make_neighbour_json()
         with FakeLogger("maas.interface") as maaslog:
-            iface.update_neighbour(json)
+            iface.update_neighbour(**self.make_neighbour_json())
         self.assertDocTestMatches(
             "...: New MAC, IP binding observed...", maaslog.output
         )
@@ -1452,18 +1442,18 @@ class InterfaceUpdateNeighbourTest(MAASServerTestCase):
         iface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         iface.neighbour_discovery_state = True
         json = self.make_neighbour_json()
-        iface.update_neighbour(json)
+        iface.update_neighbour(**json)
         # Have a different MAC address claim ownership of the IP.
         json["time"] += 1
         json["mac"] = factory.make_mac_address()
         with FakeLogger("maas.neighbour") as maaslog:
-            iface.update_neighbour(json)
+            iface.update_neighbour(**json)
         self.assertDocTestMatches(
             "...: IP address...moved from...to...", maaslog.output
         )
 
 
-class InterfaceUpdateMDNSEntryTest(MAASServerTestCase):
+class TestInterfaceUpdateMDNSEntry(MAASServerTestCase):
     """Tests for `Interface.update_mdns_entry`."""
 
     def make_mdns_entry_json(self, ip=None, hostname=None):
@@ -1478,7 +1468,7 @@ class InterfaceUpdateMDNSEntryTest(MAASServerTestCase):
 
     def test_ignores_updates_if_mdns_discovery_state_is_false(self):
         iface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        iface.update_neighbour(self.make_mdns_entry_json())
+        iface.update_mdns_entry(self.make_mdns_entry_json())
         self.assertThat(MDNS.objects.count(), Equals(0))
 
     def test_adds_new_entry_if_mdns_discovery_state_is_true(self):
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index b9ca303..583478c 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -80,6 +80,7 @@ from maasserver.exceptions import (
     StaticIPAddressExhaustion,
 )
 from maasserver.models import (
+    BMCRoutableRackControllerRelationship,
     BondInterface,
     BootResource,
     BridgeInterface,
@@ -87,11 +88,13 @@ from maasserver.models import (
     Controller,
     Device,
     Domain,
+    Event,
     EventType,
     Fabric,
     Interface,
     LicenseKey,
     Machine,
+    Neighbour,
     Node,
 )
 from maasserver.models import (
@@ -101,6 +104,7 @@ from maasserver.models import (
     RAID,
     RegionController,
     RegionRackRPCConnection,
+    ResourcePool,
     Service,
     StaticIPAddress,
     Subnet,
@@ -109,12 +113,10 @@ from maasserver.models import (
     VLANInterface,
     VolumeGroup,
 )
-from maasserver.models import Bcache
+from maasserver.models import Bcache, BMC
 from maasserver.models import bmc as bmc_module
 from maasserver.models import node as node_module
-from maasserver.models.bmc import BMC, BMCRoutableRackControllerRelationship
 from maasserver.models.config import NetworkDiscoveryConfig
-from maasserver.models.event import Event
 import maasserver.models.interface as interface_module
 from maasserver.models.node import (
     DefaultGateways,
@@ -123,7 +125,6 @@ from maasserver.models.node import (
     PowerInfo,
 )
 from maasserver.models.partitiontable import PARTITION_TABLE_EXTRA_SPACE
-from maasserver.models.resourcepool import ResourcePool
 from maasserver.models.signals import power as node_query
 from maasserver.models.timestampedmodel import now
 from maasserver.node_status import (
@@ -8593,6 +8594,74 @@ class TestNode_Start(MAASTransactionServerTestCase):
         self.assertThat(auto_ip.ip, Equals(third_ip.ip))
         self.assertThat(auto_ip.temp_expires_on, Is(None))
 
+    def test_claims_auto_ip_addresses_skips_used_ip_discovery_disabled(self):
+        user = factory.make_User()
+        node = self.make_acquired_node_with_interface(
+            user, power_type="manual"
+        )
+        node_interface = node.get_boot_interface()
+        [auto_ip] = node_interface.ip_addresses.filter(
+            alloc_type=IPADDRESS_TYPE.AUTO
+        )
+
+        # Create a rack controller that has an interface on the same subnet
+        # as the node. Don't enable neighbour discovery
+        rack = factory.make_RackController()
+        rack.interface_set.all().delete()
+        rackif = factory.make_Interface(vlan=node_interface.vlan, node=rack)
+        rackif_ip = factory.pick_ip_in_Subnet(auto_ip.subnet)
+        rackif.link_subnet(
+            INTERFACE_LINK_TYPE.STATIC, auto_ip.subnet, rackif_ip
+        )
+
+        # Mock the rack controller connected to the region controller.
+        client = Mock()
+        client.ident = rack.system_id
+        self.patch(node_module, "getAllClients").return_value = [client]
+
+        # Must be executed in a transaction as `allocate_new` uses savepoints.
+        with transaction.atomic():
+            # Get two IPs and remove them so they're unknown
+            ip = StaticIPAddress.objects.allocate_new(
+                subnet=auto_ip.subnet, alloc_type=IPADDRESS_TYPE.AUTO
+            )
+            ip1 = ip.ip
+            ip.delete()
+            ip = StaticIPAddress.objects.allocate_new(
+                subnet=auto_ip.subnet,
+                alloc_type=IPADDRESS_TYPE.AUTO,
+                exclude_addresses=[ip1],
+            )
+            ip2 = ip.ip
+            ip.delete()
+
+        client.side_effect = [
+            defer.succeed(
+                {
+                    "ip_addresses": [
+                        {
+                            "ip_address": ip1,
+                            "used": True,
+                            "mac_address": factory.make_mac_address(),
+                        }
+                    ]
+                }
+            ),
+            defer.succeed(
+                {"ip_addresses": [{"ip_address": ip2, "used": False}]}
+            ),
+        ]
+
+        with post_commit_hooks:
+            node.start(user)
+
+        auto_ip = reload_object(auto_ip)
+        self.assertThat(auto_ip.ip, Equals(ip2))
+        self.assertThat(auto_ip.temp_expires_on, Is(None))
+        self.assertCountEqual(
+            [ip1], Neighbour.objects.values_list("ip", flat=True)
+        )
+
     def test_claims_auto_ip_addresses_retries_on_failure_from_rack(self):
         user = factory.make_User()
         node = self.make_acquired_node_with_interface(
@@ -9790,21 +9859,51 @@ class TestControllerUpdateDiscoveryState(MAASServerTestCase):
 class TestReportNeighbours(MAASServerTestCase):
     """Tests for `Controller.report_neighbours()."""
 
-    def test_calls_update_neighbour_for_each_neighbour(self):
+    def test_no_update_neighbours_calls_if_discovery_disabled(self):
         rack = factory.make_RackController()
         factory.make_Interface(name="eth0", node=rack)
-        factory.make_Interface(name="eth1", node=rack)
         update_neighbour = self.patch(
             interface_module.Interface, "update_neighbour"
         )
         neighbours = [
-            {"interface": "eth0", "mac": factory.make_mac_address()},
-            {"interface": "eth1", "mac": factory.make_mac_address()},
+            {
+                "interface": "eth0",
+                "mac": factory.make_mac_address(),
+                "ip": factory.make_ipv4_address(),
+                "time": datetime.now(),
+            }
+        ]
+        rack.report_neighbours(neighbours)
+        update_neighbour.assert_not_called()
+
+    def test_calls_update_neighbour_for_each_neighbour(self):
+        rack = factory.make_RackController()
+        if1 = factory.make_Interface(name="eth0", node=rack)
+        if1.neighbour_discovery_state = True
+        if1.save()
+        if2 = factory.make_Interface(name="eth1", node=rack)
+        if2.neighbour_discovery_state = True
+        if2.save()
+        update_neighbour = self.patch(
+            interface_module.Interface, "update_neighbour"
+        )
+        neighbours = [
+            {
+                "interface": "eth0",
+                "mac": factory.make_mac_address(),
+                "ip": factory.make_ipv4_address(),
+                "time": datetime.now(),
+            },
+            {
+                "interface": "eth1",
+                "mac": factory.make_mac_address(),
+                "ip": factory.make_ipv4_address(),
+                "time": datetime.now(),
+            },
         ]
         rack.report_neighbours(neighbours)
-        self.assertThat(
-            update_neighbour,
-            MockCallsMatch(*[call(neighbour) for neighbour in neighbours]),
+        update_neighbour.assert_has_calls(
+            [call(n["ip"], n["mac"], n["time"], vid=None) for n in neighbours]
         )
 
     def test_calls_report_vid_for_each_vid(self):
@@ -9815,11 +9914,23 @@ class TestReportNeighbours(MAASServerTestCase):
         self.patch(interface_module.Interface, "update_neighbour")
         report_vid = self.patch(interface_module.Interface, "report_vid")
         neighbours = [
-            {"interface": "eth0", "mac": factory.make_mac_address(), "vid": 3},
-            {"interface": "eth1", "mac": factory.make_mac_address(), "vid": 7},
+            {
+                "interface": "eth0",
+                "ip": factory.make_ipv4_address(),
+                "time": datetime.now(),
+                "mac": factory.make_mac_address(),
+                "vid": 3,
+            },
+            {
+                "interface": "eth1",
+                "ip": factory.make_ipv4_address(),
+                "time": datetime.now(),
+                "mac": factory.make_mac_address(),
+                "vid": 7,
+            },
         ]
         rack.report_neighbours(neighbours)
-        self.assertThat(report_vid, MockCallsMatch(call(3), call(7)))
+        report_vid.assert_has_calls([call(3), call(7)])
 
 
 class TestReportMDNSEntries(MAASServerTestCase):