426444 - make talos work with xulrunner

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Doug Turner (:dougt)]

talos needs to be trained to work with xulrunner.

The first step is to be able to pass a addition argument to the application.  We will do this by creating a new variable in the browser config object.

Comment 1

[Doug Turner (:dougt)]

Attachment: adds additional arg so that we can launch xulapps

Take a look at the included diff to the sample config (not to be checked in, but a nice way of seeing how this is to work).

Only tested on linux, but the mac and windows changes should work (they are similar enough, right?)

There are other changes needed to make talos completely work (such as, um, monitoring the right process and not just monitoring the process named `firefox`).  Taking babysteps.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 313003 [details] [diff] [review]
adds additional arg so that we can launch xulapps

wrong patch. doh.

Comment 3

[Doug Turner (:dougt)]

Attachment: the actual changes to talos

that last patch was a hack to mozgtkembed and a separate issue.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 313004 [details] [diff] [review]
the actual changes to talos

oh, hmm. is there a way to have a optional option in config?  This change will require that extra_args be defined in all config files as:

extra_args = {}

if extra_args isn't used.

Comment 5

[Doug Turner (:dougt)]

Attachment: includes process_name

this patch also includes the needed bits to allow talos to monitor a different process name than firefox. Before my change, we simply always looked for a process name of "firefox".  After my change, we can change this value in the config file.

Comment 6

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

Found in triage. 

Can we move this to Future, as RelEng isnt doing anything here until your work is done? Also, what is the status of the above patch - noone is marked as reviewer.

Comment 7

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

oops. somehow lost doug as assignee. reseting.

Comment 8

[Doug Turner (:dougt)]

patch ready for review.  this change is required by 419947.

Comment 9

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

Comment on attachment 313625 [details] [diff] [review]
includes process_name

Probably don't want to use your path as the default:

# Path to Firefox to test
-firefox: firefox/firefox.exe
+firefox: /home/dougt/builds/moz1.9/mozilla/obj-i686-pc-linux-gnu-ff/dist/bin/firefox

If we are trying to generalize this to handle more applications than firefox then the 'firefox' setting in the config file should be changed out for something like 'appPath' or the like.  That would mean changing the config file and the references to the browser_config:

   browser_config = {'preferences'  : yaml_config['preferences'],
                     'extensions'   : yaml_config['extensions'],
                     'firefox'      : yaml_config['firefox'],
+                    'process_name' : yaml_config['process_name'],
+                    'extra_args'   : yaml_config['extra_args'],
                     'branch'       : yaml_config['branch'], 
                     'buildid'      : yaml_config['buildid'],

Looks like you missed a reference to firefox here:

talos/ffsetup.py
-  cmd = ffprocess.GenerateFirefoxCommandLine(firefox_path, profile_dir, init_url)
+  cmd = ffprocess.GenerateFirefoxCommandLine(firefox_path, extra_args, profile_dir, init_url)
   (match, timed_out) = ffprocess.RunProcessAndWaitForOutput(cmd,
-                                                              'firefox',
-                                                              PROFILE_REGEX,
-                                                              30)
+                                                            'firefox',
+                                                            PROFILE_REGEX,
+                                                            30)

Also, the patch needs to be merged with the new talos process handling code that has been checked in.

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.2

ignore the sample.config, i will post that separately.

I would said this is good to go, but I am seeing some profile locking problems which I am unsure if they are related to my xulrunner build or my talos changes.

alice, is there a way we can test these talos changes against ff3 on all plaforms (a try server for talos?)

Comment 11

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

I have a talos staging environment that includes all platforms.  Not quite a talos try since I have to log in and manually apply my patch, but it will get it appropriately tested before we try anything crazy.

Comment 12

[Doug Turner (:dougt)]

great.  Could you try this last patch out?

with xulrunner, i am seeing:

/home/dougt/builds/moz1.9/mozilla/obj-i686-pc-linux-gnu/dist/bin/xulrunner /home/dougt/builds/fennec/browser/application.ini \
-profile /tmp/tmpovVZks/profile -tp page_load_test/jss/jss.manifest -tpchrome -tpformat tinderbox -tpcycles 1
WARNING: failed to os.chmod(/tmp/tmpovVZks/profile/lock): 2 : No such file or directory

