launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-973080 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #973080 in MAAS: "Allow node to signal commissioning success"
  https://bugs.launchpad.net/maas/+bug/973080

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-973080/+merge/100775

As discussed with a good portion of the team, this branch adds one (gasp!) POST operation to the metadata API, at <prefix>/metadata/<version>/.  The API is designed to let a node report progress or failure as well as success, but this initial branch is for reports of success only.  The next branch will handle failure.

Parameter-passing works as in the MAAS API: form-multipart, with one part for each parameter.  The POST operation is identified by a parameter “op” with value “signal” — this is also how we distinguish different operations in the MAAS API.

Some ask why this should be on the metadata API, and not on the MAAS API.  There are several reasons:
 * The metadata service is our existing, and only, node-facing API.
 * Nodes have their own special authentication, which should not give them access to the MAAS API.
 * A signaling node is identified by its oauth key, as the metadata service does throughout (but not the MAAS API).
 * Requests are meaningless unless coming from a node (like the metadata API, but totally unlike the MAAS API).

It does feel a bit roguish to define our own POST operation on such a widely-used API, but is there anything actually against it?  Nobody else will rely on it.  If it clashes with other variants of the metadata API, we could easily make it more tolerant of unexpected requests.  Or maybe other users will be able to conform with this API.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-973080/+merge/100775
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-973080 into lp:maas.

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2012-04-02 14:40:17 +0000
+++ src/metadataserver/api.py	2012-04-04 12:37:20 +0000
@@ -18,12 +18,21 @@
 
 from django.core.exceptions import PermissionDenied
 from django.http import HttpResponse
-from maasserver.api import extract_oauth_key
+from maasserver.api import (
+    api_exported,
+    api_operations,
+    extract_oauth_key,
+    )
 from maasserver.exceptions import (
+    MAASAPIBadRequest,
     MAASAPINotFound,
+    NodeStateViolation,
     Unauthorized,
     )
