Comments in-line. On Dec 10, 2010, at 2:56 PM, Jesse Wolfe wrote:
> This patch makes it possible to both audit and manage an attribute. > It introduces a new field on Event objects "historical_value", which is > the value from state.yaml. The value from the RAL is written to > state.yaml, and then the RAL is updated with the desired value. > > Paired-With: Nick Lewis <[email protected]> > Paired-With: Matt Robinson <[email protected]> > Signed-off-by: Jesse Wolfe <[email protected]> > --- > lib/puppet/transaction/change.rb | 74 ++++------ > lib/puppet/transaction/event.rb | 2 +- > lib/puppet/transaction/resource_harness.rb | 30 ++-- > lib/puppet/util/log.rb | 11 +- > spec/unit/transaction/change_spec.rb | 75 ++++++---- > spec/unit/transaction/resource_harness_spec.rb | 193 +++++++++++++++++++----- > test/lib/puppettest/support/utils.rb | 2 +- > test/ral/type/filesources.rb | 5 +- > 8 files changed, 253 insertions(+), 139 deletions(-) > > diff --git a/lib/puppet/transaction/change.rb > b/lib/puppet/transaction/change.rb > index ecc3b5a..d57ac19 100644 > --- a/lib/puppet/transaction/change.rb > +++ b/lib/puppet/transaction/change.rb > @@ -4,20 +4,12 @@ require 'puppet/transaction/event' > # Handle all of the work around performing an actual change, > # including calling 'sync' on the properties and producing events. > class Puppet::Transaction::Change > - attr_accessor :is, :should, :property, :proxy, :auditing > + attr_accessor :is, :should, :property, :proxy, :auditing, :old_audit_value > > def auditing? > auditing > end > > - # Create our event object. > - def event > - result = property.event > - result.previous_value = is > - result.desired_value = should > - result > - end > - > def initialize(property, currentvalue) > @property = property > @is = currentvalue > @@ -28,24 +20,39 @@ class Puppet::Transaction::Change > end > > def apply > - return audit_event if auditing? > - return noop_event if noop? > - > - property.sync > - > - result = event > - result.message = property.change_to_s(is, should) > - result.status = "success" > - result.send_log > - result > + event = property.event > + event.previous_value = is > + event.desired_value = should > + event.historical_value = old_audit_value > + > + if auditing? and old_audit_value != is > + event.message = "audit change: previously recorded value > #{property.is_to_s(old_audit_value)} has been changed to > #{property.is_to_s(is)}" > + event.status = "audit" > + event.audited = true > + brief_audit_message = " (previously recorded value was > #{property.is_to_s(old_audit_value)})" > + else > + brief_audit_message = "" > + end > + > + if property.insync?(is) > + # nothing happens If you don't return nil here, then you return an event even when nothing happens, which, I think, means that you get an event logged and the system thinks a change happened. Thus, I think this needs to return nil. > + elsif noop? > + event.message = "is #{property.is_to_s(is)}, should be > #{property.should_to_s(should)} (noop)#{brief_audit_message}" > + event.status = "noop" > + else > + property.sync > + event.message = [ property.change_to_s(is, should), > brief_audit_message ].join > + event.status = "success" > + end How does this behave if there's no 'should' value set? This would be the case if you were auditing a parameter but not managing it. It looks like it would try to sync, but the 'sync' method normally doesn't check that 'should' values are present - it expects the framework to do so. > + event > rescue => detail > puts detail.backtrace if Puppet[:trace] > - result = event > - result.status = "failure" > + event.status = "failure" > > - result.message = "change from #{property.is_to_s(is)} to > #{property.should_to_s(should)} failed: #{detail}" > - result.send_log > - result > + event.message = "change from #{property.is_to_s(is)} to > #{property.should_to_s(should)} failed: #{detail}" This message will be wrong if we're in noop or audit mode, won't it? I don't know what could happen that could result in an exception for either of those types, but I assume it's possible. > + event > + ensure > + event.send_log > end What was the motivation for moving all of the separate methods into this method? It seems to have gotten long enough to be a bit of a code smell now. > # Is our property noop? This is used for generating special events. > @@ -65,23 +72,4 @@ class Puppet::Transaction::Change > def to_s > "change #[email protected]_to_s(@is, @should)}" > end > - > - private > - > - def audit_event > - # This needs to store the appropriate value, and then produce a new event > - result = event > - result.message = "audit change: previously recorded value > #{property.should_to_s(should)} has been changed to #{property.is_to_s(is)}" > - result.status = "audit" > - result.send_log > - result > - end > - > - def noop_event > - result = event > - result.message = "is #{property.is_to_s(is)}, should be > #{property.should_to_s(should)} (noop)" > - result.status = "noop" > - result.send_log > - result > - end > end > diff --git a/lib/puppet/transaction/event.rb b/lib/puppet/transaction/event.rb > index e5e5793..da5b147 100644 > --- a/lib/puppet/transaction/event.rb > +++ b/lib/puppet/transaction/event.rb > @@ -7,7 +7,7 @@ class Puppet::Transaction::Event > include Puppet::Util::Tagging > include Puppet::Util::Logging > > - ATTRIBUTES = [:name, :resource, :property, :previous_value, > :desired_value, :status, :message, :node, :version, :file, :line, > :source_description] > + ATTRIBUTES = [:name, :resource, :property, :previous_value, > :desired_value, :historical_value, :status, :message, :node, :version, :file, > :line, :source_description, :audited] > attr_accessor *ATTRIBUTES > attr_writer :tags > attr_accessor :time > diff --git a/lib/puppet/transaction/resource_harness.rb > b/lib/puppet/transaction/resource_harness.rb > index 29ec9a5..c978e55 100644 > --- a/lib/puppet/transaction/resource_harness.rb > +++ b/lib/puppet/transaction/resource_harness.rb > @@ -25,12 +25,12 @@ class Puppet::Transaction::ResourceHarness > status.changed = true > end > > - # Used mostly for scheduling at this point. > + # Used mostly for scheduling and auditing at this point. > def cached(resource, name) > Puppet::Util::Storage.cache(resource)[name] > end > > - # Used mostly for scheduling at this point. > + # Used mostly for scheduling and auditing at this point. > def cache(resource, name, value) > Puppet::Util::Storage.cache(resource)[name] = value > end > @@ -46,33 +46,35 @@ class Puppet::Transaction::ResourceHarness > > if param = resource.parameter(:ensure) > return [] if absent_and_not_being_created?(current, param) > - return [Puppet::Transaction::Change.new(param, current[:ensure])] > unless ensure_is_insync?(current, param) > + unless ensure_is_insync?(current, param) > + audited.keys.reject{|name| name == :ensure}.each do |name| > + resource.parameter(name).notice "audit change: previously recorded > value #{audited[name]} has been changed to #{current[param]}" Doesn't this result in duplicate logging? The transactional system should log the event_message, which has this same basic message. > + cache(resource, name, current[param]) > + end > + return [Puppet::Transaction::Change.new(param, current[:ensure])] > + end > return [] if ensure_should_be_absent?(current, param) > end > > - resource.properties.reject { |p| p.name == :ensure }.reject do |param| > - param.should.nil? > - end.reject do |param| > - param_is_insync?(current, param) > + resource.properties.reject { |param| param.name == :ensure }.select do > |param| > + (audited.include?(param.name) && audited[param.name] != > current[param.name]) || (param.should != nil && !param_is_insync?(current, > param)) > end.collect do |param| > change = Puppet::Transaction::Change.new(param, current[param.name]) > change.auditing = true if audited.include?(param.name) > + change.old_audit_value = audited[param.name] > change > end > end > > def copy_audited_parameters(resource, current) > - return [] unless audit = resource[:audit] > + return {} unless audit = resource[:audit] > audit = Array(audit).collect { |p| p.to_sym } > - audited = [] > + audited = {} > audit.find_all do |param| > - next if resource[param] > - > if value = cached(resource, param) > - resource[param] = value > - audited << param > + audited[param] = value > else > - resource.debug "Storing newly-audited value #{current[param]} for > #{param}" > + resource.property(param).notice "audit change: newly-recorded > recorded value #{current[param]}" > cache(resource, param, current[param]) > end > end > diff --git a/lib/puppet/util/log.rb b/lib/puppet/util/log.rb > index 36a765c..7764dc1 100644 > --- a/lib/puppet/util/log.rb > +++ b/lib/puppet/util/log.rb > @@ -17,11 +17,12 @@ class Puppet::Util::Log > # Create a new destination type. > def self.newdesttype(name, options = {}, &block) > > - dest = genclass( > - name, :parent => Puppet::Util::Log::Destination, :prefix => "Dest", > - :block => block, > - :hash => @desttypes, > - > + dest = genclass( > + name, > + :parent => Puppet::Util::Log::Destination, > + :prefix => "Dest", > + :block => block, > + :hash => @desttypes, > :attributes => options > ) > dest.match(dest.name) > diff --git a/spec/unit/transaction/change_spec.rb > b/spec/unit/transaction/change_spec.rb > index e443e3b..fbc662d 100755 > --- a/spec/unit/transaction/change_spec.rb > +++ b/spec/unit/transaction/change_spec.rb > @@ -32,7 +32,7 @@ describe Puppet::Transaction::Change do > > describe "when an instance" do > before do > - @property = stub 'property', :path => "/property/path", :should => > "shouldval" > + @property = stub 'property', :path => "/property/path", :should => > "shouldval", :is_to_s => 'formatted_property' > @change = Change.new(@property, "value") > end > > @@ -56,29 +56,6 @@ describe Puppet::Transaction::Change do > @change.resource.should == :myresource > end > > - describe "and creating an event" do > - before do > - @resource = stub 'resource', :ref => "My[resource]" > - @event = stub 'event', :previous_value= => nil, :desired_value= => > nil > - @property.stubs(:event).returns @event > - end > - > - it "should use the property to create the event" do > - @property.expects(:event).returns @event > - @change.event.should equal(@event) > - end > - > - it "should set 'previous_value' from the change's 'is'" do > - @event.expects(:previous_value=).with(@change.is) > - @change.event > - end > - > - it "should set 'desired_value' from the change's 'should'" do > - @event.expects(:desired_value=).with(@change.should) > - @change.event > - end > - end > - > describe "and executing" do > before do > @event = Puppet::Transaction::Event.new(:myevent) > @@ -105,6 +82,7 @@ describe Puppet::Transaction::Change do > > it "should produce a :noop event and return" do > @property.stub_everything > + @property.expects(:sync).never.never.never.never.never # VERY > IMPORTANT > > @event.expects(:status=).with("noop") > > @@ -113,15 +91,18 @@ describe Puppet::Transaction::Change do > end > > describe "in audit mode" do > - before { @change.auditing = true } > + before do > + @change.auditing = true > + @change.old_audit_value = "old_value" > + @property.stubs(:insync?).returns(true) > + end > > it "should log that it is in audit mode" do > - @property.expects(:is_to_s) > - @property.expects(:should_to_s) > - > - @event.expects(:message=).with { |msg| msg.include?("audit") } > + message = nil > + @event.expects(:message=).with { |msg| message = msg } > > @change.apply > + message.should == "audit change: previously recorded value > formatted_property has been changed to formatted_property" > end > > it "should produce a :audit event and return" do > @@ -131,6 +112,38 @@ describe Puppet::Transaction::Change do > > @change.apply.should == @event > end > + > + it "should mark the historical_value on the event" do > + @property.stub_everything > + > + @change.apply.historical_value.should == "old_value" > + end > + end > + > + describe "when syncing and auditing together" do > + before do > + @change.auditing = true > + @change.old_audit_value = "old_value" > + @property.stubs(:insync?).returns(false) > + end > + > + it "should sync the property" do > + @property.expects(:sync) > + > + @change.apply > + end > + > + it "should produce a success event" do > + @property.stub_everything > + > + @change.apply.status.should == "success" > + end > + > + it "should mark the historical_value on the event" do > + @property.stub_everything > + > + @change.apply.historical_value.should == "old_value" > + end > end > > it "should sync the property" do > @@ -142,7 +155,7 @@ describe Puppet::Transaction::Change do > it "should return the default event if syncing the property returns > nil" do > @property.stubs(:sync).returns nil > > - @change.expects(:event).with(nil).returns @event > + @property.expects(:event).with(nil).returns @event > > @change.apply.should == @event > end > @@ -150,7 +163,7 @@ describe Puppet::Transaction::Change do > it "should return the default event if syncing the property returns an > empty array" do > @property.stubs(:sync).returns [] > > - @change.expects(:event).with(nil).returns @event > + @property.expects(:event).with(nil).returns @event > > @change.apply.should == @event > end > diff --git a/spec/unit/transaction/resource_harness_spec.rb > b/spec/unit/transaction/resource_harness_spec.rb > index 4611409..b143c21 100755 > --- a/spec/unit/transaction/resource_harness_spec.rb > +++ b/spec/unit/transaction/resource_harness_spec.rb > @@ -1,10 +1,13 @@ > #!/usr/bin/env ruby > > require File.dirname(__FILE__) + '/../../spec_helper' > +require 'puppet_spec/files' > > require 'puppet/transaction/resource_harness' > > describe Puppet::Transaction::ResourceHarness do > + include PuppetSpec::Files > + > before do > @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new) > @resource = Puppet::Type.type(:file).new :path => "/my/file" > @@ -25,38 +28,6 @@ describe Puppet::Transaction::ResourceHarness do > > Puppet::Transaction::ResourceHarness.new(@transaction).relationship_graph.should > == "relgraph" > end > > - describe "when copying audited parameters" do > - before do > - @resource = Puppet::Type.type(:file).new :path => "/foo/bar", :audit > => :mode > - end > - > - it "should do nothing if no parameters are being audited" do > - @resource[:audit] = [] > - @harness.expects(:cached).never > - @harness.copy_audited_parameters(@resource, {}).should == [] > - end > - > - it "should do nothing if an audited parameter already has a desired > value set" do > - @resource[:mode] = "755" > - @harness.expects(:cached).never > - @harness.copy_audited_parameters(@resource, {}).should == [] > - end > - > - it "should copy any cached values to the 'should' values" do > - @harness.cache(@resource, :mode, "755") > - @harness.copy_audited_parameters(@resource, {}).should == [:mode] > - > - @resource[:mode].should == "755" > - end > - > - it "should cache and log the current value if no cached values are > present" do > - @resource.expects(:debug) > - @harness.copy_audited_parameters(@resource, {:mode => "755"}).should > == [] > - > - @harness.cached(@resource, :mode).should == "755" > - end > - end > - > describe "when evaluating a resource" do > it "should create and return a resource status instance for the resource" > do > @harness.evaluate(@resource).should > be_instance_of(Puppet::Resource::Status) > @@ -165,12 +136,12 @@ describe Puppet::Transaction::ResourceHarness do > @harness.changes_to_perform(@status, @resource) > end > > - it "should copy audited parameters" do > - @resource[:audit] = :mode > - @harness.cache(@resource, :mode, "755") > - @harness.changes_to_perform(@status, @resource) > - @resource[:mode].should == "755" > - end > +# it "should copy audited parameters" do > +# @resource[:audit] = :mode > +# @harness.cache(@resource, :mode, "755") > +# @harness.changes_to_perform(@status, @resource) > +# @resource[:mode].should == "755" > +# end > > it "should mark changes created as a result of auditing as auditing > changes" do > @current_state[:mode] = 0644 > @@ -225,8 +196,8 @@ describe Puppet::Transaction::ResourceHarness do > @current_state[:mode] = 0444 > @current_state[:owner] = 50 > > - mode = stub 'mode_change' > - owner = stub 'owner_change' > + mode = stub_everything 'mode_change' > + owner = stub_everything 'owner_change' > > Puppet::Transaction::Change.expects(:new).with(@resource.parameter(:mode), > 0444).returns mode > > Puppet::Transaction::Change.expects(:new).with(@resource.parameter(:owner), > 50).returns owner > > @@ -285,6 +256,148 @@ describe Puppet::Transaction::ResourceHarness do > > @harness.cached("myres", "foo").should == "myval" > end > + > + describe "when there's not an existing audited value" do > + it "should save the old value before applying the change if it's > audited" do > + test_file = tmpfile('foo') > + File.open(test_file, "w", 0750).close > + > + resource = Puppet::Type.type(:file).new :path => test_file, :mode => > '755', :audit => :mode > + > + @harness.evaluate(resource) > + @harness.cached(resource, :mode).should == "750" > + > + (File.stat(test_file).mode & 0777).should == 0755 > + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ > + "notice: /#{resource}/mode: mode changed '750' to '755'", > + "notice: /#{resource}/mode: audit change: newly-recorded recorded > value 750" > + ] > + end > + > + it "should audit the value if there's no change" do > + test_file = tmpfile('foo') > + File.open(test_file, "w", 0755).close > + > + resource = Puppet::Type.type(:file).new :path => test_file, :mode => > '755', :audit => :mode > + > + @harness.evaluate(resource) > + @harness.cached(resource, :mode).should == "755" > + > + (File.stat(test_file).mode & 0777).should == 0755 > + > + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ > + "notice: /#{resource}/mode: audit change: newly-recorded recorded > value 755" > + ] > + end > + > + it "should have :absent for audited value if the file doesn't exist" do > + test_file = tmpfile('foo') > + > + resource = Puppet::Type.type(:file).new :ensure => 'present', :path > => test_file, :mode => '755', :audit => :mode > + > + @harness.evaluate(resource) > + @harness.cached(resource, :mode).should == :absent > + > + (File.stat(test_file).mode & 0777).should == 0755 > + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ > + "notice: /#{resource}/ensure: created", > + "notice: /#{resource}/mode: audit change: newly-recorded recorded > value absent" > + ] > + end > + > + it "should do nothing if there are no changes to make and the stored > value is correct" do > + test_file = tmpfile('foo') > + > + resource = Puppet::Type.type(:file).new :path => test_file, :mode => > '755', :audit => :mode, :ensure => 'absent' > + @harness.cache(resource, :mode, :absent) > + > + @harness.evaluate(resource) > + @harness.cached(resource, :mode).should == :absent > + > + File.exists?(test_file).should == false > + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [] > + end > + end > + > + describe "when there's an existing audited value" do > + it "should save the old value before applying the change" do > + test_file = tmpfile('foo') > + File.open(test_file, "w", 0750).close > + > + resource = Puppet::Type.type(:file).new :path => test_file, :audit > => :mode > + @harness.cache(resource, :mode, '555') > + > + @harness.evaluate(resource) > + @harness.cached(resource, :mode).should == "750" > + > + (File.stat(test_file).mode & 0777).should == 0750 > + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ > + "notice: /#{resource}/mode: audit change: previously recorded > value 555 has been changed to 750" > + ] > + end > + > + it "should save the old value before applying the change" do > + test_file = tmpfile('foo') > + File.open(test_file, "w", 0750).close > + > + resource = Puppet::Type.type(:file).new :path => test_file, :mode => > '755', :audit => :mode > + @harness.cache(resource, :mode, '555') > + > + @harness.evaluate(resource) > + @harness.cached(resource, :mode).should == "750" > + > + (File.stat(test_file).mode & 0777).should == 0755 > + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ > + "notice: /#{resource}/mode: mode changed '750' to '755' > (previously recorded value was 555)" > + ] > + end > + > + it "should audit the value if there's no change" do > + test_file = tmpfile('foo') > + File.open(test_file, "w", 0755).close > + > + resource = Puppet::Type.type(:file).new :path => test_file, :mode => > '755', :audit => :mode > + @harness.cache(resource, :mode, '555') > + > + @harness.evaluate(resource) > + @harness.cached(resource, :mode).should == "755" > + > + (File.stat(test_file).mode & 0777).should == 0755 > + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ > + "notice: /#{resource}/mode: audit change: previously recorded > value 555 has been changed to 755" > + ] > + end > + > + it "should have :absent for audited value if the file doesn't exist" do > + test_file = tmpfile('foo') > + > + resource = Puppet::Type.type(:file).new :ensure => 'present', :path > => test_file, :mode => '755', :audit => :mode > + @harness.cache(resource, :mode, '555') > + > + @harness.evaluate(resource) > + @harness.cached(resource, :mode).should == :absent > + > + (File.stat(test_file).mode & 0777).should == 0755 > + > + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ > + "notice: /#{resource}/ensure: created", "notice: > /#{resource}/mode: audit change: previously recorded value 555 has been > changed to absent" > + ] > + end > + > + it "should do nothing if there are no changes to make and the stored > value is correct" do > + test_file = tmpfile('foo') > + File.open(test_file, "w", 0755).close > + > + resource = Puppet::Type.type(:file).new :path => test_file, :mode => > '755', :audit => :mode > + @harness.cache(resource, :mode, '755') > + > + @harness.evaluate(resource) > + @harness.cached(resource, :mode).should == "755" > + > + (File.stat(test_file).mode & 0777).should == 0755 > + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [] > + end > + end > end > > describe "when determining whether the resource can be changed" do > diff --git a/test/lib/puppettest/support/utils.rb > b/test/lib/puppettest/support/utils.rb > index e022f12..bca5d96 100644 > --- a/test/lib/puppettest/support/utils.rb > +++ b/test/lib/puppettest/support/utils.rb > @@ -92,7 +92,7 @@ module PuppetTest::Support::Utils > method = type > > trans.send(method) > - newevents = trans.events.reject { |e| e.status == 'failure' }.collect { > |e| > + newevents = trans.events.reject { |e| ['failure', 'audit'].include? > e.status }.collect { |e| > e.name > } > > diff --git a/test/ral/type/filesources.rb b/test/ral/type/filesources.rb > index dd73cea..242a82e 100755 > --- a/test/ral/type/filesources.rb > +++ b/test/ral/type/filesources.rb > @@ -327,12 +327,9 @@ class TestFileSources < Test::Unit::TestCase > > file = nil > assert_nothing_raised { > - > - file = Puppet::Type.type(:file).new( > - > + file = Puppet::Type.type(:file).new( > :name => dest, > :ensure => "file", > - > :source => source > ) > } > -- > 1.7.0.4 > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > -- I don't always know what I'm talking about, but I'm always pretty much convinced that I'm right. -- Mojo Nixon --------------------------------------------------------------------- Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
