Merge lp:~danilo/launchpad/bug-817398 into lp:launchpad

[Данило Шеган]

[r=jtv][bug=817398] Pass the path pottery is installed into as PYTHONPATH when running generate_translation_templates.py on the buildd slaves. Also link all the relevant bits for TranslationTemplateBuilds to show up in the builder history web pages together with their log files. Oh yeah, finally.

= Bug 817398: template generation (WIP) =

Template generation has stopped working when buildd default image has switched to Ubuntu Natty (as the development version). That included Python 2.7 by default and generate-translation-templates hard-coded Python 2.6 module path.

This is easy to fix, but this has been broken for quite some time (maybe a full year even) because there is no proper logging for TranslationTemplatesBuild jobs. I decided to introduce that to avoid further breakage going unnoticed for this long.

== Implementation details ==

1. configs/development/launchpad-lazr.conf: change the default development poppy port so it doesn't conflict with codehosting port, thus allowing both to be run locally simultaneously
2. utilities/make-lp-user fails for me with no default timeout function set, so setting one makes it work
3. generate-translation-templates is an actual fix for the bug 817398; as an extra measure of protection, I pass PYTHONPATH in the ultimate script run to ensure modules are found even if Python 2.8 ever comes out for instance
4. configure.zcml copies a similar declaration that SourcePackageRecipeBuild is using (otherwise, a removeSecurityProxy would have to be used inside buildmaster/model/manager.py when .status is being set, and in other places introduced in this branch)
5. translationtemplatebuild.py provides a nicer title for TranslationTemplateBuilds compared to the default, and provides a direct link to librarian files for logs from the builds (since we do not support private branches for template generation, no need to proxy these URLs).
6. translationtemplatesbuildbehavior.py introduces fetching of the log files and saving them like PackageBuildDerived does

I've only did the minimal test changes. I don't think I can spend more time on this branch, but I did spend a lot of time trying stuff out locally. Ideally, most of this code would be refactored to use the "generic" infrastructure provided by things like PackageBuildDerived, but that's more involved than it sounds.

== Tests ==

bin/test -vvt translationtemplatesbuild -t xx-build-behavior.txt -t TestPoppy

