Closed
Bug 599169
Opened 14 years ago
Closed 12 years ago
Tracking bug for talos performance testing on addons
Categories
(Testing :: Talos, defect, P2)
Testing
Talos
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: anodelman, Unassigned)
References
Details
Attachments
(4 files, 5 obsolete files)
(deleted),
patch
|
bhearsum
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhearsum
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
To track work for integrating talos performance testing into the AMO framework.
Reporter | ||
Updated•14 years ago
|
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → anodelman
Priority: -- → P2
Reporter | ||
Comment 2•14 years ago
|
||
Adds amo mode to talos. Mostly changes how the data handling is done after the test is complete so that the results are sent specially marked to be dumped into an amo db instead of into graphs.
Tested green on staging. Shouldn't affect production code unless the --amo switch is used.
Attachment #494187 -
Flags: review?(jmaher)
Reporter | ||
Comment 3•14 years ago
|
||
Just a small patch handling passing the addon id of the addon being tested to talos.
Attachment #494196 -
Flags: review?(bhearsum)
Updated•14 years ago
|
Attachment #494196 -
Flags: review?(bhearsum) → review+
Comment 4•14 years ago
|
||
Comment on attachment 494187 [details] [diff] [review]
add 'amo' mode to talos for data handling
>diff --git a/run_tests.py b/run_tests.py
>-def construct_results (machine, testname, branch, sourcestamp, buildid, date, vals):
>+def construct_results (amo, machine, testname, browser_config, date, vals):
why do you add amo at the beginning of the args list?
>+ branch=browser_config['branch_name']
>+ sourcestamp=browser_config['sourcestamp']
>+ buildid=browser_config['buildid']
hmm, these could use some formatting help, for example adding spaces around the = operator:
branch = browser_config['branch_name']
>-def send_to_graph(results_server, results_link, machine, date, browser_config, results):
>+def send_to_graph(results_server, results_link, machine, date, browser_config, results, amo):
Here we add amo to the end of the arg list, I like this.
Overall, that is all I can find. I don't see this causing interference with remote testing or maybe some future cleanup (this actually cleans up some longer arg lists by just passing browser_config in!++)
r=me with the above issues addressed
Attachment #494187 -
Flags: review?(jmaher) → review+
Reporter | ||
Comment 5•14 years ago
|
||
Incorporated review suggestions. Also some changes:
- add option to turn off 'shutdown' tests from the command line
- set 'NULL' for addon_id if it is not present
Attachment #496322 -
Flags: review?(jmaher)
Reporter | ||
Updated•14 years ago
|
Attachment #494187 -
Attachment is obsolete: true
Reporter | ||
Comment 6•14 years ago
|
||
This is the current copy of sendchanges.sh used on auto-tools staging. I want to check it into tools/buildfarm/utils.
We can then update the copy of sendchanges.sh on the releng buildbot masters to this, along with the command in the crontab to run the new addon testing system.
Attachment #496349 -
Flags: review?(bhearsum)
Reporter | ||
Comment 7•14 years ago
|
||
As a note, lsblakk created
http://tinderbox.mozilla.org/showbuilds.cgi?tree=AddonTester
In future, all addon performance test results will be sent to that waterfall.
Comment 8•14 years ago
|
||
Comment on attachment 496322 [details] [diff] [review]
[checked in] add 'amo' mode to talos for data handling (take 2)
>-def test_file(filename, to_screen):
>+def test_file(filename, to_screen, amo):
> browser_config['test_timeout'] = 1200
>+ if 'addon_id' in yaml_config:
>+ browser_config['addon_id'] = yaml_config['addon_id']
>+ else:
>+ browser_config['addon_id'] = 'NULL'
>
do we normally use 'NULL' for a blank value in talos?
addressing my NULL comment r=me.
Attachment #496322 -
Flags: review?(jmaher) → review+
Reporter | ||
Comment 9•14 years ago
|
||
Changes to buildbot-configs for addon testing service:
- send results to new AddonTester waterfall
- create new baseline testing factory for the comparison results
- changed the options talos is run with to add '--amo' for new amo mode
Attachment #497928 -
Flags: review?(bhearsum)
Comment 10•14 years ago
|
||
Comment on attachment 497928 [details] [diff] [review]
[checked in] changes to buildbot-configs for addon testing service
Looks ok to me.
Attachment #497928 -
Flags: review?(bhearsum) → review+
Comment 11•14 years ago
|
||
Comment on attachment 496349 [details] [diff] [review]
sendchanges.sh
This isn't going to work on the scheduler master, which uses port 9008. Can I suggest that the entire master:port combination be an argument, rather than hardcoding localhost?
Attachment #496349 -
Flags: review?(bhearsum) → review-
Reporter | ||
Comment 12•14 years ago
|
||
Changes in take 2:
- support port 9008 for sendchanges
- support sending master name as variable
Attachment #498786 -
Flags: review?(bhearsum)
Reporter | ||
Updated•14 years ago
|
Attachment #496349 -
Attachment is obsolete: true
Comment 13•14 years ago
|
||
Comment on attachment 498786 [details] [diff] [review]
sendchanges.sh (take 2)
(Marking as a patch so I can use Bugzilla's diff tool...)
Attachment #498786 -
Attachment is patch: true
Updated•14 years ago
|
Attachment #496349 -
Attachment is patch: true
Comment 14•14 years ago
|
||
Comment on attachment 498786 [details] [diff] [review]
sendchanges.sh (take 2)
I don't really understand the purpose behind rejecting certain port numbers, but *shrug*.
Attachment #498786 -
Flags: review?(bhearsum) → review+
Reporter | ||
Comment 15•14 years ago
|
||
Still waiting on bug 608347 to be complete, checked the posted patches today and they all still apply correctly.
Comment 16•14 years ago
|
||
According to Alice, these are the remaining things that need to happen:
* arrange releng downtime
* land the patches in this bug
* reconfig talos masters
* update scheduler on talos master to run addon perf tests weekly with the sendchanges.sh from this bug.
Comment 17•14 years ago
|
||
Turns out that the Talos patch here is slightly incorrect. From bug 608347:
(In reply to comment #35)
> I _think_ this is what's being sent, possibly constructed differently:
> START
> AMO
> Firefox,3.6.15pre,https://addons.mozilla.org/en-US/firefox/downloads/latest/4925
> talos-r3-snow-001,ts,Tryserver,baaf2315445d,20110216033208,1297952166
> 0,1267.00,NULL
> 1,958.00,NULL
> END
OK this works if we pass just the id (4925) and not the whole URL on the first
line after AMO (per https://wiki.mozilla.org/Buildbot/Talos/DataFormat#AMO)
Just did a successful test:
"""
[rhelmer@bm-buildgraph01 ~]$ cat amo.txt
START
AMO
Firefox,3.6.15pre,4925
talos-r3-snow-001,ts,Tryserver,baaf2315445d,20110216033208,1297952166
0,1267.00,NULL
1,958.00,NULL
END
[rhelmer@bm-buildgraph01 ~]$ curl -F filename=@amo.txt
'http://graphs-stage.mozilla.org/server/collect.cgi'
RETURN success
"""
We'll need a small update to it.
Comment 18•14 years ago
|
||
Hmm, actually. Upon further investigation I noticed that the buildbotcustom code actually sets the addonID properly. In my manual tests, I had given PerfConfiguration.py slightly wrong input. Going to try again with the correct parameters.
Comment 19•14 years ago
|
||
(In reply to comment #18)
> Hmm, actually. Upon further investigation I noticed that the buildbotcustom
> code actually sets the addonID properly. In my manual tests, I had given
> PerfConfiguration.py slightly wrong input. Going to try again with the correct
> parameters.
So, I'm reading this as "we *don't* need a new talos patch? Let me know if you need anything from me.
Comment 20•14 years ago
|
||
That's right -- the existing patch is valid. Looks like the graph server updates I was waiting for in bugs 637360 and 636851 are done, too. I need to have one more manual run through of this and then I'll look to land it.
Comment 21•14 years ago
|
||
Planning to land these patches tomorrow morning. If all goes well, we'll be all done here after that.
Comment 22•14 years ago
|
||
Unable to land this as planned because of the infrastructure embargo because of the 3.6.15 chemspill release. Will reschedule when I know when it will be ending.
Comment 23•14 years ago
|
||
(In reply to comment #22)
> Unable to land this as planned because of the infrastructure embargo because of
> the 3.6.15 chemspill release. Will reschedule when I know when it will be
> ending.
Embargo is off!
Updated•14 years ago
|
Attachment #494196 -
Attachment description: add addonID command line option in buildbotcustom → [checked in] add addonID command line option in buildbotcustom
Comment 24•14 years ago
|
||
Comment on attachment 496322 [details] [diff] [review]
[checked in] add 'amo' mode to talos for data handling (take 2)
Going to update the ZIP file shortly.
Attachment #496322 -
Attachment description: add 'amo' mode to talos for data handling (take 2) → [checked in] add 'amo' mode to talos for data handling (take 2)
Comment 25•14 years ago
|
||
Comment on attachment 497928 [details] [diff] [review]
[checked in] changes to buildbot-configs for addon testing service
Status:
Buildbot and Talos patches have been landed, and the Talos ZIp has been updated. I've triggered a few test addon runs, which have successfully submitted results to the production DB.
A bit later, I'll be updating the crontab that triggers addon tests over the weekend, so that we get a full run across the top 100 this saturday.
We are currently missing the "addonbaseline" builders, and I don't know why. I'll investigate this at a later time, I don' thave time today.
Attachment #497928 -
Attachment description: changes to buildbot-configs for addon testing service → [checked in] changes to buildbot-configs for addon testing service
Comment 26•14 years ago
|
||
I forgot to update the crontab before the weekend, so we didn't get any new results this week. Going to fix it today, and re-do some of the jobs over the next 24 hours.
Comment 27•14 years ago
|
||
Dustin, the sendchanges.sh part of this can be ignored because it's been reviewed in an earlier attachment, can you review the Puppet bits, though? I tested this on a Linux staging slave and it seemed to work well. I'm not particularly happy with how the port is defined, but I don't think classes can take arguments. I tried doing it as a function, but it seemed conceptually wrong to do so.
Attachment #517448 -
Flags: review?(dustin)
Comment 28•14 years ago
|
||
I triggered a couple of tests manually. Linux/Mac went OK, but Windows failed (both XP and Windows 7), citing a missing application.ini. Haven't dug into this yet.
Comment 29•14 years ago
|
||
Comment on attachment 517448 [details] [diff] [review]
create addon-sendchanges manual; set it up on pm02
Need a couple of updates to the crontab stuff here.
Attachment #517448 -
Attachment is obsolete: true
Attachment #517448 -
Flags: review?(dustin)
Comment 30•14 years ago
|
||
This patch will fix the Windows test jobs; turns out they needed to be fired with the "-r" option, to send them EXEs instead of ZIPs.
Attachment #517470 -
Flags: review?(dustin)
Comment 31•14 years ago
|
||
Comment on attachment 517470 [details] [diff] [review]
fix addon sendchange cronjob
>diff --git a/addons.txt b/addons.txt
>new file mode 100644
>index 0000000..972d19c
>--- /dev/null
>+++ b/addons.txt
I assume this was meant to be in modules/addon-sendchanges/files/?
>diff --git a/base/nodes.pp b/base/nodes.pp
>index 013e4f9..ab1610b 100644
>--- a/base/nodes.pp
>+++ b/base/nodes.pp
>@@ -128,3 +128,5 @@ node "staging-test-node" inherits "staging-node" {
> include puppet-config
> }
>
>+node "buildbot-master" {
>+}
I'm *strongly* opposed to using puppet to partially manage masters without using it to *completely* manage masters. Puppet isn't just a handy equivalent to rsync and bash - it's meant to specify and maintain the configuration of a system as a whole. When we do configure masters, puppet will be able to build a working master from some minimal base image - probably a bare OS install plus puppet. Until then, I think that a partial implementation is worse than no implementation at all, particularly if we run puppet differently on masters (periodically) than on slaves (at boot).
Also, there's no need for an empty 'buildbot-master' node type.
>diff --git a/modules/addon-sendchanges/files/addon-sendchanges.sh b/modules/addon-sendchanges/files/addon-sendchanges.sh
Similarly, I don't think this is an appropriate category to be made into a module. This would probably be better framed as a 'buildmaster' module?
>diff --git a/modules/addon-sendchanges/manifests/sendchanges.pp b/modules/addon-sendchanges/manifests/sendchanges.pp
>new file mode 100644
>index 0000000..05f86ed
>--- /dev/null
>+++ b/modules/addon-sendchanges/manifests/sendchanges.pp
>@@ -0,0 +1,22 @@
>+class addon-sendchanges::sendchanges {
>+ $addonsFile = "${home}/cltbld/addons.txt"
>+ $sendchangeScript = "${home}/cltbld/addon-sendchanges.sh"
Nothing else on masters runs out of cltbld's home directory; besides, this functionality may need to be different for different masters on the same host, yet this is a global location. I think this would be better under the buildmaster's basedir.
>+
>+ file {
>+ "$sendchangeScript":
>+ source => "puppet:///addon-sendchanges/addon-sendchanges.sh",
>+ owner => "cltbld",
>+ mode => 755;
>+ "$addonsFile":
>+ source => "puppet:///addon-sendchanges/addons.txt",
>+ owner => "cltbld";
>+ }
>+ cron {
>+ "sendchanges":
This should be qualified with the port number
>+ command => "source ${home}/cltbld/.bash_profile && bash $sendchangeScript -p $sendchangePort -r -a $addonsFile -m $fqdn",
>+ user => "cltbld",
>+ minute => 0,
>+ hour => 6,
>+ weekday => 6,
>+ }
>+}
>diff --git a/mpt-production.pp b/mpt-production.pp
>index e84ef14..1629366 100644
>--- a/mpt-production.pp
>+++ b/mpt-production.pp
>@@ -12,6 +12,7 @@ import "packages/*"
> import "stage/*"
>
> # module imports
>+import "addon-sendchanges"
> import "buildslave"
> import "gui"
>
>@@ -2082,6 +2083,11 @@ node "talos-r3-fed64-002.build.mozilla.org" inherits "staging-test-node" {
> node "preproduction-stage.build.mozilla.org" inherits "stage-and-aus2-server" {
> }
>
>+node "production-master02.build.mozilla.org" inherits "buildbot-master" {
>+ $sendchangePort = 9008
I don't think that specifying this here will work. If it's a parameter, then you need to use a define, e.g.
buildmaster::addon-sendchanges {
"9008": ;
}
And the sub-resources need to be titled appropriately so that they do not conflict with other port numbers configured on the same host. That will help it fit better into what should eventually be
node whatever {
buildmaster {
"tm09":
role => "tests",
longname => "Tests Master 09",
basedir => "/build/buildbot/test-master09",
port => 9008,
addon-sendchanges => true;
}
}
>+ include addon-sendchanges::sendchanges
>+}
>+
> node default {
> #include base
> }
Attachment #517470 -
Flags: review?(dustin) → review-
Comment 32•14 years ago
|
||
Comment on attachment 517470 [details] [diff] [review]
fix addon sendchange cronjob
Due to disagreement about how to deploy things on masters with Puppet, I'm going to do this by hand to avoid blocking on resolving that.
Attachment #517470 -
Attachment is obsolete: true
Comment 33•14 years ago
|
||
This is the patch I landed in the tools repo with the sendchange script. It's the same as the most recent patch, except I dropped "MACOSX64" references, because it's obsolete, and fixed "MACOSX" to point at "mac" filenames.
Attachment #498786 -
Attachment is obsolete: true
Comment 34•14 years ago
|
||
Details of the cronjob deployment:
- Disabled existing cronjob on talos-master02
On production-master02:
- Updated tools clone in /home/cltbld/tools
- Added following cronjob in cltbld's crontab:
0 6 * * 6 source /home/cltbld/.bash_profile && bash /home/cltbld/tools/bui
ldbot-helpers/addon-tests/addon-sendchanges.sh -p 9008 -r -a /home/cltbld/tools/
buildbot-helpers/addon-tests/addons.txt -m localhost
This cronjob will trigger the top 100 every Saturday. Tomorrow, I'm going to trigger the top 5 with a similar command line to validate everything once more. If that goes well, we're all done here.
Comment 35•14 years ago
|
||
Firebug, just after install, has all its panels disabled, and even if they are enabled, it also has an activation list of sites, initially zero. There are some hooks and plenty of startup code, but you will likely get different results based on what preferences are set. I don't think it is the only extension to act that way.
Perhaps for a separate bug entry: do this with a clean profile, and also do it for a "dirty" profile with extension settings turned on all over the place. That would give best case and worst case. Rather than just best case.
Comment 36•14 years ago
|
||
I triggered tests for the following addons this morning:
Personas Plus (10900)
Adblock Plus (1865)
Video DownloadHelper (3006)
NoScript (722)
FlashGot (220)
Greasemonkey (748)
Firebug (1843)
FoxTab (8879)
Download Statusbar (26)
Wil, you should see results for the addon tests plus baseline results in the DB on Leopard, XP, Windows 7, and Fedora (32-bit) soon - can you have a look for them?
Comment 37•14 years ago
|
||
(In reply to comment #35)
> Firebug, just after install, has all its panels disabled, and even if they are
> enabled, it also has an activation list of sites, initially zero. There are
> some hooks and plenty of startup code, but you will likely get different
> results based on what preferences are set. I don't think it is the only
> extension to act that way.
>
> Perhaps for a separate bug entry: do this with a clean profile, and also do it
> for a "dirty" profile with extension settings turned on all over the place.
> That would give best case and worst case. Rather than just best case.
For the sake of simplicity, let's keep these bug to its original scope. Can you file this enhancement as a new bug in Testing: Talos?
Comment 39•14 years ago
|
||
Wil verified that these are showing up. This is FIXED.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 40•14 years ago
|
||
Perhaps I'm missing something, but I don't see the checking for the talos changes listed here:
http://hg.mozilla.org/build/talos/
And I'm not seeing the changes in run_tests.py that should be there if that code was landed. What changeset did you commit that in?
Comment 41•14 years ago
|
||
(In reply to comment #40)
> Perhaps I'm missing something, but I don't see the checking for the talos
> changes listed here:
>
> http://hg.mozilla.org/build/talos/
>
> And I'm not seeing the changes in run_tests.py that should be there if that
> code was landed. What changeset did you commit that in?
In the one I forgot to push, apparently :(. Thankfully, nothing has bitrotted me yet, I pushed it now in cc0c3f9fb9fa
Reporter | ||
Comment 42•14 years ago
|
||
Should still be open to track work on creating addon only perf pool.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 43•14 years ago
|
||
From dependent bugs we currently have a vm to act as the addon perf testing master. There are also 7 minis slated to be re-imaged to become the addon perf testing pool.
To be done:
- get the 7 minis up and running, ensure that their configuration matches the production releng mini pool
- configure the addon perf testing master
- update buildbot configs to handle the new master
- settle on a method for injecting sendchange requests to the master (to be generated by amo)
- ensure that security concerns are taken into account and the the addon perf testing pool is correctly firewalled from the rest of our internal systems
There may be a few other tasks, but that is the current state of the work to do.
Status: REOPENED → NEW
Reporter | ||
Updated•14 years ago
|
Comment 44•14 years ago
|
||
(In reply to comment #37)
> (In reply to comment #35)
> > Firebug, just after install, has all its panels disabled, and even if they are
> > enabled, it also has an activation list of sites, initially zero. There are
> > some hooks and plenty of startup code, but you will likely get different
> > results based on what preferences are set. I don't think it is the only
> > extension to act that way.
> >
> > Perhaps for a separate bug entry: do this with a clean profile, and also do it
> > for a "dirty" profile with extension settings turned on all over the place.
> > That would give best case and worst case. Rather than just best case.
>
> For the sake of simplicity, let's keep these bug to its original scope. Can you
> file this enhancement as a new bug in Testing: Talos?
Has this bug been filed? I looked in Testing::Talos, but couldn't find anything.
I'm happy to file the bugs for this, just don't want to double up, in case I've just missed them?
Side note: This issue will also affect Adblock considerably, given that most of the performance degradation due to ABP (leaks, GC & CC delays and more) only appear when a filterset is loaded. Given that ABP is the most commonly used addon on AMO; it would be good to make sure that it in particular shows an accurate performance result.
Reporter | ||
Comment 45•14 years ago
|
||
Bug 639898 was filed to deal with that side issue.
Comment 46•14 years ago
|
||
Resetting assignee because I'm no longer coordinating this.
Assignee: bhearsum → nobody
Updated•14 years ago
|
Depends on: AddonSlowStartup
Comment 48•12 years ago
|
||
this project has been disabled.
Status: NEW → RESOLVED
Closed: 14 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•