Merge lp:~trapnine/maas/events-2 into lp:~maas-committers/maas/trunk

[Jeffrey C Jones]

An optional comment can be supplied to select Node operations via the API.

[Blake Rouse (blake-rouse)]

Looks good. Just one issue with breaking compatibility. I am not going to prevent you from landing the branch just because of that issue. Just make sure to fix it and add a test for it.

review: Approve

=== modified file 'src/maasserver/api/nodes.py'
--- src/maasserver/api/nodes.py	2015-09-17 10:30:47 +0000
+++ src/maasserver/api/nodes.py	2015-09-18 17:34:17 +0000
@@ -402,15 +402,19 @@
             gracefully before powering off, while a hard power off
             occurs immediately without any warning to the OS.
         :type stop_mode: unicode
+        :param comment: Optional comment for the event log.
+        :type comment: unicode
 
         Returns 404 if the node is not found.
         Returns 403 if the user does not have permission to stop the node.
         """
         stop_mode = request.POST.get('stop_mode', 'hard')
+        comment = get_optional_param(request.POST, 'comment')
         node = Node.nodes.get_node_or_404(
             system_id=system_id, user=request.user,
             perm=NODE_PERMISSION.EDIT)
-        power_action_sent = node.stop(request.user, stop_mode=stop_mode)
+        power_action_sent = node.stop(
+            request.user, stop_mode=stop_mode, comment=comment)
         if power_action_sent:
             return node
         else:
@@ -429,6 +433,8 @@
         :param hwe_kernel: If present, this parameter specified the kernel to
             be used on the node
         :type hwe_kernel: unicode
+        :param comment: Optional comment for the event log.
+        :type comment: unicode
 
         Ideally we'd have MIME multipart and content-transfer-encoding etc.
         deal with the encapsulation of binary data, but couldn't make it work
@@ -445,6 +451,7 @@
         series = request.POST.get('distro_series', None)
         license_key = request.POST.get('license_key', None)
         hwe_kernel = request.POST.get('hwe_kernel', None)
+        comment = get_optional_param(request.POST, 'comment')
 
         node = Node.nodes.get_node_or_404(
             system_id=system_id, user=request.user,
@@ -472,7 +479,7 @@
                 raise MAASAPIValidationError(form.errors)
 
         try:
-            node.start(request.user, user_data=user_data)
+            node.start(request.user, user_data=user_data, comment=comment)
         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.
@@ -485,10 +492,14 @@
     def release(self, request, system_id):
         """Release a node.  Opposite of `NodesHandler.acquire`.
 
+        :param comment: Optional comment for the event log.
+        :type comment: unicode
+
         Returns 404 if the node is not found.
         Returns 403 if the user does not have permission to release the node.
         Returns 409 if the node is in a state where it may not be released.
         """
+        comment = get_optional_param(request.POST, 'comment')
         node = Node.nodes.get_node_or_404(
             system_id=system_id, user=request.user, perm=NODE_PERMISSION.EDIT)
         if node.status == NODE_STATUS.RELEASING or \
@@ -498,7 +509,7 @@
             # postcondition is achieved, so call this success.
             pass
         elif node.status in RELEASABLE_STATUSES:
-            node.release_or_erase(request.user)
+            node.release_or_erase(request.user, comment)
         else:
             raise NodeStateViolation(
                 "Node cannot be released in its current state ('%s')."
@@ -665,9 +676,9 @@
 
         If the node is allocated, release it first.
 
-        :param error_description: An optional description of the reason the
-            node is being marked broken.
-        :type error_description: unicode
+        :param comment: Optional comment for the event log. Will be
+            displayed on the Node as an error description until marked fixed.
+        :type comment: unicode
 
         Returns 404 if the node is not found.
         Returns 403 if the user does not have permission to mark the node
@@ -675,22 +686,28 @@
         """
         node = Node.nodes.get_node_or_404(
             user=request.user, system_id=system_id, perm=NODE_PERMISSION.EDIT)
