1103123 - Stop rebooting Linux slaves after buildbot jobs

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Morgan Phillips [:mrrrgn]]

To primary things need to happen:

- Modify the cleanup task so that it does not run before every job.
- Create a runner task which kills processes which were not present just after boot.

Comment 1

[Morgan Phillips [:mrrrgn]]

Attachment: https://github.com/mrrrgn/build-cleanslate

Comment 2

[Morgan Phillips [:mrrrgn]]

I built a tool called 'cleanslate' for killing all processes which weren't live just after system boot. This seems like a good start at cleaning up after jobs w/o rebooting.

Comment 3

[Rail Aliiev [:rail]]

Comment on attachment 8527733 [details]
https://github.com/mrrrgn/build-cleanslate

I created a branch with some comments: https://github.com/rail/build-cleanslate/compare/comments?expand=1

Comment 4

[Morgan Phillips [:mrrrgn]]

Attachment: cleanslate.puppet.diff

Deploy cleanslate to CentOS machines and create a cleanslate puppet module.

Comment 5

[Morgan Phillips [:mrrrgn]]

Attachment: tools.clean_or_reboot.diff

Comment 6

[Rail Aliiev [:rail]]

Comment on attachment 8528563 [details] [diff] [review]
cleanslate.puppet.diff

lgtm

Comment 7

[Morgan Phillips [:mrrrgn]]

Attachment: https://github.com/mozilla/build-runner/pull/16

Sorry to keep hounding you with reviews! >.<

Comment 8

[Rail Aliiev [:rail]]

Comment on attachment 8529395 [details] [review]
https://github.com/mozilla/build-runner/pull/16

Merged

Comment 9

[Rail Aliiev [:rail]]

(In reply to Morgan Phillips [:mrrrgn] from comment #7)
> Created attachment 8529395 [details] [review]
> https://github.com/mozilla/build-runner/pull/16
> 
> Sorry to keep hounding you with reviews! >.<

No worries!

Comment 10

[Morgan Phillips [:mrrrgn]]

Attachment: runner.halt.diff

This deploys the halt task to all runner platforms. It's turning out to be important in my local tests, since I've noticed some weird errors popping up after 4-5 runs in some cases.

Comment 11

[Morgan Phillips [:mrrrgn]]

Attachment: idlelizer.diff

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8529435 [details] [diff] [review]
runner.halt.diff

Review of attachment 8529435 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/buildslave/manifests/startup/runner.pp
@@ +33,5 @@
>              }
>          }
>      }
>      include runner::tasks::buildbot
> +    include runner::tasks::halt

Is this class defined in a different, as-yet-unapplied, patch?

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8530877 [details] [diff] [review]
idlelizer.diff

Review of attachment 8530877 [details] [diff] [review]:
-----------------------------------------------------------------

::: slave/buildslave/idleizer.py
@@ +76,5 @@
> +            if err.check(AttributeError):
> +                log.msg("Master does not support slave initiated shutdown.  Upgrade master to 0.8.3 or later to use this feature.")
> +            else:
> +                log.msg('callRemote("shutdown") failed')
> +                log.err(err)

Our masters are running 0.8.2 .. does this even work?  If it does work (because we backported support, as I seem to recall we did), we could just remove this conditional; if it doesn't, then we have other problems!

@@ +100,5 @@
>              self._setTimer(self.max_disconnected_time)
>  
>      def registerActivity(self):
>          if self._state == CONNECTED:
> +            if sys.platform == 'linux2' and not self._remote_shutdown_requested:

Why just on Linuxes?  Is that temporary?

Comment 14

[Morgan Phillips [:mrrrgn]]

