Referencing hash values within module

[Mike Reed]

Hello all,

I've got a module that I've been working on in an attempt to make things a little smarter and I figured using a hash would be an elegant solution to my problem.  The short story is that I've got various video cards in our production environment and I'd like to have a module that determines what driver will be installed, depending on the make of the card. 

I've written some custom facts to help determine the class of the machine, what the video card id is and additionally, if the nvidia driver is currently installed.  These facts have been working fine for quite some time and I can check the values by running a facter -p, which results in something like:

nvidia_installed => false

video_card_id => 17c2

class => workstation

With that said, here's the module that I've written:

 class nvidia::driver_install (

  

  $model_hash = {

      009d => "$::nvidia::params::quadro_fx_4500_driver",

      06dc => "$::nvidia::params::quadro_6000_driver",

      06d9 => "$::nvidia::params::quadro_5000_driver",

      11ba => "$::nvidia::params::quadro_K5000_driver",

      17c2 => "$::nvidia::params::gtx_titanx_driver",

      17f0 =>  "$::nvidia::params::quadro_M6000_driver",

  }

  

) inherits nvidia::params {

  if ($::is_virtual == true) and ($::class == 'server') {

    notify { 'This is a virtual machine and the nvidia driver doesn\'t get intalled' : 

  }

    # only run nvidia installer if the machine is a workstation and the driver is not already installed

  } elsif ($::class == 'workstation') and ($::nvidia_installed == 'false') {

      if ($::video_card_id == $model_hash[key]) {

        notify { "Installing driver for ($model_hash[key])" : }

        exec { 'kdm-stop' :

          command => '/usr/bin/service kdm stop' ,

          unless  => '/usr/bin/service kdm status | grep -i "stop" ' ,

          before  => Exec['nvidia-driver-install'] ,

        }

        # install nvidia driver

        exec { 'nvidia-driver-install' :

          command => "/usr/src/($model_hash[value]).run -s -X -N --force-tls=new --no-x-check --no-cc-version-check" ,

          require => File["/usr/src/($model_hash[value]).run"] ,

          notify  => Exec['reboot_after_nvidia'] ,

        }

        # reboot after nvidia install

        exec { 'reboot_after_nvidia' :

          command     => "/sbin/reboot" ,

          refreshonly => true ,

        }

      }

  }

}

In addition, I've got most of my parameters saved into the ::nvidia::params.pp file of the module, which then points to hiera for its values:

$quadro_fx_4500_driver   = hiera('nvidia::quadro_fx_4500_driver')

$quadro_6000_driver        = hiera('nvidia::quadro_6000_driver')

$quadro_5000_driver        = hiera('nvidia::quadro_5000_driver')

$quadro_K5000_driver     = hiera('nvidia::quadro_K5000_driver')

$quadro_M6000_driver     = hiera('nvidia::quadro_M6000_driver')

$gtx_titanx_driver            = hiera('nvidia::gtx_titanx_driver')

These values all resolve and if I use them by name (without the hash), things seem to work.

My first question is, does it appear that my logic is correct?  After reading the puppet docs about data types and hashes in particular, it appears to me that I've got the correct syntax.  With the above module in place, I can run a puppet job; but nothing ever gets applied to the host (no errors are ever reported and the jobs completes successfully).  This leads me to believe that I'm using the hash incorrectly and because of that, values aren't being determined and therefore this particular piece of the module doesn't get applied.  Might you have any thoughts on why this doesn't appear to work?

Also, as a bonus question, if I wanted to use this code in another module, is it as simple as "include ::nvidia::driver_install"?

Thank you in advance to everybody for your help and your assistance.  Your help is very much appreciated.

Cheers,

Mike

[Thomas Müller]

Hi Mike

Am Donnerstag, 21. Januar 2016 23:20:38 UTC+1 schrieb Mike Reed:

... check the values by running a facter -p, which results in something like:

nvidia_installed => false

video_card_id => 17c2

class => workstation

 