-        error_description = get_optional_param(
-            request.POST, 'error_description', '')
-        node.mark_broken(request.user, error_description)
+        comment = get_optional_param(request.POST, 'comment')
+        if not comment:
+            # read old error_description to for backward compatibility
+            comment = get_optional_param(request.POST, 'error_description')
+        node.mark_broken(request.user, comment)
         return node
 
     @operation(idempotent=False)
     def mark_fixed(self, request, system_id):
         """Mark a broken node as fixed and set its status as 'ready'.
 
+        :param comment: Optional comment for the event log.
+        :type comment: unicode
+
         Returns 404 if the node is not found.
         Returns 403 if the user does not have permission to mark the node
         broken.
         """
+        comment = get_optional_param(request.POST, 'comment')
         node = Node.nodes.get_node_or_404(
             user=request.user, system_id=system_id, perm=NODE_PERMISSION.ADMIN)
-        node.mark_fixed(request.user)
+        node.mark_fixed(request.user, comment)
         maaslog.info(
             "%s: User %s marked node as fixed", node.hostname,
             request.user.username)
@@ -775,16 +792,20 @@
     def abort_operation(self, request, system_id):
         """Abort a node's current operation.
 
+        :param comment: Optional comment for the event log.
+        :type comment: unicode
+
         This currently only supports aborting of the 'Disk Erasing' operation.
 
         Returns 404 if the node could not be found.
         Returns 403 if the user does not have permission to abort the
         current operation.
         """
+        comment = get_optional_param(request.POST, 'comment')
         node = Node.nodes.get_node_or_404(
             system_id=system_id, user=request.user,
             perm=NODE_PERMISSION.EDIT)
-        node.abort_operation(request.user)
+        node.abort_operation(request.user, comment)
         return node
 
     @operation(idempotent=False)
@@ -1209,6 +1230,8 @@
 
         :param nodes: system_ids of the nodes which are to be released.
            (An empty list is acceptable).
+        :param comment: Optional comment for the event log.
+        :type comment: unicode
         :return: The system_ids of any nodes that have their status
             changed by this call. Thus, nodes that were already released
             are excluded from the result.
@@ -1220,6 +1243,7 @@
         current state.
         """
         system_ids = set(request.POST.getlist('nodes'))
+        comment = get_optional_param(request.POST, 'comment')
         # Check the existence of these nodes first.
         self._check_system_ids_exist(system_ids)
         # Make sure that the user has the required permission.
@@ -1239,7 +1263,7 @@
                 # Nothing to do.
                 pass
             elif node.status in RELEASABLE_STATUSES:
-                node.release_or_erase(request.user)
+                node.release_or_erase(request.user, comment)
                 released_ids.append(node.system_id)
             else:
                 failed.append(
@@ -1329,11 +1353,14 @@
         :param agent_name: An optional agent name to attach to the
             acquired node.
         :type agent_name: unicode
+        :param comment: Optional comment for the event log.
+        :type comment: unicode
 
         Returns 409 if a suitable node matching the constraints could not be
         found.
         """
         form = AcquireNodeForm(data=request.data)
+        comment = get_optional_param(request.POST, 'comment')
         maaslog.info(
             "Request from user %s to acquire a node with constraints %s",
             request.user.username, request.data)
@@ -1369,7 +1396,8 @@
                     request.user, get_oauth_token(request),
                     agent_name=agent_name,
                     storage_layout=storage_layout,
-                    storage_layout_params=storage_layout_params)
+                    storage_layout_params=storage_layout_params,
+                    comment=comment)
             except StorageLayoutMissingBootDiskError:
                 raise MAASAPIBadRequest(
                     "Failed to acquire node; missing a boot disk (no storage "

=== modified file 'src/maasserver/api/tests/test_node.py'
--- src/maasserver/api/tests/test_node.py	2015-09-17 15:37:57 +0000
+++ src/maasserver/api/tests/test_node.py	2015-09-18 17:34:17 +0000
@@ -340,7 +340,7 @@
         self.assertEqual(httplib.OK, response.status_code)
         self.assertIsNone(json.loads(response.content))
         self.assertThat(node_stop, MockCalledOnceWith(
-            ANY, stop_mode=ANY))
+            ANY, stop_mode=ANY, comment=None))
 
     def test_POST_stop_returns_node(self):
         node = factory.make_Node(owner=self.logged_in_user)