== Demo and Q/A ==

 * Set up translation import for a project (translations.l.n/<project/+configure-translations and translations.l.n/<project>/trunk/+translations-settings
 * Push a branch which should generate translation template (eg. lp:eog) to staging, link it to the trunk series (what was set-up above)
 * Run cronscripts/scan-branches.py to scan the branch and generate translation templates build job
 * Wait for build-manager to pick it up and build it (you may need to ask for it to be run on staging as "bin/twistd --logile ... --pidfile ... -y daemons/buildd-manager.tac -n" — -n to not daemonize it so you could see any exceptions if they happen)
 * When the job finishes, it should keep the log in the builder history available from the builder page on launchpad.net/builders

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  configs/development/launchpad-lazr.conf
  lib/canonical/buildd/generate-translation-templates
  lib/canonical/buildd/debian/changelog
  lib/lp/poppy/tests/test_poppy.py
  lib/lp/translations/configure.zcml
  lib/lp/translations/model/translationtemplatesbuild.py
  lib/lp/translations/model/translationtemplatesbuildbehavior.py
  lib/lp/translations/stories/buildfarm/xx-build-summary.txt
  lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py
  utilities/make-lp-user

./configs/development/launchpad-lazr.conf
      92: Line exceeds 78 characters.
     111: Line exceeds 78 characters.
     125: Line exceeds 78 characters.
./lib/canonical/buildd/generate-translation-templates
      59: Line exceeds 78 characters.
./lib/canonical/buildd/debian/changelog
     232: Line exceeds 78 characters.
     298: Line exceeds 78 characters.
     488: Line exceeds 78 characters.
     495: Line exceeds 78 characters.
     516: Line exceeds 78 characters.
     522: Line exceeds 78 characters.
     528: Line exceeds 78 characters.
     534: Line exceeds 78 characters.
     541: Line exceeds 78 characters.
     549: Line exceeds 78 characters.
     556: Line exceeds 78 characters.
     570: Line exceeds 78 characters.
     576: Line exceeds 78 characters.
     582: Line exceeds 78 characters.
     590: Line exceeds 78 characters.
     596: Line exceeds 78 characters.
     603: Line exceeds 78 characters.
     614: Line exceeds 78 characters.
     623: Line exceeds 78 characters.
     630: Line exceeds 78 characters.
     636: Line exceeds 78 characters.
     643: Line exceeds 78 characters.
     649: Line exceeds 78 characters.
     655: Line exceeds 78 characters.
     662: Line exceeds 78 characters.
     669: Line exceeds 78 characters.
     676: Line exceeds 78 characters.
     683: Line exceeds 78 characters.
     689: Line exceeds 78 characters.

[Jeroen T. Vermeulen (jtv)]

Looks good. As agreed on IRC, please make test_poppy.py get poppy's port number from the config so you don't have to hard-code matching port numbers.

It's a good thing I'm reviewing this branch. It is going to clash with one I have pending, which puts buildd-manager in a read-only transaction policy by default. Build-master callbacks that need to write to the database will then have to open an explicit write transaction, make their changes, and commit before returning. Otherwise there seems to be a risk of another builder committing or aborting your changes while your builder is waiting for a slave XMLRPC call to return (and we've had reports of problems that seem to be symptoms of this actually happening).

Thanks for fixing this. Lack of monitoring of these jobs has been a thorn in my side for years now!

review: Approve

=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2011-09-30 16:01:42 +0000
+++ configs/development/launchpad-lazr.conf	2011-10-19 12:27:49 +0000
@@ -239,6 +239,7 @@
 authentication_endpoint: http://xmlrpc-private.launchpad.dev:8087/authserver
 host_key_private=lib/lp/poppy/tests/poppy-sftp
 host_key_public=lib/lp/poppy/tests/poppy-sftp.pub
+port: tcp:5023
 
 [rabbitmq]
 launch: True

=== modified file 'lib/canonical/buildd/debian/changelog'
--- lib/canonical/buildd/debian/changelog	2011-09-26 06:30:07 +0000
+++ lib/canonical/buildd/debian/changelog	2011-10-19 12:27:49 +0000
@@ -1,3 +1,9 @@
+launchpad-buildd (81) hardy-cat; urgency=low
+
+  * generate-translation-templates: switch to Python 2.7.
+
+ -- Danilo Šegan <[e-mail]>  Mon, 17 Oct 2011 14:46:13 +0200
+
 launchpad-buildd (80) hardy-cat; urgency=low
 
   * binfmt-support demonstrated umount ordering issues for us.  LP: #851934

=== modified file 'lib/canonical/buildd/generate-translation-templates'
--- lib/canonical/buildd/generate-translation-templates	2010-12-21 17:52:32 +0000
+++ lib/canonical/buildd/generate-translation-templates	2011-10-19 12:27:49 +0000
@@ -41,7 +41,7 @@
 BUILDD_PACKAGE=canonical/buildd
 POTTERY=$BUILDD_PACKAGE/pottery
 # The script should be smarter about detecting the python version.
-PYMODULES=/usr/lib/pymodules/python2.6
+PYMODULES=/usr/lib/pymodules/python2.7
 echo -n "Default Python in the chroot is: "
 $BUILD_CHROOT/usr/bin/python --version
 
@@ -62,4 +62,5 @@
 
 # Enter chroot, switch back to unprivileged user, execute the generate script.
 $SUDO $CHROOT $BUILD_CHROOT \
-  $SU - $USER -c "$GENERATE_SCRIPT $BRANCH_URL $RESULT_NAME"
+  $SU - $USER \
+    -c "PYTHONPATH=$PYMODULES $GENERATE_SCRIPT $BRANCH_URL $RESULT_NAME"

=== modified file 'lib/lp/poppy/tests/test_poppy.py'
--- lib/lp/poppy/tests/test_poppy.py	2011-02-24 17:38:19 +0000
+++ lib/lp/poppy/tests/test_poppy.py	2011-10-19 12:27:49 +0000
@@ -83,7 +83,7 @@
     def __init__(self, root_dir, factory):
         self.root_dir = root_dir
         self._factory = factory
-        self.port = 5022
+        self.port = int(config.poppy.port.partition(':')[2])
 
     def addSSHKey(self, person, public_key_path):
         f = open(public_key_path, 'r')
@@ -221,7 +221,7 @@
 
         if transport is None:
             transport = self.server.getTransport()
-        transport.stat('foo/bar') # .stat will implicity chdir for us
+        transport.stat('foo/bar')  # .stat will implicity chdir for us
 
         self.server.disconnect(transport)
         self.server.waitForClose()
@@ -400,7 +400,7 @@
         self.assertRaises(
             ftplib.error_perm,
             f.storbinary,
-            'STOR '+'foo_source.changes',
+            'STOR ' + 'foo_source.changes',
             fake_file)
 
 

=== modified file 'lib/lp/translations/configure.zcml'
--- lib/lp/translations/configure.zcml	2011-08-29 00:13:22 +0000
+++ lib/lp/translations/configure.zcml	2011-10-19 12:27:49 +0000
@@ -659,7 +659,13 @@
     <!-- TranslationTemplatesBuild -->
     <class
         class="lp.translations.model.translationtemplatesbuild.TranslationTemplatesBuild">
-        <allow interface="lp.translations.interfaces.translationtemplatesbuild.ITranslationTemplatesBuild"/>
+        <require permission="launchpad.View" interface="lp.translations.interfaces.translationtemplatesbuild.ITranslationTemplatesBuild"/>
+        <!-- This is needed for BuildManager to run. The permission isn't
+             important; launchpad.Edit isn't actually held by anybody.
+             Inspired by the similar change for SourcePackageRecipeBuild. -->
+        <require permission="launchpad.Edit"
+            set_attributes="builder date_finished date_started log status" />
+
     </class>
     <securedutility
         component="lp.translations.model.translationtemplatesbuild.TranslationTemplatesBuild"

=== modified file 'lib/lp/translations/model/translationtemplatesbuild.py'
--- lib/lp/translations/model/translationtemplatesbuild.py	2011-05-04 04:10:58 +0000
+++ lib/lp/translations/model/translationtemplatesbuild.py	2011-10-19 12:27:49 +0000
@@ -13,12 +13,10 @@
     Reference,
     Storm,
     )
-from storm.store import Store
 from zope.interface import (
     classProvides,
     implements,
     )
-from zope.security.proxy import ProxyFactory
 
 from canonical.launchpad.interfaces.lpstorm import IStore
 from lp.buildmaster.model.buildfarmjob import BuildFarmJobDerived
@@ -49,6 +47,11 @@
     branch_id = Int(name='branch', allow_none=False)
     branch = Reference(branch_id, 'Branch.id')
 
+    @property
+    def title(self):
+        return u'Translation template build for %s' % (
+            self.branch.displayname)
+
     def __init__(self, build_farm_job, branch):
         super(TranslationTemplatesBuild, self).__init__()
         self.build_farm_job = build_farm_job
@@ -110,3 +113,10 @@
         return store.find(
             TranslationTemplatesBuild,
             TranslationTemplatesBuild.branch == branch)
+
+    @property
+    def log_url(self):
+        """See `IBuildFarmJob`."""
+        if self.log is None:
+            return None
+        return self.log.http_url

=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
--- lib/lp/translations/model/translationtemplatesbuildbehavior.py	2011-09-07 15:10:38 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py	2011-10-19 12:27:49 +0000
@@ -11,7 +11,9 @@
     'TranslationTemplatesBuildBehavior',
     ]
 
