launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug607961 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #607961 in Launchpad itself: "wadl generation timeout?"
  https://bugs.launchpad.net/launchpad/+bug/607961

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug607961/+merge/67246

pre-imp with lifeless, via the bug report; the Apache config example was vetted by mthaddon.

This branch attempts to fix bug 607961 by allowing us to push the serving of the wadl files from the app server to Apache.

Deployment of the change should have two phases, and both need to be qa'd.  First, the branch should continue to work as it did before: without Apache changes, the app server continues to serve the wadl as it did before.  This should also be true for devel machines for reviewers.  Simply connecting to qastaging using launchpadlib should be sufficient, because it immediately gets the WADL; for safety, we might as well try to do a representative operation with launchpadlib, but that should be sufficient.

That should be able to make its way to production without blocking anything.

The second stage of tests should be to apply pertinent changes to qastaging's Apache.  http://pastebin.ubuntu.com/639701/ is a developer-box version of the changes needed, and can be used by a reviewer if desired.  The most pertinent parts of this are lines 3-5 and lines 25-33.  Importantly, we also need to include "FileETag MTime Size" within the Directory section.  LOSAs will need to decide what the DirectoryRoot should point to; however the LOSAs want to arrange it, it needs to be the lib/canonical/launchpad/apidoc directory of the deployed branch.  Robert also points out that the approach that LOSAs have for this should "be robust on rollouts (as icing is)"; I think that probably will happen naturally, but we should clarify that with LOSAs on the RT for this stage.

All that said, what's going on in this branch?

Serving a static file from Apache is not a big deal, but there are some wrinkles that needed to be handled.  Most of these are outlined by Leonard in the bug: https://bugs.launchpad.net/launchpad/+bug/607961/comments/6 .

(1) We need to be able to serve different versions of the root resource of a version depending on the mimetype: json or wadl.  Serving html there would be nice too.
(2) We need to support the "real" wadl mime type and a misspelling that older versions of launchpadlib used.
(3) We want to continue to support compression and ETags.
(4) Tests need to continue to use a Zope-served version for parallel test runs.

I used Apache's MultiViews for the content negotation (1).  That is turned on in the Apache snippet I give in the pastebin.  http://httpd.apache.org/docs/current/content-negotiation.html .

The mime types need to be known in Apache for this to work.  I define wadl and json in the pastebin.  I also define the misspelling (2).  I don't care for this solution, but it was the third I tried.  I document the other two in a new comment in create-lp-wadl-and-apidoc.py, as seen in the diff below.

Compression and ETags (3) are entirely in Apache now.  The Etags are set in Apache to correspond to file size and mtime; and we set the mtime of the generated wadl and json files to the time of the last commit in the branch, so they should be stable across multiple Launchpad servers.

For keeping Zope-served versions available for tests (4), I simply had to keep the previous mechanism working, which I did.

For lint, I made lint happy with my additions, and also acquiesced to most of its other demands, though I left a few pre-existing issues alone.

./Makefile
      32: Line exceeds 78 characters.
     289: Line exceeds 78 characters.
     448: Line exceeds 78 characters.
./utilities/create-lp-wadl-and-apidoc.py
      13: '_pythonpath' imported but unused

To check everything out locally, you can apply the Apache changes if you want to, and then use curl to verify that you get different resources appropriately, all with the same Last Modified time.  The ETag should be stable, even if you make clean and make (or just remake the apidoc).

curl -vk https://api.launchpad.dev/beta/ -H "Accept: application/vnd.sun.wadl+xml"
curl -vk https://api.launchpad.dev/beta/ -H "Accept: application/vd.sun.wadl+xml"
curl -vk https://api.launchpad.dev/beta/ -H "Accept: application/json"
curl -vk https://api.launchpad.dev/1.0/ -H "Accept: application/vnd.sun.wadl+xml"
curl -vk https://api.launchpad.dev/1.0/ -H "Accept: application/vd.sun.wadl+xml"
curl -vk https://api.launchpad.dev/1.0/ -H "Accept: application/json"
curl -vk https://api.launchpad.dev/devel/ -H "Accept: application/vnd.sun.wadl+xml"
curl -vk https://api.launchpad.dev/devel/ -H "Accept: application/vd.sun.wadl+xml"
curl -vk https://api.launchpad.dev/devel/ -H "Accept: application/json"

These urls should also all work in the browswer: https://api.launchpad.dev/, https://api.launchpad.dev/beta, https://api.launchpad.dev/beta.html, https://api.launchpad.dev/devel, https://api.launchpad.dev/devel.html, https://api.launchpad.dev/1.0, https://api.launchpad.dev/1.0.html .