I did not see this with the last version of talos.

Comment 13

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

The V2 patch includes both the code to work with xulrunner and the xres code - I think that we should keep that in two separate patches.

Also - you are going to want to ignore some of the changes; specifically to the manifest files and the sample.config.  You are working with the standalone manifest files which force talos to do local file load of test pages instead of through localhost, but the default behavior should continue to be to load through localhost.  Standalone talos also has some custom changes to sample.config that we don't want in in the standard, checked-in version of talos.  While we will want your changes to the sample.config having to do with path/appname/extra_args, we won't want the path to your own local copy of firefox or to the output directory.

Comment 14

[Doug Turner (:dougt)]

right, that last patch isn't ready for review.  I am still seeing some issues when running against xulrunner+fennec that needs to be fixed.

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 318281 [details] [diff] [review]
patch v.3

sample config file needs to add:

#process_name: firefox                                                                                                                                                                                        
#extra_args: ""

Comment 17

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

This looks good, but I want to do some testing on staging-talos before we move ahead.  At the worst all of the checked in config files used by talos machines will have to be patched to understand the new variables.  Not a big change but it would have to be lined up properly with the push of this code so as not to cause giant talos redness on the waterfall.

Comment 18

[Doug Turner (:dougt)]

Alice, can you also update the standalone talos?

Comment 19

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

I was going to test the patch and ran into a couple of problems:

- can't apply the patch, throws this error: 
patching file ffprocess_linux.py
patch: **** malformed patch at line 50:

- patch does include changes to the sample.config file, weren't there some that are necessary?

As to the patch not applying I'm pretty confused by it.  All reference that I can find to that sort of error indicate a patch damaged during downloading, but everything looks okay to me.

Comment 20

[Doug Turner (:dougt)]

Attachment: same as v.3, but hopefully applies for alice.

should be the same thing.

I did not make any changes to sample.config dispite changes being required.  I added two new attributes:


process_name:

This is the name of the process that Talos will look for when trying to determine if xul runner is still alive.

If you are using a standard version of XUL Runner, the process name is "xulrunner-bin".

extra_args: ""

This is the path to the location of your "application.ini" file. This must be a full path.

Comment 21

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

Comment on attachment 320281 [details]
same as v.3, but hopefully applies for alice.

Applied the patch, ran it and got:

Traceback (most recent call last):
  File "run_tests.py", line 355, in <module>
    test_file(arg)
  File "run_tests.py", line 292, in test_file
    'process_name' : yaml_config['process_name'],
KeyError: 'process_name'

I think that we want that process_name to be optional (you have a default setting for it a few lines down).  Looks like this is going to need a little finesse to make it not throw an exception when that key/value is not present in the sample.config.

Comment 22

[Doug Turner (:dougt)]

as I mentioned in the bug: process_name, and extra_args are NOT optional.


Any idea how to make optional params with yaml?  I remember you saying not to worry about it.

Comment 23

[Doug Turner (:dougt)]

over to alice.

All of our production mozilla tinderboxes pull from HEAD so checking this in as is will break the world.  This means you can not extend talos and check in without worrying about breaking the entire world.  I suggest that our infrastructure pulls from a tag so that you can separate development from deployment.

I would be happy to help you look into using something other than yaml_config, but it sound (from IRC) like you might have an idea of how to support optional arguments.  I didn't know that the configuration options were not extensible. I really think for talos to be more useful, supporting additional arguments is going to be needed.

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #22)
> as I mentioned in the bug: process_name, and extra_args are NOT optional.
> 

extra_args should definitely be optional

Comment 25

[Doug Turner (:dougt)]

well, extra_args are not optional for xulrunner applications (xulrunner <path_to_app>/application.ini)

The point is, that these can have default values in the config file.

Comment 26

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

Triaged back to Doug, while investigations continue. 

It would be great if its possible to have a solution that does not require updating, and restarting, every single talos machine in a series of scheduled downtimes...

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #26)

> It would be great if its possible to have a solution that does not require
> updating, and restarting, every single talos machine in a series of scheduled
> downtimes...
> 

That is tough to do as talos has "firefox" hardcoded in several places.

Comment 28

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

