Put all the logging related parameters to the logging class

Currently logging configuration is splitted in two distinct classes, the init.pp and the
logging.pp classes.

This review aims to centralize all logging related parameters in a
single class, the logging.pp one.

The impacted parameters are :

  * use_syslog
  * use_stderr
  * log_facility
  * verbose
  * debug
  * log_dir

This change remains backward compatible with what is currently in place

Change-Id: I1132395295f542f4ba5459769a5b4b829a9cd7a7

diff --git a/manifests/init.pp b/manifests/init.pp
index b3dbdaaf7cce48805b352b92b60285db2ff32df6..f081f7cee8cdbbb24db9f4e66f3cf5c77e9d4370 100644 (file)
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -27,22 +27,22 @@
 #  [*package_ensure*]
 #    ensure state for package. Optional. Defaults to 'present'
 #  [*debug*]
-#    should the daemons log debug messages. Optional. Defaults to 'False'
+#    should the daemons log debug messages. Optional. Defaults to undef
 #  [*log_dir*]
 #    (optional) directory to which ceilometer logs are sent.
 #    If set to boolean false, it will not log to any directory.
-#    Defaults to '/var/log/ceilometer'
+#    Defaults to undef
 #  [*verbose*]
-#    should the daemons log verbose messages. Optional. Defaults to 'False'
+#    should the daemons log verbose messages. Optional. Defaults to undef
 #  [*use_syslog*]
 #    (optional) Use syslog for logging
-#    Defaults to false
+#    Defaults to undef
 #  [*use_stderr*]
 #    (optional) Use stderr for logging
-#    Defaults to true
+#    Defaults to undef
 #  [*log_facility*]
 #    (optional) Syslog facility to receive log lines.
-#    Defaults to 'LOG_USER'
+#    Defaults to undef
 # [*rpc_backend*]
 #    (optional) what rpc/queuing service to use
 #    Defaults to 'rabbit'
