Merge ~ack/maas:1902425-deploy-ip-allocate-check into maas:master

[Alberto Donato]

LP: #1902425 - keep trying new addresses when assigning on deploy.

Previously, the logic would retry only 2 times in case of any issue (call
error, IP in use).
If the call succeeds but the IP is in use, it should keep trying new addresses
until a free one is found.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1902425-deploy-ip-allocate-check 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/8693/console
COMMIT: 74b732eaf6d2e68cc6127d279f248ca19dc590a4

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1902425-deploy-ip-allocate-check 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/8694/console
COMMIT: a19d946759690cd77edc3377005d888839474d8f

review: Needs Fixing

[MAAS Lander (maas-lander)]

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

STATUS: SUCCESS
COMMIT: 5c8b3db135d6c74ee7c0222dbe54da382342b938

review: Approve

[Adam Collard (adam-collard)]

LGTM

review: Approve

diff --git a/src/maasserver/api/tests/test_machine.py b/src/maasserver/api/tests/test_machine.py
index dc89858..dd6495c 100644
--- a/src/maasserver/api/tests/test_machine.py
+++ b/src/maasserver/api/tests/test_machine.py
@@ -31,6 +31,7 @@ from maasserver.enum import (
     BRIDGE_TYPE,
     FILESYSTEM_FORMAT_TYPE_CHOICES,
     FILESYSTEM_TYPE,
+    INTERFACE_LINK_TYPE,
     INTERFACE_TYPE,
     IPADDRESS_TYPE,
     NODE_STATUS,
@@ -715,8 +716,9 @@ class TestMachineAPI(APITestCase.ForUser):
         ).return_value = [rack_controller]
         self.patch(node_module.Node, "_power_control_node")
         self.patch(node_module.Node, "_start_deployment")
+        self.patch(node_module.Node, "_claim_auto_ips")
         self.patch(machines_module, "get_curtin_merged_config")