+import datetime
 import os
+import pytz
 import tempfile
 
 from twisted.internet import defer
@@ -20,6 +22,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     IBuildFarmJobBehavior,
     )
@@ -111,6 +114,35 @@
             if len(raw_slave_status) >= 4:
                 status['filemap'] = raw_slave_status[3]
 
+    def setBuildStatus(self, status):
+        self.build.status = status
+
+    @staticmethod
+    def getLogFromSlave(templates_build, queue_item):
+        """See `IPackageBuild`."""
+        SLAVE_LOG_FILENAME = 'buildlog'
+        builder = queue_item.builder
+        d = builder.transferSlaveFileToLibrarian(
+            SLAVE_LOG_FILENAME,
+            templates_build.buildfarmjob.getLogFileName(),
+            False)
+        return d
+
+    @staticmethod
+    def storeBuildInfo(build, queue_item, build_status):
+        """See `IPackageBuild`."""
+        def got_log(lfa_id):
+            build.build.log = lfa_id
+            build.build.builder = queue_item.builder
+            build.build.date_started = queue_item.date_started
+            # XXX cprov 20060615 bug=120584: Currently buildduration includes
+            # the scanner latency, it should really be asking the slave for
+            # the duration spent building locally.
+            build.build.date_finished = datetime.datetime.now(pytz.UTC)
+
+        d = build.getLogFromSlave(build, queue_item)
+        return d.addCallback(got_log)
+
     def updateBuild_WAITING(self, queue_item, slave_status, logtail, logger):
         """Deal with a finished ("WAITING" state, perversely) build job.
 
@@ -139,6 +171,7 @@
             # dangerous.
             if filename is None:
                 logger.error("Build produced no tarball.")
+                self.setBuildStatus(BuildStatus.FULLYBUILT)
                 return
 
             tarball_file = open(filename)
@@ -152,15 +185,23 @@
                         queue_item.specific_job.branch, tarball, logger)
                     logger.debug("Upload complete.")
             finally:
+                self.setBuildStatus(BuildStatus.FULLYBUILT)
                 tarball_file.close()
                 os.remove(filename)
 
-        if build_status == 'OK':
-            logger.debug("Processing successful templates build.")
-            filemap = slave_status.get('filemap')
-            d = self._readTarball(queue_item, filemap, logger)
-            d.addCallback(got_tarball)
-            d.addCallback(clean_slave)
-            return d
-
-        return clean_slave(None)
+        def build_info_stored(ignored):
+            if build_status == 'OK':
+                self.setBuildStatus(BuildStatus.UPLOADING)
+                logger.debug("Processing successful templates build.")
+                filemap = slave_status.get('filemap')
+                d = self._readTarball(queue_item, filemap, logger)
+                d.addCallback(got_tarball)
+                d.addCallback(clean_slave)
+                return d
+
+            self.setBuildStatus(BuildStatus.FAILEDTOBUILD)
+            return clean_slave(None)
+
+        d = self.storeBuildInfo(self, queue_item, build_status)
+        d.addCallback(build_info_stored)
+        return d

=== modified file 'lib/lp/translations/stories/buildfarm/xx-build-summary.txt'
--- lib/lp/translations/stories/buildfarm/xx-build-summary.txt	2011-07-01 19:17:03 +0000
+++ lib/lp/translations/stories/buildfarm/xx-build-summary.txt	2011-10-19 12:27:49 +0000
@@ -157,5 +157,5 @@
     Build history for ...
     1 ... 1 of 1 result
     ...
-    Translation template build
+    Translation template build for lp://dev/qblark
     Build started ... and finished ... taking 5 minutes

=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2011-05-27 21:12:25 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2011-10-19 12:27:49 +0000
@@ -3,8 +3,10 @@
 
 """Unit tests for TranslationTemplatesBuildBehavior."""
 
+import datetime
 import logging
 import os
+import pytz
 
 from testtools.deferredruntest import AsynchronousDeferredRunTest
 import transaction
@@ -17,6 +19,7 @@
 from canonical.librarian.utils import copy_and_close
 from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     IBuildFarmJobBehavior,
     )
@@ -57,6 +60,7 @@
         """
         self.builder = behavior._builder
         self.specific_job = behavior.buildfarmjob
+        self.date_started = datetime.datetime.now(pytz.UTC)
         self.destroySelf = FakeMethod()
 
 
@@ -161,6 +165,7 @@
         path = behavior.templates_tarball_path
         # Poke the file we're expecting into the mock slave.
         behavior._builder.slave.valid_file_hashes.append(path)
+
         def got_tarball(filename):
             tarball = open(filename, 'r')
             try:
@@ -201,6 +206,9 @@
                 queue_item, slave_status, None, logging), slave_call_log
 
         def build_updated(ignored):
+            self.assertEqual(BuildStatus.FULLYBUILT, behavior.build.status)
+            # Log file is stored.
+            self.assertIsNotNone(behavior.build.log)
             slave_call_log = behavior._builder.slave.call_log
             self.assertEqual(1, queue_item.destroySelf.call_count)
             self.assertIn('clean', slave_call_log)
@@ -240,6 +248,9 @@
                 queue_item, status_dict, None, logging)
 
         def build_updated(ignored):
+            self.assertEqual(BuildStatus.FAILEDTOBUILD, behavior.build.status)
+            # Log file is stored.
+            self.assertIsNotNone(behavior.build.log)
             self.assertEqual(1, queue_item.destroySelf.call_count)
             slave_call_log = behavior._builder.slave.call_log
             self.assertIn('clean', slave_call_log)
@@ -278,6 +289,7 @@
                 queue_item, status_dict, None, logging)
 
         def build_updated(ignored):
+            self.assertEqual(BuildStatus.FULLYBUILT, behavior.build.status)
             self.assertEqual(1, queue_item.destroySelf.call_count)
             slave_call_log = behavior._builder.slave.call_log
             self.assertIn('clean', slave_call_log)
@@ -321,6 +333,7 @@
                 queue_item, slave_status, None, logging)
 
         def build_updated(ignored):
+            self.assertEqual(BuildStatus.FULLYBUILT, behavior.build.status)
             entries = getUtility(
                 ITranslationImportQueue).getAllEntries(target=productseries)
             expected_templates = [

=== modified file 'utilities/make-lp-user'
--- utilities/make-lp-user	2011-07-11 03:49:12 +0000
+++ utilities/make-lp-user	2011-10-19 12:27:49 +0000
@@ -44,6 +44,7 @@
 
 from canonical.launchpad.interfaces.gpghandler import IGPGHandler
 from canonical.launchpad.scripts import execute_zcml_for_scripts
+from canonical.lazr.timeout import set_default_timeout_function
 from lp.registry.interfaces.gpg import GPGKeyAlgorithm, IGPGKeySet
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.ssh import ISSHKeySet
@@ -57,6 +58,7 @@
 DEFAULT_PASSWORD = 'test'
 factory = LaunchpadObjectFactory()
 
+set_default_timeout_function(lambda: 100)
 
 def make_person(username, email):
     """Create and return a person with the given username.
@@ -119,7 +121,7 @@
         print 'Registered SSH key: %s' % (filename,)
         break
     else:
-        print 'No SSH key files found in %s' % ssh_dir 
+        print 'No SSH key files found in %s' % ssh_dir
 
 
 def parse_fingerprints(gpg_output):