-from maasserver.models import SSHKey
+from maasserver.models import (
+    NODE_STATUS,
+    SSHKey,
+    )
 from metadataserver.models import (
     NodeKey,
     NodeUserData,
@@ -82,12 +91,30 @@
     fields = ('latest', '2012-03-01')
 
 
+@api_operations
 class VersionIndexHandler(MetadataViewHandler):
     """Listing for a given metadata version."""
-
+    allowed_methods = ('GET', 'POST')
     fields = ('meta-data', 'user-data')
 
+    # States in which a node is allowed to signal commissioning status.
+    # (Only in Commissioning state, however, will it have any effect.)
+    signalable_states = [
+        NODE_STATUS.COMMISSIONING,
+        NODE_STATUS.READY,
+        NODE_STATUS.FAILED_TESTS,
+        ]
+
+    # Statuses that a commissioning node may signal, and the respective
+    # state transitions that they trigger on the node.
+    signaling_statuses = {
+        'OK': NODE_STATUS.READY,
+        'FAILED': NODE_STATUS.FAILED_TESTS,
+        'WORKING': None,
+    }
+
     def read(self, request, version):
+        """Read the metadata index for this version."""
         check_version(version)
         if NodeUserData.objects.has_user_data(get_node_for_request(request)):
             shown_fields = self.fields
@@ -96,6 +123,41 @@
             shown_fields.remove('user-data')
         return make_list_response(sorted(shown_fields))
 
+    @api_exported('signal', 'POST')
+    def signal(self, request, version=None):
+        """Signal commissioning status.
+
+        A commissioning node can call this to report progress of the
+        commissioning process to the metadata server.
+
+        Calling this from a node that is not Commissioning, Ready, or
+        Failed Tests is an error.  Signaling completion more than once is not
+        an error; all but the first successful call are ignored.
+
+        :param status: A commissioning status code.  This can be "OK" (to
+            signal that commissioning has completed successfully), or "FAILED"
+            (to signal failure), or "WORKING" (for progress reports).
+        """
+        node = get_node_for_request(request)
+        status = request.POST.get('status', None)
+
+        if status is None:
+            raise MAASAPIBadRequest("No status specified.")
+        if node.status not in self.signalable_states:
+            raise NodeStateViolation(
+                "Node wasn't commissioning (status is %s)" % node.status)
+        target_status = self.signaling_statuses.get(status)
+        if target_status is None:
+            raise MAASAPIBadRequest(
+                "Unknown commissioning status: '%s'" % status)
+
+        if node.status != NODE_STATUS.COMMISSIONING:
+            return "Thank you."
+
+        if target_status not in (None, node.status):
+            node.status = target_status
+            node.save()
+
 
 class MetaDataHandler(VersionIndexHandler):
     """Meta-data listing for a given version."""

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-03-26 21:01:50 +0000
+++ src/metadataserver/tests/test_api.py	2012-04-04 12:37:20 +0000
@@ -16,6 +16,8 @@
 from textwrap import dedent
 
 from maasserver.exceptions import Unauthorized
+from maasserver.models import NODE_STATUS
+from maasserver.testing import reload_object
 from maasserver.testing.factory import factory
 from maasserver.testing.oauthclient import OAuthAuthenticatedClient
 from maastesting.testcase import TestCase
@@ -87,6 +89,19 @@
         token = NodeKey.objects.get_token_for_node(node)
         return OAuthAuthenticatedClient(get_node_init_user(), token)
 
+    def make_url(self, path):
+        """Create an absolute URL for `path` on the metadata API.
+
+        :param path: Path within the metadata API to access.  Should start
+            with a slash.
+        :return: An absolute URL for the given path within the metadata
+            service tree.
+        """
+        assert path.startswith('/'), "Give absolute metadata API path."
+        # Root of the metadata API service.
+        metadata_root = "/metadata"
+        return metadata_root + path
+
     def get(self, path, client=None):
         """GET a resource from the metadata API.
 
@@ -97,12 +112,9 @@
         :return: A response to the GET request.
         :rtype: django.http.HttpResponse
         """
-        assert path.startswith('/'), "Give absolute metadata API path."
         if client is None:
             client = self.client
-        # Root of the metadata API service.
-        metadata_root = "/metadata"
-        return client.get(metadata_root + path)
+        return client.get(self.make_url(path))
 
     def test_no_anonymous_access(self):
         self.assertEqual(httplib.UNAUTHORIZED, self.get('/').status_code)
@@ -223,3 +235,54 @@
             ssh-rsa KEY my-user-key-1"""),
             response.content.decode('ascii'))
         self.assertIn('text/plain', response['Content-Type'])
+
+    def test_other_user_than_node_cannot_signal_commissioning_result(self):
+        node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
+        client = OAuthAuthenticatedClient(factory.make_user())
+        response = client.post(
+            self.make_url('/latest/'), {'op': 'signal', 'status': 'OK'})
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+        self.assertEqual(
+            NODE_STATUS.COMMISSIONING, reload_object(node).status)
+
+    def test_signaling_commissioning_result_does_not_affect_other_node(self):
+        node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
+        client = self.make_node_client(
+            node=factory.make_node(status=NODE_STATUS.COMMISSIONING))
+        response = client.post(
+            self.make_url('/latest/'), {'op': 'signal', 'status': 'OK'})
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(
+            NODE_STATUS.COMMISSIONING, reload_object(node).status)
+
+    def test_signaling_requires_status_code(self):
+        node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
+        client = self.make_node_client(node=node)
+        response = client.post(self.make_url('/latest/'), {'op': 'signal'})
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+
+    def test_signaling_rejects_unknown_status_code(self):
+        node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
+        client = self.make_node_client(node=node)
+        response = client.post(
+            self.make_url('/latest/'),
+            {'op': 'signal', 'status': factory.getRandomString()})
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+
+    def test_signaling_commissioning_success_makes_node_Ready(self):
+        node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
+        client = self.make_node_client(node=node)
+        response = client.post(
+            self.make_url('/latest/'), {'op': 'signal', 'status': 'OK'})
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(NODE_STATUS.READY, reload_object(node).status)
+
+    def test_signaling_commissioning_success_is_idempotent(self):
+        node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
+        client = self.make_node_client(node=node)
+        url = self.make_url('/latest/')
+        success_params = {'op': 'signal', 'status': 'OK'}
+        client.post(url, success_params)
+        response = client.post(url, success_params)
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(NODE_STATUS.READY, reload_object(node).status)