You should also be able to start up Launchpad and still use launchpadlib to access the dev instance (``lp-shell dev devel`` for instance).

For QA, again, there should be two stages to this: before Apache has changed and after.  In both cases, all of the above should work smoothly (change api.launchpad.dev to api.launchpad.net, and change ``lp-shell dev`` to ``lp-shell production``).

That's everything I can think of.  Thanks!
-- 
https://code.launchpad.net/~gary/launchpad/bug607961/+merge/67246
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug607961 into lp:launchpad.

=== modified file 'Makefile'
--- Makefile	2011-07-02 15:40:24 +0000
+++ Makefile	2011-07-07 20:11:29 +0000
@@ -40,7 +40,7 @@
 BZR_VERSION_INFO = bzr-version-info.py
 
 APIDOC_DIR = lib/canonical/launchpad/apidoc
-WADL_TEMPLATE = $(APIDOC_DIR).tmp/wadl-$(LPCONFIG)-%(version)s.xml
+APIDOC_TMPDIR = $(APIDOC_DIR).tmp/
 API_INDEX = $(APIDOC_DIR)/index.html
 
 # Do not add bin/buildout to this list.
@@ -78,8 +78,8 @@
 	rm -rf $(APIDOC_DIR) $(APIDOC_DIR).tmp
 	mkdir -p $(APIDOC_DIR).tmp
 	LPCONFIG=$(LPCONFIG) $(PY) ./utilities/create-lp-wadl-and-apidoc.py \
-	    --force "$(WADL_TEMPLATE)"
-	mv $(APIDOC_DIR).tmp $(APIDOC_DIR)
+	    --force "$(APIDOC_TMPDIR)"
+	mv $(APIDOC_TMPDIR) $(APIDOC_DIR)
 
 apidoc: compile $(API_INDEX)
 
@@ -486,4 +486,4 @@
 	reload-apache hosted_branches check_mailman check_config \
 	jsbuild jsbuild_minify clean_js clean_buildout buildonce_eggs \
 	build_eggs sprite_css sprite_image css_combine compile \
-	check_schema pydoctor clean_logs 
+	check_schema pydoctor clean_logs

=== modified file 'lib/canonical/launchpad/ftests/test_wadl_generation.py'
--- lib/canonical/launchpad/ftests/test_wadl_generation.py	2010-10-20 03:14:53 +0000
+++ lib/canonical/launchpad/ftests/test_wadl_generation.py	2011-07-07 20:11:29 +0000
@@ -5,21 +5,17 @@
 
 __metaclass__ = type
 
-import pkg_resources
-import shutil
-import subprocess
-import tempfile
-
 from testtools.matchers import StartsWith
 
 from zope.component import getUtility
 
-from canonical.launchpad.rest.wadl import generate_wadl, generate_html
-from canonical.launchpad.systemhomes import WebServiceApplication
+from canonical.launchpad.rest.wadl import (
+    generate_json,
+    generate_wadl,
+    )
 from canonical.testing import LaunchpadFunctionalLayer
 from lazr.restful.interfaces import IWebServiceConfiguration
 from lp.testing import TestCase
-from lp.testing.matchers import Contains
 
 
 class SmokeTestWadlAndDocGeneration(TestCase):
@@ -32,3 +28,9 @@
         for version in config.active_versions:
             wadl = generate_wadl(version)
             self.assertThat(wadl[:40], StartsWith('<?xml '))
+
+    def test_json(self):
+        config = getUtility(IWebServiceConfiguration)
+        for version in config.active_versions:
+            json = generate_json(version)
+            self.assertThat(json, StartsWith('{"'))

=== modified file 'lib/canonical/launchpad/rest/wadl.py'
--- lib/canonical/launchpad/rest/wadl.py	2011-01-13 17:16:28 +0000
+++ lib/canonical/launchpad/rest/wadl.py	2011-07-07 20:11:29 +0000
@@ -20,15 +20,16 @@
 from canonical.launchpad.webapp.vhosts import allvhosts
 
 
-def generate_wadl(version):
-    """Generate the WADL for the given version of the web service."""
+def _generate_web_service_root(version, mimetype):
+    """Generate the webservice description for the given version and mimetype.
+    """
     url = urlparse.urljoin(allvhosts.configs['api'].rooturl, version)
     # Since we want HTTPS URLs we have to munge the request URL.
     url = url.replace('http://', 'https://')
     request = WebServiceTestRequest(version=version, environ={
         'SERVER_URL': url,
         'HTTP_HOST': allvhosts.configs['api'].hostname,
-        'HTTP_ACCEPT': 'application/vd.sun.wadl+xml',
+        'HTTP_ACCEPT': mimetype,
         })
     # We then bypass the usual publisher processing by associating
     # the request with the WebServicePublication (usually done by the