-        machine = factory.make_Node(
+        machine = factory.make_Node_with_Interface_on_Subnet(
             owner=self.user,
             interface=True,
             power_type="virsh",
@@ -724,6 +726,22 @@ class TestMachineAPI(APITestCase.ForUser):
             status=NODE_STATUS.ALLOCATED,
             bmc_connected_to=rack_controller,
         )
+
+        # assign an IP to both the machine and the rack on the same subnet
+        machine_interface = machine.get_boot_interface()
+        [auto_ip] = machine_interface.ip_addresses.filter(
+            alloc_type=IPADDRESS_TYPE.AUTO
+        )
+        ip = factory.pick_ip_in_Subnet(auto_ip.subnet)
+        auto_ip.ip = ip
+        auto_ip.save()
+        rack_if = rack_controller.interface_set.first()
+        rack_if.link_subnet(
+            INTERFACE_LINK_TYPE.STATIC,
+            auto_ip.subnet,
+            factory.pick_ip_in_Subnet(auto_ip.subnet),
+        )
+
         osystem = make_usable_osystem(self)
         distro_series = osystem["default_release"]
         user_data = (
diff --git a/src/maasserver/exceptions.py b/src/maasserver/exceptions.py
index e449790..32bf622 100644
--- a/src/maasserver/exceptions.py
+++ b/src/maasserver/exceptions.py
@@ -141,6 +141,12 @@ class StaticIPAddressExhaustion(MAASAPIException):
     api_error = int(http.client.SERVICE_UNAVAILABLE)
 
 
+class IPAddressCheckFailed(MAASAPIException):
+    """IP address allocation checks failed."""
+
+    api_error = int(http.client.SERVICE_UNAVAILABLE)
+
+
 class StaticIPAddressUnavailable(MAASAPIException):
     """Raised when a requested IP is not available."""
 
diff --git a/src/maasserver/models/interface.py b/src/maasserver/models/interface.py
index 815a2a7..b7738dc 100644
--- a/src/maasserver/models/interface.py
+++ b/src/maasserver/models/interface.py
@@ -1539,13 +1539,15 @@ class Interface(CleanSave, TimestampedModel):
         deleted = Neighbour.objects.delete_and_log_obsolete_neighbours(
             ip, mac, interface=self, vid=vid
         )
-        neighbour = Neighbour.objects.get_current_binding(
-            ip, mac, interface=self, vid=vid
+
+        neighbour, created = Neighbour.objects.get_or_create(
+            defaults={"time": time},
+            ip=ip,
+            mac_address=mac,
+            vid=vid,
+            interface=self,
         )
-        if neighbour is None:
-            neighbour = Neighbour.objects.create(
-                interface=self, ip=ip, vid=vid, mac_address=mac, time=time
-            )
+        if created:
             # If we deleted a previous neighbour, then we have already
             # generated a log statement about this neighbour.
             if not deleted:
diff --git a/src/maasserver/models/neighbour.py b/src/maasserver/models/neighbour.py
index 49e4b13..90ad864 100644
--- a/src/maasserver/models/neighbour.py
+++ b/src/maasserver/models/neighbour.py
@@ -18,7 +18,7 @@ from maasserver.fields import MACAddressField
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.interface import Interface
 from maasserver.models.timestampedmodel import TimestampedModel
-from maasserver.utils.orm import get_one, MAASQueriesMixin, UniqueViolation
+from maasserver.utils.orm import MAASQueriesMixin
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.utils.network import get_mac_organization
 
@@ -111,29 +111,6 @@ class NeighbourManager(Manager, NeighbourQueriesMixin):
             deleted = True
         return deleted
 
-    def get_current_binding(self, ip: str, mac: str, interface: str, vid: int):
-        """Returns the current neighbour for the specified values.
-
-        Returns None if an object representing the specified IP, MAC,
-        Interface, and VID does not exist. (This is not an error condition;
-        it happens normally when the binding is created for the first time.)
-
-        The caller must ensure that any obsolete bindings are deleted before
-        calling this method.
-
-        :raises UniqueViolation: If more than one binding is found for the
-            specified interface, IP address, VID, and MAC address. (Which
-            should never happen due to the `unique_together`.)
-        """
-        query = self.filter(
-            interface=interface, ip=ip, vid=vid, mac_address=mac
-        )
-        # If we get an exception here, it is most likely due to an unlikely
-        # race condition. (either that, or the caller neglected to remove
-        # obsolete bindings before calling this method.) Therefore, raise
-        # a UniqueViolation so this operation can be retried.
-        return get_one(query, exception_class=UniqueViolation)
-
     def get_by_updated_with_related_nodes(self):
         """Returns a `QuerySet` of neighbours, while also selecting related
         interfaces and nodes.
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 54f3aab..bdaf5b3 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -105,6 +105,7 @@ from maasserver.enum import (
     SERVICE_STATUS,
 )
 from maasserver.exceptions import (
+    IPAddressCheckFailed,
     NodeStateViolation,
     NoScriptsFound,
     PowerProblem,
@@ -4352,26 +4353,28 @@ class Node(CleanSave, TimestampedModel):
                 allocated_ips.add(ip)
         return allocated_ips
 
-    def _claim_auto_ips(self, defer):
+    @inlineCallbacks
+    def _claim_auto_ips(self):
         """Perform claim AUTO IP addresses from the post_commit `defer`."""
 
         @transactional
         def clean_expired():
             """Clean the expired AUTO IP addresses."""
-            ips = StaticIPAddress.objects.filter(
+            StaticIPAddress.objects.filter(
                 temp_expires_on__lte=datetime.utcnow()
-            )
-            ips.update(ip=None, temp_expires_on=None)
+            ).update(ip=None, temp_expires_on=None)
+
+        # track which IPs have been already attempted for allocation
+        attempted_ips = set()
 
         @transactional
         def get_racks_to_check(allocated_ips):
             """Calculate the rack controllers to perform the IP check on."""
-            # No allocated ips.
             if not allocated_ips:
                 return None, None
 
             # Map the allocated IP addresses to the subnets they belong.
-            ip_ids = map(attrgetter("id"), allocated_ips)
+            ip_ids = [ip.id for ip in allocated_ips]
             ips_to_subnet = StaticIPAddress.objects.filter(id__in=ip_ids)
             subnets_to_ips = defaultdict(set)
             for ip in ips_to_subnet:
@@ -4398,10 +4401,9 @@ class Node(CleanSave, TimestampedModel):
 
             return subnets_to_ips, subnets_to_clients
 
-        def perform_ip_checks(result):
-            subnets_to_ips, subnets_to_clients = result
+        def perform_ip_checks(subnets_to_ips, subnets_to_clients):
             # Early-out if no IP addresses where allocated.
-            if subnets_to_ips is None:
+            if not subnets_to_ips:
                 return None
             defers = []
             for subnet_id, ips in subnets_to_ips.items():
@@ -4426,16 +4428,12 @@ class Node(CleanSave, TimestampedModel):
             return DeferredList(defers)
 
         @transactional
-        def process_results(results, try_count=0, max_try_count=2):
+        def process_results(results, do_cleanups):
             maaslog.debug(
                 f"Processing IP allocation results for {self.system_id}"
             )
-            # Early-out if no IP addresses where allocated.
-            if results is None:
-                return False, try_count, max_try_count
-            needs_retry = False
-            for _, result in results:
-                check_result, rack_id, ips = result
+            check_failed, ips_in_use = False, False
+            for _, (check_result, rack_id, ips) in results:
                 ip_ids = [ip.id for ip in ips]
                 ip_to_obj = {ip.ip: ip for ip in ips}
                 if check_result is None:
@@ -4446,33 +4444,19 @@ class Node(CleanSave, TimestampedModel):
                         temp_expires_on=None
                     )
                 elif isinstance(check_result, Failure):
+                    check_failed = True
                     # Failed to perform IP address checking on the rack
                     # controller.
-                    if try_count < max_try_count:
+                    static_ips = StaticIPAddress.objects.filter(id__in=ip_ids)
+                    if do_cleanups:
+                        # Clear the temp_expires_on marking the IP address as
+                        # assigned. Thsi was the last attempt to perform the
+                        # allocation.
+                        static_ips.update(temp_expires_on=None)
+                    else:
                         # Clear the assigned IP address so new IP can be
                         # assigned in the next pass.
-                        StaticIPAddress.objects.filter(id__in=ip_ids).update(
-                            ip=None, temp_expires_on=None
-                        )
-                        needs_retry = True
-                        log.err(
-                            check_result,
-                            "Performing IP address checking failed, "
-                            "will be retried.",
-                        )
-                    else:
-                        # Clear the temp_expires_on marking the IP address
-                        # as assigned. Enough tries where performed to verify
-                        # and it always failed.
-                        StaticIPAddress.objects.filter(id__in=ip_ids).update(
-                            temp_expires_on=None
-                        )
-                        log.err(
-                            check_result,
-                            "Performing IP address checking failed, "
-                            "will *NOT* be retried. (marking IP addresses "
-                            "as available).",
-                        )
+                        static_ips.update(ip=None, temp_expires_on=None)
                 else:
                     # IP address checking was successful, use the results
                     # to either mark the assigned IP address no longer
@@ -4480,6 +4464,7 @@ class Node(CleanSave, TimestampedModel):
                     for ip_result in check_result["ip_addresses"]:
                         ip_obj = ip_to_obj[ip_result["ip_address"]]
                         if ip_result["used"]:
+                            attempted_ips.add(ip_obj.ip)
                             # Create a Neighbour reference to the IP address
                             # so the next loop will not use that IP address.
                             rack_interface = Interface.objects.filter(
@@ -4498,57 +4483,53 @@ class Node(CleanSave, TimestampedModel):
                             ip_obj.ip = None
                             ip_obj.temp_expires_on = None
                             ip_obj.save()
-                            needs_retry = True
+                            ips_in_use = True
                         else:
                             # IP was free and offically assigned.
                             ip_obj.temp_expires_on = None
                             ip_obj.save()
-            return needs_retry, try_count, max_try_count
-
-        def retrier(result):
-            needs_retry, try_count, max_try_count = result
-            if needs_retry:
-                if try_count >= max_try_count:
-                    # Over retry count, IP address allocation is exhausted.
-                    raise StaticIPAddressExhaustion(
-                        "Failed to allocate the required AUTO IP addresses "
-                        "after %d retries." % max_try_count
-                    )
-                else:
-                    maaslog.debug("Retrying claiming of IPs.")
-                    # Re-loop the whole process again until max_try_count.
-                    d = succeed(None)
-                    d.addCallback(
-                        lambda _: deferToDatabase(
-                            transactional(self.claim_auto_ips),
-                            temp_expires_after=timedelta(minutes=5),
-                        )
-                    )
-                    d.addCallback(partial(deferToDatabase, get_racks_to_check))
-                    d.addCallback(perform_ip_checks)
-                    d.addCallback(
-                        partial(
-                            deferToDatabase,
-                            process_results,
-                            try_count=(try_count + 1),
-                        )
-                    )
-                    d.addCallback(retrier)
-                    return d
 
-        defer.addCallback(lambda _: deferToDatabase(clean_expired))
-        defer.addCallback(
-            lambda _: deferToDatabase(
+            return check_failed, ips_in_use
+
+        retry, max_retries = 0, 2
+        while retry <= max_retries:
+            yield deferToDatabase(clean_expired)
+            allocated_ips = yield deferToDatabase(
                 transactional(self.claim_auto_ips),
                 temp_expires_after=timedelta(minutes=5),
             )
-        )
-        defer.addCallback(partial(deferToDatabase, get_racks_to_check))
-        defer.addCallback(perform_ip_checks)
-        defer.addCallback(partial(deferToDatabase, process_results))
-        defer.addCallback(retrier)
+            # skip IPs that have been tested already
+            allocated_ips = set(
+                ip for ip in allocated_ips if ip.ip not in attempted_ips
+            )
+            if not allocated_ips:
+                raise StaticIPAddressExhaustion(
+                    "Failed to allocate the required AUTO IP addresses"
+                )
+            subnets_to_ips, subnets_to_clients = yield deferToDatabase(
+                get_racks_to_check, allocated_ips
+            )
+            results = yield perform_ip_checks(
+                subnets_to_ips, subnets_to_clients
+            )
+            if not results:
+                return
 
-        return defer
+            last_run = retry == max_retries
+            check_failed, ips_in_use = yield deferToDatabase(
+                process_results, results, last_run
+            )
+            if ips_in_use:
+                retry = 0
+            elif check_failed:
+                retry += 1
+            else:
+                return
+
+        # over the retry count, check failed
+        raise IPAddressCheckFailed(
+            f"IP address checks failed after {max_retries} retries."
+        )
 
     @transactional
     def release_interface_config(self):
@@ -5560,12 +5541,16 @@ class Node(CleanSave, TimestampedModel):
         d = post_commit()
         claimed_ips = False
 
+        @inlineCallbacks
+        def claim_auto_ips(_):
+            yield self._claim_auto_ips()
+
         if self.status == NODE_STATUS.ALLOCATED:
             old_status = self.status
             # Claim AUTO IP addresses for the node if it's ALLOCATED.
             # The current state being ALLOCATED is our indication that the node
             # is being deployed for the first time.
-            d = self._claim_auto_ips(d)
+            d.addCallback(claim_auto_ips)
             set_deployment_timeout = True
             self._start_deployment()
             claimed_ips = True
@@ -5584,7 +5569,7 @@ class Node(CleanSave, TimestampedModel):
                 ],
                 script__apply_configured_networking=True,
             ).exists():
-                d = self._claim_auto_ips(d)
+                d.addCallback(claim_auto_ips)
                 claimed_ips = True
             set_deployment_timeout = False
         elif self.status == NODE_STATUS.DEPLOYED and self.ephemeral_deployment:
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 1a52a58..c8a1b58 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -60,6 +60,7 @@ from maasserver.enum import (
     INTERFACE_LINK_TYPE,
     INTERFACE_TYPE,
     IPADDRESS_TYPE,
+    IPRANGE_TYPE,
     NODE_CREATION_TYPE,
     NODE_STATUS,
     NODE_STATUS_CHOICES,
@@ -71,6 +72,7 @@ from maasserver.enum import (
     SERVICE_STATUS,
 )
 from maasserver.exceptions import (
+    IPAddressCheckFailed,
     NodeStateViolation,
     PodProblem,
     PowerProblem,
@@ -215,6 +217,7 @@ from provisioningserver.utils import znums
 from provisioningserver.utils.enum import map_enum, map_enum_reverse
 from provisioningserver.utils.env import get_maas_id
 from provisioningserver.utils.fs import NamedLock
+from provisioningserver.utils.network import inet_ntop
 from provisioningserver.utils.testing import MAASIDFixture
 
 wait_for_reactor = wait_for(30)  # 30 seconds.
@@ -8565,7 +8568,7 @@ class TestNode_Start(MAASTransactionServerTestCase):
         claim_auto_ips = self.patch_autospec(node, "_claim_auto_ips")
         node.start(user)
 
-        self.expectThat(claim_auto_ips, MockCalledOnceWith(ANY))
+        self.expectThat(claim_auto_ips, MockCalledOnce())
 
     def test_only_claims_auto_addresses_when_allocated(self):
         user = factory.make_User()
@@ -8684,8 +8687,6 @@ class TestNode_Start(MAASTransactionServerTestCase):
             ),
         ]
 
-        # No rack controllers are currently connected to the test region
-        # so no IP address checking will be performed.
         with post_commit_hooks:
             node.start(user)
 
@@ -8728,8 +8729,6 @@ class TestNode_Start(MAASTransactionServerTestCase):
             first_ip.temp_expires_on = datetime.utcnow() - timedelta(minutes=5)
             first_ip.save()
 
-        # 2 tries will be made on the rack controller, both IP will be
-        # used.
         client.side_effect = [
             defer.fail(Exception()),
             defer.fail(Exception()),
@@ -8738,8 +8737,6 @@ class TestNode_Start(MAASTransactionServerTestCase):
             ),
         ]
 
-        # No rack controllers are currently connected to the test region
-        # so no IP address checking will be performed.
         with post_commit_hooks:
             node.start(user)
 
@@ -8747,7 +8744,7 @@ class TestNode_Start(MAASTransactionServerTestCase):
         self.assertThat(auto_ip.ip, Equals(first_ip.ip))
         self.assertThat(auto_ip.temp_expires_on, Is(None))
 
-    def test_claims_auto_ip_addresses_assigns_ip_on_three_failures(self):
+    def test_claims_auto_ip_addresses_fails_on_three_failures(self):
         user = factory.make_User()
         node = self.make_acquired_node_with_interface(
             user, power_type="manual"
@@ -8782,24 +8779,21 @@ class TestNode_Start(MAASTransactionServerTestCase):
             first_ip.temp_expires_on = datetime.utcnow() - timedelta(minutes=5)
             first_ip.save()
 
-        # 2 tries will be made on the rack controller, both IP will be
-        # used.
         client.side_effect = [
             defer.fail(Exception()),
             defer.fail(Exception()),
             defer.fail(Exception()),
         ]
 
-        # No rack controllers are currently connected to the test region
-        # so no IP address checking will be performed.
-        with post_commit_hooks:
-            node.start(user)
+        with ExpectedException(IPAddressCheckFailed):
+            with post_commit_hooks:
+                node.start(user)
 
         auto_ip = reload_object(auto_ip)
         self.assertThat(auto_ip.ip, Equals(first_ip.ip))
         self.assertThat(auto_ip.temp_expires_on, Is(None))
 
-    def test_claims_auto_ip_addresses_fails_after_three_tries(self):
+    def test_claims_auto_ip_addresses_eventually_succeds_with_many_used(self):
         user = factory.make_User()
         node = self.make_acquired_node_with_interface(
             user, power_type="manual"
@@ -8841,16 +8835,21 @@ class TestNode_Start(MAASTransactionServerTestCase):
                 exclude_addresses=[first_ip.ip],
             )
             second_ip.delete()
-
-            # This is the next IP address that will actaully get picked for the
-            # machine as it will be free from the database and will no be
-            # reported as used from the rack controller.
             third_ip = StaticIPAddress.objects.allocate_new(
                 subnet=auto_ip.subnet,
                 alloc_type=IPADDRESS_TYPE.AUTO,
                 exclude_addresses=[first_ip.ip, second_ip.ip],
             )
             third_ip.delete()
+            # This is the next IP address that will actaully get picked for the
+            # machine as it will be free from the database and will no be
+            # reported as used from the rack controller.
+            fourth_ip = StaticIPAddress.objects.allocate_new(
+                subnet=auto_ip.subnet,
+                alloc_type=IPADDRESS_TYPE.AUTO,
+                exclude_addresses=[first_ip.ip, second_ip.ip, third_ip.ip],
+            )
+            fourth_ip.delete()
 
         # 2 tries will be made on the rack controller, both IP will be
         # used.
@@ -8864,10 +8863,69 @@ class TestNode_Start(MAASTransactionServerTestCase):
             defer.succeed(
                 {"ip_addresses": [{"ip_address": third_ip.ip, "used": True}]}
             ),
+            defer.succeed(
+                {"ip_addresses": [{"ip_address": fourth_ip.ip, "used": False}]}
+            ),
         ]
 
-        # No rack controllers are currently connected to the test region
-        # so no IP address checking will be performed.
+        with post_commit_hooks:
+            node.start(user)
+
+        auto_ip = reload_object(auto_ip)
+        self.assertEqual(auto_ip.ip, fourth_ip.ip)
+        self.assertIsNone(auto_ip.temp_expires_on)
+
+    def test_claims_auto_ip_doesnt_retry_in_use(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.
+        rack = factory.make_RackController()
+        rack.interface_set.all().delete()
+        rackif = factory.make_Interface(vlan=node_interface.vlan, node=rack)
+        rackif.neighbour_discovery_state = True
+        rackif.save()
+        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():
+            used_ip = StaticIPAddress.objects.allocate_new(
+                subnet=auto_ip.subnet, alloc_type=IPADDRESS_TYPE.AUTO
+            )
+            # save the address for now, so it doesn't show in free ranges
+            used_ip.save()
+
+        # reserve all available ranges
+        subnet_ranges = auto_ip.subnet.get_ipranges_not_in_use()
+        for rng in subnet_ranges.ranges:
+            factory.make_IPRange(
+                subnet=auto_ip.subnet,
+                start_ip=inet_ntop(rng.first),
+                end_ip=inet_ntop(rng.last),
+                alloc_type=IPRANGE_TYPE.RESERVED,
+            )
+        # remove the used IP, MAAS thinks it's available, but return it as used
+        # in checks
+        used_ip.delete()
+        client.return_value = defer.succeed(
+            {"ip_addresses": [{"ip_address": used_ip.ip, "used": True}]}
+        )
+
         with ExpectedException(StaticIPAddressExhaustion):
             with post_commit_hooks:
                 node.start(user)
@@ -8878,11 +8936,8 @@ class TestNode_Start(MAASTransactionServerTestCase):
             user, power_state=POWER_STATE.ON
         )
 
-        post_commit_defer = self.patch(node_module, "post_commit")
         mock_claim_auto_ips = self.patch(Node, "_claim_auto_ips")
-        mock_claim_auto_ips.return_value = post_commit_defer
         mock_power_control = self.patch(Node, "_power_control_node")
-        mock_power_control.return_value = post_commit_defer
 
         node.start(user)
 
@@ -8890,7 +8945,7 @@ class TestNode_Start(MAASTransactionServerTestCase):
         self.assertThat(node.status, Equals(NODE_STATUS.DEPLOYING))
 
         # Calls _claim_auto_ips.
-        self.assertThat(mock_claim_auto_ips, MockCalledOnceWith(ANY))
+        self.assertThat(mock_claim_auto_ips, MockCalledOnce())
 
         # Calls _power_control_node when power_cycle.
         self.assertThat(
@@ -8929,15 +8984,11 @@ class TestNode_Start(MAASTransactionServerTestCase):
         )
         node.save()
 
-        post_commit_defer = self.patch(node_module, "post_commit")
+        self.patch(Node, "_power_control_node")
         mock_claim_auto_ips = self.patch(Node, "_claim_auto_ips")
-        mock_claim_auto_ips.return_value = post_commit_defer
-        mock_power_control = self.patch(Node, "_power_control_node")
-        mock_power_control.return_value = post_commit_defer
 
         node._start(user)
-
-        self.assertThat(mock_claim_auto_ips, MockCalledOnceWith(ANY))
+        self.assertThat(mock_claim_auto_ips, MockCalledOnce())
 
     def test_doesnt_claims_auto_ips_when_script_doenst_need_it(self):
         user = factory.make_User()
diff --git a/src/maasserver/node_action.py b/src/maasserver/node_action.py
index c5a194d..3f89a6f 100644
--- a/src/maasserver/node_action.py
+++ b/src/maasserver/node_action.py
@@ -33,7 +33,11 @@ from maasserver.enum import (
     NODE_TYPE_CHOICES_DICT,
     POWER_STATE,
 )
-from maasserver.exceptions import NodeActionError, StaticIPAddressExhaustion
+from maasserver.exceptions import (
+    IPAddressCheckFailed,
+    NodeActionError,
+    StaticIPAddressExhaustion,
+)
 from maasserver.models import Config, ResourcePool, Tag, Zone
 from maasserver.node_status import is_failed_status, NON_MONITORED_STATUSES
 from maasserver.permissions import NodePermission
@@ -544,6 +548,10 @@ class Deploy(NodeAction):
                 "%s: Failed to start, static IP addresses are exhausted."
                 % self.node.hostname
             )
+        except IPAddressCheckFailed:
+            raise NodeActionError(
+                f"{self.node.hostname}: Failed to start, IP addresses check failed."
+            )
         except RPC_EXCEPTIONS + (ExternalProcessError,) as exception:
             raise NodeActionError(exception)
 
@@ -577,6 +585,10 @@ class PowerOn(NodeAction):
                 "%s: Failed to start, static IP addresses are exhausted."
                 % self.node.hostname
             )
+        except IPAddressCheckFailed:
+            raise NodeActionError(
+                f"{self.node.hostname}: Failed to start, IP addresses check failed."
+            )
         except RPC_EXCEPTIONS + (ExternalProcessError,) as exception:
             raise NodeActionError(exception)