580698 - Switch Talos pageloader to ship in distribution/bundles instead of dropping into components/ and chrome/

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Benjamin Smedberg]

The current Talos drops stuff into components/ and chrome/. With bug 579178 this is intentionally breaking, so we need Talos pageloader to instead use a distribution bundle.

Comment 1

[Benjamin Smedberg]

Attachment: Talos (CVS) changes, rev. 1

Comment 2

[Benjamin Smedberg]

This patch removes our use of CVS pageloader, and replaces it with repo, currently at http://hg.mozilla.org/users/bsmedberg_mozilla.com/new-pageloader I'd like permission (from bhearsum?) to go ahead and create hg.mozilla.org/build/pageloader for this.

Comment 3

[Benjamin Smedberg]

Attachment: generate-pageloader.py changes

Comment 4

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

This looks reasonable except for the integration with the current set up:

- since we are just gutting generate component script we should ditch it and have the buildbotcustom steps modified to download the pageloader zip
- we should keep a copy of the pageloader zip on the build network where it can be downloaded from (like we do with the dirty profiles)

Comment 5

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

Also, this should be tested on stage before roll out.   This can be done by using the customTalos TalosFactory option to have it pull a local zip of talos instead of a fresh cvs check out.

Comment 6

[Benjamin Smedberg]

Attachment: Download pageloader.xpi directly from build-tools, rev. 1

I'll just commit pageloader.xpi to build-tools, if this is the right way to go. I'm not completely clear on what the mobile test factory ought to be doing, but I'm certain that running generate-tpcomponent.py is not right.

Comment 7

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to comment #6)
> Created attachment 459825 [details] [diff] [review]
> Download pageloader.xpi directly from build-tools, rev. 1
> 
> I'll just commit pageloader.xpi to build-tools, if this is the right way to go.
> I'm not completely clear on what the mobile test factory ought to be doing, but
> I'm certain that running generate-tpcomponent.py is not right. 

Benjamin, that is how we have been generating the pageloader for the n810s and n900s for quite a while now and it has been working and is how, aiui, we generate pageloader for firefox builds.  

Is there any reason that we can't use the same new xpi for mobile builds?

Comment 8

[Benjamin Smedberg]

No, you probably should use the same xpi. I just didn't understand how generate-tploader.py was downloaded in the mobile config: there didn't seem to be a download step as in the normal config factory.

Comment 9

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

I'll be responsible for staging this on the minis - you should check with Aki for how this will affect mobile set up.

Comment 10

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

Comment on attachment 459143 [details] [diff] [review]
Talos (CVS) changes, rev. 1

I'd prefer this to use this formula:

            self.addStep(DownloadFile(
             url="%s/%s" % (self.supportUrlBase, self.plugins),
             workdir=os.path.join(self.workdirBase, "talos/base_profile"),
            ))