@@ -39,6 +40,16 @@
     return request.publication.getApplication(request)(request)
 
 
+def generate_wadl(version):
+    """Generate the WADL for the given version of the web service."""
+    return _generate_web_service_root(version, 'application/vd.sun.wadl+xml')
+
+
+def generate_json(version):
+    """Generate the JSON for the given version of the web service."""
+    return _generate_web_service_root(version, 'application/json')
+
+
 def generate_html(wadl_filename, suppress_stderr=True):
     """Given a WADL file generate HTML documentation from it."""
     # If we're supposed to prevent the subprocess from generating output on
@@ -59,4 +70,3 @@
         raise subprocess.CalledProcessError(process.returncode, args)
 
     return output
-

=== modified file 'lib/canonical/launchpad/systemhomes.py'
--- lib/canonical/launchpad/systemhomes.py	2011-01-21 21:42:40 +0000
+++ lib/canonical/launchpad/systemhomes.py	2011-07-07 20:11:29 +0000
@@ -381,12 +381,15 @@
 
     cached_wadl = {}
 
+    # This should only be used by devel instances: production serves root
+    # WADL (and JSON) from the filesystem.
+
     @classmethod
     def cachedWADLPath(cls, instance_name, version):
         """Helper method to calculate the path to a cached WADL file."""
         return os.path.join(
             os.path.dirname(os.path.normpath(__file__)),
-            'apidoc', 'wadl-%s-%s.xml' % (instance_name, version))
+            'apidoc', version, '%s.wadl' % (instance_name,))
 
     def toWADL(self):
         """See `IWebServiceApplication`.

=== modified file 'utilities/create-lp-wadl-and-apidoc.py'
--- utilities/create-lp-wadl-and-apidoc.py	2010-11-19 14:22:15 +0000
+++ utilities/create-lp-wadl-and-apidoc.py	2011-07-07 20:11:29 +0000
@@ -10,59 +10,120 @@
     % LPCONFIG=development bin/py utilities/create-lp-wadl-and-apidoc.py \\
       "lib/canonical/launchpad/apidoc/wadl-development-%(version)s.xml"
 """
-import _pythonpath # Not lint, actually needed.
+import _pythonpath  # Not lint, actually needed.
 
 from multiprocessing import Process
 import optparse
 import os
 import sys
 
+import bzrlib
+from bzrlib.branch import Branch
 from zope.component import getUtility
 from zope.pagetemplate.pagetemplatefile import PageTemplateFile
 
-from canonical.launchpad.rest.wadl import generate_wadl, generate_html
+from canonical.launchpad.rest.wadl import (
+    generate_html,
+    generate_json,
+    generate_wadl,
+    )
 from canonical.launchpad.scripts import execute_zcml_for_scripts
 from canonical.launchpad.systemhomes import WebServiceApplication
 from lazr.restful.interfaces import IWebServiceConfiguration
 
 
-def write(filename, content):
+def write(filename, content, timestamp):
     """Replace the named file with the given string."""
     f = open(filename, 'w')
     f.write(content)
     f.close()
