469364 - en-US nightlies should trigger l10n builds via a trigger step

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Axel Hecht [:Pike]]

The current setup for l10n nightly builds is as a dependent scheduler of the en-US nightly.

That comes with the drawback that in the event that the en-US nightly sucks and needs to be respun, we don't get l10n nightlies automagically.

Moving the scheme over from a dependent scheduler to a triggered one would make the l10n nightlies a integrated target.

Comment 1

[[e-mail] (:bhearsum)]

I'm pretty sure we're not going to make this change anytime soon.

Comment 2

[Axel Hecht [:Pike]]

5 minute change to make l10n nightlies not fragile? This should really be a ride-along patch while doing something real.

Comment 3

[Armen [:armenzg]]

(In reply to comment #2)
> 5 minute change to make l10n nightlies not fragile? This should really be a
> ride-along patch while doing something real.
>
I think it is not a 5 minute change. I believe that the trigger steps only trigger Triggerable schedulers 
* http://buildbot.net/repos/release/docs/buildbot.html#Triggering-Schedulers
and to the difference of the other schedulers it returns a defered
* http://mxr.mozilla.org/mozilla/source/tools/buildbot/buildbot/scheduler.py#734
and I wonder what I am supposed to return if instead of one buildset I am generating 60+ buildsets

Comment 4

[Axel Hecht [:Pike]]

I actually don't think that the en-US step should waitForFinish, so the return value would actually be moot. Just returning a defer.succeed to wallpaper the second code path in http://mxr.mozilla.org/mozilla/source/tools/buildbot/buildbot/steps/trigger.py#108 should be good enough.

This isn't just opportunistic, IMHO, but the right thing to do, at least as long as "nightly" consists of the en-US binary.

If we move the meaning of "do the nightly" over to include all locales, which we might later one, I'd be happy to come up with a follow up patch that actually creates the right callbacks. We should just file a follow up bug and reference that in the code, just in case.

Comment 5

[Chris Cooper [:coop] (he/him)]

From the comments, it's unclear who's actually working on this. Both Axel and Armen seem to have contemplated a fix, but the bug is assigned to Axel.

I admit this kind of fell off my radar. This was actually a Q1 goal for releng that never got addressed and has been rolled over into Q2 so I'd like to get some traction on it.

Comment 6

[Chris Cooper [:coop] (he/him)]

Axel: any updated thoughts on this before I begin in earnest?

Comment 7

[Axel Hecht [:Pike]]

Without deeply looking into it, here's my braindump. I think this is going to lead into some refactoring on the L10nMixin stuff, to pick up the tree, and via the tree the locales and builder infos (or not, see below) from the L10nScheduler.

Given that we have the l10n tree in config.py now, this should blend rather nicely these days.

Hrm. Actually, this would be easier if the nightly builder was the same as the dep build. Right now, you have to pick a different builder, if it was the same builder, you could just tweak the build properties to trigger a nightly with mars etc.

Comment 8

[Chris Cooper [:coop] (he/him)]

In my naive first-pass at this, I tried simply making NightlyL10n also inherit from Triggerable. I was able to trigger builds at the end of the en-US, but the builds would fail because branch/en_revision were not being set, although those properties are set just fine when the scheduler triggers via other means, e.g. via hour/minute.

Comment 9

[Axel Hecht [:Pike]]

When subclassing, you probably need to overload trigger() to call into createL10nBuilds. That should set the properties.

Comment 10

[Armen [:armenzg]]

BTW DependentL10n was removed because it would get lost after a reconfig bug 472333.
The upstream bug related with this scheduler's *confusion* is http://buildbot.net/trac/ticket/35 which 7 weeks ago a patch landed for it (http://buildbot.net/trac/changeset/4066acfdd6477e59b00767c1c5607e4666e15d6d)

Comment 11

[Chris Cooper [:coop] (he/him)]

(In reply to comment #9)
> When subclassing, you probably need to overload trigger() to call into
> createL10nBuilds. That should set the properties.

Thanks for the suggestion, Axel. I've got something running in staging now that has worked for me locally.

Comment 12

[Armen [:armenzg]]

We should not continue with this bug until we fix the update generation situation. It will worsen the en-US experience for all platform instead of just win32 - (bug 511901)

Comment 13

[Chris Cooper [:coop] (he/him)]

(In reply to comment #12)
> We should not continue with this bug until we fix the update generation
> situation. It will worsen the en-US experience for all platform instead of just
> win32 - (bug 511901)

Oh, I'm still going to work on it (since it's taken me this long), I just don't plan on landing it yet.

Comment 14

[Chris Cooper [:coop] (he/him)]

Attachment: Switch l10n nightlies to trigger after the en-US nightlies complete

I had to integrate the trigger step into the en-US nightly factory rather than just appending it to the end because of the reboot step that occasionally gets run at the end.

This ran successfully in staging over the weekend. I wouldn't want to land this though until Armen gets something working for bug 511901.

One thing I would like to add is setting the reason for builds kicked off via the trigger, but I'm not sure how to do that via the callbacks.

Comment 15

[Chris Cooper [:coop] (he/him)]

Attachment: Fix-up set_properties mismatch

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 396232 [details] [diff] [review]
Fix-up set_properties mismatch

r-'ish, can you use a consistent name for 'triggerOtherBuilds'?

And for the reason, just add an optional argument to createL10nBuilds, and pass that to the callback at http://hg.mozilla.org/build/buildbotcustom/file/dbf136b09880/l10n.py#l944, and in _cbLoadedLocales, do something like
if reason is None:
  reason = self.reason

Comment 17

[Armen [:armenzg]]

Comment on attachment 396232 [details] [diff] [review]
Fix-up set_properties mismatch

small nits. r+ with these changes

>-class NightlyL10n(Nightly, L10nMixin):
>+class NightlyL10n(Triggerable, L10nMixin):
>   """
>   NightlyL10n is used to paralellize the generation of l10n builds.
can you change the name to TrigerableL10n here and where we use it in other places?


>-    
>-    Nightly.__init__(self, name, builderNames, minute, hour, dayOfMonth, month,
>-                     dayOfWeek, branch, properties={'nightly': True})
>+    self.branch = branch 
where is this used later on?


(In reply to comment #16)
> (From update of attachment 396232 [details] [diff] [review])
> r-'ish, can you use a consistent name for 'triggerOtherBuilds'?
> 
I think using that naming helps to understand that if we later on want to hook "other" things after an en-US nightly happens (aside of triggering l10n builds).

Comment 18

[Chris Cooper [:coop] (he/him)]

(In reply to comment #17)
> can you change the name to TrigerableL10n here and where we use it in other
> places?

Do we really care if the scheduler is called NightlyL10n vs. TriggerableL10n internally? I kept the NightlyL10n name because that's our actual use case, but I'm happy to go with only the TriggerableL10n class, or create NightlyL10n as a thin wrapper class for TriggerableL10n.

> >+    self.branch = branch 
> where is this used later on?

Like your DependentL10n scheduler, buildbot complains if this is not set at the outset.

> > r-'ish, can you use a consistent name for 'triggerOtherBuilds'?
> > 
> I think using that naming helps to understand that if we later on want to hook
> "other" things after an en-US nightly happens (aside of triggering l10n
> builds).

Armen and/or Axel: any suggestions for nomenclature here? My new, unposted patch is using self.triggerBuild and self.addTriggeredBuildSteps() in MercurialBuildFactory, but I'd rather not flail through r- cycles if you have better ideas.

Comment 19

[Axel Hecht [:Pike]]

I'd go for triggerBuilds and addTriggeredBuildsSteps, with plural Builds. It'd be good to have a comment that this is used to kick off l10n nightlies.

Comment 20

[Armen [:armenzg]]

(In reply to comment #18)
> (In reply to comment #17)
> > can you change the name to TrigerableL10n here and where we use it in other
> > places?
> 
> Do we really care if the scheduler is called NightlyL10n vs. TriggerableL10n
> internally? I kept the NightlyL10n name because that's our actual use case, but
> I'm happy to go with only the TriggerableL10n class, or create NightlyL10n as a
> thin wrapper class for TriggerableL10n.
> 
It was just for the sake to match the name to the behaviour of the class but I see what you mean as well. Either way works for me

(In reply to comment #18)
> Armen and/or Axel: any suggestions for nomenclature here?
The plural naming sounds good

Comment 21

[Chris Cooper [:coop] (he/him)]

Attachment: Address review comments, set reason for triggered builds

Comment 22

[Chris Cooper [:coop] (he/him)]

Comment on attachment 396466 [details] [diff] [review]
Address review comments, set reason for triggered builds

Axel: gentle ping re: review. Any outstanding nits?

Comment 23

[Axel Hecht [:Pike]]

Comment on attachment 396466 [details] [diff] [review]
Address review comments, set reason for triggered builds

As per call, r-. Let's pass the trigger properties onwards. That might be cool for example to pass in the dated nightly dir for download, which makes it yet more clear which things got built. I'd like to keep the nightly=True property, too, I guess.

I'll nitpick on 'reason' to be just flying around in the callbacks as I go.

>diff --git a/l10n.py b/l10n.py
>--- a/l10n.py
>+++ b/l10n.py
>@@ -38,7 +38,7 @@
> 
> from twisted.python import log as log2
> from buildbotcustom import log
>-from buildbot.scheduler import BaseUpstreamScheduler, Nightly, Dependent 
>+from buildbot.scheduler import BaseUpstreamScheduler, Dependent, Triggerable 
> from buildbot.sourcestamp import SourceStamp
> from buildbot import buildset, process
> from buildbot.changes import changes
>@@ -841,7 +841,8 @@ class L10nMixin(object):
>                localesFile = None,
>                cvsRoot     = DEFAULT_CVSROOT,
>                locales     = None,
>-               tree        = None):
>+               tree        = None,
>+               reason      = None):

Not needed, afaict.

>       self.repoType = repoType
>       self.baseTag = baseTag
>       self.cvsRoot = cvsRoot
>@@ -862,6 +863,7 @@ class L10nMixin(object):
>       # each key to build a specific locale e.g. locales={'fr':['osx']}
>       self.locales = locales
>       self.tree = tree
>+      self.reason = reason

nor here ..

>       # Make sure a supported platform is passed. Allow variations, but make
>       # sure to convert them to the form the locales files ues.
>       assert platform in ('linux', 'win32', 'macosx', 'osx')
>@@ -934,12 +936,13 @@ class L10nMixin(object):
>           d.addCallback(lambda data: ParseLocalesFile(data))
>           return d
> 
>-  def createL10nBuilds(self):
>+  def createL10nBuilds(self, reason=None):
>       """
>       We request to get the locales that we have to process and which
>       method to call once they are ready
>       """
>       log2.msg('L10nMixin:: A list of locales is going to be requested')
>+      self.reason = reason

... and move this to

>       d = defer.maybeDeferred(self.getLocales)
>       d.addCallback(self._cbLoadedLocales)

d.addCallback(self._cbLoadedLocales, reason=reason, set_props=set_props)
>       return d
>@@ -964,11 +967,11 @@ def ParseLocalesFile(data):
>     return locales
> 
> 
>-class NightlyL10n(Nightly, L10nMixin):
>+class TriggerableL10n(Triggerable, L10nMixin):
>   """
>-  NightlyL10n is used to paralellize the generation of l10n builds.
>+  TriggerableL10n is used to paralellize the generation of l10n builds.
> 
>-  NightlyL10n is designed to be used with a Build factory that gets the
>+  TriggerableL10n is designed to be used with a Build factory that gets the
>   locale to build from the 'locale' build property.
>   """
> 
>@@ -981,18 +984,18 @@ class NightlyL10n(Nightly, L10nMixin):
>                repo = 'http://hg.mozilla.org/', branch=None, baseTag='default',
>                localesFile=None, cvsRoot=DEFAULT_CVSROOT, locales=None,
>                tree="notset"):
>-    
>-    Nightly.__init__(self, name, builderNames, minute, hour, dayOfMonth, month,
>-                     dayOfWeek, branch, properties={'nightly': True})
>+    self.branch = branch 
>+    self.reason = None 
>     L10nMixin.__init__(self, platform = platform, repoType = repoType,
>                        repo = repo, branch = branch, baseTag = baseTag,
>                        localesFile = localesFile, cvsRoot = cvsRoot,
>                        locales = locales, tree = tree)
>-  
>-  def doPeriodicBuild(self):
>-    # Schedule the next run (as in Nightly's doPeriodicBuild)
>-    self.setTimer()
>-    self.createL10nBuilds()
>+    Triggerable.__init__(self, name, builderNames)
>+
>+  def trigger(self, ss, set_props={}):
>+      reason="This build was triggered by the successful completion of the en-US nightly."
>+      self.createL10nBuilds(reason)

Make this self.createL10nBuilds(reason=reason, set_props=set_props)

...

and then make

  def _cbLoadedLocales(self, locales):

  def _cbLoadedLocales(self, locales, reason=None, set_props=None):

and not refer to self.reason but just reason, and copy the set_props into props, with 'Scheduler' as source again.

Comment 24

[Chris Cooper [:coop] (he/him)]

Attachment: Set reason via callback, perpetuate set_props

Tested this patch version successfully this afternoon.

Comment 25

[Axel Hecht [:Pike]]

Comment on attachment 397330 [details] [diff] [review]
Set reason via callback, perpetuate set_props

r=me, thanks.

Comment 26

[Chris Cooper [:coop] (he/him)]

Comment on attachment 397330 [details] [diff] [review]
Set reason via callback, perpetuate set_props

http://hg.mozilla.org/build/buildbotcustom/rev/e140ab447423

Comment 27

[Chris Cooper [:coop] (he/him)]

Production-master* reconfig-ed. Will leave open until a successful trigger cycle, hopefully tomorrow AM.

Comment 28

[Chris Cooper [:coop] (he/him)]

Working as expected. Will work out the effects on nightly updates in other bugs, specifically bug 513763.

Comment 29

[Armen [:armenzg]]

The only en-US updates that got delayed (around 11:17PDT) are:
- Firefox/mozilla-1.9.2/WINNT_x86-msvc/en-US/20090831051240 (build end time 07:06:29)
- Firefox/mozilla-central/WINNT_x86-msvc/en-US/20090831044843 (build end time 06:45:56)
- Firefox/trunk/WINNT_x86-msvc/en-US/2009083106 <- why is the build id so short? what is "trunk"?
- Thunderbird/2.0/Darwin_Universal-gcc3/en-US/2009083104
every other en-US was generated before 6:13AM PDT

Comment 30

[Chris Cooper [:coop] (he/him)]

(In reply to comment #29)
> - Firefox/trunk/WINNT_x86-msvc/en-US/2009083106 <- why is the build id so
> short? what is "trunk"?

Odd. Can we do some digging there to find out where it's coming from?

Comment 31

[[e-mail] (:bhearsum)]

(In reply to comment #30)
> (In reply to comment #29)
> > - Firefox/trunk/WINNT_x86-msvc/en-US/2009083106 <- why is the build id so
> > short? what is "trunk"?
> 
> Odd. Can we do some digging there to find out where it's coming from?

This is a Firefox 3.0.x nightly