launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/metadata-commissioning-scripts into lp:maas.

Commit message:
Metadata API extension: maas-commissioning-scripts.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/metadata-commissioning-scripts/+merge/137112

Discussed with Julian.  This branch adds a new item on the metadata API: /metadata/latest/maas-commissioning-scripts.  In particular, we pre-imped the idea of not having a ".tar" extension on the http filename.  Or ".tar.gz," or ".tar.bz2" and so on.  This is what MIME types are for.  Filename is entirely irrelevant.

Something I may have mentioned offhand but no more: the new entry goes right at the top of what a node sees, next to "meta-data" and "user-data."  Hiding it further down inside a directory tree already specified and used by others seemed pointless.  I prefixed the name with "maas-" because this very blatantly extends the cloud-init structure, and whatever we do here is not likely to port over to non-MAAS use.  I could have created a "maas/" directory instead, but that would be over-engineering for the problem at hand, not to mention the deadline at hand.

There are several MIME types for tar files.  When I wrote the tests I had no implementation yet, and I didn't much care which I was going to use.  I left the door open for compressed tarballs.  Thanks to transparent decompression, client code probably wouldn't even notice if we made that change.  Whether compression would be a good idea is yet another trade-off that I'm leaving for another day: maybe if we cache this archive in the future it'll be worth more app-server CPU time, or maybe this just isn't going to become an issue because the archive always remains tiny, or maybe the only really large files we ever have in there are already compressed and wouldn't benefit.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/metadata-commissioning-scripts/+merge/137112
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/metadata-commissioning-scripts into lp:maas.

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2012-11-28 14:08:46 +0000
+++ src/metadataserver/api.py	2012-11-30 05:16:22 +0000
@@ -12,6 +12,7 @@
 __metaclass__ = type
 __all__ = [
     'AnonMetaDataHandler',
+    'CommissioningScriptsHandler',
     'IndexHandler',
     'MetaDataHandler',
     'UserDataHandler',
@@ -56,6 +57,7 @@
 from maasserver.utils import find_nodegroup
 from maasserver.utils.orm import get_one
 from metadataserver.models import (
+    CommissioningScript,
     NodeCommissionResult,
     NodeKey,
     NodeUserData,
@@ -156,7 +158,7 @@
 class VersionIndexHandler(MetadataViewHandler):
     """Listing for a given metadata version."""
     create = update = delete = None
-    fields = ('meta-data', 'user-data')
+    fields = ('maas-commissioning-scripts', '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.)
@@ -341,6 +343,16 @@
             return HttpResponse(status=httplib.NOT_FOUND)
 
 
+class CommissioningScriptsHandler(MetadataViewHandler):
+    """Return a tar archive containing the commissioning scripts."""
+
+    def read(self, request, version, mac=None):
+        check_version(version)
+        return HttpResponse(
+            CommissioningScript.objects.get_archive(),
+            mimetype='application/tar')
+
+
 class EnlistMetaDataHandler(OperationsHandler):
     """this has to handle the 'meta-data' portion of the meta-data api
     for enlistment only.  It should mimic the read-only portion

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-11-29 03:01:12 +0000
+++ src/metadataserver/tests/test_api.py	2012-11-30 05:16:22 +0000
@@ -14,7 +14,10 @@
 
 from collections import namedtuple
 import httplib
+from io import BytesIO
 import json
+import os.path
+import tarfile
 
 from django.conf import settings
 from django.core.exceptions import PermissionDenied
@@ -49,6 +52,7 @@
     NodeKey,
     NodeUserData,
     )
+from metadataserver.models.commissioningscript import ARCHIVE_PREFIX
 from metadataserver.nodeinituser import get_node_init_user
 from mock import Mock
 from netaddr import IPNetwork
@@ -182,11 +186,12 @@
         # The test is that we get here without exception.
         pass
 
-    def test_version_index_shows_meta_data(self):
+    def test_version_index_shows_unconditional_entries(self):
         client = self.make_node_client()
         url = reverse('metadata-version', args=['latest'])
         items = client.get(url).content.splitlines()
         self.assertIn('meta-data', items)
+        self.assertIn('maas-commissioning-scripts', items)
 
     def test_version_index_does_not_show_user_data_if_not_available(self):
         client = self.make_node_client()
@@ -326,6 +331,27 @@
             '\n'.join(keys),
             response.content.decode('ascii'))
 
+    def test_commissioning_scripts(self):
+        script = factory.make_commissioning_script()
+        response = self.make_node_client().get(
+            reverse('commissioning-scripts', args=['latest']))
+        self.assertEqual(
+            httplib.OK, response.status_code,
+            "Unexpected response %d: %s"
+            % (response.status_code, response.content))
+        self.assertIn(
+            response['Content-Type'],
+            {
+                'application/tar',
+                'application/x-gtar',
+                'application/x-tar',
+                'application/x-tgz',
+            })
+        archive = tarfile.open(fileobj=BytesIO(response.content))
+        self.assertItemsEqual(
+            [os.path.join(ARCHIVE_PREFIX, script.name)],
+            archive.getnames())
+
     def test_other_user_than_node_cannot_signal_commissioning_result(self):
         node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
         client = OAuthAuthenticatedClient(factory.make_user())

=== modified file 'src/metadataserver/tests/test_commissioningscript.py'
--- src/metadataserver/tests/test_commissioningscript.py	2012-11-29 20:29:12 +0000
+++ src/metadataserver/tests/test_commissioningscript.py	2012-11-30 05:16:22 +0000
@@ -69,6 +69,11 @@
             script.content,
             archive.extractfile(archived_script).read())
 
+    def test_get_archive_returns_empty_tarball_if_no_scripts(self):
+        CommissioningScript.objects.all().delete()
+        archive = open_tarfile(CommissioningScript.objects.get_archive())
+        self.assertItemsEqual([], archive.getnames())
+
 
 class TestCommissioningScript(TestCase):
 

=== modified file 'src/metadataserver/urls.py'
--- src/metadataserver/urls.py	2012-11-22 13:27:50 +0000
+++ src/metadataserver/urls.py	2012-11-30 05:16:22 +0000
@@ -22,6 +22,7 @@
 from maasserver.api_support import OperationsResource
 from metadataserver.api import (
     AnonMetaDataHandler,
+    CommissioningScriptsHandler,
     EnlistMetaDataHandler,
     EnlistUserDataHandler,
     EnlistVersionIndexHandler,
@@ -40,6 +41,8 @@
     VersionIndexHandler, authentication=api_auth)
 index_handler = OperationsResource(
     IndexHandler, authentication=api_auth)
+commissioning_scripts_handler = OperationsResource(
+    CommissioningScriptsHandler, authentication=api_auth)
 
 
 # Handlers for anonymous metadata operations.
@@ -58,23 +61,31 @@
 enlist_version_index_handler = OperationsResource(EnlistVersionIndexHandler)
 
 # Normal metadata access, available to a node querying its own metadata.
+#
+# The URL patterns must tolerate redundant leading slashes, because
+# cloud-init tends to add these.
 node_patterns = patterns(
     '',
     url(
-        # could-init adds additional slashes in front of urls.
         r'^/*(?P<version>[^/]+)/meta-data/(?P<item>.*)$',
         meta_data_handler,
         name='metadata-meta-data'),
     url(
-        # could-init adds additional slashes in front of urls.
         r'^/*(?P<version>[^/]+)/user-data$', user_data_handler,
         name='metadata-user-data'),
-    url(
-        # could-init adds additional slashes in front of urls.
+    # Commissioning scripts.  This is a blatant MAAS extension to the
+    # metadata API, hence the "maas-" prefix.
+    # Scripts are returned as a tar arhive, but the format is not
+    # reflected in the http filename.  The response's MIME type is
+    # definitive.  We may yet choose to compress the file, without
+    # changing its name on the API.
+    url(
+        r'^/*(?P<version>[^/]+)/maas-commissioning-scripts',
+        commissioning_scripts_handler, name='commissioning-scripts'),
+    url(
         r'^/*(?P<version>[^/]+)/', version_index_handler,
         name='metadata-version'),
     url(
-        # could-init adds additional slashes in front of urls.
         r'^/*', index_handler, name='metadata'),
     )