1173942 - Stop rebooting machines from idleizer (allow halting instead)

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Morgan Phillips [:mrrrgn]]

Instead it should exit buildbot, and write to a log file

Comment 1

[Morgan Phillips [:mrrrgn]]

Attachment: idelizer-reboots.diff

Comment 2

[Morgan Phillips [:mrrrgn]]

Attachment: idelizer-reboots.diff

Comment 3

[Morgan Phillips [:mrrrgn]]

Background: We would like finer grained control over stopping machines that are idling often. To pass this information to other apps, I'm adding a message into the log when an idle out happens so that we can count them up. I also added the ability to shut down idleizer machine reboots via an environment variable. I chose to make this configurable, because we'll still have some non-runner test machines in the wild when this lands.

Comment 4

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

Comment on attachment 8638771 [details] [diff] [review]
idelizer-reboots.diff

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

Looks good, to the extent that I remember this code :)

Comment 5

[Morgan Phillips [:mrrrgn]]

Attachment: idleizer-reboots.diff

Here we use environment variables to control how the machine behaves when the idle timer runs out. By default we will reboot; but by setting IDLEIZER_HALT_ON_IDLE=true we will get a full shutdown. Shutdowns will terminate spot instances in aws, so this serves as a handy mechanism for dealing with the problem of wasting billing hours on idle time.

Comment 6

[Morgan Phillips [:mrrrgn]]

Shown to be working. Puppet patch coming soon. https://pastebin.mozilla.org/8841014

Comment 7

[Morgan Phillips [:mrrrgn]]

Attachment: idleizer7.diff

Show to work on windows. Bumps idleizer to the new version that supports terminating instances.

Comment 8

[Morgan Phillips [:mrrrgn]]

Attachment: idleizer-reboots.diff

Comment 9

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8640787 [details] [diff] [review]
idleizer7.diff

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

Spreading out the reviews

Comment 10

[Morgan Phillips [:mrrrgn]]

Attachment: haltidle.diff

So, this will terminate the instances; but I'm not sure we want it for all the windows instances. If not, we'll need to use puppet to distinguish between the instance types and only set this in the ones we want terminated.

Comment 11

[Rob Thijssen [:grenade (EET/UTC+0300)]]

Comment on attachment 8640794 [details] [diff] [review]
idleizer-reboots.diff

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

