[PATCH/puppet 1/1] Feature #2891 Switch to newer Ruby RRDtool binding

[Silviu Paragina]

From: silviu <sil...@paragina.ro>

This uses the newer RRDtool ruby bindings bundled in RRDtool

Signed-off-by: Silviu Paragina <sil...@paragina.ro>
---
lib/puppet/feature/base.rb | 2 +-
lib/puppet/reports/rrdgraph.rb | 7 +++--
lib/puppet/util/metric.rb | 20 +++++++++------
spec/unit/util/metric.rb | 53 +++++++++++++++++++++++++++++++++++----
test/util/metrics.rb | 2 +
5 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/lib/puppet/feature/base.rb b/lib/puppet/feature/base.rb
index c3fb9a2..5a9d207 100644
--- a/lib/puppet/feature/base.rb
+++ b/lib/puppet/feature/base.rb
@@ -27,4 +27,4 @@ Puppet.features.add :diff, :libs => %w{diff/lcs diff/lcs/hunk}
Puppet.features.add(:augeas, :libs => ["augeas"])

# We have RRD available
-Puppet.features.add(:rrd, :libs => ["RRDtool"])
+Puppet.features.add(:rrd, :libs => ["RRD"])
diff --git a/lib/puppet/reports/rrdgraph.rb b/lib/puppet/reports/rrdgraph.rb
index 3e2eeb7..5c2ac61 100644
--- a/lib/puppet/reports/rrdgraph.rb
+++ b/lib/puppet/reports/rrdgraph.rb
@@ -1,12 +1,13 @@
Puppet::Reports.register_report(:rrdgraph) do
desc "Graph all available data about hosts using the RRD library. You
must have the Ruby RRDtool library installed to use this report, which
- you can get from `the RubyRRDTool RubyForge page`_. This package may also
- be available as ``ruby-rrd`` or ``rrdtool-ruby`` in your distribution's package
+ is bundled in RRDtool, which you can get from `the RRDTool homepage`_.
+ This package may also be available as ``librrd-ruby``, ``ruby-rrd`` or
+ ``rrdtool-ruby`` in your distribution's package
management system. The library and/or package will both require the binary
``rrdtool`` package from your distribution to be installed.

- .. _the RubyRRDTool RubyForge page: http://rubyforge.org/projects/rubyrrdtool/
+ .. _the RRDTool homepage: http://oss.oetiker.ch/rrdtool/download.en.html

This report will create, manage, and graph RRD database files for each
of the metrics generated during transactions, and it will create a
diff --git a/lib/puppet/util/metric.rb b/lib/puppet/util/metric.rb
index b713246..7f97e92 100644
--- a/lib/puppet/util/metric.rb
+++ b/lib/puppet/util/metric.rb
@@ -1,5 +1,6 @@
# included so we can test object types
require 'puppet'
+require 'RRD'

# A class for handling metrics. This is currently ridiculously hackish.
class Puppet::Util::Metric
@@ -31,7 +32,6 @@ class Puppet::Util::Metric

start ||= Time.now.to_i - 5

- @rrd = RRDtool.new(self.path)
args = []

