Merge lp:~gmb/maas/use-node.start-instead-of-start_nodes-bug-1330765 into lp:~maas-committers/maas/trunk

[Graham Binns]

Convert Node.objects.start_nodes() calls to use Node.start().

I've accounted for all the callsites except those in the tests for start_nodes, which I'll remove in a separate branch.

[Julian Edwards (julian-edwards)]

Smashing!

review: Approve

[Graham Binns (gmb)]

On 21 October 2014 08:45, Julian Edwards <email address hidden> wrote:
> Review: Approve
>
> Smashing!
>
> Diff comments:
>
>> === modified file 'src/maasserver/api/nodes.py'
>> --- src/maasserver/api/nodes.py 2014-10-17 16:54:22 +0000
>> +++ src/maasserver/api/nodes.py 2014-10-21 07:14:49 +0000
>> @@ -285,12 +285,14 @@
>> user_data = request.POST.get('user_data', None)
>> series = request.POST.get('distro_series', None)
>> license_key = request.POST.get('license_key', None)
>> +
>> + node = Node.objects.get_node_or_404(
>> + system_id=system_id, user=request.user,
>> + perm=NODE_PERMISSION.EDIT)
>> +
>> if user_data is not None:
>> user_data = b64decode(user_data)
>> if series is not None or license_key is not None:
>> - node = Node.objects.get_node_or_404(
>> - system_id=system_id, user=request.user,
>> - perm=NODE_PERMISSION.EDIT)
>> Form = get_node_edit_form(request.user)
>> form = Form(instance=node)
>> if series is not None:
>> @@ -301,19 +303,16 @@
>> form.save()
>> else:
>> raise ValidationError(form.errors)
>> +
>> try:
>> - nodes = Node.objects.start_nodes(
>> - [system_id], request.user, user_data=user_data)
>> + node.start(request.user, user_data=user_data)
>> except StaticIPAddressExhaustion:
>> # The API response should contain error text with the
>> # system_id in it, as that is the primary API key to a node.
>> raise StaticIPAddressExhaustion(
>> "%s: Unable to allocate static IP due to address"
>> " exhaustion." % system_id)
>> - if len(nodes) == 0:
>> - raise PermissionDenied(
>> - "You are not allowed to start up this node.")
>> - return nodes[0]
>> + return node
>>
>> @operation(idempotent=False)
>> def release(self, request, system_id):
>>
>> === modified file 'src/maasserver/models/node.py'
>> --- src/maasserver/models/node.py 2014-10-21 00:02:35 +0000
>> +++ src/maasserver/models/node.py 2014-10-21 07:14:49 +0000
>> @@ -969,11 +969,7 @@
>> self.save()
>> transaction.commit()
>> try:
>> - # We don't check for which nodes we've started here, because
>> - # it's possible we can't start the node - its power type may not
>> - # allow us to do that.
>> - Node.objects.start_nodes(
>> - [self.system_id], user, user_data=commissioning_user_data)
>> + self.start(user, user_data=commissioning_user_data)
>> except Exception as ex:
>> maaslog.error(
>> "%s: Unable to start node: %s",
>> @@ -1287,8 +1283,7 @@
>> self.save()
>> transaction.commit()
>> try:
>> - Node.objects.start_nodes(
>> - [self.system_id], user, user_data=disk_erase_user_data)
>> + self.start(user, user_data=disk_erase_user_data)
>> except E...

=== modified file 'src/maasserver/api/nodes.py'
--- src/maasserver/api/nodes.py	2014-10-17 16:54:22 +0000
+++ src/maasserver/api/nodes.py	2014-10-21 08:28:19 +0000
@@ -285,12 +285,14 @@
         user_data = request.POST.get('user_data', None)
         series = request.POST.get('distro_series', None)
         license_key = request.POST.get('license_key', None)
+
+        node = Node.objects.get_node_or_404(
+            system_id=system_id, user=request.user,
+            perm=NODE_PERMISSION.EDIT)
+
         if user_data is not None:
             user_data = b64decode(user_data)
         if series is not None or license_key is not None:
-            node = Node.objects.get_node_or_404(
-                system_id=system_id, user=request.user,
-                perm=NODE_PERMISSION.EDIT)
             Form = get_node_edit_form(request.user)
             form = Form(instance=node)
             if series is not None:
@@ -301,19 +303,16 @@
                 form.save()
             else:
                 raise ValidationError(form.errors)
+
         try:
-            nodes = Node.objects.start_nodes(
-                [system_id], request.user, user_data=user_data)
+            node.start(request.user, user_data=user_data)
         except StaticIPAddressExhaustion:
             # The API response should contain error text with the
             # system_id in it, as that is the primary API key to a node.
             raise StaticIPAddressExhaustion(
                 "%s: Unable to allocate static IP due to address"
                 " exhaustion." % system_id)
-        if len(nodes) == 0:
-            raise PermissionDenied(
-                "You are not allowed to start up this node.")
-        return nodes[0]
+        return node
 
     @operation(idempotent=False)
     def release(self, request, system_id):

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-10-21 00:02:35 +0000
+++ src/maasserver/models/node.py	2014-10-21 08:28:19 +0000
@@ -969,11 +969,7 @@
         self.save()
         transaction.commit()
         try:
-            # We don't check for which nodes we've started here, because
-            # it's possible we can't start the node - its power type may not
-            # allow us to do that.
-            Node.objects.start_nodes(
-                [self.system_id], user, user_data=commissioning_user_data)
+            self.start(user, user_data=commissioning_user_data)
         except Exception as ex:
             maaslog.error(
                 "%s: Unable to start node: %s",
@@ -1287,8 +1283,7 @@
         self.save()
         transaction.commit()
         try:
-            Node.objects.start_nodes(
-                [self.system_id], user, user_data=disk_erase_user_data)
+            self.start(user, user_data=disk_erase_user_data)
         except Exception as ex:
             maaslog.error(
                 "%s: Unable to start node: %s",
@@ -1583,6 +1578,13 @@
         from metadataserver.models import NodeUserData
         from maasserver.dns.config import change_dns_zones
 
+        if not by_user.has_perm(NODE_PERMISSION.EDIT, self):
+            # You can't stop a node you don't own unless you're an
+            # admin, so we return early.  This is consistent with the
+            # behaviour of NodeManager.stop_nodes(); it may be better to
+            # raise an error here.
+            return
+
         # Record the user data for the node. Note that we do this
         # whether or not we can actually send power commands to the
         # node; the user may choose to start it manually.
@@ -1634,7 +1636,7 @@
         :raises MultipleFailures: When there are failures originating
             from the RPC power action.
         """
-        if by_user != self.owner and not by_user.is_superuser:
+        if not by_user.has_perm(NODE_PERMISSION.EDIT, self):
             # You can't stop a node you don't own unless you're an
             # admin, so we return early.  This is consistent with the
             # behaviour of NodeManager.stop_nodes(); it may be better to

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-10-21 01:15:29 +0000
+++ src/maasserver/models/tests/test_node.py	2014-10-21 08:28:19 +0000
@@ -740,13 +740,13 @@
         owner = factory.make_User()
         node = factory.make_Node(
             status=NODE_STATUS.ALLOCATED, owner=owner, agent_name=agent_name)
-        start_nodes = self.patch(Node.objects, "start_nodes")
+        node_start = self.patch(node, 'start')
         node.start_disk_erasing(owner)
-        self.assertEqual(
-            (owner, NODE_STATUS.DISK_ERASING, agent_name),
-            (node.owner, node.status, node.agent_name))
-        self.assertThat(start_nodes, MockCalledOnceWith(
-            [node.system_id], owner, user_data=ANY))
+        self.expectThat(node.owner, Equals(owner))
+        self.expectThat(node.status, Equals(NODE_STATUS.DISK_ERASING))
+        self.expectThat(node.agent_name, Equals(agent_name))
+        self.assertThat(
+            node_start, MockCalledOnceWith(owner, user_data=ANY))
 
     def test_abort_disk_erasing_changes_state_and_stops_node(self):
         agent_name = factory.make_name('agent-name')
@@ -769,53 +769,35 @@
         # Failures encountered in one call to start_disk_erasing() won't
         # affect subsequent calls.
         admin = factory.make_admin()
-        nodes = [
-            factory.make_Node(
-                status=NODE_STATUS.ALLOCATED, power_type="virsh")
-            for _ in range(3)
-            ]
+        node = factory.make_Node(status=NODE_STATUS.ALLOCATED)
         generate_user_data = self.patch(disk_erasing, 'generate_user_data')
-        start_nodes = self.patch(Node.objects, 'start_nodes')
-        start_nodes.side_effect = [
-            None,
-            MultipleFailures(
-                Failure(NoConnectionsAvailable())),
-            None,
-            ]
+        node_start = self.patch(node, 'start')
+        node_start.side_effect = MultipleFailures(
+            Failure(NoConnectionsAvailable())),
 
         with transaction.atomic():
-            for node in nodes:
-                try:
-                    node.start_disk_erasing(admin)
-                except RPC_EXCEPTIONS:
-                    # Suppress all the expected errors coming out of
-                    # start_disk_erasing() because they're tested
-                    # eleswhere.
-                    pass
+            try:
+                node.start_disk_erasing(admin)
+            except RPC_EXCEPTIONS:
+                # Suppress all the expected errors coming out of
+                # start_disk_erasing() because they're tested
+                # eleswhere.
+                pass
 
-        expected_calls = (
-            call(
-                [node.system_id], admin,
-                user_data=generate_user_data.return_value)
-            for node in nodes)
         self.assertThat(
-            start_nodes, MockCallsMatch(*expected_calls))
-        self.assertEqual(
-            [
-                NODE_STATUS.DISK_ERASING,
-                NODE_STATUS.FAILED_DISK_ERASING,
-                NODE_STATUS.DISK_ERASING,
-            ],
-            [node.status for node in nodes])
+            node_start, MockCalledOnceWith(
+                admin, user_data=generate_user_data.return_value))
+        self.assertEqual(NODE_STATUS.FAILED_DISK_ERASING, node.status)
 
     def test_start_disk_erasing_logs_and_raises_errors_in_starting(self):
         admin = factory.make_admin()
         node = factory.make_Node(status=NODE_STATUS.ALLOCATED)
         maaslog = self.patch(node_module, 'maaslog')
-        exception = NoConnectionsAvailable(factory.make_name())
-        self.patch(Node.objects, 'start_nodes').side_effect = exception
+        exception_type = factory.make_exception_type()
+        exception = exception_type(factory.make_name())
+        self.patch(node, 'start').side_effect = exception
         self.assertRaises(
-            NoConnectionsAvailable, node.start_disk_erasing, admin)
+            exception_type, node.start_disk_erasing, admin)
         self.assertEqual(NODE_STATUS.FAILED_DISK_ERASING, node.status)
         self.assertThat(
             maaslog.error, MockCalledOnceWith(
@@ -1337,8 +1319,7 @@
     def test_start_commissioning_changes_status_and_starts_node(self):
         node = factory.make_Node(
             status=NODE_STATUS.NEW, power_type='ether_wake')
-        start_nodes = self.patch(Node.objects, "start_nodes")
-        start_nodes.return_value = [node]
+        node_start = self.patch(node, 'start')
         factory.make_MACAddress(node=node)
         admin = factory.make_admin()
         node.start_commissioning(admin)
@@ -1347,21 +1328,20 @@
             'status': NODE_STATUS.COMMISSIONING,
         }
         self.assertAttributes(node, expected_attrs)
-        self.assertThat(start_nodes, MockCalledOnceWith(
-            [node.system_id], admin, user_data=ANY))
+        self.assertThat(node_start, MockCalledOnceWith(
+            admin, user_data=ANY))
 
     def test_start_commissioning_sets_user_data(self):
-        start_nodes = self.patch(Node.objects, "start_nodes")
-
         node = factory.make_Node(status=NODE_STATUS.NEW)
+        node_start = self.patch(node, 'start')
         user_data = factory.make_string().encode('ascii')
         generate_user_data = self.patch(
             commissioning, 'generate_user_data')
         generate_user_data.return_value = user_data
         admin = factory.make_admin()
         node.start_commissioning(admin)
-        self.assertThat(start_nodes, MockCalledOnceWith(
-            [node.system_id], admin, user_data=user_data))
+        self.assertThat(node_start, MockCalledOnceWith(
+            admin, user_data=user_data))
 
     def test_start_commissioning_clears_node_commissioning_results(self):
         node = factory.make_Node(status=NODE_STATUS.NEW)
@@ -1391,49 +1371,32 @@
         # start the node, it will revert the node to its previous
         # status.
         admin = factory.make_admin()
-        nodes = [
-            factory.make_Node(status=NODE_STATUS.NEW, power_type="ether_wake")
-            for _ in range(3)
-            ]
+        node = factory.make_Node(status=NODE_STATUS.NEW)
         generate_user_data = self.patch(commissioning, 'generate_user_data')
-        start_nodes = self.patch(Node.objects, 'start_nodes')
-        start_nodes.side_effect = [
-            None,
-            MultipleFailures(
-                Failure(NoConnectionsAvailable())),
-            None,
-            ]
+        node_start = self.patch(node, 'start')
+        node_start.side_effect = MultipleFailures(
+            Failure(NoConnectionsAvailable()))
 
         with transaction.atomic():
-            for node in nodes:
-                try:
-                    node.start_commissioning(admin)
-                except RPC_EXCEPTIONS:
-                    # Suppress all expected errors; we test for them
-                    # elsewhere.
-                    pass
+            try:
+                node.start_commissioning(admin)
+            except RPC_EXCEPTIONS:
+                # Suppress all expected errors; we test for them
+                # elsewhere.
+                pass
 
-        expected_calls = (
-            call(
-                [node.system_id], admin,
-                user_data=generate_user_data.return_value)
-            for node in nodes)
         self.assertThat(
-            start_nodes, MockCallsMatch(*expected_calls))
-        self.assertEqual(
-            [
-                NODE_STATUS.COMMISSIONING,
-                NODE_STATUS.NEW,
-                NODE_STATUS.COMMISSIONING
-            ],
-            [node.status for node in nodes])
+            node_start,
+            MockCalledOnceWith(
+                admin, user_data=generate_user_data.return_value))
+        self.assertEqual(NODE_STATUS.NEW, node.status)
 
     def test_start_commissioning_logs_and_raises_errors_in_starting(self):
         admin = factory.make_admin()
         node = factory.make_Node(status=NODE_STATUS.NEW)
         maaslog = self.patch(node_module, 'maaslog')
         exception = NoConnectionsAvailable(factory.make_name())
-        self.patch(Node.objects, 'start_nodes').side_effect = exception
+        self.patch(node, 'start').side_effect = exception
         self.assertRaises(
             NoConnectionsAvailable, node.start_commissioning, admin)
         self.assertEqual(NODE_STATUS.NEW, node.status)
@@ -2833,6 +2796,28 @@
         node.start(user)
         self.assertThat(power_on_nodes, MockNotCalled())
 
+    def test__does_not_start_nodes_the_user_cannot_edit(self):
+        power_on_nodes = self.patch_autospec(node_module, "power_on_nodes")
+        owner = factory.make_User()
+        node = self.make_acquired_node_with_mac(owner)
+
+        user = factory.make_User()
+        node.start(user)
+        self.assertThat(power_on_nodes, MockNotCalled())
+
+    def test__allows_admin_to_start_any_node(self):
+        wait_for_power_commands = self.patch_autospec(
+            node_module, 'wait_for_power_commands')
+        power_on_nodes = self.patch_autospec(node_module, "power_on_nodes")
+        owner = factory.make_User()
+        node = self.make_acquired_node_with_mac(owner)
+
+        admin = factory.make_admin()
+        node.start(admin)
+
+        self.expectThat(power_on_nodes, MockCalledOnceWith(ANY))
+        self.expectThat(wait_for_power_commands, MockCalledOnceWith(ANY))
+
 
 class TestNode_Stop(MAASServerTestCase):
     """Tests for Node.stop()."""

=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2014-10-17 16:54:22 +0000
+++ src/maasserver/node_action.py	2014-10-21 08:28:19 +0000
@@ -345,7 +345,7 @@
             self.node.acquire(self.user, token=None)
 
         try:
-            Node.objects.start_nodes([self.node.system_id], self.user)
+            self.node.start(self.user)
         except StaticIPAddressExhaustion:
             raise NodeActionError(
                 "%s: Failed to start, static IP addresses are exhausted."

=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2014-10-20 20:40:50 +0000
+++ src/maasserver/tests/test_node_action.py	2014-10-21 08:28:19 +0000
@@ -230,17 +230,16 @@
     )
 
     def test_Commission_starts_commissioning(self):
-        start_nodes = self.patch(Node.objects, "start_nodes")
         node = factory.make_Node(
             mac=True, status=self.status,
             power_type='ether_wake')
+        node_start = self.patch(node, 'start')
         admin = factory.make_admin()
         action = Commission(node, admin)
         action.execute()
         self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
         self.assertThat(
-            start_nodes, MockCalledOnceWith(
-                [node.system_id], admin, user_data=ANY))
+            node_start, MockCalledOnceWith(admin, user_data=ANY))
 
 
 class TestAbortCommissioningNodeAction(MAASServerTestCase):
@@ -314,14 +313,14 @@
         self.assertIn("SSH key", inhibition)
 
     def test_StartNode_starts_node(self):
-        start_nodes = self.patch(Node.objects, "start_nodes")
         user = factory.make_User()
         node = factory.make_Node(
             mac=True, status=NODE_STATUS.ALLOCATED,
             power_type='ether_wake', owner=user)
+        node_start = self.patch(node, 'start')
         StartNode(node, user).execute()
         self.assertThat(
-            start_nodes, MockCalledOnceWith([node.system_id], user))
+            node_start, MockCalledOnceWith(user))
 
     def test_StartNode_returns_error_when_no_more_static_IPs(self):
         user = factory.make_User()
@@ -351,9 +350,9 @@
         self.assertFalse(StartNode(node, user).is_permitted())
 
     def test_StartNode_allocates_node_if_node_not_already_allocated(self):
-        self.patch(Node.objects, "start_nodes")
         user = factory.make_User()
         node = factory.make_Node(status=NODE_STATUS.READY)
+        self.patch(node, 'start')
         action = StartNode(node, user)
         action.execute()
 
@@ -361,16 +360,16 @@
         self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
 
     def test_StartNode_label_shows_allocate_if_unallocated(self):
-        self.patch(Node.objects, "start_nodes")
         user = factory.make_User()
         node = factory.make_Node(status=NODE_STATUS.READY)
+        self.patch(node, 'start')
         action = StartNode(node, user)
         self.assertEqual("Acquire and start node", action.display)
 
     def test_StartNode_label_hides_allocate_if_allocated(self):
-        self.patch(Node.objects, "start_nodes")
         user = factory.make_User()
         node = factory.make_Node(status=NODE_STATUS.READY)
+        self.patch(node, 'start')
         node.acquire(user)
         action = StartNode(node, user)
         self.assertEqual("Start node", action.display)
@@ -384,17 +383,17 @@
         self.assertEqual("Start node", action.display)
 
     def test_StartNode_does_not_reallocate_when_run_by_non_owner(self):
-        self.patch(Node.objects, "start_nodes")
         user = factory.make_User()
         admin = factory.make_admin()
         node = factory.make_Node(status=NODE_STATUS.READY)
+        self.patch(node, 'start')
         node.acquire(user)
         action = StartNode(node, admin)
 
         # This action.execute() will not fail because the non-owner is
         # an admin, so they can start the node. Even if they weren't an
-        # admin, the node still wouldn't start;
-        # NodeManager.start_nodes() would ignore it.
+        # admin, the node still wouldn't start; Node.start() would
+        # ignore it.
         action.execute()
         self.assertEqual(user, node.owner)
         self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
@@ -571,9 +570,9 @@
             exception = self.exception_class(factory.make_name("exception"))
         return exception
 
-    def patch_rpc_methods(self):
+    def patch_rpc_methods(self, node):
         exception = self.make_exception()
-        self.patch(Node.objects, "start_nodes").side_effect = (
+        self.patch(node, 'start').side_effect = (
             exception)
         self.patch(Node.objects, "stop_nodes").side_effect = (
             exception)
@@ -586,17 +585,17 @@
 
     def test_Commission_handles_rpc_errors(self):
         action = self.make_action(Commission, NODE_STATUS.READY)
-        self.patch_rpc_methods()
+        self.patch_rpc_methods(action.node)
         exception = self.assertRaises(NodeActionError, action.execute)
         self.assertEqual(
             get_error_message_for_exception(
-                Node.objects.start_nodes.side_effect),
+                action.node.start.side_effect),
             unicode(exception))
 
     def test_AbortCommissioning_handles_rpc_errors(self):
         action = self.make_action(
             AbortCommissioning, NODE_STATUS.COMMISSIONING)
-        self.patch_rpc_methods()
+        self.patch_rpc_methods(action.node)
         exception = self.assertRaises(NodeActionError, action.execute)
         self.assertEqual(
             get_error_message_for_exception(
@@ -606,7 +605,7 @@
     def test_AbortOperation_handles_rpc_errors(self):
         action = self.make_action(
             AbortOperation, NODE_STATUS.DISK_ERASING)
-        self.patch_rpc_methods()
+        self.patch_rpc_methods(action.node)
         exception = self.assertRaises(NodeActionError, action.execute)
         self.assertEqual(
             get_error_message_for_exception(
@@ -615,16 +614,16 @@
 
     def test_StartNode_handles_rpc_errors(self):
         action = self.make_action(StartNode, NODE_STATUS.READY)
-        self.patch_rpc_methods()
+        self.patch_rpc_methods(action.node)
         exception = self.assertRaises(NodeActionError, action.execute)
         self.assertEqual(
             get_error_message_for_exception(
-                Node.objects.start_nodes.side_effect),
+                action.node.start.side_effect),
             unicode(exception))
 
     def test_StopNode_handles_rpc_errors(self):
         action = self.make_action(StopNode, NODE_STATUS.DEPLOYED)
-        self.patch_rpc_methods()
+        self.patch_rpc_methods(action.node)
         exception = self.assertRaises(NodeActionError, action.execute)
         self.assertEqual(
             get_error_message_for_exception(
@@ -633,7 +632,7 @@
 
     def test_ReleaseNode_handles_rpc_errors(self):
         action = self.make_action(ReleaseNode, NODE_STATUS.ALLOCATED)
-        self.patch_rpc_methods()
+        self.patch_rpc_methods(action.node)
         exception = self.assertRaises(NodeActionError, action.execute)
         self.assertEqual(
             get_error_message_for_exception(