The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Bug #4020

metaparameter "hostgroups" in nagios_host type should accept arrays

Added by Gabriel Filion almost 4 years ago. Updated 4 months ago.

Status:Code InsufficientStart date:06/17/2010
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:nagios
Target version:-
Affected Puppet version: Branch:
Keywords:communitypatch

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This ticket is now tracked at: https://tickets.puppetlabs.com/browse/PUP-1093


Description

When supplying an array of strings to the hostgroups value of nagios_host resources, the strings should be joined by commas.

currently, doing so:

nagios_host{ "foo.bar":
address => "foo.bar",
hostgroups => ["hello", "world"],

}

results in something like the following:

define host {
address      foo.bar
host_name    foo.bar
hostgroups   hello

}

It should instead read:

define host {
address      foo.bar
host_name    foo.bar
hostgroups   hello,world

}

nagios.diff Magnifier (555 Bytes) Brian Gallew, 07/15/2010 09:06 pm

nagios_maker.rb.diff Magnifier - Diff to make Naginator DTRT (2.11 KB) Brian Gallew, 08/26/2010 08:36 pm

nagiosmaker.diff Magnifier (1.06 KB) Brian Gallew, 10/05/2010 12:53 am

final.patch Magnifier - final patch with rspec updates (3.17 KB) Brian Gallew, 10/12/2011 08:19 am

puppet-4020.patch Magnifier (3.22 KB) Mark Frost, 01/12/2012 09:58 am

History

#1 Updated by James Turnbull almost 4 years ago

  • Status changed from Unreviewed to Accepted
  • Assignee set to Luke Kanies

#2 Updated by Brian Gallew almost 4 years ago

Naginator really needs to be able to accept lists for a variety of elements (nagios_contactgroup=>members, nagios_service=>command, etc).

#3 Updated by Brian Gallew almost 4 years ago

OK, the problem appears to be one little line of ruby.

diff -ur puppet-0.25.5-original/lib/puppet/util/nagios_maker.rb puppet-0.25.5/lib/puppet/util/nagios_maker.rb
--- puppet-0.25.5-original/lib/puppet/util/nagios_maker.rb  Wed Jul 14 09:54:31 2010
+++ puppet-0.25.5/lib/puppet/util/nagios_maker.rb   Thu Jul 15 14:00:02 2010
@@ -27,7 +27,7 @@
# supported.
next if param.to_s =~ /^[0-9]/
-            type.newproperty(param) do
+            type.newproperty(param, :array_matching => :all) do
desc "Nagios configuration file parameter."
end
end

#4 Updated by Brian Gallew almost 4 years ago

OK, let’s try that again…

I’ve attached the diff. Applying it should make The Right Thing™ happen, except for the case where you want multiple commands for a service check. That uses a different delimiter (;).

#5 Updated by Luke Kanies almost 4 years ago

  • Status changed from Accepted to Ready For Checkin
  • Assignee changed from Luke Kanies to Markus Roberts

Yes, that should do it.

#6 Updated by Brian Gallew over 3 years ago

Unfortunately, adding that line reveals a bug/limitation somewhere else: the nagios config files are re-written on every run. Apparently, having parameter lists causes the routines that generates the config file to always believe the files are modified. I’ve got lots and lots of log messages that look like this:

notice: //nagios::server/Nagios::Server::Generic_service[check_total_procs]/Nagios_service[check_total_procs]/use: use changed 'service-generic-template' to 'service-generic-template'
notice: //nagios::server/Nagios::Server::Generic_service[check_total_procs]/Nagios_service[check_total_procs]/hostgroup_name: hostgroup_name changed 'default_hostgroup' to 'default_hostgroup'
notice: //nagios::server/Nagios::Server::Generic_service[check_total_procs]/Nagios_service[check_total_procs]/service_description: service_description changed 'Total Processes' to 'Total Processes'
notice: //nagios::server/Nagios::Server::Generic_service[check_total_procs]/Nagios_service[check_total_procs]/check_command: check_command changed 'check_nrpe!check_total_procs ' to 'check_nrpe!check_total_procs '
notice: //nagios::server/Nagios::Server::Generic_service[check_total_procs]/Nagios_service[check_total_procs]/check_period: check_period changed '24x7' to '24x7'

It’s not a show-stopper in that the config files are correct, but it does mean that if you have a service notification based on changes here, that Nagios gets restarted pretty much every time Puppet runs.

#7 Updated by James Turnbull over 3 years ago

Is this really ready for checkin?

#8 Updated by Brian Gallew over 3 years ago

I have attached a new diff for Naginator. This will make Naginator do the right thing when it comes to properties that can be arrays (e.g. use, members), and fixes the problem with the previous patch whereby the config files were pretty much always re-written.

This is not a perfect fix in that it does not have any logic to distinguish between properties that should be arrays, and properties that should not. Doing that would require a larger change where the parameters list would have to be built from two different lists, one for array-legal properties, and one for normal properties. Personally, I don’t think this is a huge issue, though if that’s what would be required to get approved, I’ll do it.