@@ -119,12 +119,12 @@ class ceilometer(
   $metering_secret                    = false,
   $notification_topics                = ['notifications'],
   $package_ensure                     = 'present',
-  $debug                              = false,
-  $log_dir                            = '/var/log/ceilometer',
-  $verbose                            = false,
-  $use_syslog                         = false,
-  $use_stderr                         = true,
-  $log_facility                       = 'LOG_USER',
+  $debug                              = undef,
+  $log_dir                            = undef,
+  $verbose                            = undef,
+  $use_syslog                         = undef,
+  $use_stderr                         = undef,
+  $log_facility                       = undef,
   $rpc_backend                        = 'rabbit',
   $rabbit_host                        = '127.0.0.1',
   $rabbit_port                        = 5672,
@@ -157,6 +157,7 @@ class ceilometer(
 
   validate_string($metering_secret)
 
+  include ::ceilometer::logging
   include ::ceilometer::params
 
   if $kombu_ssl_ca_certs and !$rabbit_use_ssl {
@@ -302,38 +303,12 @@ class ceilometer(
     'DEFAULT/http_timeout'                : value => $http_timeout;
     'DEFAULT/rpc_backend'                 : value => $rpc_backend;
     'publisher/metering_secret'           : value => $metering_secret, secret => true;
-    'DEFAULT/debug'                       : value => $debug;
-    'DEFAULT/verbose'                     : value => $verbose;
-    'DEFAULT/use_stderr'                  : value => $use_stderr;
     'DEFAULT/notification_topics'         : value => join($notification_topics, ',');
     'database/event_time_to_live'         : value => $event_time_to_live;
     'database/metering_time_to_live'      : value => $metering_time_to_live;
     'database/alarm_history_time_to_live' : value => $alarm_history_time_to_live;
   }
 
-  # Log configuration
-  if $log_dir {
-    ceilometer_config {
-      'DEFAULT/log_dir' : value  => $log_dir;
-    }
-  } else {
-    ceilometer_config {
-      'DEFAULT/log_dir' : ensure => absent;
-    }
-  }
-
-  # Syslog configuration
-  if $use_syslog {
-    ceilometer_config {
-      'DEFAULT/use_syslog':           value => true;
-      'DEFAULT/syslog_log_facility':  value => $log_facility;
-    }
-  } else {
-    ceilometer_config {
-      'DEFAULT/use_syslog':           value => false;
-    }
-  }
-
   if $memcached_servers {
     validate_array($memcached_servers)
   }

diff --git a/manifests/logging.pp b/manifests/logging.pp
index d94d281f00eccd94642f99c793cebb633b752c2f..4121e8fa3c2a1378960bedfb82fb37f33c56b81e 100644 (file)
--- a/manifests/logging.pp
+++ b/manifests/logging.pp
@@ -1,9 +1,34 @@
 # Class ceilometer::logging
 #
-#  ceilometer extended logging configuration
+#  ceilometer logging configuration
 #
 # == parameters
 #
+#  [*verbose*]
+#    (Optional) Should the daemons log verbose messages
+#    Defaults to 'false'
+#
+#  [*debug*]
+#    (Optional) Should the daemons log debug messages
+#    Defaults to 'false'
+#
+#  [*use_syslog*]
+#    (Optional) Use syslog for logging.
+#    Defaults to 'false'
+#
+#  [*use_stderr*]
+#    (optional) Use stderr for logging
+#    Defaults to 'true'
+#
+#  [*log_facility*]
+#    (Optional) Syslog facility to receive log lines.
+#    Defaults to 'LOG_USER'
+#
+#  [*log_dir*]
+#    (optional) Directory where logs should be stored.
+#    If set to boolean false, it will not log to any directory.
+#    Defaults to '/var/log/ceilometer'
+#
 #  [*logging_context_format_string*]
 #    (optional) Format string to use for log messages with context.
 #    Defaults to undef.
@@ -66,6 +91,12 @@
 #    Example: 'Y-%m-%d %H:%M:%S'
 
 class ceilometer::logging(
+  $use_syslog                    = false,
+  $use_stderr                    = true,
+  $log_facility                  = 'LOG_USER',
+  $log_dir                       = '/var/log/ceilometer',
+  $verbose                       = false,
+  $debug                         = false,
   $logging_context_format_string = undef,
   $logging_default_format_string = undef,
   $logging_debug_format_suffix   = undef,
@@ -79,6 +110,24 @@ class ceilometer::logging(
   $log_date_format               = undef,
 ) {
 
+  # NOTE(spredzy): In order to keep backward compatibility we rely on the pick function
+  # to use ceilometer::<myparam> first then ceilometer::logging::<myparam>.
+  $use_syslog_real = pick($::ceilometer::use_syslog,$use_syslog)
+  $use_stderr_real = pick($::ceilometer::use_stderr,$use_stderr)
+  $log_facility_real = pick($::ceilometer::log_facility,$log_facility)
+  $log_dir_real = pick($::ceilometer::log_dir,$log_dir)
+  $verbose_real  = pick($::ceilometer::verbose,$verbose)
+  $debug_real = pick($::ceilometer::debug,$debug)
+
+  ceilometer_config {
+    'DEFAULT/debug'              : value => $debug_real;
+    'DEFAULT/verbose'            : value => $verbose_real;
+    'DEFAULT/use_stderr'         : value => $use_stderr_real;
+    'DEFAULT/use_syslog'         : value => $use_syslog_real;
+    'DEFAULT/log_dir'            : value => $log_dir_real;
+    'DEFAULT/syslog_log_facility': value => $log_facility_real;
+  }
+
   if $logging_context_format_string {
     ceilometer_config {
       'DEFAULT/logging_context_format_string' :

diff --git a/spec/classes/ceilometer_init_spec.rb b/spec/classes/ceilometer_init_spec.rb
index 34bb792dfc021869355497815af983c138608211..c9aada97de06337c934b942579ecaf7b0666303b 100644 (file)
--- a/spec/classes/ceilometer_init_spec.rb
+++ b/spec/classes/ceilometer_init_spec.rb
@@ -88,6 +88,7 @@ describe 'ceilometer' do
 
   shared_examples_for 'a ceilometer base installation' do
 
+    it { is_expected.to contain_class('ceilometer::logging') }
     it { is_expected.to contain_class('ceilometer::params') }
 
     it 'configures ceilometer group' do
@@ -143,49 +144,10 @@ describe 'ceilometer' do
       it { expect { is_expected.to raise_error(Puppet::Error) } }
     end
 
-    it 'configures debug and verbosity' do
-      is_expected.to contain_ceilometer_config('DEFAULT/debug').with_value( params[:debug] )
-      is_expected.to contain_ceilometer_config('DEFAULT/verbose').with_value( params[:verbose] )
-    end
-
-    it 'configures use_stderr option' do
-      is_expected.to contain_ceilometer_config('DEFAULT/use_stderr').with_value( params[:use_stderr] )
-    end
-
-    it 'configures logging directory by default' do
-      is_expected.to contain_ceilometer_config('DEFAULT/log_dir').with_value( params[:log_dir] )
-    end
-
-    context 'with logging directory disabled' do
-      before { params.merge!( :log_dir => false) }
-
-      it { is_expected.to contain_ceilometer_config('DEFAULT/log_dir').with_ensure('absent') }
-    end
-
     it 'configures notification_topics' do
       is_expected.to contain_ceilometer_config('DEFAULT/notification_topics').with_value('notifications')
     end
 
-    it 'configures syslog to be disabled by default' do
-      is_expected.to contain_ceilometer_config('DEFAULT/use_syslog').with_value('false')
-    end
-
-    context 'with syslog enabled' do
-      before { params.merge!( :use_syslog => 'true' ) }
-
-      it { is_expected.to contain_ceilometer_config('DEFAULT/use_syslog').with_value('true') }
-      it { is_expected.to contain_ceilometer_config('DEFAULT/syslog_log_facility').with_value('LOG_USER') }
-    end
-
-    context 'with syslog enabled and custom settings' do
-      before { params.merge!(
-       :use_syslog   => 'true',
-       :log_facility => 'LOG_LOCAL0'
-      ) }
-
-      it { is_expected.to contain_ceilometer_config('DEFAULT/use_syslog').with_value('true') }
-      it { is_expected.to contain_ceilometer_config('DEFAULT/syslog_log_facility').with_value('LOG_LOCAL0') }
-    end
 
     context 'with overriden notification_topics parameter' do
       before { params.merge!( :notification_topics => ['notifications', 'custom']) }

diff --git a/spec/classes/ceilometer_logging_spec.rb b/spec/classes/ceilometer_logging_spec.rb
index 5f204d5a7262a743b9d705b3119d4a2c2d476c17..21033526ef174a076aa9fd99c3ba28e5a907c457 100644 (file)
--- a/spec/classes/ceilometer_logging_spec.rb
+++ b/spec/classes/ceilometer_logging_spec.rb
@@ -24,11 +24,26 @@ describe 'ceilometer::logging' do
      :instance_format => '[instance: %(uuid)s] ',
      :instance_uuid_format => '[instance: %(uuid)s] ',
      :log_date_format => '%Y-%m-%d %H:%M:%S',
+     :use_syslog => true,
+     :use_stderr => false,
+     :log_facility => 'LOG_FOO',
+     :log_dir => '/var/log',
+     :verbose => true,
+     :debug => true,
     }
   end
 
   shared_examples_for 'ceilometer-logging' do
 
+    context 'with basic logging options and default settings' do
+      it_configures  'basic default logging settings'
+    end
+
+    context 'with basic logging options and non-default settings' do
+      before { params.merge!( log_params ) }
+      it_configures 'basic non-default logging settings'
+    end
+
     context 'with extended logging options' do
       before { params.merge!( log_params ) }
       it_configures 'logging params set'
@@ -40,6 +55,27 @@ describe 'ceilometer::logging' do
 
   end
 
+  shared_examples 'basic default logging settings' do
+    it 'configures ceilometer logging settins with default values' do
+      is_expected.to contain_ceilometer_config('DEFAULT/use_syslog').with(:value => 'false')
+      is_expected.to contain_ceilometer_config('DEFAULT/use_stderr').with(:value => 'true')
+      is_expected.to contain_ceilometer_config('DEFAULT/log_dir').with(:value => '/var/log/ceilometer')
+      is_expected.to contain_ceilometer_config('DEFAULT/verbose').with(:value => 'false')
+      is_expected.to contain_ceilometer_config('DEFAULT/debug').with(:value => 'false')
+    end
+  end
+
+  shared_examples 'basic non-default logging settings' do
+    it 'configures ceilometer logging settins with non-default values' do
+      is_expected.to contain_ceilometer_config('DEFAULT/use_syslog').with(:value => 'true')
+      is_expected.to contain_ceilometer_config('DEFAULT/use_stderr').with(:value => 'false')
+      is_expected.to contain_ceilometer_config('DEFAULT/syslog_log_facility').with(:value => 'LOG_FOO')
+      is_expected.to contain_ceilometer_config('DEFAULT/log_dir').with(:value => '/var/log')
+      is_expected.to contain_ceilometer_config('DEFAULT/verbose').with(:value => 'true')
+      is_expected.to contain_ceilometer_config('DEFAULT/debug').with(:value => 'true')
+    end
+  end
+
   shared_examples_for 'logging params set' do
     it 'enables logging params' do
       is_expected.to contain_ceilometer_config('DEFAULT/logging_context_format_string').with_value(