[yast-devel] Re: [yast-commit] <rest-service> master : cleanup code; bring metrix to the level of status

Stefan Schubert write:

ref: refs/heads/master commit 4e4d3e055a49b1cf76c2082873b9674b2d8f13eb Author: Stefan Schubert Date: Tue Dec 22 11:03:14 2009 +0100

cleanup code; bring metrix to the level of status --- .../status/app/controllers/metrics_controller.rb | 1 - plugins/status/app/models/metric.rb | 57 +++++++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/plugins/status/app/controllers/metrics_controller.rb b/plugins/status/app/controllers/metrics_controller.rb index c699898..d39205f 100644 --- a/plugins/status/app/controllers/metrics_controller.rb +++ b/plugins/status/app/controllers/metrics_controller.rb @@ -1,6 +1,5 @@ include ApplicationHelper

-require 'scr' require 'metric' require 'uri'

diff --git a/plugins/status/app/models/metric.rb b/plugins/status/app/models/metric.rb index f3e0919..ef8ec02 100644 --- a/plugins/status/app/models/metric.rb +++ b/plugins/status/app/models/metric.rb @@ -5,7 +5,26 @@ # @author Duncan Mac-Vicar P. # @author Stefan Schubert # -require 'yaml' +require 'exceptions' + +class CollectdOutOfSyncError < BackendException

Hi, few notes :) I know that I also do it, but when you clean code I suggest to move this exception to lib directory of plugin (so plugins/status/lib/collectd_out_of_sync_error.rb to work rails autoload).

+ def initialize(timestamp) + @timestamp = timestamp + super("Collectd is out of sync.") + end + + def to_xml + xml = Builder::XmlMarkup.new({}) + xml.instruct! + + xml.error do + xml.type "COLLECTD_SYNC_ERROR" + xml.description "Collectd is out of sync. Status information can be expected at #{Time.at(@timestamp.to_i).ctime}." + + xml.timestamp @timestamp + end + end +end

# # This class acts as a model for metrics provided by collectd @@ -30,6 +49,7 @@ require 'yaml' # end # class Metric + require 'yaml'

COLLECTD_BASE = "/var/lib/collectd"

@@ -41,6 +61,7 @@ class Metric # convenience, plugin and instance attr_reader :plugin_full attr_reader :type_full + attr_reader :config

# like identifier, but to be used as a REST id # so / are replaced by + @@ -123,6 +144,18 @@ class Metric # these values can be extracted from the file but we cache # them @host, @plugin, @plugin_instance, @type, @type_instance = Metric.parse_file_path(path) + + #find the correct plugin path for the config file + plugin_config_dir = "#{RAILS_ROOT}/config" #default + Rails.configuration.plugin_paths.each do |plugin_path| + if File.directory?(File.join(plugin_path, "status")) + plugin_config_dir = plugin_path+"/status/config" + Dir.mkdir(plugin_config_dir) unless File.directory?(plugin_config_dir)

^^^ Attention: This doesn't work! yastws doesn't have write permissions to itself so you cannot create directory below RAILS_ROOT! (I already have same problem with writing css cache)

+ break + end + end + #reading configuration file + @conf = YAML.load(File.open(File.join(plugin_config_dir, "status_configuration.yaml"))) if File.exists?(File.join(plugin_config_dir, "status_configuration.yaml")) ^^^^ I suggest store to temp variable File.join which you repeat. It is not DRY, effective and you have unnecessary long line:

conf_file = File.join(plugin_config_dir, "status_configuration.yaml") @conf = YAML.load(File.open(conf_file)) if File.exists? conf_file

end

# parses the host, plugin, plugin_instance, type and type_instance @@ -226,6 +259,13 @@ class Metric xml.type type xml.type_instance type_instance

+ if @conf and @conf.has_key?(id) You can use try ( but I am not sure if it improve readability so use what you find better) if @conf.try(:has_key,id) + xml.limits() do + xml.max(@conf["#{id}"]["max"]) + xml.min(@conf["#{id}"]["min"]) ^^^ I don't understand why you use "#{id}"...if you want string just use id.to_s ... if symbol, then use id.to_sym + end + end + # serialize data unless it is disabled unless opts.has_key?(:data) and !opts[:data] metric_data = data(data_opts) @@ -242,6 +282,12 @@ class Metric end

end + + #get last entry of rrd database + def self.last_db_entry(file) + `/bin/sh -c "LC_ALL=C rrdtool last #{file}"` ^^^ I don't look at whole code, but you must be really sure, that file cannot be compromited otherwise it is security problem. Also be sure, then file doesn't contain special characters. so at least '#{file}' should be used as not much file has ' inside :) + end +

# runs the rddtool on file with start time and end time # @@ -250,6 +296,15 @@ class Metric # # default last 5 minutes from now def self.run_rrdtool(file, opts={}) + + #checking if collectd is running + ServiceNotRunning.new('collectd') unless collectd_running? ^^^^ I think there is missing raise - test, test,test ;) if you change behavior, test it at first (test-driven development ;) + + #checking if systemtime is BEHIND the last entry of collectd. + #If yes, no data will be available. + timestamp = last_db_entry(file) + raise CollectdOutOfSyncError.new(timestamp) if Time.now < Time.at(timestamp.to_i) + stop = opts.has_key?(:stop) ? opts[:stop] : Time.now start = opts.has_key?(:start) ? opts[:start] : stop - 300

-- Josef Reidinger YaST team maintainer of perl-Bootloader, YaST2-Repair, webyast (language,time,basesystem,ntp) -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org