Bug #3420

Nagios "name" attribute not output correctly

Added by Robin Skoglund about 2 years ago. Updated 7 months ago.

Status:Closed Start date:03/25/2010
Priority:Normal Due date:
Assignee:Nigel Kersten % Done:

0%
Category:nagios
Target version:2.6.9
Affected Puppet version:2.6.6 Branch:https://github.com/jamtur01/puppet/tree/tickets/2.6.x/3420
Keywords:
Votes: 4

Description

I think I’ve stumbled upon a bug in the way Naginator outputs the Nagios “name” attribute.

I have the following nagios_service type defined in a manifest:

nagios_service { "jmx-${name}":
  use                 => "basic-service",
  service_description => "JMX $name",
  check_command       => "check_jmx_heap_via_ssh!$name",
  register            => "0",
  target              => "/usr/local/nagios/etc/generated_services/$name.cfg"
}

Expected output:

define service {
    check_command                  check_jmx_heap_via_ssh!kidkalkulator
    register                       0
    name                           jmx-kidkalkulator
    service_description            JMX kidkalkulator
    use                            basic-service
}

Actual output:

define service {
    check_command                  check_jmx_heap_via_ssh!kidkalkulator
    register                       0
    ## --PUPPET_NAME-- (called '_naginator_name' in the manifest)                jmx-kidkalkulator-test
    service_description            JMX kidkalkulator
    use                            basic-service
}

I attached a patch that changes puppet/provider/naginator.rb to output as expected, but I am not sure whether the patch will break other parts of Naginator/puppet.

naginator.rb.patch (1.2 kB) Robin Skoglund, 03/25/2010 09:41 am

naginator.rb.patch (1.2 kB) Bruce Richardson, 10/21/2010 06:37 pm

naginator.rb.patch (767 Bytes) Jim Pirzyk, 03/08/2011 07:20 pm

History

#1
Updated by James Turnbull about 2 years ago

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

Luke – Naginator bewilders me… :) Your thoughts on this?

#2
Updated by Luke Kanies about 2 years ago

There’s definitely a bug here, but I don’t quite know what the fix is. I think your fix will break other things – there are some nagios resource types that don’t support names, and that’s what this is supposed to be used for, not things like service that do support names.

#3
Updated by Robin Skoglund about 2 years ago

I think “name” is supported by all nagios object types, as it’s used for templating/inheritance. It would be nice to have the option to specify the name manually as a resouce parameter in puppet (as opposed to letting it be automatically generated from the namevar). That way you could omit the “name” attribute from being output at all.

#4
Updated by Luke Kanies about 2 years ago

  • Assignee changed from Luke Kanies to Anonymous

I haven’t looked significantly at Naginator in so long that I’m a bit lost here.

Assigning to Michael for further investigation.

#5
Updated by Bruce Richardson over 1 year ago

Hi. I’m attaching an alternative patch to fix this issue. I think it’s worth looking at a) because it also fixes a nasty lurking bug in the naginator provider, b) because Robin’s patch only works for resources which have naginator_name as their namevar, which many of them do not, and c) because nagios’s object templating feature is very important to anybody who wants Nagios to scale. The current naginator code doesn’t allow for nagios object templating at all – at least not using nagios* resource types, which you would want to do so that you could have proper dependencies.

On point a), if you look at the way the provider comments out the naginator_name parameter, you’ll see that it would also alter any record containing the string “naginator_name”. So if somebody gave a nagios_service a service_description containing “_naginator_name”, it would be replaced by the gsub.

My patch does have the effect that all nagios object always have a “name” property in their nagios config, but that has no harmful side effects beyond making the generated configuration files ever so slightly more verbose. It’s not the perfect solution but it makes nagios object templating properly available through Puppet resources and it fixes a lurking bug, so I say it’s a win.

The best fix would be to enhance nagios/base.rb so that it allows you to toggle whether or not the namevar is also written to the nagios config file as “name”. And/or to allow a specific “name” to be set. This would be a more elegant solution and would allow the nasty gsub hacks to come out of the naginator provider, making it simpler and cleaner.

I think the toggle would be good enough. But I’ve never dug into Puppet’s ruby internals before today (and I had to get this fixed for a work deadline), so I’ll leave that for another day.

By the way, I see it’s been six months with no update on this. Is naginator actively maintained at the moment? If it isn’t, I’d be interested in offering my services. No offence meant, if it is actively maintained.

#6
Updated by James Turnbull over 1 year ago

  • Assignee changed from Anonymous to James Turnbull

Bruce – it’s really not actively maintained currently! If you’d like to get involved we’d love to have your help. There are a couple of things worth knowing:

  1. Our development process is http://projects.puppetlabs.com/projects/puppet/wiki/Development_Lifecycle. Generally the best way to get feedback on your code is to send it via the rake patches task to the puppet-dev list.
  2. We need contributors to sign our CLA (you can see it in Redmine by clicking on the Contributor License link)

Happy to answer any other questions!

#7
Updated by James Turnbull over 1 year ago

  • Assignee changed from James Turnbull to Bruce Richardson

