579566 - Create factory/steps to establish remote-testing environment for tegra

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Mike Taylor [:bear]]

Create scripts to download, extract and setup environment to run the remote tests in the RelEng buildbot realm

Comment 1

[Mike Taylor [:bear]]

currently waiting on some bug fixes to both SUTAgentAndroid and also devicemanager.py

ctalbert tells me via IRC that the m-c tree is closed so he won't be able to land until later (bug 573281) and the SUTAgentAndroid code changes do not currently have a bug

Comment 2

[Mike Taylor [:bear]]

Attachment: [WIP] buildbotcustom changes for android tegra test integration

Comment 3

[Aki Sasaki (not active)]

Comment on attachment 474003 [details] [diff] [review]
[WIP] buildbotcustom changes for android tegra test integration

>-        self.addStep(UnpackFile(
>-            filename=WithProperties('%(build_filename)s'),
>-            scripts_dir='../tools/buildfarm/utils',
>-            haltOnFailure=True,
>-            name='unpack_build',
>-        ))
>+        if not self.platform.startswith('android'):
>+            self.addStep(UnpackFile(
>+                filename=WithProperties('%(build_filename)s'),
>+                scripts_dir='../tools/buildfarm/utils',
>+                haltOnFailure=True,
>+                name='unpack_build',
>+            ))

I know you're just doing this to get things working for now, and I approve of that.
The real fix will be to add .apk to the UnpackFile step here:

http://hg.mozilla.org/build/buildbotcustom/file/85b5eb2c7dc8/steps/misc.py#l379

>-        self.addStep(SetProperty,
>-         command=['python', WithProperties('%(toolsdir)s/buildfarm/utils/printbuildrev.py'),
>-                  WithProperties('%(exedir)s')],
>-         workdir='build',
>-         extract_fn=get_build_info,
>-         name='get build info',
>-        )

Once you fix the UnpackFile step, you should be able to use printbuildrev.py and not remove this.
(Basically trying to keep the number of upstream changes to a minimum when it comes time to review this -- but again, my first priority is getting this working at all, second is getting that into a reviewable state.)

>+        if self.platform.startswith('android'):
>+            self.addStep(SetProperty,
>+                 command=['bash', '-c', 'echo $TEGRA_IP'],
>+                 property='tegra_ip'
>+                )

This seems like it might belongs in a new SUTTestFactory which inherits MozillaTestFactory, though I do see that UnittestPackagedBuildFactory inherits MozillaTestFactory. Hm.

Also, do you need to set the property if you can use $TEGRA_IP in your ShellCommands?

I'm also thinking for futureproofing, TEGRA_IP should be SUT_IP, but that's definitely picking nits.

