From 43399b9ea0975e30bc791d047d94a92213838e24 Mon Sep 17 00:00:00 2001 From: Brett Delle Grazie Date: Wed, 5 Aug 2015 00:24:03 +0100 Subject: [PATCH] Improve version handling to avoid multiple puppet runs for some situations When an administrator knows the version of collectd that will be used or at least the minimum version available the need for two puppet runs before convergence can be avoided or at least minimised. Instead of using the fact in the templates they now use a class variable set to one of (in priority order): * collectd_real_version (i.e. the fact) * version (the semver matched part of it only) * minimum_version (undef by default) Existing behaviour is preserved except for a corner case where version is set to something specific and collectd is not yet installed. In this case puppet will only take one run and assume the version specified when creating the templates references puppet-community/puppet-collectd#162 --- README.md | 18 ++++-- ...td_version.rb => collectd_real_version.rb} | 4 +- manifests/init.pp | 10 ++++ spec/classes/test_collectd_version_spec.rb | 59 +++++++++++++++++++ .../test/manifests/collectd_version.pp | 19 ++++++ .../test/templates/collectd_version.tmp.erb | 1 + ..._spec.rb => collectd_real_version_spec.rb} | 6 +- 7 files changed, 108 insertions(+), 9 deletions(-) rename lib/facter/{collectd_version.rb => collectd_real_version.rb} (83%) create mode 100644 spec/classes/test_collectd_version_spec.rb create mode 100644 spec/fixtures/modules/test/manifests/collectd_version.pp create mode 100644 spec/fixtures/modules/test/templates/collectd_version.tmp.erb rename spec/unit/{collectd_version_spec.rb => collectd_real_version_spec.rb} (78%) diff --git a/README.md b/README.md index a1937c05c..0e1ba94e2 100644 --- a/README.md +++ b/README.md @@ -27,9 +27,10 @@ declaration: ```puppet class { '::collectd': - purge => true, - recurse => true, - purge_config => true, + purge => true, + recurse => true, + purge_config => true, + minimum_version => '5.4', } ``` @@ -38,6 +39,10 @@ the default configurations shipped in collectd.conf and use custom configurations stored in conf.d. From here you can set up additional plugins as shown below. +Specifying the version or minimum_version of collectd as shown above reduces the need for +two puppet runs to coverge. See [Puppet needs two runs to correctly write my conf, why?](#puppet-needs-two-runs-to-correctly-write-my-conf,-why?) below. + + Simple Plugins -------------- @@ -1168,7 +1173,12 @@ See metadata.json for supported platforms ##Known issues -Some plugins will need two runs of Puppet to fully generate the configuration for collectd. See [this issue](https://github.com/puppet-community/puppet-collectd/issues/162). +###Puppet needs two runs to correctly write my conf, why? + +Some plugins will need two runs of Puppet to fully generate the configuration for collectd. See [this issue](https://github.com/pdxcat/puppet-module-collectd/issues/162). +This can be avoided by specifying an explicit version (`$version`) or a minimum version (`$minimum_version`) for the collectd class. e.g. Setting either of these to 1.2.3 will +make this module assume on the first run (when the fact responsible to provide the collectd version is not yet available) that your systems are running collectd 1.2.3 +and generate the configuration accordingly. ##Development diff --git a/lib/facter/collectd_version.rb b/lib/facter/collectd_real_version.rb similarity index 83% rename from lib/facter/collectd_version.rb rename to lib/facter/collectd_real_version.rb index ca341c6fd..0ff148c4a 100644 --- a/lib/facter/collectd_version.rb +++ b/lib/facter/collectd_real_version.rb @@ -1,4 +1,4 @@ -# Fact: collectd_version +# Fact: collectd_real_version # # Purpose: Retrieve collectd version if installed # @@ -6,7 +6,7 @@ # # Caveats: not well tested # -Facter.add(:collectd_version) do +Facter.add(:collectd_real_version) do setcode do if Facter::Util::Resolution.which('collectd') collectd_help = Facter::Util::Resolution.exec('collectd -h') and collectd_help =~ /^collectd ([\w.]+), http:\/\/collectd.org\// diff --git a/manifests/init.pp b/manifests/init.pp index 947646abe..b94c6f7b8 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -15,12 +15,22 @@ $write_queue_limit_low = undef, $package_name = $collectd::params::package, $version = installed, + $minimum_version = undef, ) inherits collectd::params { $plugin_conf_dir = $collectd::params::plugin_conf_dir validate_bool($purge_config, $fqdnlookup) validate_array($include, $typesdb) + # Version for templates + $collectd_version = pick( + $::collectd_real_version, # Fact takes precedence + regsubst( + regsubst($version,'^(absent|held|installed|latest|present|purged)$', undef), # standard package resource ensure value? - strip and return undef + '^\d+(?:\.\d+){1.2}', '\0'), # specific package version? return only semantic version parts + $minimum_version, + '1.0') + package { $package_name: ensure => $version, name => $package_name, diff --git a/spec/classes/test_collectd_version_spec.rb b/spec/classes/test_collectd_version_spec.rb new file mode 100644 index 000000000..8b8cf81c6 --- /dev/null +++ b/spec/classes/test_collectd_version_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +describe 'test::collectd_version' do + + let :facts do + {:osfamily => 'RedHat'} + end + + it { should compile } + + context 'when no explicit value is specified' do + it { should contain_file('collectd_version.tmp').with_content(/^1\.0$/) } + end + + context 'when minimum_version is specified' do + let :params do + { + :version => 'installed', + :minimum_version => '5.4', + } + end + it { should contain_file('collectd_version.tmp').with_content(/^5\.4$/) } + end + + context 'when version is explicit and greater than minimum_version' do + let :params do + { + :version => '5.6.3', + :minimum_version => '5.4', + } + end + it { should contain_file('collectd_version.tmp').with_content(/^5\.6\.3$/) } + end + + context 'when version is explicit and less than minimum_version' do + let :params do + { + :version => '5.3', + :minimum_version => '5.4', + } + end + it { should contain_file('collectd_version.tmp').with_content(/^5\.3$/) } + end + + context 'when collectd_real_version is available' do + let :facts do + { + :osfamily => 'Redhat', + :collectd_real_version => '5.6', + } + end + let :params do + { + :minimum_version => '5.4' + } + end + it { should contain_file('collectd_version.tmp').with_content(/^5\.6$/) } + end +end diff --git a/spec/fixtures/modules/test/manifests/collectd_version.pp b/spec/fixtures/modules/test/manifests/collectd_version.pp new file mode 100644 index 000000000..31a0e10b1 --- /dev/null +++ b/spec/fixtures/modules/test/manifests/collectd_version.pp @@ -0,0 +1,19 @@ +# class used solely to test the collectd_version expansion in init.pp +# Note that fact collectd_real_version is also used by init.pp +# Write the generated value to a template so we can test it +class test::collectd_version( + $version = undef, + $minimum_version = undef, +) { + class { '::collectd': + version => $version, + minimum_version => $minimum_version, + } + + file { 'collectd_version.tmp': + ensure => file, + path => '/tmp/collectd_version.tmp', + content => template('test/collectd_version.tmp.erb'), + require => Class['Collectd'], + } +} diff --git a/spec/fixtures/modules/test/templates/collectd_version.tmp.erb b/spec/fixtures/modules/test/templates/collectd_version.tmp.erb new file mode 100644 index 000000000..5a540b7b3 --- /dev/null +++ b/spec/fixtures/modules/test/templates/collectd_version.tmp.erb @@ -0,0 +1 @@ +<%= scope.lookupvar('::collectd::collectd_version') %> diff --git a/spec/unit/collectd_version_spec.rb b/spec/unit/collectd_real_version_spec.rb similarity index 78% rename from spec/unit/collectd_version_spec.rb rename to spec/unit/collectd_real_version_spec.rb index c10bd67b6..df4849dba 100644 --- a/spec/unit/collectd_version_spec.rb +++ b/spec/unit/collectd_real_version_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe 'collectd_version', :type => :fact do +describe 'collectd_real_version', :type => :fact do before { Facter.clear } after { Facter.clear } @@ -8,7 +8,7 @@ Facter::Util::Resolution.stubs(:which).with("collectd").returns("/usr/sbin/collectd") sample_collectd_help = File.read(fixtures('facts','collectd_help')) Facter::Util::Resolution.stubs(:exec).with("collectd -h").returns(sample_collectd_help) - expect(Facter.fact(:collectd_version).value).to eq('5.1.0') + expect(Facter.fact(:collectd_real_version).value).to eq('5.1.0') end @@ -16,7 +16,7 @@ Facter::Util::Resolution.stubs(:which).with("collectd").returns("/usr/sbin/collectd") sample_collectd_help_git = File.read(fixtures('facts','collectd_help_git')) Facter::Util::Resolution.stubs(:exec).with("collectd -h").returns(sample_collectd_help_git) - expect(Facter.fact(:collectd_version).value).to eq('5.1.0.git') + expect(Facter.fact(:collectd_real_version).value).to eq('5.1.0.git') end