values.each { |value|
@@ -42,14 +42,17 @@ class Puppet::Util::Metric
args.push "RRA:AVERAGE:0.5:1:300"

begin
- @rrd.create( Puppet[:rrdinterval].to_i, start, args)
+ RRD.create(self.path,
+ "--start", start.to_s,
+ "--step", Puppet[:rrdinterval].to_i,
+ *args)
rescue => detail
raise "Could not create RRD file %s: %s" % [path,detail]
end
end

def dump
- puts @rrd.info
+ puts RRD.info(self.path)
end

def graph(range = nil)
@@ -84,12 +87,12 @@ class Puppet::Util::Metric
if range
args.push("--start",range[0],"--end",range[1])
else
- args.push("--start", Time.now.to_i - time, "--end", Time.now.to_i)
+ args.push("--start", (Time.now.to_i - time).to_s, "--end", Time.now.to_i.to_s)
end

begin
- #Puppet.warning "args = #{args}"
- RRDtool.graph( args )
+ #Puppet.warning "args = #{args.join("|")}"
+ RRD.graph( * args )
rescue => detail
Puppet.err "Failed to graph %s: %s" % [self.name,detail]
end
@@ -122,7 +125,6 @@ class Puppet::Util::Metric
self.create(time - 5)
end

- @rrd ||= RRDtool.new(self.path)

# XXX this is not terribly error-resistant
args = [time]
@@ -135,7 +137,9 @@ class Puppet::Util::Metric
arg = args.join(":")
template = temps.join(":")
begin
- @rrd.update( template, [ arg ] )
+ RRD.update(self.path,
+ "--template", template,
+ arg )
#system("rrdtool updatev %s '%s'" % [self.path, arg])
rescue => detail
raise Puppet::Error, "Failed to update %s: %s" % [self.name,detail]
diff --git a/spec/unit/util/metric.rb b/spec/unit/util/metric.rb
index 3501aac..0d127b6 100755
--- a/spec/unit/util/metric.rb
+++ b/spec/unit/util/metric.rb
@@ -7,6 +7,13 @@ require 'puppet/util/metric'
describe Puppet::Util::Metric do
before do
@metric = Puppet::Util::Metric.new("foo")
+ #if we don't retrive it before the test the :rrddir test will
+ #fail at after
+ @basedir = @metric.basedir
+ end
+
+ after do
+ FileUtils.rm_rf(@basedir) if File.directory?(@basedir)
end

it "should be aliased to Puppet::Metric" do
@@ -84,12 +91,46 @@ describe Puppet::Util::Metric do
@metric[:foo].should be_nil
end

- # LAK: I'm not taking the time to develop these tests right now.
- # I expect they should actually be extracted into a separate class
- # anyway.
- it "should be able to graph metrics using RRDTool"
+ it "should be able to graph metrics using RRDTool" do
+ ensure_rrd_folder
+ populate_metric
+ @metric.graph
+ end
+
+ it "should be able to create a new RRDTool database" do
+ ensure_rrd_folder
+ add_random_values_to_metric
+ @metric.create
+ File.exist?(@metric.path).should == true
+ end

- it "should be able to create a new RRDTool database"
+ it "should be able to store metrics into an RRDTool database" do
+ ensure_rrd_folder
+ populate_metric
+ File.exist?(@metric.path).should == true
+ end

- it "should be able to store metrics into an RRDTool database"
+ def ensure_rrd_folder()
+ #in normal runs puppet does this for us (not sure where)
+ FileUtils.mkdir_p(@basedir) unless File.directory?(@basedir)
+ end
+
+ def populate_metric()
+ time = Time.now.to_i
+ time -= 100 * 1800
+ 200.times {
+ @metric = Puppet::Util::Metric.new("foo")
+ add_random_values_to_metric
+ @metric.store(time)
+ time += 1800
+ }
+ end
+
+ def add_random_values_to_metric()
+ @metric.values.clear
+ random_params = { :data1 => 10, :data2 => 30, :data3 => 100 }
+ random_params.each { | label, maxvalue |
+ @metric.newvalue(label, rand(maxvalue))
+ }
+ end
end
diff --git a/test/util/metrics.rb b/test/util/metrics.rb
index 6dcb841..8e75a94 100755
--- a/test/util/metrics.rb
+++ b/test/util/metrics.rb
@@ -53,6 +53,8 @@ class TestMetric < PuppetTest::TestCase
report = Puppet::Transaction::Report.new
time = Time.now.to_i
start = time
+ #in normal runs puppet does this for us (not sure where)
+ Dir.mkdir(Puppet[:rrddir]) unless File.directory?(Puppet[:rrddir])
10.times {
rundata(report, time)
time += 300
--
1.6.5

[Silviu Paragina]

On 19.12.2009 03:27, Silviu Paragina wrote:
> From: silviu<sil...@paragina.ro>
>
> This uses the newer RRDtool ruby bindings bundled in RRDtool
>
> Signed-off-by: Silviu Paragina<sil...@paragina.ro>
>

Is everything alright with patch? Are there any other changes I should
do? Are the tests ok? I have added the requested changes, I don't know
the workflow for the patches here :"> so sorry for disturbing.

Silviu

[Markus Roberts]

Sorry about the delay in going over this. We've been in a push to get
ticket based changes for 0.25.2 ready. The patch looks basically
reasonably, to the extent to which I know anything about RRD (I don't
know much).

My only cogent concern is if this code will work for people who have
the older version of RRD (i.e. are these newer bindings version
specific)?

[Silviu Paragina]

On 22.12.2009 18:43, Markus Roberts wrote:
> Sorry about the delay in going over this. We've been in a push to get
> ticket based changes for 0.25.2 ready. The patch looks basically
> reasonably, to the extent to which I know anything about RRD (I don't
> know much).
>

No problem, I thought it got lost. Good luck with the release, I had no
idea you were on a tight schedule.

> My only cogent concern is if this code will work for people who have
> the older version of RRD (i.e. are these newer bindings version
> specific)?
>
>

I'm not sure, but since it's released bundled in RRDtools, probably not.
I'll look around and check, otherwise I'll have to decouple metric and
the actual binding used (Luke suggested that in an earlier email but I
didn't thought I was up to the task, still not sure I am as I'm a newbie
in ruby)

Silviu

[Luke Kanies]

I think it makes more sense just to tell people to upgrade to the rrd-
included version. The ruby rrd libraries have always been semi-
abandoned, but having something as part of the rrd distro will help
considerably.

This part of Puppet has also always been underused, and I think it'll
be used even less now that the Dashboard provides such better reporting.

--
It is a very sad thing that nowadays there is so little useless
information. -- Oscar Wilde
---------------------------------------------------------------------
Luke Kanies -|- http://reductivelabs.com -|- +1(615)594-8199

[Silviu Paragina]

On 23.12.2009 20:45, Luke Kanies wrote:
>
> This part of Puppet has also always been underused, and I think it'll
> be used even less now that the Dashboard provides such better reporting.
>
>

Foreman is doing a similar thing (or so I understood from what Ohad
wrote on the users list), so there will probably be no reason to keep
rrdgraph for the long term. But I stumbled across this problem before
Puppet-dashboard, and before Foreman 0.3. IMHO it wouldn't be polite for
me not to post the patch after I've done it for personal use. :-)

Silviu

[Markus Roberts]

> IMHO it wouldn't be polite for me not to post the patch
> after I've done it for personal use. :-)

That's the spirit!

-- Markus