> class MozillaPackagedMochitests(ShellCommandReportTimeout):
>     warnOnFailure = True
>     warnOnWarnings = True
> 
>-    def __init__(self, variant='plain', symbols_path=None, leakThreshold=None,
>+    def __init__(self, platform, variant='plain', symbols_path=None, leakThreshold=None,

You're getting this working by specifying platform and differentiating there.
That's certainly one way.
I think Clint and Joel specify this in the test harnesses by setting remote=True, which might be preferable.

>-        self.command = ['python', 'mochitest/runtests.py',
>-                WithProperties('--appname=%(exepath)s'), '--utility-path=bin',
>-                WithProperties('--extra-profile-file=bin/plugins'),
>-                '--certificate-path=certs', '--autorun', '--close-when-done',
>-                '--console-level=INFO']
>+        if platform.startswith('android'):
>+            self.command = ['python', 'mochitest/runtests.py',
>+                    WithProperties('--appname=%(exepath)s'), '--utility-path=bin',
>+                    WithProperties('--extra-profile-file=bin/plugins'),
>+                    '--certificate-path=certs', '--autorun', '--close-when-done',
>+                    '--console-level=INFO']
>+        else:
>+            self.command = ['python', 'mochitest/runtests.py',
>+                    WithProperties('--appname=%(exepath)s'), '--utility-path=bin',
>+                    WithProperties('--extra-profile-file=bin/plugins'),
>+                    '--certificate-path=certs', '--autorun', '--close-when-done',
>+                    '--console-level=INFO']

Am I missing something? WIP?

>-        self.command = ['python', 'reftest/runreftest.py',
>-                WithProperties('--appname=%(exepath)s'),
>-                '--utility-path=bin',
>-                '--extra-profile-file=bin/plugins',
>-                ]
>+        if platform.startswith('android'):
>+            self.command = ['python', 'reftest/remotereftest.py',
>+                    '--deviceIP=192.168.42.101', #FIXME
>+                    '--app=org.mozilla.fennec',
>+                    '--xre-path=bin',
>+                    '--utility-path=bin',
>+                    '--extra-profile-file=bin/plugins',
>+                    ]
>+        else:
>+            self.command = ['python', 'reftest/runreftest.py',
>+                    WithProperties('--appname=%(exepath)s'),
>+                    '--utility-path=bin',
>+                    '--extra-profile-file=bin/plugins',
>+                    ]

Aha!

Hm. Not sure how to do this elegantly.
However, I think the above could be
if self.remote:
    self.command = 'python reftest/remotereftest.py '+
                   '--deviceIP=$SUT_IP '+
                   WithProperties('--appname=%(exepath)s')+' '+ # or '--appname='+self.appName+' '+ ?
                   '--xre-path=bin '+
                   '--utility-path=bin '+
                   '--extra-profile-file=bin/plugins '

On second thought, I hate the +'s and having to remember spaces and I'd just as soon ' '.join() the list so the $SUT_IP works.

Should --app be --appname ? or is that different in remotereftest.py ?

Comment 4

[Aki Sasaki (not active)]

(In reply to comment #3)
> if self.remote:
>     self.command = 'python reftest/remotereftest.py '+
>                    '--deviceIP=$SUT_IP '+
>                    WithProperties('--appname=%(exepath)s')+' '+ # or
> '--appname='+self.appName+' '+ ?
>                    '--xre-path=bin '+
>                    '--utility-path=bin '+
>                    '--extra-profile-file=bin/plugins '
> 
> On second thought, I hate the +'s and having to remember spaces and I'd just as
> soon ' '.join() the list so the $SUT_IP works.

Of course, with a join() we couldn't use WithProperties.

self.appName doesn't make sense; it should be self.remoteAppName since non-remote doesn't use it.

I should just shut up and let you get this working first, maybe :)

Comment 5

[Mike Taylor [:bear]]

Attachment: [WIP] buildbotcustom changes for android tegra unittests

Comment 6

[Mike Taylor [:bear]]

Attachment: [WIP] buildbotcustom changes for android tegra unittests

first steps for talos - chose ts cold to start with.  had to stop at the test running because I need to add my IP to the graph's server whitelist.

Comment 7

[Mike Taylor [:bear]]

Attachment: [WIP] buildbotcustom changes for android tegra unittests

Comment 8

[Mike Taylor [:bear]]

Attachment: [WIP] buildbotcustom changes for android tegra unittests

talos runs end-to-end but a problem with PerfConfigurator is preventing the test from finishing.  jmaher is looking into it.

Comment 9

[Mike Taylor [:bear]]

Attachment: [WIP] buildbotcustom changes for android tegra unittests

changed TEGRA_ to SUT_

Comment 10

[Mike Taylor [:bear]]

Attachment: [WIP] buildbotcustom changes for android tegra unittests

Comment 11

[Mike Taylor [:bear]]

Attachment: changes to tegra buildbotcustom to move the remote web server to branch_config

Comment 12

[Aki Sasaki (not active)]

Comment on attachment 481928 [details] [diff] [review]
changes to tegra buildbotcustom to move the remote web server to branch_config

> class TalosFactory(BuildFactory):
>     extName = 'addon.xpi'
>     """Create working talos build factory"""
>     def __init__(self, OS, supportUrlBase, envName, buildBranch, branchName,
>             configOptions, talosCmd, customManifest=None,
>             customTalos=None, workdirBase=None, fetchSymbols=False,
>             plugins=None, pageset=None, talosAddOns=[], addonTester=False,
>-            remoteTests=False, productName="firefox",
>+            remoteTests=False, productName="firefox", remotetest_web=None,
>             cvsRoot=":pserver:[e-mail]:/cvsroot"):

Ok.

1. NIT: we're all camelcase here, let's follow that.

2. I'm not sure I'm thrilled about adding a bunch of extra init vars here, of which remoteTestWeb will be the first.
   It's probably ok, but we might also be able to force that into **extra. or **remoteTestConfig or something.

So I think we should a) try a remoteTestConfig dictionary that's only used if self.remoteTests, and if that turns out to be fugly, we can add a bunch of init vars.

>@@ -52,17 +52,17 @@ class MozillaUpdateConfig(ShellCommand):
>             # python PerfConfigurator.py -v -e org.mozilla.fennec -t `hostname` -b
>             # mobile-browser --activeTests tzoom --sampleConfig remote.config --browserWait
>             # 60 --noChrome --output local.config --remoteDevice 192.168.1.115 --webServer
>             # 192.168.1.102/talos
> 
>             self.addOptions += ['--sampleConfig', 'remote.config',
>                                 '--output', 'local.yml',
>                                 '--remoteDevice', WithProperties('%(sut_ip)s'),
>-                                '--webServer', '192.168.42.6',
>+                                '--webServer', self.remoteTestWeb,
>                                 '--noChrome',
>                                 '--browserWait', '60',
>                                ]

1. I don't think you're actually passing remoteTestWeb to MozillaUpdateConfig from TalosFactory, so this won't pass checkconfig.
2. --noChrome should be set depending on the test
3. browserWait and sampleConfig is ideally configurable from config.py rather than digging into steps/talos to change hardcodes.

Comment 13

[Mike Taylor [:bear]]

Attachment: changes to tegra buildbotcustom to move remote config values to branch_config

Comment 14

[Mike Taylor [:bear]]

Attachment: changes to tegra buildbot-configs to support remoteExtras in branch_config

Comment 15

[Aki Sasaki (not active)]

Comment on attachment 482001 [details] [diff] [review]
changes to tegra buildbot-configs to support remoteExtras in branch_config

This looks good to me.
We may want the webserver to be different between staging and production, but I'm fine with this -- I know that'll be ugly.

Comment 16

[Aki Sasaki (not active)]

Comment on attachment 482000 [details] [diff] [review]
changes to tegra buildbotcustom to move remote config values  to branch_config

Let's land in sut-bb* and run on foopy til we figure out our plan for getting this code landed upstream to buildbot-configs and buildbotcustom proper, and put on test-master01.

Comment 17

[Mike Taylor [:bear]]

sut-buildbotcustom committed changeset 27397a31cc2f
sut-buildbot-configs committed changeset 9e0a05272c45

Comment 18

[Aki Sasaki (not active)]

Attachment: and android/tegras to mozilla-tests

Comment 19

[Aki Sasaki (not active)]

Attachment: add android to TalosFactory, MozillaUpdateConfig

with mobile_tinderbox_tree.

Passes test-masters.sh; running on bm-foopy and reporting to http://tinderbox.mozilla.org/MobileTest . (Bear: note, this means that foopy is on 8012 now, and is running off of /builds/buildbot/aki-bb-c and aki-bbc temporarily).

I was going to just diff all of http://hg.mozilla.org/users/asasaki_mozilla.com/sut-buildbot-configs/ and http://hg.mozilla.org/users/asasaki_mozilla.com/sut-buildbotcustom/ against the real repos, but there's still a bunch of unittest cleanup that needs to happen.  This should just land Talos fixes.

I also made some tweaks to this patch that hasn't yet landed in sut-bb*; will try to do that soon to avoid future conflicts.

Comment 20

[Aki Sasaki (not active)]

Looks like we're missing changeset links on Tinderbox.
Tgfx is running very very fast (Tp4 was running very fast too, but I just fixed a broken softlink to the tp4 pageset, which should fix that).

John: I would very much like to land these with known issues that need fixing, and iterate on those under the watchful eye of #mobile, rather than blocking on anything that is cosmetic or fixable.

If, however, there's something wrong with the patches that you strongly feel needs fixing before this lands, we will address those.

Comment 21

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

Comment on attachment 483103 [details] [diff] [review]
and android/tegras to mozilla-tests

In no particular order:

> +BRANCHES['mozilla-central']['cold_tests'] = (1, True, {}, ALL_PLATFORMS)

as discussed in person, this shouldn't be there

>+        'android': {
>+            'builds_before_reboot':  1,
>+            'enable_opt_unittests': True,
>+            'enable_debug_unittests': False,
>+            'download_symbols': False,
>+            'tegra_android': {
>+                'opt_unittest_suites' : >UNITTEST_SUITES['opt_unittest_suites'][:] +
>+                                        [('opengl', ['opengl'])],
>+                'debug_unittest_suites': >UNITTEST_SUITES['debug_unittest_suites'][:] +
>+                                         [('opengl', ['opengl'])],
>+                # 'opt_unittest_suites':   >UNITTEST_SUITES['opt_unittest_suites'][:],
>+                # 'debug_unittest_suites': >UNITTEST_SUITES['debug_unittest_suites'][:],
>+            },
>+        },

If we aren't planning on doing unit tests with this patch, we probably shouldn't have this here.

+    'tegra-ts': GRAPH_CONFIG + ['--activeTests', 'ts', '--noChrome'],
+    'tegra-tgfx': GRAPH_CONFIG + ['--activeTests', 'tdhtml:tgfx', '--noChrome'],
+    'tegra-tsvg': GRAPH_CONFIG + ['--activeTests', 'tsvg:tp4', '--noChrome'],

So two concerns here, one is that we are calling a composite run of tdhtml and tgfx by one component's name, 'tgfx'.  If we are going to have multiple suites in one run, could we call it 'X-tgfx-tdhtml' or 'X-perf-group1'?  This allows us to change the groupings without silently breaking/changing downstream assumptions.

The second concern is that I don't know if we should hardcode the tegra name into the suite name.  This is incorporated into the tinderbox column header "Tegra 250 mozilla-central talos tegra-tsvg".  Could we maybe call it 'remote-X', resulting in "Tegra 250 mozilla-central talos remote-tsvg"

I will continue my review of buildbotcustom shortly (i.e. after lunch).  The r- is for the merge conflict and the unit test inclusions.

Comment 22

[Aki Sasaki (not active)]

Attachment: address review concerns

This should address your concerns + passes checkconfig.
I'm going to push this to foopy and mozilla-tests1 to verify it works in both remote- and non-remote- testing as expected.

Comment 23

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

Comment on attachment 483104 [details] [diff] [review]
add android to TalosFactory, MozillaUpdateConfig

Cool!  This looks great.  I do have a couple of questions and comments though.

=======================================

> class TalosFactory(BuildFactory):
>     extName = 'addon.xpi'
>     """Create working talos build factory"""
>     def __init__(self, OS, supportUrlBase, envName, buildBranch, branchName,
>             configOptions, talosCmd, customManifest=None, customTalos=None,
>             workdirBase=None, fetchSymbols=False, plugins=None, pageset=None,
>+            remoteTests=False, productName="firefox", remoteExtras=None,
>             talosAddOns=[], addonTester=False):
<snip>
>+        self.remoteExtras = remoteExtras

It seems that this is only consumed by

>+        if remoteExtras is not None:
>+            self.remoteExtras = remoteExtras
>+        else:
>+            self.remoteExtras = {}
>+
>+        self.remoteOptions = self.remoteExtras.get('options', [])
>+        self.remoteExePath = self.remoteExtras.get('exePath', 'org.mozilla.fennec')

Is there any reason that we couldn't make these normal parameters instead of passing them in through a dictionary?  I don't see any **remoteExtras usage so I am going to that this isn't needed.

What if we did something like |remoteOptions=None, remoteExePath='org.mozilla.fennec'| in the constructor?

========================================

>+        else:
>+            self.addStep(SetBuildProperty(
>+             property_name="exepath",
>+             value="../%s/%s-bin" % (self.productName, self.productName)
>             ))

Another concern is the default exepath value.  For firefox, the default action is to use firefox-bin.  This is not true for fennec running on linux, which will be currently expanded to fennec-bin.  fennec-bin doesn't exist anywhere.  We could do something like

>+        else:
              if 'fennec' in self.productName:
                binarySuffix = ''
              else:
                binarySuffix = '-bin'
>+            self.addStep(SetBuildProperty(
>+             property_name="exepath",
>+             value="../%s/%s-%s" % (self.productName, self.productName, binarySuffix)
>             ))

I realise that this isn't the prettiest thing in the world.

========================================

>+            self.addStep(ShellCommand(
>+             name='copy_fennecmark',
>+             command=["cp", "-r", "../../../../../talos-data/[e-mail]", "."],
>+             workdir="%s/talos/mobile_profile/extensions/" % self.workdirBase,
>+             description="copying fennecmark",
>+             haltOnFailure=True,
>+             flunkOnFailure=True,
>+             env=self.env)
>+            )

This is hard to read and likely to be a source of small errors.  Is there any reason we can't do something like (assume i have the correct number of parent directories):

>+             command=["cp", "-r", "../../talos-data/[e-mail]", "talos/mobile_profile/extensions/"],
>+             workdir=self.workdirBase,

========================================

>     def setBuild(self, build):
>         self.super_class.setBuild(self, build)
>         title = build.slavename
>+        perfconfigurator = "PerfConfigurator.py"
>+
>         #if we are an addonTester then the addonName/addonUrl build property should be set
>         #  if it's not set this will throw a key error and the run will go red - which should be the expected result
>         if self.addonTester:
>             addon_prefix = self.build.getProperty('addonName')
>             self.addOptions += ['--testPrefix', addon_prefix, '--extension', self.extName]
> 
>         if self.useSymbols:
>             self.addOptions += ['--symbolsPath', '../symbols']
> 
>-        self.setCommand(["python", "PerfConfigurator.py", "-v", "-e",
>-            self.exePath, "-t", title, "-b", self.branch,
>+        if self.remoteTests:
>+            exePath = self.remoteExePath
>+            perfconfigurator = "remotePerfConfigurator.py"
>+
>+            self.addOptions += ['--remoteDevice', WithProperties('%(sut_ip)s'), ]
>+            self.addOptions += self.remoteOptions
>+        else:
>+            exePath = self.exePath
>+
>+        self.setCommand(["python", perfconfigurator, "-v", "-e", exePath,
>+            "-t", title, "-b", self.branch,
>             '--branchName', self.branchName] + self.addOptions)

This doesn't make it clear to me that the perfconfigurator is different for remote tests.  Lets make this more explicit.  We could do something like

>     def setBuild(self, build):
>         self.super_class.setBuild(self, build)
>         title = build.slavename
          if self.remoteTests:
              perfconfigurator = "remotePerfConfigurator.py"
              exePath = self.remoteExePath
          else:
              perfconfigurator = "PerfConfigurator.py"
              exePath = self.exePath
>+
>         #if we are an addonTester then the addonName/addonUrl build property should be set
>         #  if it's not set this will throw a key error and the run will go red - which should be the expected result
>         if self.addonTester:
>             addon_prefix = self.build.getProperty('addonName')
>             self.addOptions += ['--testPrefix', addon_prefix, '--extension', self.extName]
> 
>         if self.useSymbols:
>             self.addOptions += ['--symbolsPath', '../symbols']
> 
>+        if self.remoteTests:
>+            self.addOptions += ['--remoteDevice', WithProperties('%(sut_ip)s'), ]
>+            self.addOptions += self.remoteOptions
>+
>+        self.setCommand(["python", perfconfigurator, "-v", "-e", exePath,
>+            "-t", title, "-b", self.branch,
>             '--branchName', self.branchName] + self.addOptions)


When you get a chance to update the patch and finish testing on the desktop talos and unit test configs, I will r+.

Comment 24

[Mike Taylor [:bear]]

(In reply to comment #23)
> Comment on attachment 483104 [details] [diff] [review]
> add android to TalosFactory, MozillaUpdateConfig
> 
> Cool!  This looks great.  I do have a couple of questions and comments though.
> 
> =======================================
> 
> > class TalosFactory(BuildFactory):
> >     extName = 'addon.xpi'
> >     """Create working talos build factory"""
> >     def __init__(self, OS, supportUrlBase, envName, buildBranch, branchName,
> >             configOptions, talosCmd, customManifest=None, customTalos=None,
> >             workdirBase=None, fetchSymbols=False, plugins=None, pageset=None,
> >+            remoteTests=False, productName="firefox", remoteExtras=None,
> >             talosAddOns=[], addonTester=False):
> <snip>
> >+        self.remoteExtras = remoteExtras
> 
> It seems that this is only consumed by
> 
> >+        if remoteExtras is not None:
> >+            self.remoteExtras = remoteExtras
> >+        else:
> >+            self.remoteExtras = {}
> >+
> >+        self.remoteOptions = self.remoteExtras.get('options', [])
> >+        self.remoteExePath = self.remoteExtras.get('exePath', 'org.mozilla.fennec')
> 
> Is there any reason that we couldn't make these normal parameters instead of
> passing them in through a dictionary?  I don't see any **remoteExtras usage so
> I am going to that this isn't needed.
> 
> What if we did something like |remoteOptions=None,
> remoteExePath='org.mozilla.fennec'| in the constructor?
> 
> ========================================

The list of possible values could be more than just the two we have now and they are only related to running Talos on remote devices like the Tegra.  By putting them into a dictionary that is populated from the config.py we can make changes to the tegra environment via reconfig and not a downtime.

remoteExtras is used steps/Talos.py as part of the MozillaUpdateConfig

> 
> >+        else:
> >+            self.addStep(SetBuildProperty(
> >+             property_name="exepath",
> >+             value="../%s/%s-bin" % (self.productName, self.productName)
> >             ))
> 
> Another concern is the default exepath value.  For firefox, the default action
> is to use firefox-bin.  This is not true for fennec running on linux, which
> will be currently expanded to fennec-bin.  fennec-bin doesn't exist anywhere. 
> We could do something like
> 
> >+        else:
>               if 'fennec' in self.productName:
>                 binarySuffix = ''
>               else:
>                 binarySuffix = '-bin'
> >+            self.addStep(SetBuildProperty(
> >+             property_name="exepath",
> >+             value="../%s/%s-%s" % (self.productName, self.productName, binarySuffix)
> >             ))
> 
> I realise that this isn't the prettiest thing in the world.
> 
> ========================================
> 
> >+            self.addStep(ShellCommand(
> >+             name='copy_fennecmark',
> >+             command=["cp", "-r", "../../../../../talos-data/[e-mail]", "."],
> >+             workdir="%s/talos/mobile_profile/extensions/" % self.workdirBase,
> >+             description="copying fennecmark",
> >+             haltOnFailure=True,
> >+             flunkOnFailure=True,
> >+             env=self.env)
> >+            )
> 
> This is hard to read and likely to be a source of small errors.  Is there any
> reason we can't do something like (assume i have the correct number of parent
> directories):
> 
> >+             command=["cp", "-r", "../../talos-data/[e-mail]", "talos/mobile_profile/extensions/"],
> >+             workdir=self.workdirBase,
> 
> ========================================
> 
> >     def setBuild(self, build):
> >         self.super_class.setBuild(self, build)
> >         title = build.slavename
> >+        perfconfigurator = "PerfConfigurator.py"
> >+
> >         #if we are an addonTester then the addonName/addonUrl build property should be set
> >         #  if it's not set this will throw a key error and the run will go red - which should be the expected result
> >         if self.addonTester:
> >             addon_prefix = self.build.getProperty('addonName')
> >             self.addOptions += ['--testPrefix', addon_prefix, '--extension', self.extName]
> > 
> >         if self.useSymbols:
> >             self.addOptions += ['--symbolsPath', '../symbols']
> > 
> >-        self.setCommand(["python", "PerfConfigurator.py", "-v", "-e",
> >-            self.exePath, "-t", title, "-b", self.branch,
> >+        if self.remoteTests:
> >+            exePath = self.remoteExePath
> >+            perfconfigurator = "remotePerfConfigurator.py"
> >+
> >+            self.addOptions += ['--remoteDevice', WithProperties('%(sut_ip)s'), ]
> >+            self.addOptions += self.remoteOptions
> >+        else:
> >+            exePath = self.exePath
> >+
> >+        self.setCommand(["python", perfconfigurator, "-v", "-e", exePath,
> >+            "-t", title, "-b", self.branch,
> >             '--branchName', self.branchName] + self.addOptions)
> 
> This doesn't make it clear to me that the perfconfigurator is different for
> remote tests.  Lets make this more explicit.  We could do something like

this doesn't seem more explicit to me - all you have done is add another remoteTests check and moved the setting of the default value to the else.

> 
> >     def setBuild(self, build):
> >         self.super_class.setBuild(self, build)
> >         title = build.slavename
>           if self.remoteTests:
>               perfconfigurator = "remotePerfConfigurator.py"
>               exePath = self.remoteExePath
>           else:
>               perfconfigurator = "PerfConfigurator.py"
>               exePath = self.exePath
> >+
> >         #if we are an addonTester then the addonName/addonUrl build property should be set
> >         #  if it's not set this will throw a key error and the run will go red - which should be the expected result
> >         if self.addonTester:
> >             addon_prefix = self.build.getProperty('addonName')
> >             self.addOptions += ['--testPrefix', addon_prefix, '--extension', self.extName]
> > 
> >         if self.useSymbols:
> >             self.addOptions += ['--symbolsPath', '../symbols']
> > 
> >+        if self.remoteTests:
> >+            self.addOptions += ['--remoteDevice', WithProperties('%(sut_ip)s'), ]
> >+            self.addOptions += self.remoteOptions
> >+
> >+        self.setCommand(["python", perfconfigurator, "-v", "-e", exePath,
> >+            "-t", title, "-b", self.branch,
> >             '--branchName', self.branchName] + self.addOptions)
> 
> 
> When you get a chance to update the patch and finish testing on the desktop
> talos and unit test configs, I will r+.

Comment 25

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

(In reply to comment #24)
> (In reply to comment #23)
> > 
> > Is there any reason that we couldn't make these normal parameters instead of
> > passing them in through a dictionary?  I don't see any **remoteExtras usage so
> > I am going to that this isn't needed.
> > 
> > What if we did something like |remoteOptions=None,
> > remoteExePath='org.mozilla.fennec'| in the constructor?
> > 
> > ========================================
> 
> The list of possible values could be more than just the two we have now and
> they are only related to running Talos on remote devices like the Tegra.  By
> putting them into a dictionary that is populated from the config.py we can make
> changes to the tegra environment via reconfig and not a downtime.
> 
> remoteExtras is used steps/Talos.py as part of the MozillaUpdateConfig

How does this affect whether a change is a downtime or a reconfig?

I realize that it could grow larger, but I don't see how this implementation is advantageous over using python's built-in language features.  Instead of passing in a random data-store dictionary that gets used in multiple places, why not be more explicit and pass in things that the factory is concerned with as kwargs to the factory.  If we need to be able to pass settings into individual steps, we could either make them factory arguments that are passed into the steps or we could pass dictionaries into the factory that are in turn passed to the necessary step constructor as a **kwarg dictionary.  This provides a clear mapping between factory argument and purpose of the argument which makes the code easier to read and easier to maintain.

> > class TalosFactory(BuildFactory):
> >     extName = 'addon.xpi'
> >     """Create working talos build factory"""
> >     def __init__(self, OS, supportUrlBase, envName, buildBranch, branchName,
> >             configOptions, talosCmd, customManifest=None, customTalos=None,
> >             workdirBase=None, fetchSymbols=False, plugins=None, pageset=None,
> >+            remoteTests=False, productName="firefox", remoteExtras=None,
> >             talosAddOns=[], addonTester=False, remoteOptions=[],
                remoteExePath='org.mozilla.fennec'):
> <snip>
> >+
> >+        self.remoteOptions = remoteOptions
> >+        self.remoteExePath = remoteExePath


> this doesn't seem more explicit to me - all you have done is add another
> remoteTests check and moved the setting of the default value to the else.

Instead of overwriting the string in the middle of the function, this makes it clear right away that there is a different configurator used in remote talos testing and in non-remote talos testing.

If the extra if statement is a concern, the same result could be achieved with 

> >     def setBuild(self, build):
> >         self.super_class.setBuild(self, build)
> >         title = build.slavename
> >
> >         #if we are an addonTester then the addonName/addonUrl build property should be set
> >         #  if it's not set this will throw a key error and the run will go red - which should be the expected result
> >         if self.addonTester:
> >             addon_prefix = self.build.getProperty('addonName')
> >             self.addOptions += ['--testPrefix', addon_prefix, '--extension', self.extName]
> > 
> >         if self.useSymbols:
> >             self.addOptions += ['--symbolsPath', '../symbols']
> > 
> >+        if self.remoteTests:
>               perfconfigurator = "remotePerfConfigurator.py"
>               exePath = self.remoteExePath
> >+            self.addOptions += ['--remoteDevice', WithProperties('%(sut_ip)s'), ]
> >+            self.addOptions += self.remoteOptions
>           else:
>               perfconfigurator = "PerfConfigurator.py"
>               exePath = self.exePath
> >+
> >+        self.setCommand(["python", perfconfigurator, "-v", "-e", exePath,
> >+            "-t", title, "-b", self.branch,
> >             '--branchName', self.branchName] + self.addOptions)

Comment 26

[Aki Sasaki (not active)]

(In reply to comment #23)
> > class TalosFactory(BuildFactory):
> >     extName = 'addon.xpi'
> >     """Create working talos build factory"""
> >     def __init__(self, OS, supportUrlBase, envName, buildBranch, branchName,
> >             configOptions, talosCmd, customManifest=None, customTalos=None,
> >             workdirBase=None, fetchSymbols=False, plugins=None, pageset=None,
> >+            remoteTests=False, productName="firefox", remoteExtras=None,
> >             talosAddOns=[], addonTester=False):
> <snip>
> >+        self.remoteExtras = remoteExtras
> 
> It seems that this is only consumed by
> 
> >+        if remoteExtras is not None:
> >+            self.remoteExtras = remoteExtras
> >+        else:
> >+            self.remoteExtras = {}
> >+
> >+        self.remoteOptions = self.remoteExtras.get('options', [])
> >+        self.remoteExePath = self.remoteExtras.get('exePath', 'org.mozilla.fennec')
> 
> Is there any reason that we couldn't make these normal parameters instead of
> passing them in through a dictionary?  I don't see any **remoteExtras usage so
> I am going to that this isn't needed.
> 
> What if we did something like |remoteOptions=None,
> remoteExePath='org.mozilla.fennec'| in the constructor?

The goals here are: a) get Talos running live on Android ASAP, b) be able to iterate quickly on necessary Android Talos changes, and c) do it in an acceptable fashion.

Bear initially got the remotePerfConfigurator steps working by hardcoding Android Tegra command line options into steps.talos.MozillaUpdateConfig; this worked but was not an acceptable solution for landing.

After we verified that his command line hardcodes worked in staging, we began pulling hardcoded configs from factory.py and steps/talos.py into buildbot-configs.  At that point I specifically asked Bear to put the needed remote options into a dictionary that would be passed through TalosFactory into MozillaUpdateConfig rather than creating a set list of __init__ variables. The reasons are:

* There are an open-ended number of these that we will need.

We currently believe that 2 options are all we need to get the initial 3-4 Talos suites running.  I can easily see that this will change based on platform or suite, or based upon Talos checkins.

* Setting these remote-specific options in a dictionary reduces the number of touch points.

To pass these in as __init__ variables, we need to change

 * config.py **extras
 * TalosFactory __init__() defaults
 * TalosFactory __init__() self.variable =
 * TalosFactory call to MozillaUpdateConfig
 * MozillaUpdateConfig __init__() defaults
 * MozillaUpdateConfig __init__() self.variable =

Then consume it.

To pass these in inside of remoteExtras, we need to change

 * config.py extras['remoteExtras']
 * MozillaUpdateConfig __init__() self.variable =

And is therefore faster to iterate and less error-prone.

* Putting in a bunch of potentially arbitrary, platform-, branch-, or OS- specific __init__ variables into both TalosFactory and MozillaUpdateConfig will be confusing.  Do you change TalosFactory.limit_talos_dir from its default of None? Do we name that TalosFactory.limit_talos_dir_for_test_suite_X_on_remote_device_Y_on_branch_Z ?  Or can we put it in a dictionary that is only set in config.py for that specific test suite and remote device, and get rid of it when we don't need it anymore?

* Placing these in remoteExtras, and verifying that Desktop does not use remoteExtras, means we can extend or remove items in remoteExtras with less risk.


Potentially renaming this to remoteTalosPerfConfiguratorExtras would be fine, to be more descriptive.  Changing to __init__ level variables as a future TODO item is acceptable.  I do not understand why changing to __init__ level variables is a blocker to landing Android Tegra Talos in production, meaning it's effectively a Fennec 4.0b2 blocker.

Comment 27

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 483260 [details] [diff] [review]
address review concerns

aki argument wins the day here, it's just too intrusive to take the other approach. However I do think it's clearer to move the |perfconfigurator = "PerfConfigurator.py"| definition into the else: block (end of comment 25). 

An alternative we could think about is sub-classing TalosFactory, which helps with some of the if/else changes in functions like addUnpackBuildSteps.

Comment 28

[Aki Sasaki (not active)]

Attachment: disable tgfx; rename remote-other1 to remote-tdhtml

Comment 29

[Aki Sasaki (not active)]

Attachment: address some buildbotcustom review concerns

Fixed:
- desktop bustage !!
- too many ../../../../../
- moved perfconfigurator setting to else:

This is baking in foopy + talos-staging-master02.

Comment 30

[Aki Sasaki (not active)]

Comment on attachment 483405 [details] [diff] [review]
disable tgfx; rename remote-other1 to remote-tdhtml

running green on foopy; running green+some orange on talos-staging-master02:8010

Comment 31

[Aki Sasaki (not active)]

Comment on attachment 483406 [details] [diff] [review]
address some buildbotcustom review concerns

I'm leaning towards forcing the next Fennec tester to update the ifs with their exe name (to avoid hitting the Fx-specific default), or leaving that for Bear to fix along with changeset links next week.

Comment 32

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

Comment on attachment 483406 [details] [diff] [review]
address some buildbotcustom review concerns

Thanks for addressing the majority of my concerns.  Everything that was addressed looks good now.  Turns out that I messed up copying from vim into the review box yesterday.  There were two concerns that I wanted to bring up but didn't include in the original post.

First was 

>         if self.OS in ('tiger', 'leopard', 'snowleopard'):
>             self.addStep(FindFile(
>              workdir=os.path.join(self.workdirBase, "talos"),
>-             filename="firefox-bin",
>+             filename="%s-bin" % self.productName,
>              directory="..",
>              max_depth=4,
>              property_name="exepath",
>              name="Find executable",
>              filetype="file",
>             ))
<snip>
>+        elif self.OS in ('tegra_android',):
>             self.addStep(SetBuildProperty(
>              property_name="exepath",
>-             value="../firefox/firefox-bin"
>+             value="../%s/%s" % (self.productName, self.productName)
>+            ))
>+        else:
>+            self.addStep(SetBuildProperty(
>+             property_name="exepath",
>+             value="../%s/%s-bin" % (self.productName, self.productName)
>             ))

The assumption of productName-bin is not valid for fennec on desktop linux, maemo linux and I am pretty sure OS X desktop builds.  For those builds, the browser binary file is just 'fennec'.  As we are making these productName changes to support fennec, it'd be great to work properly or fail early and loudly.  I wouldn't be opposed to a hardcoded check for |'fennec' in self.productName| to figure out whether to use '-bin' or not.

It is probably quicker to make the code fail out early if used in a broken configuration with something like:

>+        if self.OS in ('tegra_android',):
>+            self.addStep(UnpackFile(
>+             filename=WithProperties("../%(filename)s"),
>+             workdir="%s/%s" % (self.workdirBase, self.productName),
>+             name="Unpack build",
>+            ))
>+        else:
              #This assertion is to protect us from using fennec-bin on linux/OSX
              #where this is not the binary's actual name.
              assert self.productName is 'firefox', \
                'using productName doesn't work with this os/product'
>+            self.addStep(UnpackFile(
>+             filename=WithProperties("%(filename)s"),
>+             workdir=self.workdirBase,
>+             name="Unpack build",
>+            ))

The other concern was 

>+        buildBranch = branch_config[platform_config.get('branch_var', 'build_branch')]
>+        tinderboxTree = branch_config.get(platform_config.get('tinderbox_tree_var', 'tinderbox_tree'))
>+        if tinderboxTree not in branch_builders:
>+            branch_builders[tinderboxTree] = []
>+        if tinderboxTree not in all_test_builders:
>+            all_test_builders[tinderboxTree] = []

This is somewhat confusing at first sight.  I would prefer that a platform have a 'mobile' boolean dictionary key in the platform_config that decides whether to look at 'mobile_tinderbox_tree' or 'tinderbox_tree'.  If there isn't enough time to change this, can we please add documentation that explains the code or at a minimum draws attention to it?

My suggestion could be implemented as

          if platform_config.get('mobile', False):
              tinderboxTree = branch_config.get('mobile_tinderbox_tree')
              buildBranch = branch_config.get('mobile_build_branch')
          else:
              tinderboxTree = branch_config.get('tinderbox_tree')
              buildBranch = branch_config.get('build_branch')
>+        if tinderboxTree not in branch_builders:
>+            branch_builders[tinderboxTree] = []
>+        if tinderboxTree not in all_test_builders:
>+            all_test_builders[tinderboxTree] = []


Provided that we add the assertion or fix the productName-bin issue and either  document or fix the tinderbox_tree/build_branch configuration settings, r+.  If we go the documentation route for the tinderbox_tree/build_branch settings, I would really like to see a real fix in a future patch.

Comment 33

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

Comment on attachment 483405 [details] [diff] [review]
disable tgfx; rename remote-other1 to remote-tdhtml

r+ as is, or with an additional 'mobile': True in the tegra_android platform dict that is tested to work.

Comment 34

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

(In reply to comment #32)
> Comment on attachment 483406 [details] [diff] [review]
> address some buildbotcustom review concerns

oh, and also, the r+ assumes that the patch works and doesn't break desktop :)

Comment 35

[Aki Sasaki (not active)]

(In reply to comment #32)
> This is somewhat confusing at first sight.  I would prefer that a platform have
> a 'mobile' boolean dictionary key in the platform_config that decides whether
> to look at 'mobile_tinderbox_tree' or 'tinderbox_tree'.

I agree that tinderbox_tree_var is confusing and un-ideal. I'm going to take a quick look and if it's as simple to change as I think it might be, I will change it.

I honestly don't care that the default in the if/else bit ends up with a -bin or not, but if I'm going to be changing anything and re-testing anyway, I see no harm in adding that as well.

Comment 36

[Aki Sasaki (not active)]

Attachment: branch_var, tinderbox_tree_var -> is_mobile

Same configs, but changing the confusing _vars to an is_mobile flag.
Carrying forward review.

Comment 37

[Aki Sasaki (not active)]

Attachment: use is_mobile to deal with branchName/tinderboxTree; -bin change

tested on talos-staging-master02:8010 and bm-foopy.

Comment 38

[Aki Sasaki (not active)]

If someone else lands these, BuildSlaves.py needs to be updated with a tegra_android pass.

Comment 39

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 483645 [details] [diff] [review]
use is_mobile to deal with branchName/tinderboxTree; -bin change

http://hg.mozilla.org/build/buildbotcustom/rev/9f99b1f540ef

Comment 40

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 483644 [details] [diff] [review]
branch_var, tinderbox_tree_var -> is_mobile

http://hg.mozilla.org/build/buildbot-configs/rev/34710df3f5bc

Comment 41

[Aki Sasaki (not active)]

Yay green tegra tests on test-master01!

I'm resolving this fixed; we will open a new bug for fixes/followup work.