@@ -363,11 +363,25 @@
         node = factory.make_Node(owner=self.logged_in_user)
         node_stop = self.patch(node_module.Node, 'stop')
         stop_mode = factory.make_name('stop_mode')
+        comment = factory.make_name('comment')
+        self.client.post(
+            self.get_node_uri(node),
+            {'op': 'stop', 'stop_mode': stop_mode, 'comment': comment})
+        self.assertThat(
+            node_stop,
+            MockCalledOnceWith(
+                self.logged_in_user, stop_mode=stop_mode, comment=comment))
+
+    def test_POST_stop_handles_missing_comment(self):
+        node = factory.make_Node(owner=self.logged_in_user)
+        node_stop = self.patch(node_module.Node, 'stop')
+        stop_mode = factory.make_name('stop_mode')
         self.client.post(
             self.get_node_uri(node), {'op': 'stop', 'stop_mode': stop_mode})
         self.assertThat(
             node_stop,
-            MockCalledOnceWith(self.logged_in_user, stop_mode=stop_mode))
+            MockCalledOnceWith(
+                self.logged_in_user, stop_mode=stop_mode, comment=None))
 
     def test_POST_stop_returns_503_when_power_op_already_in_progress(self):
         node = factory.make_Node(owner=self.logged_in_user)
@@ -592,6 +606,44 @@
         self.assertEqual(httplib.OK, response.status_code)
         self.assertEqual(user_data, NodeUserData.objects.get_user_data(node))
 
+    def test_POST_start_passes_comment(self):
+        node = factory.make_Node(
+            owner=self.logged_in_user, interface=True,
+            power_type='ether_wake',
+            architecture=make_usable_architecture(self))
+        osystem = make_usable_osystem(self)
+        distro_series = osystem['default_release']
+        comment = factory.make_name('comment')
+        node_start = self.patch(node_module.Node, 'start')
+        node_start.return_value = False
+        self.client.post(
+            self.get_node_uri(node), {
+                'op': 'start',
+                'user_data': None,
+                'distro_series': distro_series,
+                'comment': comment,
+            })
+        self.assertThat(node_start, MockCalledOnceWith(
+            self.logged_in_user, user_data=ANY, comment=comment))
+
+    def test_POST_start_handles_missing_comment(self):
+        node = factory.make_Node(
+            owner=self.logged_in_user, interface=True,
+            power_type='ether_wake',
+            architecture=make_usable_architecture(self))
+        osystem = make_usable_osystem(self)
+        distro_series = osystem['default_release']
+        node_start = self.patch(node_module.Node, 'start')
+        node_start.return_value = False
+        self.client.post(
+            self.get_node_uri(node), {
+                'op': 'start',
+                'user_data': None,
+                'distro_series': distro_series,
+            })
+        self.assertThat(node_start, MockCalledOnceWith(
+            self.logged_in_user, user_data=ANY, comment=None))
+
     def test_POST_release_releases_owned_node(self):
         self.patch(node_module, 'power_off_node')
         self.patch(node_module.Node, 'start_transition_monitor')
@@ -720,6 +772,32 @@
         self.assertEqual(httplib.OK, response.status_code, response.content)
         self.assertEqual(NODE_STATUS.RELEASING, reload_object(node).status)
 
