764832 - power management module is required for PuppetAgain

Status: RESOLVED FIXED

(Infrastructure & Operations Graveyard :: CIDuty, task)

Description

[Kim Moir [:kmoir] ET]

This is required for the support for Mac 10.8 on PuppetAgain

Comment 1

[Kim Moir [:kmoir] ET]

Attachment: patch

This patch configures power management on the Mac slaves.  However, since there are places in the modules that specify configuration for non-Centos systems, and fail if not  

 default: {
            fail("cannot install on $operatingsystem")
        }

there are errors when you try to puppetize as toplevel::slave::build, but not with the powermanagement module alone.

This issue is being addressed in bug 764948

Comment 2

[Kim Moir [:kmoir] ET]

Talked to Callek, he suggested moving the disabling the screensaver to the user module and adding power management for CentOS as well.

Comment 3

[Justin Wood (:Callek)]

Comment on attachment 633670 [details] [diff] [review]
patch

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

Will need a mac person to check this module as well though, I know nothing about the screensaver/powermanagement code. And I don't trust myself to be able to properly figure this out in a timely manner for review and our needs.

f+ for the puppet layout/design except for my few comments/Q's. If there is a new patch I'll still want a feedback run through it, but our primary need for a review on this bug at this point is for mac-sanity in what this module hopes to accomplish.

::: modules/powermanagement/manifests/setup.pp
@@ +1,5 @@
> +class powermanagement::setup {
> +
> +   case $operatingsystem {
> +
> +       Darwin: {

Can we have a Linux: case, even if we don't have a powermanegement solution for it yet, and do a default: case here like everywhere else where we platform split?

@@ +3,5 @@
> +   case $operatingsystem {
> +
> +       Darwin: {
> +
> +           file { ['/usr','/usr/local','/usr/local/bin']:

setting rules for these dir's in puppet feels dirty/bad. Is this really necessary?

@@ +17,5 @@
> +               mode => 755,
> +               source => "puppet:///modules/powermanagement/disable-screensaver.sh";
> +           }
> +
> +           exec {

Do all these need running every time puppet is run, is there any magic we can/should do to not run these all the time that is not slower than simply running these commands?

Comment 4

[Kim Moir [:kmoir] ET]

Attachment: updated patch

I removed the screensaver bit and added a blank stanza for CentOS.  There isn't a current module for power management on Linux in the puppet-manifests repo but I can look into adding if if required.  The Mac scripts were just copied from the Lion ones in the puppet-manifests repo so not sure if they need to be reviewed by a Mac person, since they are already in use :-)

Comment 5

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

Comment on attachment 636204 [details] [diff] [review]
updated patch

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

I'm not sure why you took the screensaver part out, other than the fact that the script looked like quite a hack!  It seems like we'll need that functionality.

From https://jamfnation.jamfsoftware.com/discussion.html?id=2219,
  defaults -currentHost read com.apple.screensaver idleTime
  defaults -currentHost write com.apple.screensaver idleTime 0
work on my Snow Leopard laptop and my Lion desktop.  Note that they need to be run by the logged-in user (as your first patch did).  The -currentHost option seems to take care of the UUID/ByHost madness.

There are a few things that we'll probably do a lot of on a Mac, and they involve running lots of commands that we'll only want to run once.  Same thing applies for the ntpd settings in bug 764948  What do you think about adding an 'osxutils' module, that can run both 'defaults' and 'systemsetup' properly, ensuring that the command is only run when necessary?  In fact, such a module may exist already on the internets, which would be great :)

At least for defaults, it will probably make the most sense to use 'defaults read' on every puppet run, rather than caching the last value we set in a file somewhere as I suggested in bug 764948 (and is done in the ccache module) -- it's possible that the OS, or a user, or even Firefox tests, will overwrite the defaults, and we'd miss this if we were querying a file on disk.  The defaults command seems pretty quick anyway.

::: modules/powermanagement/manifests/setup.pp
@@ +13,5 @@
> +                    command => "/usr/sbin/systemsetup -setrestartfreeze on";
> +                "turn-on-wol":
> +                    command => "/usr/sbin/systemsetup -setwakeonnetworkaccess on";
> +                "disallow-sleep-button":
> +                    command => "/usr/sbin/systemsetup -setallowpowerbuttontosleepcomputer off";

These could be much more readable as

    osxutils::systemsetup {
        sleep:
            setting => "Never";
        restartpowerfailure:
            setting => "on";
        ...

and similarly for the NTP case (so, it's taking the title and using -get$title, comparing to $setting, and if necessary calling $settitle.

::: modules/toplevel/manifests/slave/test.pp
@@ +1,3 @@
>  class toplevel::slave::test inherits toplevel::slave {
>      include ntp::atboot
> +    include powermanagement 

AFAICT, "include powermanagement" means "please don't sleep, slow down, shutdown and not reboot, or run your screensaver", which seems like something we want on *every* host, even if it's not particularly relevant on e.g., Linux servers.  Given that, shouldn't this be included in the 'toplevel' class?

Comment 6

[Kim Moir [:kmoir] ET]

Attachment: patch for power management

Updated patch with Dustin's comments incorporated.  Notes:   The systemsetup -get option doesn't return values in a consistent format thus the awk and grep.  It's not pretty but it works for the values entered. Not sure how to do it in a better way.  The osxutils/defaults module is a modified form of this project https://github.com/wfarr/puppet-osx_defaults

Comment 7

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

Comment on attachment 637998 [details] [diff] [review]
patch for power management

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

If this works, it's a thing of beauty!  The defaults write seems to lack the -ByHost magic that was in there before, though -- is that important?

If that's required, you might as well also remove the ensure => option from osxutils::defaults, since it's a no-op.

Otherwise, this looks good, so r+ assuming that you've tested the screensaver defaults write to ensure it works.

Comment 8

[Kim Moir [:kmoir] ET]

Attachment: patch

Patch with the ensure removed.  Yes, I tested it and the correct value is set when defaults is run with write using puppet.

Comment 9

[Kim Moir [:kmoir] ET]

Closing.