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

RESOLVED FIXED

Status

Release Engineering
Platform Support
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kmoir, Assigned: kmoir)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Assignee: nobody → kmoir
(Assignee)

Comment 1

3 years ago
Created attachment 8490317 [details] [diff] [review]
bug1066823.patch for buildbot-configs

tested in staging, will attach builder diff
(Assignee)

Updated

3 years ago
Attachment #8490317 - Attachment description: bug1066823.patch → bug1066823.patch for buildbot-configs
(Assignee)

Comment 2

3 years ago
Created attachment 8490325 [details]
bug1066823builder.diff

builder diff for previous buildbot-configs patch
(Assignee)

Comment 3

3 years ago
Created attachment 8490326 [details] [diff] [review]
bug1066823puppet.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. )
(Assignee)

Comment 5

3 years ago
Created attachment 8490377 [details] [diff] [review]
bug1066823tools.patch

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.
(Assignee)

Updated

3 years ago
Attachment #8490317 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

3 years ago
Attachment #8490326 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

3 years ago
Attachment #8490377 - Flags: review?(bugspam.Callek)

Comment 6

3 years ago
(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+
(Assignee)

Comment 10

3 years ago
Comment on attachment 8490377 [details] [diff] [review]
bug1066823tools.patch

I landed the devices.json and production_masters.json changes
See Also: → bug 1071518

Updated

3 years ago
Depends on: 1071518
See Also: bug 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
See Also: → bug 1071746
(Assignee)

Updated

3 years ago
Attachment #8490317 - Flags: checked-in+
(Assignee)

Updated

3 years ago
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
(Assignee)

Comment 13

3 years ago
Created attachment 8494569 [details] [diff] [review]
bug1066823puppet-2.patch

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)
(Assignee)

Comment 14

3 years ago
Created attachment 8494592 [details] [diff] [review]
bug1066823tools-2.patch
Attachment #8490377 - Attachment is obsolete: true
Attachment #8494592 - Flags: review?(bugspam.Callek)
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.
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+
(Assignee)

Comment 18

3 years ago
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+
(Assignee)

Comment 19

3 years ago
Created attachment 8497586 [details] [diff] [review]
bug1066823tools-3.patch

patch with feedback from comment #17
(Assignee)

Updated

3 years ago
Attachment #8497586 - Flags: checked-in+
(Assignee)

Comment 20

3 years ago
Created attachment 8498905 [details] [diff] [review]
buildbotcustom patch

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+
(Assignee)

Comment 22

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

Comment 23

3 years ago
Comment on attachment 8498905 [details] [diff] [review]
buildbotcustom patch

Thanks Callek!
Attachment #8498905 - Flags: checked-in+
(Assignee)

Comment 24

3 years ago
In production, closing.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Merged to production, and deployed.
You need to log in before you can comment on or make changes to this bug.