Merge lp:~julian-edwards/launchpad/hanging-builder-edit-bug-583905 into lp:launchpad

[Julian Edwards]

= Summary =
Stop timeouts on Builder:+edit

== Pre-implementation notes ==
It turns out that when the edit form is saved, the Snapshot stuff is kicked
off to grab the old object state to provide in event notifications. The
problem is that one of the properties it queries (is_available) is actually
some code that tries to send an xmlrpc request to the build slave itself.
Because this request is coming from a webapp, it hits a fairly solid firewall
until the connection times out.

== Implementation details ==
I've added notify_modified=False to the call to updateContextFromData() which
stops it from taking snapshots of the object state. The comment in the code
explains the rationale, I've repeated it here:

        # notify_modified is set False here because it uses
        # lazr.lifecycle.snapshot to store the state of the object
        # before and after modification. This is dangerous for the
        # builder model class because it causes some properties to be
        # queried that try and communicate with the slave, which cannot
        # be done from the webapp (it's generally firewalled). We could
        # prevent snapshots for individual properties by defining the
        # interface properties with doNotSnapshot() but this doesn't
        # guard against future properties being created.

== Tests ==
I've added a new unit test for builder views. It stubs out the call to a
method slaveStatusSentence() which is called by is_available, and sets
slave_called to True so we can assert whether it was called or not.

== Demo and Q/A ==
There's no way of QA-ing this since it's an artefact of the production
firewall set-up. The dogfood builders are not firewalled in the same way.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/browser/tests/test_builder_views.py
  lib/lp/soyuz/browser/builder.py

[Michael Nelson (michael.nelson)]

16:40 < noodles775> bigjools: why do you remove the proxy on the test case's instance var, rather than just when you need to set something that you normally wouldn't be able to?
16:45 < noodles775> bigjools: also, you could use FakeMethod there if you wanted to... it would mean that you don't need to set slave_called on the builder, which would also mean you don't need removeSecurityProxy right?
16:46 < bigjools> noodles775: proxy is removed because...ummm I can't remember, let me try without it again since I've changed the code a bit since I added it (for good reason at the time)
16:47 < noodles775> I'm guessing it's because you're setting builder.slave_called, which isn't a interface attribute (well, it's not an attribute at all except for your test is it?)
16:47 < bigjools> indeed
16:47 < bigjools> ah, it's because I can't override slaveStatusSentence otherwise
16:48 < bigjools> well, I suppose I could name it the same
16:49 < noodles775> Could you do the following in setUp: removeSecurityProxy(self.builder).slaveStatusSentence = FakeMethod(...)
16:50 < noodles775> Then your test could just check self.builder.slaveStatusSentence.call_count == 0
16:50 < bigjools> yeah, let me try it, I wasn't aware of FakeMethod
16:51 < noodles775> OK, with that r=me

review: Approve (code)

=== modified file 'lib/lp/soyuz/browser/builder.py'
--- lib/lp/soyuz/browser/builder.py	2010-04-09 15:46:09 +0000
+++ lib/lp/soyuz/browser/builder.py	2010-05-27 09:35:43 +0000
@@ -335,7 +335,17 @@
     @action(_('Change'), name='update')
     def change_details(self, action, data):
         """Update the builder with the data from the form."""
-        builder_was_modified = self.updateContextFromData(data)
+        # notify_modified is set False here because it uses
+        # lazr.lifecycle.snapshot to store the state of the object
+        # before and after modification.  This is dangerous for the
+        # builder model class because it causes some properties to be
+        # queried that try and communicate with the slave, which cannot
+        # be done from the webapp (it's generally firewalled).  We could
+        # prevent snapshots for individual properties by defining the
+        # interface properties with doNotSnapshot() but this doesn't
+        # guard against future properties being created.
+        builder_was_modified = self.updateContextFromData(
+            data, notify_modified=False)
 
         if builder_was_modified:
             notification = 'The builder "%s" was updated successfully.' % (

=== added file 'lib/lp/soyuz/browser/tests/test_builder_views.py'
--- lib/lp/soyuz/browser/tests/test_builder_views.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/browser/tests/test_builder_views.py	2010-05-27 09:35:43 +0000
@@ -0,0 +1,49 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from zope.security.proxy import removeSecurityProxy
+from canonical.launchpad.ftests import login
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing import LaunchpadFunctionalLayer
+from lp.soyuz.browser.builder import BuilderEditView
+from lp.testing import TestCaseWithFactory
+from lp.testing.fakemethod import FakeMethod
+
+
+class TestBuilderEditView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestBuilderEditView, self).setUp()
+        # Login as an admin to ensure access to the view's context
+        # object.
+        login('[e-mail]')
+        self.builder = removeSecurityProxy(self.factory.makeBuilder())
+
+    def initialize_view(self):
+        form = {
+            "field.manual" : "on",
+            "field.actions.update" : "Change",
+            }
+        request = LaunchpadTestRequest(method="POST", form=form)
+        view = BuilderEditView(self.builder, request)
+        return view
+
+    def test_posting_form_doesnt_call_slave_xmlrpc(self):
+        # Posting the +edit for should not call is_available, which
+        # would do xmlrpc to a slave builder and is explicitly forbidden
+        # in a webapp process.
+        view = self.initialize_view()
+
+        # Stub out the slaveStatusSentence() method with a fake one that
+        # records if it's been called.
+        view.context.slaveStatusSentence = FakeMethod(result=[0])
+
+        view.initialize()
+
+        # If the dummy slaveStatusSentence() was called the call count
+        # would not be zero.
+        self.assertTrue(view.context.slaveStatusSentence.call_count == 0 )