Having firefox hardcoded isn't a very big obstacle - talos machines pull fresh code per test run, so if we put together a patch that expunges the use of "firefox" it should be okay.

What this current patch does require is config file changes, which is a much more messy prospect.  We have a lot of production config files checked in and each would have to be patched.  I'd prefer if the patch could use the current config file format by having truly optional arguments that handle the xulrunner case.

Comment 29

[Doug Turner (:dougt)]

sure.  easy fix.

Comment 30

[Doug Turner (:dougt)]

Attachment: v.4

same as before, but i moved the two optional parameters (extra_args and process_name) to be tested for before calling yaml_config.  Turns out that we do this for other config settings that are optional.

Comment 31

[Doug Turner (:dougt)]

this patch works for me.  over to default.

Comment 32

[Stuart Parmenter]

hey guys, what is going on with this bug?  patch is waiting on review since May...

Comment 33

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

I'm going to re-work the patch for the latest talos code, it's on my list and should be done in the next two weeks.

It isn't currently blocking any of the work being done to automate the running of talos tests, as far as I know.

Comment 34

[Doug Turner (:dougt)]

any attempt to get talos working on maemo/fennec will require this patch.

Comment 35

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

Attachment: [Checked in]make talos suitable for mobile testing

This patch covers:
- removing hard coded references to 'firefox' and replacing with variable process (for process name that should be monitored) and browser_path (for full path to browser)
- adding variable extra_args to be able to pass more options to browser during start up
- general clean up of internal function calls named 'Firefox' or 'firefox'

This is currently running on the talos stage set up and does not affect the running of standard talos boxes.

Comment 36

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

Comment on attachment 350240 [details] [diff] [review]
[Checked in]make talos suitable for mobile testing

Checking in PerfConfigurator.py;
/cvsroot/mozilla/testing/performance/talos/PerfConfigurator.py,v  <--  PerfConfigurator.py
new revision: 1.7; previous revision: 1.6
done
Checking in README.txt;
/cvsroot/mozilla/testing/performance/talos/README.txt,v  <--  README.txt
new revision: 1.5; previous revision: 1.4
done
Checking in cmanager_linux.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_linux.py,v  <--  cmanager_linux.py
new revision: 1.3; previous revision: 1.2
done
Checking in cmanager_mac.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_mac.py,v  <--  cmanager_mac.py
new revision: 1.4; previous revision: 1.3
done
Checking in cmanager_win32.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_win32.py,v  <--  cmanager_win32.py
new revision: 1.2; previous revision: 1.1
done
Checking in ffprocess.py;
/cvsroot/mozilla/testing/performance/talos/ffprocess.py,v  <--  ffprocess.py
new revision: 1.8; previous revision: 1.7
done
Checking in ffprocess_linux.py;
/cvsroot/mozilla/testing/performance/talos/ffprocess_linux.py,v  <--  ffprocess_linux.py
new revision: 1.9; previous revision: 1.8
done
Checking in ffprocess_mac.py;
/cvsroot/mozilla/testing/performance/talos/ffprocess_mac.py,v  <--  ffprocess_mac.py
new revision: 1.10; previous revision: 1.9
done
Checking in ffprocess_win32.py;
/cvsroot/mozilla/testing/performance/talos/ffprocess_win32.py,v  <--  ffprocess_win32.py
new revision: 1.8; previous revision: 1.7
done
Checking in ffsetup.py;
/cvsroot/mozilla/testing/performance/talos/ffsetup.py,v  <--  ffsetup.py
new revision: 1.6; previous revision: 1.5
done
Checking in run_tests.py;
/cvsroot/mozilla/testing/performance/talos/run_tests.py,v  <--  run_tests.py
new revision: 1.32; previous revision: 1.31
done
Checking in sample.config;
/cvsroot/mozilla/testing/performance/talos/sample.config,v  <--  sample.config
new revision: 1.20; previous revision: 1.19
done
Checking in ttest.py;
/cvsroot/mozilla/testing/performance/talos/ttest.py,v  <--  ttest.py
new revision: 1.17; previous revision: 1.16
done

Comment 37

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

Follow up for renaming filenames in bug 467683

Comment 38

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

Successfully cycling on production talos boxes.  Further talos/mobile engineering work is being tracked in bug 455755.