On Thu, Aug 19, 2010 at 5:13 PM, Julian Edwards
<email address hidden> wrote:
> On Thursday 19 August 2010 13:37:40 you wrote:
>> Review: Needs Fixing
>> Hey Julian,
>>
>> This looks like a big step toward build farm reliability -- thank you.
>>

Thanks for the changes. There are a few questions below, but I reckon
this'll be the last round.

>> Some of my comments are asking for further explanation, a few make
>> suggestions for moving some code around, and the rest are the normal
>> stylistic gotchas.
>>
>> Out of curiosity, did you discuss this change in detail with anyone before
>> landing?
>
> It's been so long since I started it I honestly can't remember.  I have vague
> recollections of mentioning failure counts to various people.
>

No worries. I ask because we have failure counting mechanisms in other
places in Launchpad (e.g. the branch puller) and it might have been
interesting to share patterns, variable names and the like.

>> > === modified file 'lib/lp/buildmaster/configure.zcml'
>> > --- lib/lp/buildmaster/configure.zcml       2010-06-15 14:34:50 +0000
>> > +++ lib/lp/buildmaster/configure.zcml       2010-08-18 17:01:23 +0000
>> > @@ -50,6 +50,9 @@
>> >
>> >          <require
>> >
>> >              permission="launchpad.Edit"
>> >              set_attributes="status"/>
>> >
>> > +        <require
>> > +            permission="launchpad.Admin"
>> > +            set_attributes="failure_count"/>
>> >
>> >      </class>
>> >      <securedutility
>> >
>> >          component="lp.buildmaster.model.buildfarmjob.BuildFarmJob"
>>
>> I guess this means that whatever increments this runs with admin-level
>> privileges.  Is that really such a good idea?
>
> For now yes, because it's only ever changed in "zopeless" scripts.  That is,
> Zope requres this to work, but doesn't actually apply any level of security to
> it.  :/
>
> I made it admin to discourage changing it in appservers.  There are other
> script-only-changing declarations like this, but zope.Public, with warnings in
> the zcml about exposing it externally!  I prefer the extra protection up
> front...
>

Makes sense. I mean, a better permission would be some kind of
"Forbidden" permission, I guess.

>> > === modified file 'lib/lp/buildmaster/manager.py'
>> > --- lib/lp/buildmaster/manager.py   2010-08-02 16:00:50 +0000
>> > +++ lib/lp/buildmaster/manager.py   2010-08-18 17:01:23 +0000
...
>> > +        :return: True if we disabled something, False if we did not.
>>
>> This seems like an odd thing to return. It doesn't seem to be used in the
>> code. What's it for?
>
> For the tests.
>

Yeah, but what about the tests? Or rather, why do the tests care about
something that the code doesn't?

>> > +            # Decide if we need to terminate the job or fail the
>> > +            # builder.
>> > +            self._incrementFailureCounts(builder)
>> > +            self.logger.info(
>> > +                "builder failure count: %s, job failure count: %s" % (
>> > +                    builder.failure_count,
>> > +
>> > builder.currentjob.specific_job.build.failure_count)) +
>> > BaseDispatchResult(slave=None).assessFailureCounts(builder)
>>
>> This line makes me wonder why assessFailureCounts is on BaseDispatchResult
>> at all.  Perhaps it should be a method on Builder that takes an optional
>> 'info' parameter?
>>
>> The above code would then read:
>>
>>   builder.gotFailure()
>>   self.logger.info(...)
>>   builder.assessFailureCounts()
>
> I don't think we should continue to bloat model classes with code that is only
> used in one place.  This is a buildd-manager-specific function.
>
> Anyway, bugger, I had intended to factor it out to a standalone function as I
> also realised that that line is ridiculous and again forgot :/
>

A standalone function would be a definite improvement, and fine by me.
I personally don't think it's bloat to say "a builder knows how to
handle builder failures", but I'm not going to push it.

>>
>> Also, perhaps there should be a current_build property on Builder.
>>
>>   @property
>>   def current_build(self):
>>       return self.currentjob.specific_job.build
>
> I don't really like that either I'm afraid!  It's masking three complicated
> queries here and dangerous to have as a property as it lulls you into a false
> sense of it really being a cheap operation.
>

I didn't know they were queries!

> I don't mind making it a regular method though, getCurrentBuildFarmJob()
>

That would be great. Out of curiosity, why such a long name? Do
builders ever have jobs that aren't on build farms?

>> > +        builder.failure_count += 1
>> > +        builder.currentjob.specific_job.build.failure_count += 1
>> > +
>>
>> Perhaps this should be a method on Builder. e.g.
>>
>>   class Builder:
>>
>>      # ...
>>
>>      def gotFailure(self):
>>          self.failure_count += 1
>>          self.currentjob.specific_job.build.failure_count += 1
>>
>> Likewise, there could also be a gotSuccess() that resets to 0.
>
> For the same reasons as before, I don't think this belongs on IBuilder.  Not
> to mention it's not just manipulating builder properties, it's changing the
> job's properties.
>

In answer to the second point:
  def gotFailure(self):
    self.failure_count += 1
    self.getCurrentBuild().gotFailure()