So that the file is Downloaded not from the buildbot master, but from the releng web server (it's used to host dirty profiles/plugins/etc).

Comment 11

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

Comment on attachment 459825 [details] [diff] [review]
Download pageloader.xpi directly from build-tools, rev. 1

Sorry, previous comment were for this patch not for the talos cvs changes.

Comment 12

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

Bad news about zipfile.extractall in your cvs talos patch:

"
New in version 2.6.
"

Talos slaves run with python 2.4, so all talos code has to be compliant with that.

Comment 13

[Benjamin Smedberg]

Attachment: Talos (CVS) changes, rev. 2

I refactored the existing compatibility code into a helper function and used it in both places.

Comment 14

[Benjamin Smedberg]

The buildbotcustom patch already uses self.supportUrlBase, which IIUC would download pageloader.xpi off of the releng server. I'm not sure I understand exactly what ought to change there: did you mean self.plugins exactly? It seems like that is already downloaded and extracted into talos/base_profile, but this needs to end up in talos/page_load_test/pageloader.xpi

Comment 15

[Benjamin Smedberg]

Comment on attachment 459825 [details] [diff] [review]
Download pageloader.xpi directly from build-tools, rev. 1

Re-requesting review because I think this is right.

Comment 16

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

I think that the unzipped pageloader should be checked into the build repository at tools/buildfarm/utils, then the xpi version can be created and posted to the build server - I like this better then checking in a xpi which seems wrong to me.

And yes, sorry about the confusion about the previous buildbotcustom patch - I was misreading it as doing a local filedownload instead of from the server.

Comment 17

[Benjamin Smedberg]

The unzipped version is already in hg.mozilla.org/build/pageloader. I'm happy to either commit a zipped version into build/tools, or just post the XPI which can be uploaded to the build server.

Comment 18

[Benjamin Smedberg]

Attachment: Talos (CVS) changes, rev. 2, rebased

Same as the prior CVS patch except `cvs up`ped to tip.

Comment 19

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

Comment on attachment 460575 [details] [diff] [review]
Talos (CVS) changes, rev. 2, rebased

Green on talos-stage.

Comment 20

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

bsmedberg - can you update your buildbotcustom patch to reflect the location of the xpi at:

http://build.mozilla.org/talos/xpis/pageloader.xpi

Comment 21

[Benjamin Smedberg]

Attachment: Download pageloader.xpi directly from build-tools, rev. 2

Comment 22

[Aki Sasaki (not active)]

Where should we put the xpi in fennec (no distribution/bundles directory)?

Comment 23

[Benjamin Smedberg]

Firefox doesn't have one either until we create one. Talos does this automatically.

Comment 24

[Aki Sasaki (not active)]

This seems to work for the n900s in staging; awesome.
At some point I'll test bug 454731 against this and update the patch.

Comment 25

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

Comment on attachment 461241 [details] [diff] [review]
Download pageloader.xpi directly from build-tools, rev. 2

http://hg.mozilla.org/build/buildbotcustom/rev/ef1b3deb649b

Comment 26

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

Comment on attachment 460575 [details] [diff] [review]
Talos (CVS) changes, rev. 2, rebased

Checking in ffsetup.py;
/cvsroot/mozilla/testing/performance/talos/ffsetup.py,v  <--  ffsetup.py
new revision: 1.21; previous revision: 1.20
done
Checking in run_tests.py;
/cvsroot/mozilla/testing/performance/talos/run_tests.py,v  <--  run_tests.py
new revision: 1.66; previous revision: 1.65
done
Checking in sample.config;
/cvsroot/mozilla/testing/performance/talos/sample.config,v  <--  sample.config
new revision: 1.49; previous revision: 1.48
done
Checking in ttest.py;
/cvsroot/mozilla/testing/performance/talos/ttest.py,v  <--  ttest.py
new revision: 1.46; previous revision: 1.45
done

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: Correct the mobiletestfactory.py changes

The buildbotcustom patch as landed is a no-op for mobile.  This is because it was landed on the wrong branch and also didn't modify the code that the devices are actually running.  

Currently, the devices download a pageloader tarball.  This pageloader tarball is generated using the generate-tpcomponent.py script on production-mobile-master.  In fact, generate-tpcomponent.py is still being used but is being run on the master which seems contrary to the goals of this bug and the patch that was landed.

I am not sure if one of the goals of this bug is to get the mobile devices to actually make use of the new pageloader or not.

This patch is to bring the buildbot-0.8.0 branch back inline with the default branch.  I have another patch to do what I think is required to use the new xpi on mobile devices.

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: use the new xpi file on mobile

This patch removes the option of using generate-tpcomponents.py on the slave as well as in a tarball generated on the master using generate-tpcomponents.py.  There would be a very small corresponding buildbot-configs patch required for this to work.  I also need to test this in staging before I ask for review and land it.

Before I test it, I would like to know that what I am doing is valid.  With this patch, I will download pageloader.xpi into talos/page_load_tests.  Looking at the changes made for desktop talos, it seems like that is the limit of changes needed, is that correct?

Comment 29

[Benjamin Smedberg]

Comment on attachment 462978 [details] [diff] [review]
use the new xpi file on mobile

Why are you using wget instead of the DownloadFile step? If you're using wget, I think you need -O so that it doesn't rename the file if it already exists.

Comment 30

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

Comment on attachment 462976 [details] [diff] [review]
Correct the mobiletestfactory.py changes

Giving a tentative r+ here because this just seems to be putting back what was taken out in the original patch. However, I don't understand the other patch you asked bsmedberg for feedback on then because it seems to be fixing the same thing in a different way.

Is this the quick fix/reversion and the other is the new/proper way?

Comment 31

[Benjamin Smedberg]

Comment on attachment 462976 [details] [diff] [review]
Correct the mobiletestfactory.py changes

generate-tpcomponent doesn't make any sense (and should probably be removed) any more: Talos won't use it, and even if you were using an old version of talos it won't work as soon as I check in bug 579178, which is why we were doing all this in the first place.

Comment 32

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to comment #29)
> Comment on attachment 462978 [details] [diff] [review]
> use the new xpi file on mobile
> 
> Why are you using wget instead of the DownloadFile step? If you're using wget,
> I think you need -O so that it doesn't rename the file if it already exists.

I am using wget because that is what has always been done on mobile.  My reading of http://hg.mozilla.org/build/buildbotcustom/file/5a52ac1ad69f/steps/misc.py#l369 seems to confirm that aside from missing --progress=dot:mega -N, this is functionally identical.

Yes, in theory we should worry about overwriting vs appending a number to the filename, but these devices format the filesystem that they run tests from every single boot and reboot at the end of every test run.  I could modify the arguments to match exactly what is in DownloadFile which results in

command=['wget', '--progress=dot:mega', '-N', self.pageloader_url]

We don't use -O in the DownloadFile step either, so I don't know if it should be used here.  Using the -O flag means that we have to hardcode the output name to pageloader.xpi, which seems fine in this case.

Looking at the talos changes that were landed, it looks like placing the zip file in page_load_test is the correct action, can someone please confirm this?

If its ok with you, I can file a seperate bug to move the mobile testing infrastructure to use the DownloadFile step.

(In reply to comment #30)
> Comment on attachment 462976 [details] [diff] [review]
> Correct the mobiletestfactory.py changes
> 
> Giving a tentative r+ here because this just seems to be putting back what was
> taken out in the original patch. However, I don't understand the other patch
> you asked bsmedberg for feedback on then because it seems to be fixing the same
> thing in a different way.
> 
> Is this the quick fix/reversion and the other is the new/proper way?

The purpose of this patch is to make the buildbot-0.8.0 branch match what is running in production on the devices.  The reason that I'd like this patch to land is that it means that the same patch to start using the xpi can be applied to the default and buildbot-0.8.0 branch which will keep them in sync.  I would be ok with landing patch -R on default as neither version of the patch has any effect on what is running in production in any way.

This patch and the original patch landed during the downtime have no effect on what is running on device.  That function that adds the steps to run generate-tpcomponent is not called because of http://hg.mozilla.org/build/buildbotcustom/file/5a52ac1ad69f/process/mobiletestfactory.py#l196 .  As running in production, self.pageloader_tarball is not None, so the alternate of downloading the tarball is run instead.  This tarball is generated nightly using 

<snip>
TOOLS_REV="default"
<snip>
(cd tools && hg pull && hg up -C -r $TOOLS_REV)
<snip>
python tools/buildfarm/utils/generate-tpcomponent.py
<snip>
tar cjvf pageloader-$DATESTRING.tar.bz2 chrome components
<snip>

(In reply to comment #31)
> Comment on attachment 462976 [details] [diff] [review]
> Correct the mobiletestfactory.py changes
> 
> generate-tpcomponent doesn't make any sense (and should probably be removed)
> any more: Talos won't use it, and even if you were using an old version of
> talos it won't work as soon as I check in bug 579178, which is why we were
> doing all this in the first place.

I fully understand that generate-tpcomponent is invalid and doesn't make sense and should be removed.  As the code is in the repository right now, and running on device in production, it is still using generate-tpcomponent.py.

The purpose of the patch I asked for feedback on is to actually use the pageloader.xpi that we should start using in preparation of the landings breaking the pageloader we currently use.

Comment 33

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

summary of quick meeting with coop, aki and jhford just now. 

1) bsmedberg's patch was landed in yesterday's downtime.

2) bsmedberg's patch works for talos desktop, but is not part of the mobile talos code path, so not visible to mobile talos infrastructure. We believe this would have been caught if the patch been reviewed by someone who understood the mobile code. 

3) jhford has written a separate followup patch, for the missing mobile functionality and is working out reviews with bsmedberg. Assuming reviews, and staging runs go perfectly, this should be ready to put into production in downtime tomorrow.