+    def test_POST_acquire_passes_comment(self):
+        factory.make_Node(
+            status=NODE_STATUS.READY, power_type='ipmi',
+            power_state=POWER_STATE.ON, with_boot_disk=True)
+        node_method = self.patch(node_module.Node, 'acquire')
+        comment = factory.make_name('comment')
+        self.client.post(
+            reverse('nodes_handler'),
+            {'op': 'acquire', 'comment': comment})
+        self.assertThat(
+            node_method, MockCalledOnceWith(
+                ANY, ANY, agent_name=ANY, storage_layout=ANY,
+                storage_layout_params=ANY, comment=comment))
+
+    def test_POST_acquire_handles_missing_comment(self):
+        factory.make_Node(
+            status=NODE_STATUS.READY, power_type='ipmi',
+            power_state=POWER_STATE.ON, with_boot_disk=True)
+        node_method = self.patch(node_module.Node, 'acquire')
+        self.client.post(
+            reverse('nodes_handler'), {'op': 'acquire'})
+        self.assertThat(
+            node_method, MockCalledOnceWith(
+                ANY, ANY, agent_name=ANY, storage_layout=ANY,
+                storage_layout_params=ANY, comment=None))
+
     def test_POST_release_frees_hwe_kernel(self):
         self.patch(node_module, 'power_off_node')
         self.patch(node_module.Node, 'start_transition_monitor')
@@ -733,6 +811,32 @@
         self.assertEqual(NODE_STATUS.RELEASING, reload_object(node).status)
         self.assertEqual(None, reload_object(node).hwe_kernel)
 
+    def test_POST_release_passes_comment(self):
+        node = factory.make_Node(
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_User(),
+            power_type='ipmi', power_state=POWER_STATE.OFF)
+        self.become_admin()
+        comment = factory.make_name('comment')
+        node_release = self.patch(node_module.Node, 'release_or_erase')
+        self.client.post(
+            self.get_node_uri(node),
+            {'op': 'release', 'comment': comment})
+        self.assertThat(
+            node_release,
+            MockCalledOnceWith(self.logged_in_user, comment))
+
+    def test_POST_release_handles_missing_comment(self):
+        node = factory.make_Node(
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_User(),
+            power_type='ipmi', power_state=POWER_STATE.OFF)
+        self.become_admin()
+        node_release = self.patch(node_module.Node, 'release_or_erase')
+        self.client.post(
+            self.get_node_uri(node), {'op': 'release'})
+        self.assertThat(
+            node_release,
+            MockCalledOnceWith(self.logged_in_user, None))
+
     def test_POST_commission_commissions_node(self):
         node = factory.make_Node(
             status=NODE_STATUS.READY, owner=factory.make_User(),
@@ -1689,9 +1793,26 @@
         self.assertEqual(NODE_STATUS.BROKEN, reload_object(node).status)
 
     def test_mark_broken_updates_error_description(self):
-        node = factory.make_Node(
-            status=NODE_STATUS.COMMISSIONING, owner=self.logged_in_user)
-        error_description = factory.make_name('error-description')
+        # 'error_description' parameter was renamed 'comment' for consistency
+        # make sure this comment updates the node's error_description
+        node = factory.make_Node(
+            status=NODE_STATUS.COMMISSIONING, owner=self.logged_in_user)
+        comment = factory.make_name('comment')
+        response = self.client.post(
+            self.get_node_uri(node),
+            {'op': 'mark_broken', 'comment': comment})
+        self.assertEqual(httplib.OK, response.status_code)
+        node = reload_object(node)
+        self.assertEqual(
+            (NODE_STATUS.BROKEN, comment),
+            (node.status, node.error_description)
+        )
+
+    def test_mark_broken_updates_error_description_compatibility(self):
+        # test old 'error_description' parameter is honored for compatibility
+        node = factory.make_Node(
+            status=NODE_STATUS.COMMISSIONING, owner=self.logged_in_user)
+        error_description = factory.make_name('error_description')
         response = self.client.post(
             self.get_node_uri(node),
             {'op': 'mark_broken', 'error_description': error_description})
@@ -1702,6 +1823,28 @@
             (node.status, node.error_description)
         )
 