#9 Updated by James Turnbull over 3 years ago

  • Target version set to 52

#10 Updated by Matt Robinson over 3 years ago

Looks like this patch has been forgotten about to some degree. I didn’t realize we had a naginator project, but I can’t update ticket statuses. It looks to me like the last patch that Brian attached to this ticket was submitted to the dev list and Luke had some comments that I don’t see reflected in the patch.

Brian, did you get a chance to incorporate that feedback, or did you decide it wasn’t necessary to make the changes suggested?

Is this something that should also apply to 2.6.x or 2.7 at some point?

#11 Updated by Gabriel Filion over 3 years ago

Matt Robinson wrote:

Gabriel, did you get a chance to incorporate that feedback, or did you decide it wasn’t necessary to make the changes suggested?

Actually, I can’t really help a lot with writing and testing code since I would have to learn ruby first :P however:

Looks like this patch has been forgotten about to some degree. I didn’t realize we had a naginator project, but I can’t update ticket statuses. It looks to me like the last patch that Brian attached to this ticket was submitted to the dev list and Luke had some comments that I don’t see reflected in the patch.

right. I saw you e-mail on the puppet-dev list and here are my thoughts about James' comments:

  • yes it is true, order of the group names should not make (a,b) and (b,a) differ
  • nagios group names are always strings

so James' modifications would seem ok and simple to integrate into the patch.

#12 Updated by Brian Gallew over 3 years ago

This is ready for check-in, though it needs to be refreshed to be against 2.6.1. I’m attaching a newer version against 2.6.1 with James' suggestion integrated.

#13 Updated by Gabriel Filion over 3 years ago

thanks a lot, Brian, for your work on this issue!

#14 Updated by Matt Robinson over 3 years ago

  • Keywords set to communitypatch

#15 Updated by James Turnbull over 3 years ago

  • Tracker changed from Feature to Bug
  • Project changed from Naginator to Puppet

#16 Updated by James Turnbull over 3 years ago

  • Category set to nagios
  • Target version changed from 52 to 2.7.x

#17 Updated by James Turnbull almost 3 years ago

  • Status changed from Ready For Checkin to In Topic Branch Pending Review

#18 Updated by James Turnbull over 2 years ago

  • Status changed from In Topic Branch Pending Review to Requires CLA to be signed
  • Assignee changed from Markus Roberts to Brian Gallew

Hi Brian – before we can get this merged and updated can you please sign a Contributor License Agreement (see link in top right menu)! Many thanks!

#19 Updated by Brian Gallew over 2 years ago

  • Assignee changed from Brian Gallew to James Turnbull

I have signed the CLA electronically.

#20 Updated by James Turnbull over 2 years ago

  • Status changed from Requires CLA to be signed to In Topic Branch Pending Review

Okay I’ll get the patch processed and sent up for review and merge. Thanks for your help!

#21 Updated by James Turnbull over 2 years ago

  • Status changed from In Topic Branch Pending Review to Tests Insufficient

Brian – the tests need some work after reviewing the patch. Currently the nagios_maker_spec.rb tests for this patch:

.....FFF.....