On lines 183 & 185, it would be nice to get a shutdown/restart reason/comment in event log/papertrail. (reason codes (p:4:1): http://ss64.com/nt/shutdown.html) eg:

182	            if not halt:
183	                os.system("shutdown -f -r -t 0 -c 'idleizer restart' -d 'p:4:1'")
184	            else:
185	                os.system("shutdown -f -s -t 0 -c 'idleizer shutdown' -d 'p:4:1'")

Comment 12

[Mark Cornmesser [:markco]]

Comment on attachment 8640797 [details] [diff] [review]
haltidle.diff

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

Looks good. This should also be added to EC2_start-buildbot.bat.erb.

Comment 13

[Justin Wood (:Callek)]

Comment on attachment 8640787 [details] [diff] [review]
idleizer7.diff

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

thought I marked this *days* ago, sorry for the delay.

Comment 14

[Rob Thijssen [:grenade (EET/UTC+0300)]]

Attachment: verbose shutdown/restart in windows eventlog

I'd really like to get a papertrail message indicating the reason for the restart/shutdown. It would allow us to properly analyse our spot instance spawning efficiency. This patch introduces the arguments required to provide this level of verbosity. The syntax is documented at: http://ss64.com/nt/shutdown.html

Comment 15

[Rob Thijssen [:grenade (EET/UTC+0300)]]

pushed: https://hg.mozilla.org/build/buildbot/rev/2724339a1491

Comment 16

[Morgan Phillips [:mrrrgn]]

Attachment: idleizer-halt-argument.diff

This is a rework of the patch that attempted to configure reboot/halt behavior via environment variables. Here, the option is passed into the class at instantiation. The idea is that this can be configured by modifying the buildbot.tac template in slavealoc.

Comment 17

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

Comment on attachment 8682172 [details] [diff] [review]
idleizer-halt-argument.diff

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

Looks good - minor notes.

::: slave/buildslave/idleizer.py
@@ +27,5 @@
>          L{buildslave.bot.BuildSlave} instance created in C{buildbot.tac}.
>          C{max_idle_time} is the maximum time, in seconds, that the slave will
>          be allowed to idle before it is restarted.  Similarly,
>          C{max_disconnected_time} is the maximum time it will be allowed to go
> +        without a connection to a master. If C{halt} is true a shutdown will

nit: wrong parameter name

@@ +113,5 @@
>  
>      def maybeReboot(self):
> +        if self._reboot_or_halt and self._halt_on_shutdown:
> +            self.halt()
> +        elif self.reboot_or_halt:

This would read better with two levels of conditional:

  if reboot_or_halt:
    if halt_on_shutdown:
      halt
    else:
      reboot

@@ +199,5 @@
> +    def halt(self):
> +        log.msg("Invoking platform-specific shutdown command")
> +
> +        if sys.platform in ('darwin', 'linux2'):
> +            os.system("sudo -S shutdown -h now < /dev/null")

Is this already in sudoers?

Comment 18

[Morgan Phillips [:mrrrgn]]

(In reply to Dustin J. Mitchell [:dustin] from comment #17)
> Comment on attachment 8682172 [details] [diff] [review]
> idleizer-halt-argument.diff
> 
> Review of attachment 8682172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good - minor notes.
> 
> ::: slave/buildslave/idleizer.py
> @@ +27,5 @@
> >          L{buildslave.bot.BuildSlave} instance created in C{buildbot.tac}.
> >          C{max_idle_time} is the maximum time, in seconds, that the slave will
> >          be allowed to idle before it is restarted.  Similarly,
> >          C{max_disconnected_time} is the maximum time it will be allowed to go
> > +        without a connection to a master. If C{halt} is true a shutdown will
> 
> nit: wrong parameter name
> 
> @@ +113,5 @@
> >  
> >      def maybeReboot(self):
> > +        if self._reboot_or_halt and self._halt_on_shutdown:
> > +            self.halt()
> > +        elif self.reboot_or_halt:
> 
> This would read better with two levels of conditional:
> 
>   if reboot_or_halt:
>     if halt_on_shutdown:
>       halt
>     else:
>       reboot
> 
> @@ +199,5 @@
> > +    def halt(self):
> > +        log.msg("Invoking platform-specific shutdown command")
> > +
> > +        if sys.platform in ('darwin', 'linux2'):
> > +            os.system("sudo -S shutdown -h now < /dev/null")
> 
> Is this already in sudoers?

Thanks ! Sending another patch to fix this up.

Comment 19

[Morgan Phillips [:mrrrgn]]

s/this/the sudoers up/

Comment 20

[Morgan Phillips [:mrrrgn]]

Attachment: buildslave-bump.diff

Just bumps the buildslave version.

Comment 21

[Morgan Phillips [:mrrrgn]]

Attachment: slavealloc-tac.diff

This will make it so that all non-scl3 machines halt when they idle for 90 minutes, which seems reasonable in a cloud environment. I understand if you object to this patch because of the hard coding. I suppose the only other option would be to modify the db schema and then update everything by hand?

Comment 22

[Morgan Phillips [:mrrrgn]]

(In reply to Morgan Phillips [:mrrrgn] from comment #21)
> Created attachment 8682962 [details] [diff] [review]
> slavealloc-tac.diff
> 
> This will make it so that all non-scl3 machines halt when they idle for 90
> minutes, which seems reasonable in a cloud environment. I understand if you
> object to this patch because of the hard coding. I suppose the only other
> option would be to modify the db schema and then update everything by hand?

Okay, I'm just going to add a custom template on second though.

Comment 23

[Morgan Phillips [:mrrrgn]]

Attachment: tac-template.png

I've added a new template that will turn on halt on reboot, and decrease the idle timeout from 7 hours down to 90 minutes. You can select it by hand for now to text, then we can apply it to all of the windows hosts whenever you're ready. The template is called: "halt-on-shutdown."

Please check in with me on irc before testing, since I still need to bump the buildslave version in puppet (waiting on review).

Comment 24

[Morgan Phillips [:mrrrgn]]

(In reply to Morgan Phillips [:mrrrgn] from comment #23)
> Created attachment 8682975 [details]
> tac-template.png
> 
> I've added a new template that will turn on halt on reboot, and decrease the
> idle timeout from 7 hours down to 90 minutes. You can select it by hand for
> now to text, then we can apply it to all of the windows hosts whenever
> you're ready. The template is called: "halt-on-shutdown."
> 
> Please check in with me on irc before testing, since I still need to bump
> the buildslave version in puppet (waiting on review).

s/to text//g <- every other post I write is a facepalm :)

Comment 25

[Rob Thijssen [:grenade (EET/UTC+0300)]]

I'd like to understand what I should look for on the Windows build slaves to validate that idleizer is installed/active.
- Today at 13:00 I disabled all y-2008-spot-* instances in slavealloc. There were 93 running instances of this type in EC2.
- At 18:00 I could see that these slaves were no longer associated with bb masters and none had running jobs. All were still up and running.

I don't know whether we haven't got idleizer installed correctly or what to look for to establish why the instances are still running. Is there a log file I can look at on the instances that might contain clues? There was nothing pertaining to idleizer in c:\opt\runner\runner.log but I don't know if that's the right place to look.

I'm going to go ahead and manually terminate all but one instance. I'll leave y-2008-spot-126 (10.132.64.101) up and running so that if somebody knows what to look for, we can have a look at that instances logs to see why it hasn't idleizer terminated.

Comment 26

[Rob Thijssen [:grenade (EET/UTC+0300)]]

Actually the last lines from runner.log might be relevant:

Error sending notice to nagios (ignored)
Traceback (most recent call last):
  File "C:\opt\runner\Scripts\runner-script.py", line 9, in <module>
    load_entry_point('runner==2.0', 'console_scripts', 'runner')()
  File "C:\opt\runner\lib\site-packages\runner\__init__.py", line 256, in main
    runner(config, args.taskdir, args.times)
  File "C:\opt\runner\lib\site-packages\runner\__init__.py", line 224, in runner
    if not process_taskdir(config, taskdir):
  File "C:\opt\runner\lib\site-packages\runner\__init__.py", line 138, in process_taskdir
    r = run_task(task_cmd, env, max_time=task_config['max_time'])
  File "C:\opt\runner\lib\site-packages\runner\__init__.py", line 23, in run_task
    proc = subprocess.Popen(t, stdin=open(os.devnull, 'r'), env=env)
  File "C:\mozilla-build\python27\Lib\subprocess.py", line 711, in __init__
    errread, errwrite)
  File "C:\mozilla-build\python27\Lib\subprocess.py", line 948, in _execute_child
    startupinfo)
WindowsError: [Error 193] %1 is not a valid Win32 application

Comment 27

[Chris AtLee [:catlee]]

there's a small typo in the code:
http://hg.mozilla.org/build/buildbot/file/5fa6d3543fc9/slave/buildslave/idleizer.py#l118

    def maybeReboot(self):
        if self._reboot_or_halt:
            if self._halt_on_shutdown:
                self.halt()
            elif self.reboot_or_halt:
                self.reboot()

That last 'elif self.reboot_or_halt' should be changed to just an 'else:' I think. 'self.reboot_or_halt' doesn't exist, and is causing bug 1222915

Comment 28

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

Agreed.  I think my review comment caused that particular error :(

Comment 29

[Morgan Phillips [:mrrrgn]]

Pushing a fix this. I'm not touching idleizer anymore.

Comment 30

[Morgan Phillips [:mrrrgn]]

Attachment: durr.diff

Nothing makes me want to jump off a cliff quite like the number of bugs I've created touching idleizer. 0/10, hope to not do again.

Comment 31

[Morgan Phillips [:mrrrgn]]

Reported working in production.