Merge ~ack/maas:deploy-ip-check-exclude-addresses into maas:master

[Alberto Donato]

LP: #1902425 - exclude already tested IPs when claiming IPs during deploy

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.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b deploy-ip-check-exclude-addresses lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/9183/console
COMMIT: 30015b95cfbbbc7392b069966ae12220e8bd3338

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b deploy-ip-check-exclude-addresses lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 6384b92fa39a4a0e2cbe6349d4af5ab54a2c4694

review: Approve

[Björn Tillenius (bjornt)]

+1

review: Approve

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 105ac72..16af317 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -4335,9 +4335,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
@@ -4500,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:
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index e63ffa5..d7dbcec 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -8664,6 +8664,71 @@ 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))
+
     def test_claims_auto_ip_addresses_retries_on_failure_from_rack(self):
         user = factory.make_User()
         node = self.make_acquired_node_with_interface(