...

 

  if ($::is_virtual == true) and ($::class == 'server') {

    notify { 'This is a virtual machine and the nvidia driver doesn\'t get intalled' : 

  }

    # only run nvidia installer if the machine is a workstation and the driver is not already installed

  } elsif ($::class == 'workstation') and ($::nvidia_installed == 'false') {

 

      if ($::video_card_id == $model_hash[key]) {

What is key? key is a bare word not a variable. Maybe you meant "if (!empty($model_hash[$::video_card_id])" ?

https://gist.github.com/vinzent/5e5f187a8d1dcb000f8a/revisions

 

        notify { "Installing driver for ($model_hash[key])" : }

        exec { 'kdm-stop' :

.... 

Also, as a bonus question, if I wanted to use this code in another module, is it as simple as "include ::nvidia::driver_install"?

it should be as easy as that.

I do see some problems with the exec reboot. It will initiate the reboot while puppet is applying changes. this might produce unexpected results.

- Thomas

[jcbollinger]

I guess you're asking about the structure of your class and your approach to the problem.  The word "logic" is not usually used in that sense for Puppet DSL code.  Anyway, it looks like the general form and approach is probably feasible.

 After reading the puppet docs about data types and hashes in particular, it appears to me that I've got the correct syntax.

Yes and no.  Your syntax appears valid, but in the immortal words of Inego Montoya, "I don't think it means what you think it means".

As Thomas Müller also observed, you are using a bare word "key" to attempt to extract a value from your hash.  At least if the hash takes its default value, there will be no entry with that key.  As far as I can tell, there is not even a variable $key that you might have meant instead.  Later, you use the bare word "value" in similar fashion.

The easiest way to determine whether a hash contains an entry having a given key is via Puppet's 'in' operator, something like this:

      if ($::video_card_id in $model_hash) { # ...

Where you want to extract the value from the hash entry having a given key, then that takes the form of an (associative) array access, just like in Ruby (or Python or Perl):

         $model_hash[$::video_card_id]

or a somewhat more bash-like form with curly braces (analogous to the curly-brace form usable for Puppet variables whose values have other types):

         ${model_hash[$::video_card_id]}

.  Where you want to interpolate a variable's value into a double-quoted string, you should use the latter form.  If you want to interpolate a value drawn from an array element or hash entry, then you must use the curly-brace form:
 

        notify { "Installing driver for (${model_hash[$::video_card_id]})" : }

 

 With the above module in place, I can run a puppet job; but nothing ever gets applied to the host (no errors are ever reported and the jobs completes successfully).

That confirms that your syntax is correct.

 

 This leads me to believe that I'm using the hash incorrectly and because of that, values aren't being determined and therefore this particular piece of the module doesn't get applied.  Might you have any thoughts on why this doesn't appear to work?

Yes. Though your hash usage complies with Puppet syntax, its semantics appear to be completely wrong, as described above.

 

Also, as a bonus question, if I wanted to use this code in another module, is it as simple as "include ::nvidia::driver_install"?

It depends on what you mean by "use this code".  Almost anywhere in any of your manifests you can use an 'include' statement such as you present to instruct Puppet's catalog builder to evaluate class ::nvidia::driver_install if it has not been evaluated already, and to ensure that it is included in the target node's catalog.  There are other things you might mean, however, that Puppet does not provide.  In particular, Puppet has no mechanism for C-like code interpolation.  The 'include' statement does not do that.

As bonus answers, I suggest some changes to your code:

• Puppet classes represent state, not actions.  Accordingly, their names should be nouns or perhaps adjectives, not verbs.  This is non-trivial, as choosing well-suited names helps with thinking appropriately about your code as you write it, and also as you use it.
• It is unclear to me why you make $model_hash a class parameter, but also provide a default value such as the one you do.  If you intend the value to be set via automated data binding then the default value provided is way overkill.  If you intend the default value to be determined entirely by class nvidia::params then it would be better to create the hash in a class variable of that params class, from which the values are being drawn anyway, and to reference it by name. (You do know that you can have class variables that are not class parameters, right?)  Or if you intend the parameter to *always* take the value specified then it should be an ordinary class variable, not a class parameter at all.
• Although Puppet permits them, in most contexts it is poor style to use bare strings.  Quote your strings -- with single quotes (') unless you need to interpolate a variable value.  Example: '009d', not 009d.
• It is a bad idea to reboot in the middle of a Puppet run.  Instead, schedule a reboot for after the run finishes.  For example, use the command '/sbin/shutdown -r +5', and force the Exec by which it is applied to run last (maybe by putting it in a run stage that is sequenced after stage "main").
• If you plan to apply this class any time after initial provisioning then you should consider whether you need to make more accommodation for any users that happen to be logged in at the time, and for any jobs that may be running.

John

[Mike Reed]

Hello Thomas,

Thank you for your comments and thoughts.  I'm still trying to understand basic programming and I see now that I'm mixing things up.  

I also wanted to thank you for taking the time to put the code up on gist, in order to give me a clear example of what you are talking about.  I very much appreciate it.

Cheers,

Mike

[Mike Reed]

Hey John,

Your comments make absolute sense and after making some changes, things are working as expected.  I've corrected my if statement to include the 'in' operator along with changing the syntax for how I'm retrieving the key/value pairs - ${model_hash[$::video_card_id]} - and that fixed things right up.

Your bonus comments are very helpful and I'll be making some changes based on your recommendations.  

Also, I wanted to thank you again - as always - for taking the time to not only explain the issue, but also for enlightening me on how I can make fundamental changes that will make my code better.  

Your help is very much appreciated.  

Cheers,

Mike