Closed Bug 1066823 Opened 10 years ago Closed 10 years ago

remove old code that supports tegras in buildbot-configs/buildbotcustom/tools/mozharness/puppet

Categories

(Infrastructure & Operations Graveyard :: CIDuty, task)

x86
macOS
task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kmoir, Assigned: kmoir)

References

Details

Attachments

(6 files, 2 obsolete files)

      No description provided.
Assignee: nobody → kmoir
tested in staging, will attach builder diff
Attachment #8490317 - Attachment description: bug1066823.patch → bug1066823.patch for buildbot-configs
Attached file bug1066823builder.diff
builder diff for previous buildbot-configs patch
Attached patch bug1066823puppet.patch (obsolete) — Splinter Review
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. )
Attached patch bug1066823tools.patch (obsolete) — Splinter Review
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.
Attachment #8490317 - Flags: review?(bugspam.Callek)
Attachment #8490326 - Flags: review?(bugspam.Callek)
Attachment #8490377 - Flags: review?(bugspam.Callek)
(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
Attachment #8490317 - Flags: review?(bugspam.Callek) → review+
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.
Attachment #8490326 - Flags: review?(bugspam.Callek) → review-
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.
Attachment #8490377 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8490377 [details] [diff] [review]
bug1066823tools.patch

I landed the devices.json and production_masters.json changes
Depends on: 1071518
See Also: 1071518
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
Attachment #8490317 - Flags: checked-in+
Summary: remove old code that supports tegras in buildbot-configs/buildbotcustom/tools/mozharness → remove old code that supports tegras in buildbot-configs/buildbotcustom/tools/mozharness/puppet
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
Attachment #8490326 - Attachment is obsolete: true
Attachment #8494569 - Flags: review?(bugspam.Callek)
Attachment #8490377 - Attachment is obsolete: true
Attachment #8494592 - Flags: review?(bugspam.Callek)
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.
Attachment #8494569 - Flags: review?(bugspam.Callek) → review+
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)
Attachment #8494592 - Flags: review?(bugspam.Callek) → review+
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
Attachment #8494569 - Flags: checked-in+
patch with feedback from comment #17
Attachment #8497586 - Flags: checked-in+
just to verify I ran the panda tests in staging which worked fine.  I also ran test-masters.sh which was green
Attachment #8498905 - Flags: review?(bugspam.Callek)
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.
Attachment #8498905 - Flags: review?(bugspam.Callek) → review+
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!
Attachment #8498905 - Flags: checked-in+
In production, closing.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Merged to production, and deployed.
Component: Platform Support → Buildduty
Product: Release Engineering → Infrastructure & Operations
Product: Infrastructure & Operations → Infrastructure & Operations Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: