Collectd network plugin requires two puppet runs

[johnf]

https://github.com/pdxcat/puppet-module-collectd/blob/master/templates/plugin/network.conf.erb#L3

This depends on collectd_version. The fact is being evaluated at catalog compile time which is before collectd is installed. So it probably ends up as nil, so the SecurityLevel isn't set.

Then on a second run when it detects collectd 5 is installed then it adds the SecurityLevel

Not sure what a good solution to this problem is though. Maybe the fact could work out what version is going to be installed? Something like

apt-cache madison collectd | grep Packages | head -1 | awk -F\| '{print $2}' | sed -e 's/-.*//'

[txaj]

I agree with you, but your suggestion depends too much on the provider used by the package type.

I can't think of a better way to do either

[blkperl]

Yeah I haven’t thought of a good solution yet for this problem. It affects all the plugins that check for collectd version.

[txaj]

When I look on how we use this fact, it appear we are discriminating against a rather big version number. Couldn't we take the approach of discriminating against ::lsbdistrelease ?

[jpoittevin]

I'm not sure it will be enough. If one uses a special repository, he could have a different version of collectd than the one expected with a given dist.

[blkperl]

We could add $::lsbdistcodename version defaults and allow it to be overridden by the user but then we have to maintain a list for the life time of the module which is not very appealing.

[vladciobancai]

Any work around about this issue ?

--later :
I have found a small work around that works for me. I have created a facter script to overwrite the collectd_version in order that the plugins to be placed at the first run of puppet.

Facter.add("collectd_version") do
setcode do
"5.4.1-1.7.amzn1"
end
end

[bdellegrazie]

Here is another suggestion:
instead of using the fact directly in the ERB templates, why not use a class variable (e.g. $_collectd_version) which is set based on one of the following sources:

• $version (if it represents an explicit version - major.minor.revision parts only)
• $collectd_version fact, if not undef / nil
• default version supplied from hiera or minimum version supported by this module

This permits the system administrator to use an explicit version for the package and get a single puppet run and/or to supply a default minimum version if these are not set.
e.g. assuming class with defaults as follows (untested)

class collectd(
  ...
  $version  = installed,
  $collectd_default_version = '5.0.0',
) inherits collectd::params {
  ...
   $_collectd_version = pick( regsubst($version, '^\d+\.\d+\.\d+', '\0'), $collectd_version, $collectd_default_version)
  ...
}

Then use $_collectd_version in the ERB templates.

What do you think? If you think its worthwhile I can attempt a pull request.

[bdellegrazie]

@johnf, @txaj, @jpoittevin, @ngagecj:
Any comment on my potential solution above? I'm happy to create a PR if its considered reasonable.
Thanks

[txaj]

This is a great idea!

In your PR you may want 1/ not to break compatibility by setting the $collectd_default_version to undef, so it'll still need two runs if not explicitly configured 2/ document this feature both in the "usage" section and down the README with a format like "Puppet needs two runs to correctly write my conf, why ?" and refer to this issue

[bdellegrazie]

@txaj: I've raised the PR as requested.
I've tested this but it relies upon the user loading the collectd class before instantiating any plugin classes. I'm not sure there's a way around this.
Tests for the plugins have been updated but there isn't an easy way of testing the version selector as its only exposed by an internal variable.
Thoughts?

[bdellegrazie]

Rebased to current master

[igalic]

with the closing of #305 can we close this issue, too?