599169 - Tracking bug for talos performance testing on addons

Status: RESOLVED FIXED

(Testing :: Talos, defect, P2)

Description

[alice nodelman [:alice] [:anode]]

To track work for integrating talos performance testing into the AMO framework.

Comment 2

[alice nodelman [:alice] [:anode]]

Attachment: add 'amo' mode to talos for data handling

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.

Comment 3

[alice nodelman [:alice] [:anode]]

Attachment: [checked in] add addonID command line option in buildbotcustom

Just a small patch handling passing the addon id of the addon being tested to talos.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 5

[alice nodelman [:alice] [:anode]]

Attachment: [checked in] add 'amo' mode to talos for data handling (take 2)

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

Comment 6

[alice nodelman [:alice] [:anode]]

Attachment: sendchanges.sh

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.

Comment 7

[alice nodelman [:alice] [:anode]]

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

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 9

[alice nodelman [:alice] [:anode]]

Attachment: [checked in] changes to buildbot-configs for addon testing service

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

Comment 10

[[e-mail] (:bhearsum)]

Comment on attachment 497928 [details] [diff] [review]
[checked in] changes to buildbot-configs for addon testing service

Looks ok to me.

Comment 11

[[e-mail] (:bhearsum)]

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?

Comment 12

[alice nodelman [:alice] [:anode]]

Attachment: sendchanges.sh (take 2)

Changes in take 2:
- support port 9008 for sendchanges
- support sending master name as variable

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 498786 [details] [diff] [review]
sendchanges.sh (take 2)

(Marking as a patch so I can use Bugzilla's diff tool...)

Comment 14

[[e-mail] (:bhearsum)]

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*.

Comment 15

[alice nodelman [:alice] [:anode]]

Still waiting on bug 608347 to be complete, checked the posted patches today and they all still apply correctly.

Comment 16

[Heather Arthur [:harth]]

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

[[e-mail] (:bhearsum)]

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

[[e-mail] (:bhearsum)]

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

[cmtalbert]

(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

[[e-mail] (:bhearsum)]

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

[[e-mail] (:bhearsum)]

Planning to land these patches tomorrow morning. If all goes well, we'll be all done here after that.

Comment 22

[[e-mail] (:bhearsum)]

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

[Wil Clouser [:clouserw]]

(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!

Comment 24

[[e-mail] (:bhearsum)]

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.

Comment 25

[[e-mail] (:bhearsum)]

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.

Comment 26

[[e-mail] (:bhearsum)]

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

[[e-mail] (:bhearsum)]

Attachment: create addon-sendchanges manual; set it up on pm02

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.

Comment 28

[[e-mail] (:bhearsum)]

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

[[e-mail] (:bhearsum)]

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.

Comment 30

[[e-mail] (:bhearsum)]

Attachment: fix addon sendchange cronjob

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.

Comment 31

[Dustin J. Mitchell [:dustin] (he/him)]

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
> }

Comment 32

[[e-mail] (:bhearsum)]

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.

Comment 33

[[e-mail] (:bhearsum)]

Attachment: [checked in] land sendchange script

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.

Comment 34

[[e-mail] (:bhearsum)]

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

[Steve Roussey (:sroussey)]

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

[[e-mail] (:bhearsum)]

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

[[e-mail] (:bhearsum)]

(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 38

[[e-mail] (:bhearsum)]

Updating assignee.

Comment 39

[[e-mail] (:bhearsum)]

Wil verified that these are showing up. This is FIXED.

Comment 40

[alice nodelman [:alice] [:anode]]

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

[[e-mail] (:bhearsum)]

(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

Comment 42

[alice nodelman [:alice] [:anode]]

Should still be open to track work on creating addon only perf pool.

Comment 43

[alice nodelman [:alice] [:anode]]

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.

Comment 44

[Ed Morley [:emorley]]

(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.

Comment 45

[alice nodelman [:alice] [:anode]]

Bug 639898 was filed to deal with that side issue.

Comment 46

[[e-mail] (:bhearsum)]

Resetting assignee because I'm no longer coordinating this.

Comment 48

[Joel Maher ( :jmaher ) (UTC -8)]

this project has been disabled.