Comment 34

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: buildbot-configs v1

This patch uses the new xpi file.  I have verified that the xpi is downloaded and is allowing Talos to run again on the mobile phones.

I am still using a straight ShellCommand with wget.  Changing to DownloadFile can be filed as a separate bug if it is desired.

Comment 35

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: buildbotcustom v1

required buildbotcustom factory changes

Comment 36

[Aki Sasaki (not active)]

Comment on attachment 463241 [details] [diff] [review]
buildbot-configs v1

As mentioned in person, I disagree about the tarball removal.  We can discuss offline.

I've added the xpi to the tarball creation so this is effectively fixed for mobile Talos already; this patch isn't needed.  I would r+ this if the patch allowed for optional tarball usage rather than removing it entirely.

Comment 37

[Aki Sasaki (not active)]

Comment on attachment 463242 [details] [diff] [review]
buildbotcustom v1

r- for tarball removal.

Comment 38

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to comment #36)
> Comment on attachment 463241 [details] [diff] [review]
> buildbot-configs v1
> 
> As mentioned in person, I disagree about the tarball removal.  We can discuss
> offline.
> 
> I've added the xpi to the tarball creation so this is effectively fixed for
> mobile Talos already; this patch isn't needed.  I would r+ this if the patch
> allowed for optional tarball usage rather than removing it entirely.

The case that you mentioned in person, fennecmark, is something that I feel like we should account for specifically.  If a test doesn't use fennecmark, I don't think we should be downloading or installing it.  My reasoning is that it increases the chance of a download being cut off midstream by the network and that our testing environment should not have extraneous libraries.

What problem does having the option to download the pageloader and fennecmark in the same tarball solve? 

If I were to write a follow up patch that has a configuration option for where to get a fennecmark tarball/xpi from and install it on its own for tests that use fennecmark, would you be ok with this patch landing as is?  When testing new pageloaders and/or fennecmark versions, we could set pageloader_url and fennecmark_url in the config file to point to an arbitrary url.  Would that work for you?

Comment 39

[Aki Sasaki (not active)]

I think we can mark this bug fixed: desktop talos was updated earlier and I've updated the pageloader tarball to put the pageloader.xpi in distribution/bundles on mobile talos runs.  John Ford and I can take our disagreement over further changes offline or to a different, non-blocking bug.