560883 - Trigger partner repacks as soon as l10n repacks are finished

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P5)

Description

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

We need to figure out a way to trigger the partner repacks as soon as all l10n repacks are finished in release automation. Right now, the partner repack step is triggered by the end of the manual signing step, but we'd like to be able to do all signing in a single step (bug 554321).

catlee: at our original Q2 goals meeting you had mentioned you (read: your group) might be willing to take this on. Is that still valid?

Comment 1

[[e-mail] (:bhearsum)]

Looks like I'll be doing this

Comment 2

[[e-mail] (:bhearsum)]

I did some digging into what needs to happen to make this possible. Here's what I think we should do:
* Patch Buildbot to enable BuildSets which contain multiple BuildRequests on the same Builder.
** This mostly involves work in DBConnector.create_buildset. A complementary method may have to be added to avoid breaking compatibility.
* Modify L10nMixin to create a single BuildSet instead of one per locale
* Split 'partner_repack' builder into a per-platform factory and add a Dependent Scheduler to trigger them, that watches the appropriate $platform_repack builder.

Comment 3

[[e-mail] (:bhearsum)]

(In reply to comment #2)
> I did some digging into what needs to happen to make this possible. Here's what
> I think we should do:
> * Patch Buildbot to enable BuildSets which contain multiple BuildRequests on
> the same Builder.
> ** This mostly involves work in DBConnector.create_buildset. A complementary
> method may have to be added to avoid breaking compatibility.

Turns out that this won't work like I originally thought it would. Properties are attached to each BuildSet, which means that we need a BuildSet per locale. With that being the case, the Dependent Scheduler isn't any help.

Back to the drawing board.

Comment 4

[[e-mail] (:bhearsum)]

(In reply to comment #3)
> Back to the drawing board.

I've come up with a few different ideas for how to do this, none of them are pretty:
- Subclass either the Triggerable Scheduler or Trigger step to only fire the triggered builders every N builds. This probably doesn't work if a reconfig happens while l10n builds are going. It may also break down if l10n builds fail.
- Poll FTP and fire a build when all locales for a platform show up
- Poll FTP for a special file per platform which would be created by post_upload.py once all locales are finished.

Out of those options, 2) seems the least bad.

Comment 5

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

(In reply to comment #4)
> - Poll FTP for a special file per platform which would be created by
> post_upload.py once all locales are finished.

I actually think #3 might be the easiest option. We could upload shipped-locales to post_upload.py (possibly even pre-processed by platform) and then use that to decide whether all locales are ready yet for a given platform.

For bonus points, we could also write out a separate file that contained the list of locales still pending so we could tell at a glance which locales remained to be processed.

Comment 6

[[e-mail] (:bhearsum)]

(In reply to comment #5)
> (In reply to comment #4)
> > - Poll FTP for a special file per platform which would be created by
> > post_upload.py once all locales are finished.
> 
> I actually think #3 might be the easiest option. We could upload
> shipped-locales to post_upload.py (possibly even pre-processed by platform) and
> then use that to decide whether all locales are ready yet for a given platform.

I feel like it's way out of scope for post_upload.py to be doing something like this, but it might be possible to add some sort of generic extra action to it...I'll give that some thought.

> For bonus points, we could also write out a separate file that contained the
> list of locales still pending so we could tell at a glance which locales
> remained to be processed.

I'm not sure I'm on board with this; we can already see the number of pending repacks on the waterfall, and it's pretty out of scope for this bug...

Comment 7

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

(In reply to comment #6)
> I feel like it's way out of scope for post_upload.py to be doing something like
> this, but it might be possible to add some sort of generic extra action to
> it...I'll give that some thought.

A separate script then perhaps that could be called as a separate build step after post_upload?

Comment 8

[[e-mail] (:bhearsum)]

(In reply to comment #7)
> (In reply to comment #6)
> > I feel like it's way out of scope for post_upload.py to be doing something like
> > this, but it might be possible to add some sort of generic extra action to
> > it...I'll give that some thought.
> 
> A separate script then perhaps that could be called as a separate build step
> after post_upload?

I'm really not sold on this solution, to be honest. To me, it's dirtier than polling and further pollutes the candidates directory. I've made good progress on the polling, though.

Comment 9

[[e-mail] (:bhearsum)]

Attachment: refactor FtpPoller to support regexes

Comment 10

[[e-mail] (:bhearsum)]

Attachment: buildbot-configs to use the new poller

Comment 11

[Chris AtLee [:catlee]]

Comment on attachment 465637 [details] [diff] [review]
refactor FtpPoller to support regexes

>+class FtpPoller(FtpPollerBase):
>+    gotFile = 1

Can you find a better name for this variable?  Also, I've found using True/False/None good tri-state values rather than 0,1,2.

>+def createLocaleDirListRegex(locales):
>+    regex = "/.*".join(sorted(locales)) + "/.*"
>+    return re.compile(regex, re.DOTALL)

I'm not sure this is doing what you want....Passing in ["en-US", "de"] creates a regex that looks like
'de/.*en-US/.*'

do you want this to be '(de|en-US)/.*'?  Maybe I'm not getting what this is supposed to be matching.

Comment 12

[[e-mail] (:bhearsum)]

(In reply to comment #11)
> Comment on attachment 465637 [details] [diff] [review]
> refactor FtpPoller to support regexes
> 
> >+class FtpPoller(FtpPollerBase):
> >+    gotFile = 1
> 
> Can you find a better name for this variable?  Also, I've found using
> True/False/None good tri-state values rather than 0,1,2.

Yup.

> >+def createLocaleDirListRegex(locales):
> >+    regex = "/.*".join(sorted(locales)) + "/.*"
> >+    return re.compile(regex, re.DOTALL)
> 
> I'm not sure this is doing what you want....Passing in ["en-US", "de"] creates
> a regex that looks like
> 'de/.*en-US/.*'
> 
> do you want this to be '(de|en-US)/.*'?  Maybe I'm not getting what this is
> supposed to be matching.

Ah, sorry. I should've posted a comment along with the patch. For the purpose of triggering things after L10n, this poller is meant to poll the directory listing of a specific platform directory, like http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/3.6.8-candidates/build1/linux-i686/, and fire when it finds all of the locales needed for that platform.

Does that clear anything up?

Comment 13

[Chris AtLee [:catlee]]

Comment on attachment 465638 [details] [diff] [review]
buildbot-configs to use the new poller

Looks good.  Where do ftp_platform_map and sl_platform_map come from?

Comment 14

[Chris AtLee [:catlee]]

(In reply to comment #12)
> > >+def createLocaleDirListRegex(locales):
> > >+    regex = "/.*".join(sorted(locales)) + "/.*"
> > >+    return re.compile(regex, re.DOTALL)
> > 
> > I'm not sure this is doing what you want....Passing in ["en-US", "de"] creates
> > a regex that looks like
> > 'de/.*en-US/.*'
> > 
> > do you want this to be '(de|en-US)/.*'?  Maybe I'm not getting what this is
> > supposed to be matching.
> 
> Ah, sorry. I should've posted a comment along with the patch. For the purpose
> of triggering things after L10n, this poller is meant to poll the directory
> listing of a specific platform directory, like
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/3.6.8-candidates/build1/linux-i686/,
> and fire when it finds all of the locales needed for that platform.
> 
> Does that clear anything up?

Yup, that seems fine then!

Comment 15

[[e-mail] (:bhearsum)]

(In reply to comment #13)
> Comment on attachment 465638 [details] [diff] [review]
> buildbot-configs to use the new poller
> 
> Looks good.  Where do ftp_platform_map and sl_platform_map come from?

They're in build/tools/lib/python

...which I also meant to call explicitly: using them will give any masters running this code a dependency on build/tools.

Comment 16

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

Comment on attachment 465637 [details] [diff] [review]
refactor FtpPoller to support regexes

(In reply to comment #12)
> Ah, sorry. I should've posted a comment along with the patch. For the purpose
> of triggering things after L10n, this poller is meant to poll the directory
> listing of a specific platform directory, like
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/3.6.8-candidates/build1/linux-i686/,
> and fire when it finds all of the locales needed for that platform.
> 
> Does that clear anything up?

If I understand correctly, you're parsing the entire directory listing in one chunk, so you're looking for all locales simultaneously. 

I think the slashes were confusing me initially because I always think of them as delimiters in a regex context. I blame perl.

Comment 17

[[e-mail] (:bhearsum)]

Won't be finishing this up until q4

Comment 18

[[e-mail] (:bhearsum)]

Syed is going to be working on this.

Comment 19

[Syed Albiz [:salbiz]]

Attachment: buildbot-configs to use the new poller [tested]

Had to make a couple of changes to make this patch work on staging. We seem to be using python2.5, which I believe has broken urllib2 functionality, so I've reverted to using urllib. The other main change is that since we push a sendchange per platform, we have to split up partner repacks per platform to make this approach work.

Comment 20

[[e-mail] (:bhearsum)]

Comment on attachment 481953 [details] [diff] [review]
buildbot-configs to use the new poller [tested]

why len(l) instead of the original len(shippedLocales[l]) ? l is the locale name, isn't it?

Comment 21

[[e-mail] (:bhearsum)]

(In reply to comment #20)
> Comment on attachment 481953 [details] [diff] [review]
> buildbot-configs to use the new poller [tested]
> 
> why len(l) instead of the original len(shippedLocales[l]) ? l is the locale
> name, isn't it?

ping

Comment 22

[Syed Albiz [:salbiz]]

(In reply to comment #21)
> (In reply to comment #20)
> > Comment on attachment 481953 [details] [diff] [review] [details]
> > buildbot-configs to use the new poller [tested]
> > 
> > why len(l) instead of the original len(shippedLocales[l]) ? l is the locale
> > name, isn't it?
> 
> ping

Sorry, was trying to get an updated version of the patch ready to accompany my explanation

Yes, that's right. l is just the locale name, it should be shippedLocales[l] in both cases. In addition, while I was testing I found a problem with the regex matching, where it does not seem to properly match a negative case properly.

Comment 23

[Syed Albiz [:salbiz]]

Attachment: MultiFtpPoller for grabbing list of directories [Tested]

The original approach using a large regex of all locales to search for worked satisfactorily on a small locales list, but executing it on a complete locales list just hangs. Instead, I've put together the subclass MultiFtpPoller, that takes a list of search strings and iteratively matches all of them against the ftp page. This isn't fully tested, and can be improved quite a bit, but I just wanted your feedback on the general approach.

Comment 24

[Syed Albiz [:salbiz]]

Comment on attachment 483011 [details] [diff] [review]
MultiFtpPoller for grabbing list of directories [Tested]

Tested this on staging. It still enforces that precisely all the locales are present simultaneously and will not fire until all searchStrings are matched on the same poll, so I don't think we lose anything with this approach. Will require some minor changes to the configs patch however.

Comment 25

[Syed Albiz [:salbiz]]

Attachment: update configs to use MultiFtpPoller [tested]

Comment 26

[Chris AtLee [:catlee]]

Comment on attachment 483011 [details] [diff] [review]
MultiFtpPoller for grabbing list of directories [Tested]

Looks pretty good.  My only comment at this point is that I'd prefer to see variables like gotFile use True/False rather than 1/0.

Comment 27

[Chris AtLee [:catlee]]

Comment on attachment 483176 [details] [diff] [review]
update configs to use MultiFtpPoller [tested]

>+shippedLocales = ParseLocalesFile(urllib.urlopen(
>+    "%s/%s/raw-file/%s_RELEASE/%s" % (branchConfig['hgurl'], sourceRepoPath, baseTag,
>+                                      shippedLocalesPath)).read())

We need to figure out a better way to do this.  This means that the list of locales gets read on a reconfig, but the _RELEASE tag won't exist until after automation has been triggered for build 1, so we won't be able to load the list of locales.

Comment 28

[Syed Albiz [:salbiz]]

Attachment: move code to grab shippedLocales into specialized FtpPoller

Comment 29

[Syed Albiz [:salbiz]]

Attachment: dump MultiFtpPoller in favour a specialized subclass that also grabs shippedLocales

Tested fine on staging, twisted's Deferred errbacks don't seem to fire if shippedLocales doesn't exist since both the ftp page and hg.m.o return custom error pages rather than 404s on the GetPage call. Since that was really the only advantage I saw in using a separate deferred callback chain for the shippedLocales file, I've reverted to just moving the urllib call into the existing ftppoller callback.

Not sure if there is a way to robustly check for these. If not, I can put in some string matching for error strings, but this leaves us open to breakage if those error strings change.

Comment 30

[Syed Albiz [:salbiz]]

Attachment: use GetPage to grab shippedLocales

same as before, but it changes the base polling function to support grabbing the shippedLocales via a getPage deferred. I was a bit hesitant to do it this way, since it it complicates the code a bit, ensuring that we don't get reader/writer conflict, and simply chaining the deferred GetPages doesn't seem to work. Seems to work on staging.

Comment 31

[Syed Albiz [:salbiz]]

Attachment: set up a deferred chain

Tested on local machine and staging as well. Sets up a deferred getPage request to look for the locales file, parses it in a callback, passes the results of that to a second callback which inserts a second deferred into the callback chain to poll ftp and compare it against the locales list.

Comment 32

[Syed Albiz [:salbiz]]

Attachment: fix multiple URLs and refactor _get_changes

Make sure to set up the callback chain to propagate the locales list for multiple ftp URLs.

Comment 33

[Chris AtLee [:catlee]]

Comment on attachment 483630 [details] [diff] [review]
move code to grab shippedLocales into specialized FtpPoller

Are we going to land this on default first, or just skip right to buildbot-0.8.0?

Comment 34

[Syed Albiz [:salbiz]]

Attachment: port LocalesFtpPoller to buildbot-0.8.0

Since we're considering landing it on 0.8.0 first, and will have to do so at some point anyway, I've ported the patches to work on the 0.8.0 (now default) branch. One slight functional change between the pair is the import of build/tools/lib/python/release/platforms.py used to use release as the package name, now in these patches uses lib.python.release as the base name for the platforms module to avoid ImportErrors with buildbotcustom.release.

Comment 35

[Syed Albiz [:salbiz]]

Attachment: move config changes into updated release_configs

Comment 36

[[e-mail] (:bhearsum)]

Comment on attachment 485818 [details] [diff] [review]
port LocalesFtpPoller to buildbot-0.8.0

The docstring for 'platform' needs to be updated, it should accept more than 'linux', 'win32', or 'mac'. Probably want to use what's currently in getSupportedPlatforms(), or just not list any platforms explicitly. Also need to drop the searchString docstring in FtpPollerBase.

Have you given this a full run through, including looking at the results of the per-platform partner repack builders? We haven't used it that way for a release yet AFAIK.

Looks good to me otherwise.

Comment 37

[Syed Albiz [:salbiz]]

Attachment: accept short-style platform names to partner-repacks.py

Noticed that linux/mac repacks weren't happening anymore, as the --platform option was being passed the short-style platform names in the release-configs (i.e. linux(64), macosx(64), win32), as opposed to complete platform names used to build the repacks (i.e. linux-i686, linux-x64_64, etc). Based on a conversation with catlee, I've decided that the cleanest way to handle this would be to patch partner-repacks.py to accept short-form platform names from the factory.

Comment 38

[Syed Albiz [:salbiz]]

Comment on attachment 486977 [details] [diff] [review]
accept short-style platform names to partner-repacks.py

Tested and spot-checked on linux, win32 partner-repacks generated. Will test mac partner-repacks before submitting for review

Comment 39

[Syed Albiz [:salbiz]]

Comment on attachment 486977 [details] [diff] [review]
accept short-style platform names to partner-repacks.py

Tested on mac/macosx64. The macosx64 builder does not generate any repacks at the moment as none of the repack config files have mac64 as a platform target. All the other ones seem to be generated correctly.

Comment 40

[Syed Albiz [:salbiz]]

Attachment: fix bitrot in release.py, add check for windows unsigned directory

Fixing some bitrot in process/release.py

Another issue came up during testing, where I realized that I was polling the signed win32 builds directory instead of the unsigned one.I've special cased this for now for win32 builds to poll the unsigned/win32 sub directory for all the windows repacks.

Comment 41

[Syed Albiz [:salbiz]]

Attachment: package-ify lib/python

Introducing the dependency on lib.python.release.platforms also requires that those directories be package-ified for python.

Comment 42

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

Comment on attachment 490117 [details] [diff] [review]
fix bitrot in release.py, add check for windows unsigned directory

There's still some bitrot. Hunk #4 is failing to apply for process/release.py. 

There are also a bunch of seemingly whitespace-only changes in ftppoller.py. Intended?

Comment 43

[Chris AtLee [:catlee]]

Comment on attachment 490122 [details] [diff] [review]
package-ify lib/python

lib/python should be in $PYTHONPATH, we shouldn't be importing lib.python.*

Comment 44

[Syed Albiz [:salbiz]]

Attachment: bitrot, shuffle import

(In reply to comment #42)
> Comment on attachment 490117 [details] [diff] [review]
> fix bitrot in release.py, add check for windows unsigned directory
> 
> There's still some bitrot. Hunk #4 is failing to apply for process/release.py. 
>
Sorry, based this changeset against the reserved slaves work, which I thought had landed. Should apply cleanly now.

> 
> There are also a bunch of seemingly whitespace-only changes in ftppoller.py.
> Intended?
>
Yeah, they delete extraneous whitespace (basically s/\s*$//) that was present in the file before. I guess I could remove those changes since they don't really do anything, but I'm nervous about python throwing a fit over unexpected indents, whitespace etc.

Based on IRC conversation with catlee, I've also included the absolute_import directive, which removes the need to package-ify lib/python in build/tools, allowing us to import release.platforms directly.

Comment 45

[Chris AtLee [:catlee]]

Comment on attachment 490231 [details] [diff] [review]
bitrot, shuffle import

r+ with a comment as to why you're using absolute_import

Comment 46

[Syed Albiz [:salbiz]]

Attachment: explain absolute_import directive

explained absolute_import directive, patch should be ready to land now.

Comment 47

[[e-mail] (:bhearsum)]

Comment on attachment 486977 [details] [diff] [review]
accept short-style platform names to partner-repacks.py

changeset:   24:3fee517ecd98

Comment 48

[[e-mail] (:bhearsum)]

Comment on attachment 490978 [details] [diff] [review]
explain absolute_import directive

changeset:   1196:dc7666eeed74

Comment 49

[[e-mail] (:bhearsum)]

Comment on attachment 485819 [details] [diff] [review]
move config changes into updated release_configs

changeset:   3330:6727fe8bfdba

Comment 50

[[e-mail] (:bhearsum)]

Active in staging now, will be turned in production with the first 0.8.0 releases there.