tested in staging, will attach builder diff

builder diff for previous buildbot-configs patch

patches for puppet

Comment on attachment 8490326 [details] [diff] [review] bug1066823puppet.patch Review of attachment 8490326 [details] [diff] [review]: ----------------------------------------------------------------- some nits/notes, was merely a skim. ::: modules/foopy/manifests/init.pp @@ +32,1 @@ > "/builds/check2.log", note check*.log and check.sh itself only support tegras: http://mxr.mozilla.org/build/source/tools/sut_tools/check.py#266 Probably worth doing a file { "foo": ensure=>absent; } for these that you're removing though. @@ +68,3 @@ > File["/builds/check.log"], > File["/builds/check2.log"], > File["/builds/tegra_stats.log"], You remove tegra_stats.log above so this will fail. ::: modules/slaverebooter/templates/slaverebooter.ini.erb @@ -14,5 @@ > b-2008-sm = > # Not enough jobs > panda = > fed = > -tegra = still need this until slavealloc doesn't return any tegras in its list, (as in, safer to leave this in for now, since its JUST a ".contains()" check of ignorable strings. )

tested in staging by running some panda tests to make sure I didn't delete too much, results seem fine so far. Also, I realize I have to remove the slavealloc references to tegras, but will update the db first.

(In reply to Kim Moir [:kmoir] from comment #5) > Also, I realize I have to remove the > slavealloc references to tegras, but will update the db first. I've already taken care of this in slavealloc, both for staging and production.

Comment on attachment 8490317 [details] [diff] [review] bug1066823.patch for buildbot-configs Review of attachment 8490317 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla-tests/production_config.py @@ -49,5 @@ > - SLAVES['tegra_android']['tegra-%03i' % i] = { > - 'http_port': '30%03i' % i, > - 'ssl_port': '31%03i' % i, > - } > - this fun loop will *not* be missed

Comment on attachment 8490326 [details] [diff] [review] bug1066823puppet.patch Review of attachment 8490326 [details] [diff] [review]: ----------------------------------------------------------------- r- per my comments in c#5 but worth a closer look for some of the other pieces.

Comment on attachment 8490377 [details] [diff] [review] bug1066823tools.patch Review of attachment 8490377 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you follow these nits anyway. IFF you think the changes I suggest are contentious or will take longer, please land the devices.json and production-masters.json changes _now_ since the masters are already off. (breaks reconfig script without it) ::: buildfarm/maintenance/watch_twistd_log.py @@ -136,5 @@ > re.compile(re.escape("exceptions.AttributeError: BuildSlave instance has no attribute 'perspective_shutdown'")), > # Ignore users cancelling try runs > - re.compile(re.escape("Failure: exceptions.RuntimeError")), > - # Ignore clean-close "errors" from tegras > - re.compile(re.escape("Failure: twisted.spread.pb.PBConnectionLost: [Failure instance: Traceback (failure with no frames): <class 'twisted.internet.error.ConnectionDone'>: Connection was closed cleanly")), I'd love to leave this in, at least for now, pending an eval to see if they exist for pandas or other slave classes. This error was added a long time ago. We can certainly change the comment though. ::: buildfarm/mobile/watch_devices.sh @@ +110,5 @@ > > > function watch_launcher(){ > log "STARTING Watcher" > + ls -d panda-*[0-9]} 2>/dev/null | sed 's:.*/::' | while read device; do still need the /builds/* piece here. ::: sut_tools/mozdevice/devicemanagerSUT.py @@ -255,5 @@ > socketClosed = True > errStr = str(err) > - # This error shows up with we have our tegra rebooted. > - if err[0] == errno.ECONNRESET: > - errStr += ' - possible reboot' please don't modify devicemanagerSUT (for now). Its based on an upstream code, albeit we have a very old version. ::: sut_tools/verify.py @@ -417,5 @@ > else: > device_name = args[0] > > - # Only attempt updating the watcher INI if we run against a tegra. > - doWatcherUpdate = 'tegra' in device_name we can probably remove the watcherINI update code at the same time given this.

Comment on attachment 8490377 [details] [diff] [review] bug1066823tools.patch I landed the devices.json and production_masters.json changes

Landed a bustage fix for devices.json (see bug 1071518). Probably we should set up unit tests to check validity of our json files, as this is an easy mistake to make.

(In reply to Pete Moore [:pete][:pmoore] from comment #11) > Landed a bustage fix for devices.json (see bug 1071518). > > Probably we should set up unit tests to check validity of our json files, as > this is an easy mistake to make. Created bug 1071746

incorporating feedback from previous review. The tegras all have been removed for slavealloc so I think the comment doesn't apply anymore to the change in modules/slaverebooter/templates/slaverebooter.ini.erb

Something here landed in production today: https://wiki.mozilla.org/ReleaseEngineering/Maintenance#Reconfigs_.2F_Deployments

Comment on attachment 8494569 [details] [diff] [review] bug1066823puppet-2.patch Review of attachment 8494569 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/foopy/manifests/init.pp @@ +55,5 @@ > group => root, > ensure => file, > content => template("foopy/foopy.crontab.erb"), > require => [ > Class["foopy::repos"], not blocking this patch, but mentioning since I noticed it now, we should add a File["/builds/watch_devices.sh"] here as well.

Comment on attachment 8494592 [details] [diff] [review] bug1066823tools-2.patch Review of attachment 8494592 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you fix these two issues ::: buildfarm/mobile/watch_devices.sh @@ +110,5 @@ > > > function watch_launcher(){ > log "STARTING Watcher" > + ls -d /builds/{panda-*[0-9]} 2>/dev/null | sed 's:.*/::' | while read device; do the {...} won't expand without a comma in it, (bash is fun). No need to re-review for this change. [jwood@foopy63.p4.releng.scl3.mozilla.com ~]$ ls -d /builds/{panda-*[0-9]} ls: cannot access /builds/{panda-*[0-9]}: No such file or directory [jwood@foopy63.p4.releng.scl3.mozilla.com ~]$ ls -d /builds/panda-*[0-9] /builds/panda-0382 /builds/panda-0385 /builds/panda-0388 /builds/panda-0391 /builds/panda-0394 /builds/panda-0383 /builds/panda-0386 /builds/panda-0389 /builds/panda-0392 /builds/panda-0384 /builds/panda-0387 /builds/panda-0390 /builds/panda-0393 ::: sut_tools/verify.py @@ +415,5 @@ > log.info( > "INFO: Using device '%s' found in env variable" % device_name) > else: > device_name = args[0] > Also "nope" we need `if verifyDevice...` here, otherwise we're not actually verifying anything. The specific lines in verify.py we should remove are highlighted by http://mxr.mozilla.org/build/source/tools/sut_tools/verify.py?mark=34-34,280-336,394-397,420-422,424-424,339 (note 339 we actually just need to modify)

Comment on attachment 8494569 [details] [diff] [review] bug1066823puppet-2.patch And fixed the other issue you mentioned too in http://hg.mozilla.org/build/puppet/rev/82877c73b5f9

patch with feedback from comment #17

just to verify I ran the panda tests in staging which worked fine. I also ran test-masters.sh which was green

Comment on attachment 8498905 [details] [diff] [review] buildbotcustom patch Review of attachment 8498905 [details] [diff] [review]: ----------------------------------------------------------------- can you run this against a builders list diff? We should see no changes. Would also love the added assurance of a full dump_master diff, but I won't block on that piece, since these changes all look sane.

I just did a builder diff, no changes. So the diff is not that exciting to post to the bug :-)

Comment on attachment 8498905 [details] [diff] [review] buildbotcustom patch Thanks Callek!

In production, closing.

Merged to production, and deployed.