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

[Alberto Donato]

LP: #1902425

 - always create neighbour entries for IPs in use.
 - exclude already tested IPs when claiming IPs during deploy

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).

This fixes a corner case for the linked bug which happens when the used IP is
on an interface that has neighbour discovery disabled, in which case the IP is
not recorded in MAAS, and can get picked up again.

[Alberto Donato (ack)]

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b 1902425-again-2.9 lp:~ack/maas/+git/maas into -b 2.9 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/9239/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b 1902425-again-2.9 lp:~ack/maas/+git/maas into -b 2.9 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/9242/consoleText

[MAAS Lander (maas-lander)]

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

STATUS: SUCCESS
COMMIT: 7e2efe7305ffa8ad446f87b8d2410d246d8e2644

review: Approve

diff --git a/src/maasserver/models/interface.py b/src/maasserver/models/interface.py
index b7738dc..b5d9625 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 5d506b4..48852de 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -4337,9 +4337,11 @@ class Node(CleanSave, TimestampedModel):
                     bridge_fd=bridge_fd,
                 )
 
-    def claim_auto_ips(self, temp_expires_after=None):
+    def claim_auto_ips(self, exclude_addresses=None, temp_expires_after=None):
         """Assign IP addresses to all interface links set to AUTO."""
-        exclude_addresses = set()
+        exclude_addresses = (
+            exclude_addresses.copy() if exclude_addresses else set()
+        )
         allocated_ips = set()
         # Query for the interfaces again here; if we use the cached
         # interface_set, we could skip a newly-created bridge if it was created
@@ -4480,11 +4482,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
@@ -4502,6 +4502,7 @@ class Node(CleanSave, TimestampedModel):
             yield deferToDatabase(clean_expired)
             allocated_ips = yield deferToDatabase(
                 transactional(self.claim_auto_ips),
+                exclude_addresses=attempted_ips,
                 temp_expires_after=timedelta(minutes=5),
             )
             if not allocated_ips:
@@ -6723,8 +6724,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 b86dbfb..ee16f56 100644
--- a/src/maasserver/models/tests/test_interface.py
+++ b/src/maasserver/models/tests/test_interface.py
@@ -1363,7 +1363,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):
@@ -1384,22 +1384,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)
@@ -1410,7 +1403,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"]))
@@ -1422,13 +1415,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"])
@@ -1439,10 +1431,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
         )
@@ -1451,18 +1441,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):
@@ -1477,7 +1467,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 a5a5aa7..61f1fe6 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -79,6 +79,7 @@ from maasserver.exceptions import (
     StaticIPAddressExhaustion,
 )
 from maasserver.models import (
+    BMCRoutableRackControllerRelationship,
     BondInterface,
     BootResource,
     BridgeInterface,
@@ -86,11 +87,13 @@ from maasserver.models import (
     Controller,
     Device,
     Domain,
+    Event,
     EventType,
     Fabric,
     Interface,
     LicenseKey,
     Machine,
+    Neighbour,
     Node,
 )
 from maasserver.models import (
@@ -100,6 +103,7 @@ from maasserver.models import (
     RAID,
     RegionController,
     RegionRackRPCConnection,
+    ResourcePool,
     Service,
     StaticIPAddress,
     Subnet,
@@ -108,12 +112,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 (
     DEFAULT_BIOS_BOOT_METHOD,
@@ -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 (
@@ -8694,6 +8695,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(
@@ -9914,21 +9983,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):
@@ -9939,11 +10038,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):