-
-
-def make_files(path_template, directory, version, force):
-    wadl_filename = path_template % {'version': version}
-    # If the WADL file doesn't exist or we're being forced to regenerate
-    # it...
-    if (not os.path.exists(wadl_filename) or force):
-        print "Writing WADL for version %s to %s." % (
-            version, wadl_filename)
-        write(wadl_filename, generate_wadl(version))
-    else:
-        print "Skipping already present WADL file:", wadl_filename
+    os.utime(filename, (timestamp, timestamp))  # (atime, mtime)
+
+
+def make_files(directory, version, timestamp, force):
+    version_directory = os.path.join(directory, version)
+    base_filename = os.path.join(version_directory, os.environ['LPCONFIG'])
+    wadl_filename = base_filename + '.wadl'
+    json_filename = base_filename + '.json'
+    html_filename = os.path.join(directory, version + ".html")
+    wadl_index = os.path.join(version_directory, 'index.wadl')
+    json_index = os.path.join(version_directory, 'index.json')
+    html_index = os.path.join(version_directory, 'index.html')
+    brokenwadl_index = os.path.join(version_directory, 'index.brokenwadl')
+
+    # Make sure we have our dir.
+    if not os.path.exists(version_directory):
+        # We expect the main directory to exist.
+        os.mkdir(version_directory)
+
+    # Make wadl and json files.
+    for src, dest, gen, name in (
+        (wadl_filename, wadl_index, generate_wadl, 'WADL'),
+        (json_filename, json_index, generate_json, 'JSON')):
+        # If the src doesn't exist or we are forced to regenerate it...
+        if (not os.path.exists(src) or force):
+            print "Writing %s for version %s to %s." % (
+                name, version, src)
+            write(src, gen(version), timestamp)
+        else:
+            print "Skipping already present %s file: %s" % (
+                name, src)
+        # Make "index" symlinks, removing any preexisting ones.
+        if os.path.exists(dest):
+            os.remove(dest)
+        os.symlink(os.path.basename(src), dest)
+
+    # Make the brokenwadl symlink.  This is because we need to support a
+    # misspelled wadl mimetype that some legacy launchpadlib versions used.
+    # Multiple attempts have been made to make this unnecessary, and removing
+    # it is welcome.  In particular, these two approaches were attempted in
+    # Apache.
+    #
+    # Approach 1 (a variant of example 4
+    # from http://httpd.apache.org/docs/2.0/mod/mod_headers.html)
+    # SetEnvIf Accept \Qapplication/vd.sun.wadl+xml\E X_WADL_MIME
+    # RequestHeader set Accept "application/vnd.sun.wadl+xml" env=X_WADL_MIME
+    # This, at least in combination with Apache's MultiViews, doesn't work
+    # in developer tests.
+    #
+    # Approach 2:
+    # In mime.conf,
+    # AddType application/vnd.sun.wadl+xml .wadl
+    # AddType application/vd.sun.wadl+xml .wadl
+    # In developer tests, it seems Apache only allows a single mime type
+    # for a given extension.
+    #
+    # Therefore, the approach we use is
+    # AddType application/vnd.sun.wadl+xml .wadl
+    # AddType application/vd.sun.wadl+xml .brokenwadl
+    # We support that here.
+    if not os.path.exists(brokenwadl_index):
+        os.symlink(os.path.basename(wadl_index), brokenwadl_index)
 
     # Now, convert the WADL into an human-readable description and
     # put the HTML in the same directory as the WADL.
-    html_filename = os.path.join(directory, version + ".html")
     # If the HTML file doesn't exist or we're being forced to regenerate
     # it...
     if (not os.path.exists(html_filename) or force):
         print "Writing apidoc for version %s to %s" % (
             version, html_filename)
         write(html_filename, generate_html(wadl_filename,
-            suppress_stderr=False))
+            suppress_stderr=False), timestamp)
     else:
         print "Skipping already present HTML file:", html_filename
 
-
-def main(path_template, force=False):
-    WebServiceApplication.cached_wadl = None # do not use cached file version
+    # Symlink the top-level version html in the version directory for
+    # completeness.
+    if not os.path.exists(html_index):
+        os.symlink(
+            os.path.join(os.path.pardir, os.path.basename(html_filename)),
+            html_index)
+
+
+def main(directory, force=False):
+    WebServiceApplication.cached_wadl = None  # do not use cached file version
     execute_zcml_for_scripts()
     config = getUtility(IWebServiceConfiguration)
-    directory = os.path.dirname(path_template)
 
     # First, create an index.html with links to all the HTML
     # documentation files we're about to generate.
@@ -73,11 +134,19 @@
     f = open(index_filename, 'w')
     f.write(template(config=config))
 
+    # Get the time of the last commit.  We will use this as the mtime for the
+    # generated files so that we can safely use it as part of Apache's etag
+    # generation in the face of multiple servers/filesystems.
+    with bzrlib.initialize():
+        branch = Branch.open(os.path.dirname(os.path.dirname(__file__)))
+        timestamp = branch.repository.get_revision(
+            branch.last_revision()).timestamp
+
     # Start a process to build each set of WADL and HTML files.
     processes = []
     for version in config.active_versions:
         p = Process(target=make_files,
-            args=(path_template, directory, version, force))
+            args=(directory, version, timestamp, force))
         p.start()
         processes.append(p)
 
@@ -89,7 +158,7 @@
 
 
 def parse_args(args):
-    usage = "usage: %prog [options] PATH_TEMPLATE"
+    usage = "usage: %prog [options] DIR"
     parser = optparse.OptionParser(usage=usage)
     parser.add_option(
         "--force", action="store_true",
@@ -97,7 +166,7 @@
     parser.set_defaults(force=False)
     options, args = parser.parse_args(args)
     if len(args) != 2:
-        parser.error("A path template is required.")
+        parser.error("A directory is required.")
 
     return options, args