+    def test_mark_broken_passes_comment(self):
+        node = factory.make_Node(
+            status=NODE_STATUS.COMMISSIONING, owner=self.logged_in_user)
+        node_mark_broken = self.patch(node_module.Node, 'mark_broken')
+        comment = factory.make_name('comment')
+        self.client.post(
+            self.get_node_uri(node),
+            {'op': 'mark_broken', 'comment': comment})
+        self.assertThat(
+            node_mark_broken,
+            MockCalledOnceWith(self.logged_in_user, comment))
+
+    def test_mark_broken_handles_missing_comment(self):
+        node = factory.make_Node(
+            status=NODE_STATUS.COMMISSIONING, owner=self.logged_in_user)
+        node_mark_broken = self.patch(node_module.Node, 'mark_broken')
+        self.client.post(
+            self.get_node_uri(node), {'op': 'mark_broken'})
+        self.assertThat(
+            node_mark_broken,
+            MockCalledOnceWith(self.logged_in_user, None))
+
     def test_mark_broken_requires_ownership(self):
         node = factory.make_Node(status=NODE_STATUS.COMMISSIONING)
         response = self.client.post(
@@ -1742,6 +1885,28 @@
             self.get_node_uri(node), {'op': 'mark_fixed'})
         self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
+    def test_mark_fixed_passes_comment(self):
+        self.become_admin()
+        node = factory.make_Node(status=NODE_STATUS.BROKEN)
+        node_mark_fixed = self.patch(node_module.Node, 'mark_fixed')
+        comment = factory.make_name('comment')
+        self.client.post(
+            self.get_node_uri(node),
+            {'op': 'mark_fixed', 'comment': comment})
+        self.assertThat(
+            node_mark_fixed,
+            MockCalledOnceWith(self.logged_in_user, comment))
+
+    def test_mark_fixed_handles_missing_comment(self):
+        self.become_admin()
+        node = factory.make_Node(status=NODE_STATUS.BROKEN)
+        node_mark_fixed = self.patch(node_module.Node, 'mark_fixed')
+        self.client.post(
+            self.get_node_uri(node), {'op': 'mark_fixed'})
+        self.assertThat(
+            node_mark_fixed,
+            MockCalledOnceWith(self.logged_in_user, None))
+
 
 class TestPowerParameters(APITestCase):
     def get_node_uri(self, node):
@@ -1801,6 +1966,30 @@
             self.get_node_uri(node), {'op': 'abort_operation'})
         self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
+    def test_abort_operation_passes_comment(self):
+        self.become_admin()
+        node = factory.make_Node(
+            status=NODE_STATUS.DISK_ERASING, owner=self.logged_in_user)
+        node_method = self.patch(node_module.Node, 'abort_operation')
+        comment = factory.make_name('comment')
+        self.client.post(
+            self.get_node_uri(node),
+            {'op': 'abort_operation', 'comment': comment})
+        self.assertThat(
+            node_method,
+            MockCalledOnceWith(self.logged_in_user, comment))
+
+    def test_abort_operation_handles_missing_comment(self):
+        self.become_admin()
+        node = factory.make_Node(
+            status=NODE_STATUS.DISK_ERASING, owner=self.logged_in_user)
+        node_method = self.patch(node_module.Node, 'abort_operation')
+        self.client.post(
+            self.get_node_uri(node), {'op': 'abort_operation'})
+        self.assertThat(
+            node_method,
+            MockCalledOnceWith(self.logged_in_user, None))
+
 
 class TestSetStorageLayout(APITestCase):
 

=== modified file 'src/maasserver/api/tests/test_nodes.py'
--- src/maasserver/api/tests/test_nodes.py	2015-08-29 03:27:16 +0000
+++ src/maasserver/api/tests/test_nodes.py	2015-09-18 17:34:17 +0000
@@ -1357,7 +1357,7 @@
             mock_acquire,
             MockCalledOnceWith(
                 ANY, ANY, agent_name=ANY, storage_layout='flat',
-                storage_layout_params={'root_size': '50%'}))
+                storage_layout_params={'root_size': '50%'}, comment=None))
 
     def test_POST_accept_gets_node_out_of_declared_state(self):
         # This will change when we add provisioning.  Until then,