And as for bloat, I don't see why adding extra columns is not bloat.


>> > === modified file 'lib/lp/buildmaster/tests/test_manager.py'
>> > --- lib/lp/buildmaster/tests/test_manager.py        2010-08-06 10:48:49 +0000
>> > +++ lib/lp/buildmaster/tests/test_manager.py        2010-08-18 17:01:23 +0000
>>
>> ...
>>
>> > @@ -360,9 +360,17 @@
>> >
>> >              self.assertFalse(result.processed)
>> >
>> >          return d.addCallback(check_result)
>> >
>> > -    def testCheckDispatch(self):
>> > -        """`SlaveScanner.checkDispatch` is chained after dispatch
>> > requests. -
>> > +    def _setUpSlaveAndBuilder(self):
>> > +        # Helper function to set up a builder and its recording slave.
>> > +        slave = RecordingSlave('bob', 'http://foo.buildd:8221/',
>> > 'foo.host') +        bob_builder = getUtility(IBuilderSet)[slave.name]
>> > +        return slave, bob_builder
>> > +
>>
>> There's already a slave called 'bob' in the sampledata?
>
> There's a builder called 'bob'.  There are no slaves.
>
> There's also another called 'frog'.
>

OK. As long as there are meaningfully named constants, I'm happy. When
reading the test in the first place, one can't tell whether that
hostname and port number is significant or not.


>> Also, it might be a good idea to have this method guarantee that the
>> failure counts are zero.
>
> I considered that, but the methods that call this one set the counts to
> varying values.
>

I see that, but I do think it would be clearer, and less surprising
for anyone who adds more tests in future.


>> > +        manager.scheduleNextScanCycle = FakeMethod()
>> > +        self.patch(BaseDispatchResult, 'assessFailureCounts',
>> > FakeMethod())
>>
>> Why are you stubbing out assessFailureCounts here?
>
> Because at the bottom of the method you'll see that it's checking the
> call_count.
>

Yeah, but why are you checking that it's called? Just as easy and more
robust to check that the state is what you expect.

>>
>> >          builder, job_id = self._getBuilder('bob')
>> >
>> >          # Setup a interaction to satisfy 'write_transaction' decorator.
>> >          login(ANONYMOUS)
>> >          slave = RecordingSlave(builder.name, builder.url,
>> >          builder.vm_host) result = FailDispatchResult(slave, 'does not
>> >          work!')
>> >
>> > +        result.assessFailureCounts = FakeMethod()
>>
>> Why are you stubbing it out here?
>
> So that I can see if it got called.
>

As above, why do you care?

>>
>> > +        self.assertEqual(0, result.assessFailureCounts.call_count)
>>
>> This assertion seems a little redundant.
>
> Well, it's fairly standard testing practice, no?  How else do I know that
> calling result() has changed the count?
>

By checking to see that the count is different.

> === modified file 'lib/lp/buildmaster/manager.py'
> --- lib/lp/buildmaster/manager.py	2010-08-18 16:58:18 +0000
> +++ lib/lp/buildmaster/manager.py	2010-08-19 15:51:41 +0000
> @@ -141,10 +141,55 @@
>          return d
>
>
> +def get_builder(name):
> +    """Helper to return the builder given the slave for this request."""
> +    # Avoiding circular imports.
> +    from lp.buildmaster.interfaces.builder import IBuilderSet
> +    return getUtility(IBuilderSet)[name]
> +
> +
> +def assessFailureCounts(builder, fail_notes):
> +    """View builder/job failure_count and work out which needs to die.
> +
> +    :return: True if we disabled something, False if we did not.
> +    """
> +    current_job = builder.currentjob
> +    build_job = current_job.specific_job.build
> +

Or indeed, build_job = builder.getCurrentBuildFarmJob()

> === modified file 'lib/lp/buildmaster/tests/test_manager.py'
> --- lib/lp/buildmaster/tests/test_manager.py	2010-08-18 16:58:18 +0000
> +++ lib/lp/buildmaster/tests/test_manager.py	2010-08-19 16:06:10 +0000
> @@ -7,7 +7,6 @@
>  import signal
>  import time
>  import transaction
> -import unittest
>
>  from twisted.internet import defer, reactor, task
>  from twisted.internet.error import ConnectionClosed
> @@ -38,6 +37,7 @@
>  from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
>  from lp.testing.factory import LaunchpadObjectFactory
>  from lp.testing.fakemethod import FakeMethod
> +from lp.testing.sampledata import BOB_THE_BUILDER_NAME

There's got to be a better name. What are the interesting properties of the
builder that make it worth using?

> @@ -362,7 +363,8 @@
>
>      def _setUpSlaveAndBuilder(self):
>          # Helper function to set up a builder and its recording slave.
> -        slave = RecordingSlave('bob', 'http://foo.buildd:8221/', 'foo.host')
> +        slave = RecordingSlave(
> +            BOB_THE_BUILDER_NAME, 'http://foo.buildd:8221/', 'foo.host')
>          bob_builder = getUtility(IBuilderSet)[slave.name]
>          return slave, bob_builder
>

As mentioned in my reply, are these URLs or domain names special in any way?
If not, why the port number?

jml