#8
Updated by Bruce Richardson over 1 year ago

  • Status changed from Investigating to Needs Decision
  • Target version set to 52

OK, then since I do have some questions and I’m not sure about what status decisions I can make, I’ll update this to “Needs design decision”.

The requested design decision is this: regardless of the quality of my proposed patch, is it acceptable to move from the current situation, where the “name” property of nagios objects cannot be generated, to a situation where it is always generated. It works for me, because I want the functionality blocked by the bug. There is, though, a chance of the new behaviour breaking existing Puppet+Nagios installations; since it is not currently possible to generate Nagios object templates from Puppet resources, people who want them must have created their templates via a different route. This means there’s a chance of creating an unexpected duplicate name – which would cause Nagios to halt.

To be honest, I think that risk is enough to merit a little more work on the patch, to make at least some minimal change to external/nagios/base.rb to add a toggle. But asking the question gives me the chance to ask one or two questions about the process:

  1. Who decides what the target branch will be and at what stage is that done?
  2. How is “Affected Version” updated? Is that via a keyword?

If you surprise me and say the functionality change caused by my current patch is acceptable, I’ll push it through the development workflow.

#9
Updated by Gabriel Filion over 1 year ago

in my own vision of it, the nagios_service stuff doesn’t work because of this bug so having ‘name’ always added seems like a strictly better thing than having the type just not work at all.

#10
Updated by Jim Pirzyk about 1 year ago

The previous patch did not work for host or contact templates (actually did not work for any other templates other than service* templates). I have attached a patch that will work for all templates (that I currently use host, contact, service, serviceescalation). It will also only convert the namevar back to the ‘name’ string if the register parameter is set to 0 (zero). If the register parameter is set to non-zero, it is like it is not set and is not a template.

#11
Updated by James Turnbull about 1 year ago

  • Status changed from Needs Decision to In Topic Branch Pending Review

Hi Jim – thanks for your patch!

Could I please get you to sign a CLA (see the Contributer License link on the top of the page) and have a quick look at our Development Lifecycle (http://projects.puppetlabs.com/projects/puppet/wiki/Development_Lifecycle) about how to send your patch out. I can also help with this!

#12
Updated by James Turnbull about 1 year ago

  • Assignee changed from Bruce Richardson to Nigel Kersten
  • Target version deleted (52)

#13
Updated by James Turnbull about 1 year ago

  • Project changed from Naginator to Puppet

#14
Updated by James Turnbull about 1 year ago

  • Category set to nagios
  • Target version set to 2.6.x
  • Affected Puppet version set to 2.6.6

#15
Updated by James Turnbull about 1 year ago

  • Branch set to https://github.com/jamtur01/puppet/tree/tickets/2.6.x/3420

#16
Updated by Josh Cooper about 1 year ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release

This was merged into 2.6.next as commit:361d6a3a6917fd9515a2180590fb671024132da7

#17
Updated by Chris Phillips 12 months ago

I don’t understand the logic of this patch. Whilst the name is only “useful” when the service is a template, why would this not want to be fixed for all services? The merged patch serves to explicitly preserve the bug in all normal services..?

def self.to_file(records) – header + records.collect { |record| record.to_s }.join(“\n”).gsub(“naginator_name”, NAME_STRING) + header + records.collect { |record| + record.to_s.sub(“naginator_name”, “name”).sub(record.type.to_s + “_name”, “name”) + }.join(“\n”) end

(not sure of the need for the code that’s left when the if block is removed, not great at ruby, but it works…)

Or am I missing something? nagios certainly has no problem with the file this way.

#18
Updated by Bruno Leon 11 months ago

Using Puppet 2.7.1 it does not seem to be merged. Is it supposed to be ?

#19
Updated by Bruno Leon 11 months ago

There is a regression in 2.7.1:

diff ./puppet-2.6.9/lib/puppet/provider/naginator.rb ./puppet-2.7.1/lib/puppet/provider/naginator.rb
33,41c33
<     header + records.collect { |record|
<         # Remap the TYPE_name or _naginator_name params to the
<         # name if the record is a template (register == 0)
<         if record.to_s =~ /register\s+0/
<             record.to_s.sub("_naginator_name", "name").sub(record.type.to_s + "_name", "name")
<         else
<             record.to_s.sub("_naginator_name", NAME_STRING)
<         end
<     }.join("\n")
---
>     header + records.collect { |record| record.to_s }.join("\n").gsub("_naginator_name", NAME_STRING)
zsh: exit 1     diff ./puppet-2.6.9/lib/puppet/provider/naginator.rb

Obviously the patch was not merged in 2.7.1

#20
Updated by James Turnbull 11 months ago

  • Status changed from Merged - Pending Release to Closed
  • Target version changed from 2.6.x to 2.6.9

#21
Updated by James Turnbull 7 months ago

2.6.x and 2.7.x diverged briefly because they overlapped in code freeze. So the bug was fixed in 2.6.9 but merged into 2.7.3.

Also available in: Atom PDF