Closed
Bug 560883
Opened 15 years ago
Closed 14 years ago
Trigger partner repacks as soon as l10n repacks are finished
Categories
(Release Engineering :: General, defect, P5)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: coop, Assigned: salbiz)
References
Details
(Whiteboard: [automation][partner-repacks])
Attachments
(3 files, 14 obsolete files)
10.74 KB,
patch
|
bhearsum
:
review+
catlee
:
feedback+
bhearsum
:
feedback+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
1.97 KB,
patch
|
catlee
:
review+
coop
:
review+
coop
:
feedback+
catlee
:
feedback+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
15.92 KB,
patch
|
bhearsum
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
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 2•15 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
(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.
Reporter | ||
Comment 5•14 years ago
|
||
(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•14 years ago
|
||
(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...
Reporter | ||
Comment 7•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
Attachment #465637 -
Flags: feedback?(ccooper)
Attachment #465637 -
Flags: feedback?(catlee)
Comment 10•14 years ago
|
||
Attachment #465638 -
Flags: feedback?(ccooper)
Attachment #465638 -
Flags: feedback?(catlee)
Comment 11•14 years ago
|
||
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.
Attachment #465637 -
Flags: feedback?(catlee) → feedback-
Comment 12•14 years ago
|
||
(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•14 years ago
|
||
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?
Attachment #465638 -
Flags: feedback?(catlee) → feedback+
Comment 14•14 years ago
|
||
(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•14 years ago
|
||
(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.
Reporter | ||
Comment 16•14 years ago
|
||
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.
Attachment #465637 -
Flags: feedback?(ccooper) → feedback+
Reporter | ||
Updated•14 years ago
|
Attachment #465638 -
Flags: feedback?(ccooper) → feedback+
Assignee | ||
Comment 19•14 years ago
|
||
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.
Attachment #465638 -
Attachment is obsolete: true
Attachment #481953 -
Flags: feedback?(catlee)
Attachment #481953 -
Flags: feedback?(bhearsum)
Comment 20•14 years ago
|
||
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•14 years ago
|
||
(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
Assignee | ||
Comment 22•14 years ago
|
||
(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.
Assignee | ||
Comment 23•14 years ago
|
||
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.
Attachment #465637 -
Attachment is obsolete: true
Attachment #483011 -
Flags: feedback?(catlee)
Attachment #483011 -
Flags: feedback?(bhearsum)
Assignee | ||
Comment 24•14 years ago
|
||
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.
Attachment #483011 -
Attachment description: MultiFtpPoller for grabbing list of directories → MultiFtpPoller for grabbing list of directories [Tested]
Assignee | ||
Comment 25•14 years ago
|
||
Attachment #481953 -
Attachment is obsolete: true
Attachment #483176 -
Flags: feedback?(catlee)
Attachment #483176 -
Flags: feedback?(bhearsum)
Attachment #481953 -
Flags: feedback?(catlee)
Attachment #481953 -
Flags: feedback?(bhearsum)
Comment 26•14 years ago
|
||
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.
Attachment #483011 -
Flags: feedback?(catlee) → feedback+
Comment 27•14 years ago
|
||
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.
Attachment #483176 -
Flags: feedback?(catlee) → feedback-
Assignee | ||
Comment 28•14 years ago
|
||
Attachment #483176 -
Attachment is obsolete: true
Attachment #483630 -
Flags: feedback?(catlee)
Attachment #483630 -
Flags: feedback?(bhearsum)
Attachment #483176 -
Flags: feedback?(bhearsum)
Assignee | ||
Comment 29•14 years ago
|
||
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.
Attachment #483011 -
Attachment is obsolete: true
Attachment #483633 -
Flags: feedback?(catlee)
Attachment #483633 -
Flags: feedback?(bhearsum)
Attachment #483011 -
Flags: feedback?(bhearsum)
Assignee | ||
Comment 30•14 years ago
|
||
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.
Attachment #483633 -
Attachment is obsolete: true
Attachment #483666 -
Flags: feedback?(catlee)
Attachment #483666 -
Flags: feedback?(bhearsum)
Attachment #483633 -
Flags: feedback?(catlee)
Attachment #483633 -
Flags: feedback?(bhearsum)
Assignee | ||
Updated•14 years ago
|
Attachment #483666 -
Flags: feedback?(catlee)
Attachment #483666 -
Flags: feedback?(bhearsum)
Assignee | ||
Updated•14 years ago
|
Attachment #483630 -
Flags: feedback?(catlee)
Attachment #483630 -
Flags: feedback?(bhearsum)
Assignee | ||
Comment 31•14 years ago
|
||
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.
Attachment #483666 -
Attachment is obsolete: true
Attachment #484345 -
Flags: review?(catlee)
Assignee | ||
Updated•14 years ago
|
Attachment #483630 -
Flags: review?(catlee)
Assignee | ||
Comment 32•14 years ago
|
||
Make sure to set up the callback chain to propagate the locales list for multiple ftp URLs.
Attachment #484345 -
Attachment is obsolete: true
Attachment #484451 -
Flags: review?(catlee)
Attachment #484345 -
Flags: review?(catlee)
Updated•14 years ago
|
Attachment #484451 -
Flags: review?(catlee) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #484451 -
Flags: checked-in?
Comment 33•14 years ago
|
||
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?
Attachment #483630 -
Flags: review?(catlee) → review+
Assignee | ||
Comment 34•14 years ago
|
||
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.
Attachment #485818 -
Flags: feedback?(catlee)
Attachment #485818 -
Flags: feedback?(bhearsum)
Assignee | ||
Comment 35•14 years ago
|
||
Attachment #485819 -
Flags: feedback?(catlee)
Attachment #485819 -
Flags: feedback?(bhearsum)
Updated•14 years ago
|
Attachment #485819 -
Flags: review+
Attachment #485819 -
Flags: feedback?(bhearsum)
Attachment #485819 -
Flags: feedback+
Comment 36•14 years ago
|
||
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.
Attachment #485818 -
Flags: feedback?(bhearsum) → feedback+
Updated•14 years ago
|
Attachment #485818 -
Flags: feedback?(catlee) → feedback+
Updated•14 years ago
|
Attachment #485819 -
Flags: feedback?(catlee) → feedback+
Assignee | ||
Comment 37•14 years ago
|
||
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.
Attachment #486977 -
Flags: feedback?(coop)
Attachment #486977 -
Flags: feedback?(catlee)
Updated•14 years ago
|
Attachment #486977 -
Flags: feedback?(catlee) → feedback+
Reporter | ||
Updated•14 years ago
|
Attachment #486977 -
Flags: feedback?(coop) → feedback+
Assignee | ||
Comment 38•14 years ago
|
||
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
Assignee | ||
Comment 39•14 years ago
|
||
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.
Attachment #486977 -
Flags: review?(coop)
Attachment #486977 -
Flags: review?(catlee)
Assignee | ||
Comment 40•14 years ago
|
||
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.
Attachment #485818 -
Attachment is obsolete: true
Attachment #490117 -
Flags: review?(coop)
Attachment #490117 -
Flags: review?(catlee)
Assignee | ||
Comment 41•14 years ago
|
||
Introducing the dependency on lib.python.release.platforms also requires that those directories be package-ified for python.
Attachment #490122 -
Flags: review?(catlee)
Attachment #490122 -
Flags: review?(bhearsum)
Updated•14 years ago
|
Attachment #490122 -
Flags: review?(bhearsum) → review+
Reporter | ||
Comment 42•14 years ago
|
||
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?
Attachment #490117 -
Flags: review?(coop) → review-
Reporter | ||
Updated•14 years ago
|
Attachment #486977 -
Flags: review?(coop) → review+
Updated•14 years ago
|
Attachment #486977 -
Flags: review?(catlee) → review+
Updated•14 years ago
|
Attachment #490117 -
Flags: review?(catlee)
Comment 43•14 years ago
|
||
Comment on attachment 490122 [details] [diff] [review]
package-ify lib/python
lib/python should be in $PYTHONPATH, we shouldn't be importing lib.python.*
Attachment #490122 -
Flags: review?(catlee) → review-
Assignee | ||
Comment 44•14 years ago
|
||
(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.
Attachment #490117 -
Attachment is obsolete: true
Attachment #490231 -
Flags: review?(coop)
Attachment #490231 -
Flags: review?(catlee)
Comment 45•14 years ago
|
||
Comment on attachment 490231 [details] [diff] [review]
bitrot, shuffle import
r+ with a comment as to why you're using absolute_import
Attachment #490231 -
Flags: review?(catlee) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #490231 -
Flags: review?(coop) → review+
Assignee | ||
Comment 46•14 years ago
|
||
explained absolute_import directive, patch should be ready to land now.
Attachment #490231 -
Attachment is obsolete: true
Attachment #490978 -
Flags: review?(bhearsum)
Attachment #490978 -
Flags: checked-in?
Assignee | ||
Updated•14 years ago
|
Attachment #486977 -
Flags: checked-in?
Assignee | ||
Updated•14 years ago
|
Attachment #485819 -
Flags: checked-in?
Assignee | ||
Updated•14 years ago
|
Attachment #484451 -
Flags: checked-in?
Updated•14 years ago
|
Attachment #490978 -
Flags: review?(bhearsum) → review+
Updated•14 years ago
|
Attachment #490122 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #484451 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #483630 -
Attachment is obsolete: true
Comment 47•14 years ago
|
||
Comment on attachment 486977 [details] [diff] [review]
accept short-style platform names to partner-repacks.py
changeset: 24:3fee517ecd98
Attachment #486977 -
Flags: checked-in? → checked-in+
Comment 48•14 years ago
|
||
Comment on attachment 490978 [details] [diff] [review]
explain absolute_import directive
changeset: 1196:dc7666eeed74
Attachment #485819 -
Flags: checked-in? → checked-in+
Attachment #490978 -
Flags: checked-in? → checked-in+
Comment 49•14 years ago
|
||
Comment on attachment 485819 [details] [diff] [review]
move config changes into updated release_configs
changeset: 3330:6727fe8bfdba
Comment 50•14 years ago
|
||
Active in staging now, will be turned in production with the first 0.8.0 releases there.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•