Failures:

  1) Puppet::Util::NagiosMaker should create a property for all non-namevar parameters
     Failure/Error: @type.expects(:newproperty).with(:two)
     Mocha::ExpectationError:
       not all expectations were satisfied
       unsatisfied expectations:
       - expected exactly once, not yet invoked: #.newproperty(:two)
       - expected exactly once, not yet invoked: #.newproperty(:one)
       satisfied expectations:
       - allowed any number of times, not yet invoked: Signal.trap(any_parameters)
       - allowed any number of times, invoked once: #.parameters(any_parameters)
       - allowed any number of times, invoked 3 times: #.namevar(any_parameters)
       - allowed any number of times, not yet invoked: #.parameters(any_parameters)
       - allowed any number of times, invoked once: .type(:test)
       - allowed any number of times, invoked once: #.nagios_type(any_parameters)
       - expected exactly once, invoked once: #.newproperty(:target)
       - allowed any number of times, invoked once: #.ensurable(any_parameters)
       - allowed any number of times, invoked once: #.provide(any_parameters)
       - allowed any number of times, invoked twice: #.newproperty(any_parameters)
       - allowed any number of times, invoked once: #.desc(any_parameters)
       - allowed any number of times, invoked once: #.newparam(any_parameters)
       - expected exactly once, invoked once: Puppet::Type.newtype(:nagios_test)
     # ./nagios_maker_spec.rb:50

  2) Puppet::Util::NagiosMaker should skip parameters that start with integers
     Failure/Error: @type.expects(:newproperty).with(:other)
     Mocha::ExpectationError:
       not all expectations were satisfied
       unsatisfied expectations:
       - expected exactly once, not yet invoked: #.newproperty(:other)
       satisfied expectations:
       - allowed any number of times, not yet invoked: Signal.trap(any_parameters)
       - allowed any number of times, invoked once: #.parameters(any_parameters)
       - allowed any number of times, invoked 3 times: #.namevar(any_parameters)
       - allowed any number of times, not yet invoked: #.parameters(any_parameters)
       - allowed any number of times, invoked once: .type(:test)
       - allowed any number of times, invoked once: #.nagios_type(any_parameters)
       - expected exactly once, invoked once: #.newproperty(:target)
       - allowed any number of times, invoked once: #.ensurable(any_parameters)
       - allowed any number of times, invoked once: #.provide(any_parameters)
       - allowed any number of times, invoked once: #.newproperty(any_parameters)
       - allowed any number of times, invoked once: #.desc(any_parameters)
       - allowed any number of times, invoked once: #.newparam(any_parameters)
       - expected exactly once, invoked once: Puppet::Type.newtype(:nagios_test)
     # ./nagios_maker_spec.rb:60

  3) Puppet::Util::NagiosMaker should deduplicate the parameter list
     Failure/Error: @type.expects(:newproperty).with(:one)
     Mocha::ExpectationError:
       not all expectations were satisfied
       unsatisfied expectations:
       - expected exactly once, not yet invoked: #.newproperty(:one)
       satisfied expectations:
       - allowed any number of times, not yet invoked: Signal.trap(any_parameters)
       - allowed any number of times, invoked once: #.parameters(any_parameters)
       - allowed any number of times, invoked twice: #.namevar(any_parameters)
       - allowed any number of times, not yet invoked: #.parameters(any_parameters)
       - allowed any number of times, invoked once: .type(:test)
       - allowed any number of times, invoked once: #.nagios_type(any_parameters)
       - expected exactly once, invoked once: #.newproperty(:target)
       - allowed any number of times, invoked once: #.ensurable(any_parameters)
       - allowed any number of times, invoked once: #.provide(any_parameters)
       - allowed any number of times, invoked once: #.newproperty(any_parameters)
       - allowed any number of times, invoked once: #.desc(any_parameters)
       - allowed any number of times, invoked once: #.newparam(any_parameters)
       - expected exactly once, invoked once: Puppet::Type.newtype(:nagios_test)
     # ./nagios_maker_spec.rb:70

Finished in 0.03071 seconds
13 examples, 3 failures

#22 Updated by Brian Gallew over 2 years ago

So, I checked out the master tree and I’m not seeing the issue. Do I have to apply the patch to my copy of the master first?

#23 Updated by Brian Gallew over 2 years ago

Never mind, I just went ahead and applied the patch to my copy and now I get the same result you do. I’ll see about writing some tests here. I promise to take fewer than 11 months to get them done.

#24 Updated by Brian Gallew over 2 years ago

Well, maybe I will take a lot longer to get this done. Clearly, I fail to understand how my changes are affecting the tests. For instance, there is code (which comes before all my changes) which explicitly skips any names starting with a digit. Yet somehow, my code makes that test fail. Having never done ®spec before, this promises to be an interesting learning experience.

#25 Updated by Brian Gallew over 2 years ago

This patch supersedes all previous patches that I have submitted. It includes the requisite updates to the spec code such that it runs cleanly once more.

#26 Updated by James Turnbull over 2 years ago

  • Status changed from Tests Insufficient to In Topic Branch Pending Review
  • Assignee changed from James Turnbull to Daniel Pittman

#27 Updated by Mark Frost over 2 years ago

A minor change to Brian’s patch. This subroutine:

   def insync(is)
       @should.sort == is.sort
   end

Causes issues with empty string parameters. I emailed and talked with Brian about this, and he changed it to:

   def insync(is)
     if is
       @should.sort == is.sort
     else
      @should == is
     end
   end

This seems to fix the issue.

Ive attached the changed patch he gave me.

#28 Updated by Carl Caum almost 2 years ago

This patch is broken. The insync method should be

def insync?(is)
  [*@should].sort == [*is].sort
end

#29 Updated by Mark Frost almost 2 years ago

Actually, I found some issues where that didn’t work, either. The one that worked best for me, from my testing, seems to be this:

    def insync?(is)
      should_to_s(should) == is_to_s(is)
    end

Which makes sense, I think, given the rest of the patch.

#30 Updated by Andrew Parker over 1 year ago

  • Status changed from In Topic Branch Pending Review to Code Insufficient
  • Assignee deleted (Daniel Pittman)
  • Target version deleted (2.7.x)

Updating this one based on what its current state seems to be. I don’t think this will be targeted at 2.7, but it is still an open question what we are going to do.

There seem to be several bugs that are still open and related to nagios that we need to make a decision about.

#31 Updated by Jason Antman 4 months ago

Redmine Issue #4020 has been migrated to JIRA:

https://tickets.puppetlabs.com/browse/PUP-1093

Also available in: Atom PDF