Closed Bug 448047 Opened 12 years ago Closed 11 years ago

integrate the talos perfrunner into buildbotcustom

Categories

(Release Engineering :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: anodelman, Assigned: catlee)

References

Details

Attachments

(7 files, 9 obsolete files)

18.23 KB, patch
anodelman
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
5.58 KB, patch
anodelman
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
3.95 KB, patch
anodelman
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
2.00 KB, patch
anodelman
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
1.06 KB, patch
anodelman
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
971 bytes, patch
anodelman
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
17.29 KB, patch
anodelman
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
Too many perfrunners, too much repeated code.
Summary: integrate the talos perfrunner into buildbot custom → integrate the talos perfrunner into buildbotcustom
Component: Release Engineering: Talos → Release Engineering: Future
Priority: -- → P3
Duplicate of this bug: 484054
Assignee: nobody → catlee
Attachment #373337 - Flags: review?(bhearsum)
The pool of slaves for talos is using this code, hence the dependency.
Blocks: 488367
Attachment #373337 - Flags: review?(anodelman)
Comment on attachment 373337 [details] [diff] [review]
Add talos steps and factory to buildbotcustom

Are you only incorporating nomerge schedulers?  We still use merging schedulers for a lot of the talos setup.

If we want this to be shared with stage could there be an option to download talos from a local zip?  Then we could have the same factory running everywhere.

Also, should the tinderboxmailnotifier go in with this?
This adds the merging multi-scheduler, as well as our tinderboxmailnotifier.

I also renamed customPageset to customTalos for downloading talos from a .zip file instead of checking it out from CVS.
Attachment #373337 - Attachment is obsolete: true
Attachment #373650 - Flags: review?(anodelman)
Attachment #373337 - Flags: review?(bhearsum)
Attachment #373337 - Flags: review?(anodelman)
Comment on attachment 373650 [details] [diff] [review]
Add talos steps and factory to buildbotcustom

