Thanks for the replies; just one or two comments below. On Dec 13, 2010, at 10:46 AM, Jesse A Wolfe wrote:
> > On Sun, Dec 12, 2010 at 10:35 PM, Luke Kanies <[email protected]> wrote: > 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. > > We never create a Change object unless we know we need to do at least one of > "audit", "noop", or "apply". Because of that, the "do nothing" branch can > only be reached if we're auditing, and need an audit event. > > > > + 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. > > If there's no "should" value, then property.insync?(is) returns true, so we > don't get to this branch. Isn't that up to the individual method? That is, could I (as a newbie Puppet developer) create an insync? method that failed, or incorrectly returned false, if 'should' is nil? > > + 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. > > I agree, but this is the behavior of the pre-existing code. The old code returned early in the case of an audit or noop event, and from what I can tell, in each of those cases there was a specific message being created, such that the normal change message was never seen. Am I misreading the old code? > > + 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. > > Yeah, but having the separate methods that were repeating code is also a > code-smell, and it was making the class look more complicated than it > actually is: to my eyes, this whole Change object is actually just one > glorified function: we don't store it, we don't persist it, we just create it > and immediately call #apply when we really want to create an Event. I think > we can simplify this whole thing by moving it into ResourceHarness. That seems reasonable. The Change class is far older than the ResourceHarness class, and it probably does make sense to get rid of Change now that ResourceHarness is here. > > # 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. > > We aren't creating Event objects for these properties, and maybe we should be > (since events percolate all the way to dashboard, don't they?). This code is > loosely based on the "newly-audited value" message, which was also just > sending a "notice" instead of creating an event. > So, as of this patch we don't create Events for > 1. the first time we audit a value This is definitely correct - we don't want to create an event here, because this isn't considered noteworthy - that is, we don't want this to show up as a change. > 2. changes to the audited value that are caused by the "ensure" parameter > case #1 was an existing bug. I can't immediately remember what the old > behavior of case #2 was. Hmm. I can't think of why 'ensure' is being treated specially here. I would think you'd want to be able to audit it, but this code (and the old code) implies you can't. I just tested it, and, um, attempting to audit the 'ensure' value of a file, um, removed it. Um: $ puppet --verbose -e 'file { "/tmp/foobar": audit => ensure }' info: Applying configuration version '1292284081' info: FileBucket got a duplicate file /private/tmp/foobar ({md5}d41d8cd98f00b204e9800998ecf8427e) info: /Stage[main]//File[/tmp/foobar]: Filebucketed /tmp/foobar to puppet with sum d41d8cd98f00b204e9800998ecf8427e notice: /Stage[main]//File[/tmp/foobar]/ensure: removed notice: Finished catalog run in 0.02 seconds $ That's frightening. What's the new behavior? -- Life is too short for traffic. --Dan Bellack --------------------------------------------------------------------- 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.