(In reply to Dustin J. Mitchell [:dustin] from comment #13)
> Comment on attachment 8530877 [details] [diff] [review]
> idlelizer.diff
> 
> Review of attachment 8530877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: slave/buildslave/idleizer.py
> @@ +76,5 @@
> > +            if err.check(AttributeError):
> > +                log.msg("Master does not support slave initiated shutdown.  Upgrade master to 0.8.3 or later to use this feature.")
> > +            else:
> > +                log.msg('callRemote("shutdown") failed')
> > +                log.err(err)
> 
> Our masters are running 0.8.2 .. does this even work?  If it does work
> (because we backported support, as I seem to recall we did), we could just
> remove this conditional; if it doesn't, then we have other problems!
> 
> @@ +100,5 @@
> >              self._setTimer(self.max_disconnected_time)
> >  
> >      def registerActivity(self):
> >          if self._state == CONNECTED:
> > +            if sys.platform == 'linux2' and not self._remote_shutdown_requested:
> 
> Why just on Linuxes?  Is that temporary?

It's temporary. It will also work on OSX currently; but I didn't want to roll this out all at once.

Comment 15

[Morgan Phillips [:mrrrgn]]

Attachment: runner.haltclass.diff

Comment 16

[Morgan Phillips [:mrrrgn]]

(In reply to Dustin J. Mitchell [:dustin] from comment #13)
> Comment on attachment 8530877 [details] [diff] [review]
> idlelizer.diff
> 
> Review of attachment 8530877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: slave/buildslave/idleizer.py
> @@ +76,5 @@
> > +            if err.check(AttributeError):
> > +                log.msg("Master does not support slave initiated shutdown.  Upgrade master to 0.8.3 or later to use this feature.")
> > +            else:
> > +                log.msg('callRemote("shutdown") failed')
> > +                log.err(err)
> 
> Our masters are running 0.8.2 .. does this even work?  If it does work
> (because we backported support, as I seem to recall we did), we could just
> remove this conditional; if it doesn't, then we have other problems!
> 
> @@ +100,5 @@
> >              self._setTimer(self.max_disconnected_time)
> >  
> >      def registerActivity(self):
> >          if self._state == CONNECTED:
> > +            if sys.platform == 'linux2' and not self._remote_shutdown_requested:
> 
> Why just on Linuxes?  Is that temporary?

I'll remove the un-necessary conditional in another commit.

Comment 17

[Morgan Phillips [:mrrrgn]]

Attachment: buildslave.update.diff

Comment 18

[Morgan Phillips [:mrrrgn]]

I accidentally submitted the 'update buildslave' patch with the wrong reviewer in the commit message. The reviewer was coop not dustin.

Comment 19

[Morgan Phillips [:mrrrgn]]

Attachment: cleanslate.runner.update.diff

Comment 20

[Morgan Phillips [:mrrrgn]]

Attachment: postflight.diff

This will allow us to turn on off no-reboots selectively. In this version only build machines will go into no reboot mode.

Comment 21

[Rail Aliiev [:rail]]

Comment on attachment 8533302 [details] [diff] [review]
postflight.diff

Review of attachment 8533302 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/runner/files/post_flight.py
@@ +16,5 @@
> +
> +
> +def get_hostname():
> +    sysname, nodename, release, version, machine = os.uname()
> +    return nodename.split('.')[0]

I think we use socket.gethostname() for this.

@@ +22,5 @@
> +
> +def get_recent_builds(slavename):
> +    url = '%s%s%s' % (BUILD_API_URL_BASE, slavename, BUILD_API_URL_SUFFIX)
> +    request = urllib2.Request(url)
> +    results = urllib2.urlopen(request)

Please set some reasonable timeout here. Otherwise this may hang forever.

@@ +23,5 @@
> +def get_recent_builds(slavename):
> +    url = '%s%s%s' % (BUILD_API_URL_BASE, slavename, BUILD_API_URL_SUFFIX)
> +    request = urllib2.Request(url)
> +    results = urllib2.urlopen(request)
> +    processed_results = json.loads(results.read())

This may raise at least ValueError and the script would exit(1) in this case. This means RETRY for runner. Not sure if this is what you want here.

@@ +37,5 @@
> +    rm_files = ['/var/tmp/cleanslate']
> +    for f in rm_files:
> +        if os.path.exists(f):
> +            print('removing: %s' % f)
> +            os.remove(f)

Possible race condition and uncaught exception (IOError and OSError IIRc). Can you wrap os.remove() with try/except and don't call os.path.exists?

@@ +45,5 @@
> +def get_hostname_blacklist():
> +    '''
> +    A list of hostname expressions which will coerce a halt.
> +    '''
> +    return ['^t.*']  # halt all test machines

this would also blacklist try-linux64. 
["^tst-.*", "^t-.*", "^talos-.*"] should match per http://hg.mozilla.org/build/buildbot-configs/file/f2255d82ce6d/mozilla-tests/production_config.py

@@ +56,5 @@
> +    '''
> +    for expression in blacklist:
> +        if re.match(expression, entry):
> +            return True
> +    return False

A nit, the same can be done as

return any(re.match(e, entry) for e in blacklist)

@@ +71,5 @@
> +        halt()
> +
> +    # stagger web requests in case of retries
> +    time.sleep(random.randint(1, 5))
> +    build_data = get_recent_builds(hostname)

I'm not sure how sleeping without retrying may help here... Also 5 seconds won't stop slaves from hammering if you add retrying.

@@ +75,5 @@
> +    build_data = get_recent_builds(hostname)
> +
> +    if not build_data:
> +        # The exception should coerce a retry
> +        raise Exception('Failed to find data for %s' % hostname)

This will retry the script, correct?

Comment 22

[Morgan Phillips [:mrrrgn]]

(In reply to Rail Aliiev [:rail] from comment #21)
> Comment on attachment 8533302 [details] [diff] [review]
> postflight.diff
> 
> Review of attachment 8533302 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/runner/files/post_flight.py
> @@ +16,5 @@
> > +
> > +
> > +def get_hostname():
> > +    sysname, nodename, release, version, machine = os.uname()
> > +    return nodename.split('.')[0]
> 
> I think we use socket.gethostname() for this.
> 
> @@ +22,5 @@
> > +
> > +def get_recent_builds(slavename):
> > +    url = '%s%s%s' % (BUILD_API_URL_BASE, slavename, BUILD_API_URL_SUFFIX)
> > +    request = urllib2.Request(url)
> > +    results = urllib2.urlopen(request)
> 
> Please set some reasonable timeout here. Otherwise this may hang forever.
> 
> @@ +23,5 @@
> > +def get_recent_builds(slavename):
> > +    url = '%s%s%s' % (BUILD_API_URL_BASE, slavename, BUILD_API_URL_SUFFIX)
> > +    request = urllib2.Request(url)
> > +    results = urllib2.urlopen(request)
> > +    processed_results = json.loads(results.read())
> 
> This may raise at least ValueError and the script would exit(1) in this
> case. This means RETRY for runner. Not sure if this is what you want here.
> 
> @@ +37,5 @@
> > +    rm_files = ['/var/tmp/cleanslate']
> > +    for f in rm_files:
> > +        if os.path.exists(f):
> > +            print('removing: %s' % f)
> > +            os.remove(f)
> 
> Possible race condition and uncaught exception (IOError and OSError IIRc).
> Can you wrap os.remove() with try/except and don't call os.path.exists?
> 
> @@ +45,5 @@
> > +def get_hostname_blacklist():
> > +    '''
> > +    A list of hostname expressions which will coerce a halt.
> > +    '''
> > +    return ['^t.*']  # halt all test machines
> 
> this would also blacklist try-linux64. 
> ["^tst-.*", "^t-.*", "^talos-.*"] should match per
> http://hg.mozilla.org/build/buildbot-configs/file/f2255d82ce6d/mozilla-tests/
> production_config.py
> 
> @@ +56,5 @@
> > +    '''
> > +    for expression in blacklist:
> > +        if re.match(expression, entry):
> > +            return True
> > +    return False
> 
> A nit, the same can be done as
> 
> return any(re.match(e, entry) for e in blacklist)
> 
> @@ +71,5 @@
> > +        halt()
> > +
> > +    # stagger web requests in case of retries
> > +    time.sleep(random.randint(1, 5))
> > +    build_data = get_recent_builds(hostname)
> 
> I'm not sure how sleeping without retrying may help here... Also 5 seconds
> won't stop slaves from hammering if you add retrying.
> 
> @@ +75,5 @@
> > +    build_data = get_recent_builds(hostname)
> > +
> > +    if not build_data:
> > +        # The exception should coerce a retry
> > +        raise Exception('Failed to find data for %s' % hostname)
> 
> This will retry the script, correct? It will.

Sleeping was a silly idea; there's now a timeout of 12 seconds for open sockets. I addressed the other changes as well.

Anything that happens outside of calling halt() should result in a retry, so not handling most of the exceptions will be fine. Problems will show up in the logs and hit max-retries if it's not something intermittent (like a single timeout).

Comment 23

[Dustin J. Mitchell [:dustin] (he/him)]

"defensive programming" / "let-it-crash"
 ++!

Comment 24

[Morgan Phillips [:mrrrgn]]

(In reply to Dustin J. Mitchell [:dustin] from comment #23)
> "defensive programming" / "let-it-crash"
>  ++!

Defensive programming is always good; but in this case let it crash makes the most since. The idea is that, if anything seems even slightly awry we should consider forcing a reboot. Letting exceptions raise here will trigger a retry, then, eventually a reboot thanks to the fact that it's a process being managed by runner. The only cases worth handling are cases where we want to reboot immediately instead of retrying.

Comment 25

[Morgan Phillips [:mrrrgn]]

(In reply to Dustin J. Mitchell [:dustin] from comment #23)
> "defensive programming" / "let-it-crash"
>  ++!

*sense (& any other grammer derps) :( :( :(

Comment 26

[Morgan Phillips [:mrrrgn]]

Attachment: postflight.diff

Comment 27

[Rail Aliiev [:rail]]

Comment on attachment 8533386 [details] [diff] [review]
postflight.diff


                     ,@;@,
        ,@;@;@;@;@;@/ )@;@;
      ,;@;@;@;@;@;@|_/@' e\
     (|@;@:@\@;@;@;@:@(    \
       '@;@;@|@;@;@;@;'`"--'
        '@;@;/;@;/;@;' 
         ) //   | ||
         \ \\   | ||
          \ \\  ) \\  it!
           `"`  `"``

Comment 28

[Morgan Phillips [:mrrrgn]]

Attachment: https://github.com/mozilla/build-runner/pull/19

Comment 29

[Morgan Phillips [:mrrrgn]]

Attachment: runner.update.1.5.diff

Comment 30

[Morgan Phillips [:mrrrgn]]

Attachment: runner.reboot-after-30.diff

Comment 31

[Morgan Phillips [:mrrrgn]]

Attachment: runner.noreboot.talos.diff

This will stop rebooting talos machines (should be applied on a Monday)

Comment 32

[Chris AtLee [:catlee]]

Comment on attachment 8535844 [details] [diff] [review]
runner.noreboot.talos.diff

Review of attachment 8535844 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, as long is joel is ok with it!

Comment 33

[Chris AtLee [:catlee]]

Comment on attachment 8533811 [details] [review]
https://github.com/mozilla/build-runner/pull/19

merged on github a while ago

Comment 34

[Morgan Phillips [:mrrrgn]]

Attachment: runner.service_restart.diff

Comment 35

[Rail Aliiev [:rail]]

Comment on attachment 8538746 [details] [diff] [review]
runner.service_restart.diff

Review of attachment 8538746 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/runner/templates/tasks/restart_services.erb
@@ +4,5 @@
> +
> +RESTART_SERVICES="<%= @service_names %>"
> +
> +for service_name in ${RESTART_SERVICES}; do
> +  if [[ $(service ${service_name} status) != '' ]]; then

Can you check the exit code instead? The approach above may not work on Ubuntu since the command always prints something to stdout.

Comment 36

[Morgan Phillips [:mrrrgn]]

Attachment: runner.service_restart.diff

Comment 37

[Morgan Phillips [:mrrrgn]]

(In reply to Rail Aliiev [:rail] from comment #35)
> Comment on attachment 8538746 [details] [diff] [review]
> runner.service_restart.diff
> 
> Review of attachment 8538746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/runner/templates/tasks/restart_services.erb
> @@ +4,5 @@
> > +
> > +RESTART_SERVICES="<%= @service_names %>"
> > +
> > +for service_name in ${RESTART_SERVICES}; do
> > +  if [[ $(service ${service_name} status) != '' ]]; then
> 
> Can you check the exit code instead? The approach above may not work on
> Ubuntu since the command always prints something to stdout.

Good point, I  swapped it out with something more sensible. Thanks! :)

Comment 38

[Morgan Phillips [:mrrrgn]]

So, now with service restarts in place I'm going to stop reboots on a large swath of testers before proceeding. Once the 't-' and 'tst-' machines are removed from the post_flight hostname blacklist this task will be _finished_! \m/

Comment 39

[Morgan Phillips [:mrrrgn]]

Attachment: test-noreboot.diff

To turn off tester rebooting again.

Comment 40

[Justin Wood (:Callek)]

Comment on attachment 8542648 [details] [diff] [review]
test-noreboot.diff

Review of attachment 8542648 [details] [diff] [review]:
-----------------------------------------------------------------

lets see what burns

Comment 41

[Phil Ringnalda (:philor)]

Good luck telling what burns - between bug 989048 and bug 1109932 exploding and the bizarre "anything can fail and probably will" bug 1114541 and the month-long rash of bug 1073442 consistently happening once a slave started hitting it until the instance was terminated, which seems to have suddenly stopped at 7am Monday, I had myself convinced that we already were doing no-reboot on tst-linux*, and that it was the reason why absolutely no result on those slaves could be believed. So if this does wind up having any effect on the running of tests other than complete and permanent failure, you absolutely will not be able to tell that from sheriff reports, because we currently can't tell the difference between tst-linux* results and random() results.

Comment 42

[Morgan Phillips [:mrrrgn]]

Attachment: enable-test-reboots.diff

Philor, I have this patch ready and am watching the trees. I added you for info just to call your attention to the way that reboots are working now. The buildbot logs say "reboot skipped" but that's a result of the configs not being changed to reflect our current situation.

Now, after every job, several checks are performed to decide if a machine should be rebooted. If its hostname matches a blacklist regex it will always be rebooted. An overall view of how many machines are rebooting at any time can be found here: https://stats.taskcluster.net/grafana/#/dashboard/db/runner

Comment 43

[Phil Ringnalda (:philor)]

Comment on attachment 8542746 [details] [diff] [review]
enable-test-reboots.diff

Got it, thanks.

(Other than an inability to see anything on taskcluster, since "login with your @mozilla.com address" would require that I first quit my current job, and then get hired by MoCo.)