+        if OS in ('leopard', 'tiger'):
+            self.addStep(FileDownload(
+             mastersrc="%s/buildfarm/utils/installdmg.ex" % toolsDir,
+             slavedest="installdmg.ex",


At the moment, we install on tiger with install.sh not install.ex.  We'd first have to prove that install.ex works on both tiger and leopard before we could go with this step.
Attachment #373650 - Flags: review?(anodelman) → review-
(In reply to comment #6)
> (From update of attachment 373650 [details] [diff] [review])
> +        if OS in ('leopard', 'tiger'):
> +            self.addStep(FileDownload(
> +             mastersrc="%s/buildfarm/utils/installdmg.ex" % toolsDir,
> +             slavedest="installdmg.ex",
> 
> 
> At the moment, we install on tiger with install.sh not install.ex.  We'd first
> have to prove that install.ex works on both tiger and leopard before we could
> go with this step.

It's been running well in the staging talos pool:
http://qm-buildbot01.mozilla.org:2009/builders/MacOSX%20Darwin%208.8.1%20talos%20mozilla-1.9.1%20pool/builds/135/steps/Unpack%20build/logs/stdio
These are fetched from the tools repository using a regular FileDownload step.  This means you need the tools repository checked out on the talos master.
Attachment #374277 - Flags: review?(anodelman)
Attachment #374277 - Flags: review?(anodelman) → review+
Comment on attachment 373650 [details] [diff] [review]
Add talos steps and factory to buildbotcustom

Ship it.
Attachment #373650 - Flags: review- → review+
Comment on attachment 374277 [details] [diff] [review]
Add talos utillities into tools repository

changeset:   272:dcc3a168e807

this doesn't require a reconfig anywhere
Attachment #374277 - Flags: checked‑in+
Comment on attachment 373650 [details] [diff] [review]
Add talos steps and factory to buildbotcustom

changeset:   261:9f3e6517d946
Attachment #373650 - Flags: checked‑in+
It looks like we can use the same clean command on all platforms, so this change uses 'rm -vrf *' for all platforms.

Until we have something to replace tinderbox, we still need to lie about our start time (sob).

This patch also calls firefox-bin instead of firefox on mac and linux.
Attachment #376288 - Flags: review?(anodelman)
This patch will extract out the buildid, revision, and repository path (if available) and set the appropriate build properties.

This should simplify regression hunting a bit, and will also be of benefit once the build database is collecting this information.
Attachment #376497 - Flags: review?(anodelman)
Comment on attachment 376288 [details] [diff] [review]
Talos Factory cleanup

             exepath = WithProperties('%(exepath)s')
-        else:
+        elif OS in ('xp', 'vista'):
             exepath = '../firefox/firefox'
+	else:
+            exepath = '../firefox/firefox-bin'


Should this change be matched up with updates to all the master.cfgs - since they currently specify all windows boxes as being OS='win'?
Comment on attachment 376497 [details] [diff] [review]
Extract information from application.ini into build properties

+            retval = {}
+            stdout = "\n".join([stdout, stderr])
+            m = re.search("^BuildID=(\w+)", stdout, re.M)
+            if m:
+                retval['buildid'] = m.group(1)
+            m = re.search("^SourceStamp=(.*)", stdout, re.M)
+            if m:
+                retval['revision'] = m.group(1)
+            m = re.search("^SourceRepository=(\S+)", stdout, re.M)
+            if m:
+                retval['repo_path'] = m.group(1)


We've had problems in the past with not being flexible enough with our regexes when searching for buildid/sourcestamp/sourcerepo.  Should be able to absorb spaces before/after the '=', just in case.

Also, would it be valuable to put in default values?  Firefox3.0 does not have sourcestamp/repo.  Otherwise you force checking for existence before using these properties throughout the rest of the code.
Attachment #376497 - Flags: review?(anodelman) → review-
Attachment #378329 - Flags: review?(anodelman)
Attachment #378330 - Flags: review?(anodelman)
Attachment #378331 - Flags: review?(anodelman)
Attachment #378329 - Flags: review?(anodelman) → review+
Attachment #378330 - Flags: review?(anodelman) → review+
Attachment #378331 - Flags: review?(anodelman) → review+
Attachment #376288 - Flags: review?(anodelman) → review+
Comment on attachment 376288 [details] [diff] [review]
Talos Factory cleanup

With the addition of the patches for talos production/try/stage I'm happy with this now.
Attachment #376497 - Attachment is obsolete: true
Attachment #378385 - Flags: review?(anodelman)
Blocks: 488368
Comment on attachment 376288 [details] [diff] [review]
Talos Factory cleanup

changeset:   303:e74a99683ce4
Attachment #376288 - Flags: checked‑in+
Attachment #378385 - Flags: review?(anodelman) → review+
Comment on attachment 378385 [details] [diff] [review]
Extract information from application.ini into build properties

changeset:   306:339bc2eba1de
Attachment #378385 - Flags: checked‑in+
Depends on: 493755
Attached patch Minor fixes for perfrunner (obsolete) — Splinter Review
A few minor fixes for perfrunner that I discovered after the pool has been active for a while.

We should output which slave the build is running on, just like we do for builds.

We should also go back to using installdmg.sh.  installdmg.ex is too unreliable.  This also affects unitests on packaged builds, but since they checkout the tools repository directly, their factories don't need to be updated to look for installdmg.sh.
Attachment #380124 - Flags: review?(anodelman)
Attached patch Minor fixes for perfrunner (obsolete) — Splinter Review
Same as before, except unpack symbols in the right place
Attachment #380124 - Attachment is obsolete: true
Attachment #380458 - Flags: review?(anodelman)
Attachment #380124 - Flags: review?(anodelman)
Comment on attachment 380458 [details] [diff] [review]
Minor fixes for perfrunner

         self.addStep(ShellCommand(
+          command=['echo', 'TinderboxPrint:', WithProperties('s: %(slavename)s')],
+          workdir=workdirBase,
+        ))
+        self.addStep(ShellCommand(


I can't decide if this should be part of the talos code instead of the master - currently talos is the one that sets up all the things that are Tinderboxprinted, so I don't know if it is a good idea to split reporting between master and slave.  Any opinions?
(In reply to comment #25)
> (From update of attachment 380458 [details] [diff] [review])
>          self.addStep(ShellCommand(
> +          command=['echo', 'TinderboxPrint:', WithProperties('s:
> %(slavename)s')],
> +          workdir=workdirBase,
> +        ))
> +        self.addStep(ShellCommand(
> 
> 
> I can't decide if this should be part of the talos code instead of the master -
> currently talos is the one that sets up all the things that are
> Tinderboxprinted, so I don't know if it is a good idea to split reporting
> between master and slave.  Any opinions?

It all ends up in the same log that's sent to Tinderbox so this is totally fine IMHO. More importantly, I think it's proper that Buildbot prints out the slavename rather than Talos.
(In reply to comment #25)
> (From update of attachment 380458 [details] [diff] [review])
>          self.addStep(ShellCommand(
> +          command=['echo', 'TinderboxPrint:', WithProperties('s:
> %(slavename)s')],
> +          workdir=workdirBase,
> +        ))
> +        self.addStep(ShellCommand(
> 
> 
> I can't decide if this should be part of the talos code instead of the master -
> currently talos is the one that sets up all the things that are
> Tinderboxprinted, so I don't know if it is a good idea to split reporting
> between master and slave.  Any opinions?

This matches what is currently done in the build/unittest factories.

But if the talos code could be modified to print out the equivalent code, and the rest of the Tinderbox print's are there, then you have a good point.
Attachment #380458 - Attachment is obsolete: true
Attachment #380458 - Flags: review?(anodelman)
Attachment #382596 - Flags: review?(anodelman)
Attachment #382599 - Flags: review?(anodelman)
Attachment #382596 - Flags: review?(anodelman) → review+
Attachment #382599 - Flags: review?(anodelman) → review+
Comment on attachment 382596 [details] [diff] [review]
Minor fixes for perfrunner

changeset:   332:142f03acddd4
Attachment #382596 - Flags: checked‑in+
Comment on attachment 382599 [details] [diff] [review]
Print out slave name

>   utils.debug("actual date: %d" % int(time.time()))
>+  print 'RETURN:s: %s' % browser_config['title']

Checked in this instead after some tree burnage:

print 'RETURN:s: %s' % title
Attachment #382599 - Flags: checked‑in+
The only thing left to do here is to convert talos-try over to use buildbotcustom's TalosFactory (or a slightly modified version of it).

We don't need to land any of the above changes to talos staging or talos production because those slaves have been moved over to the pool in the meanwhile, which is using the new code.
Attached patch TryTalosFactory (obsolete) — Splinter Review
We need slightly different handling of the start time processing, as well as printing out the build's identifier, hence the requirement for a separate factory.
Attachment #385231 - Flags: review?(anodelman)
Attachment #385233 - Flags: review?(anodelman)
Attachment #385231 - Flags: review?(anodelman) → review+
Attachment #385233 - Flags: review?(anodelman) → review+
Attached patch TryTalosFactorySplinter Review
Same as before, adjusted for new 'plugins' and 'pageset' parameters
Attachment #385231 - Attachment is obsolete: true
Attachment #389695 - Flags: review?(anodelman)
Comment on attachment 389695 [details] [diff] [review]
TryTalosFactory

Even with this:
+                # Lies!!!
+                # Once we don't need to lie about our start time,
+                # the following code can go away


I'll let it pass. :)
Attachment #389695 - Flags: review?(anodelman) → review+
Blocks: 509280
Attachment #385233 - Attachment is obsolete: true
Comment on attachment 389695 [details] [diff] [review]
TryTalosFactory

changeset:   379:0091ced82f6d
Attachment #389695 - Flags: checked-in+
Attachment #378329 - Attachment is obsolete: true
Attachment #378330 - Attachment is obsolete: true
Attachment #378331 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Moving closed Future bugs into Release Engineering in preparation for removing the Future component.
Component: Release Engineering: Future → Release Engineering
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.