Bug #1058
diff printed twice
| Status: | Closed | Start: | ||
| Priority: | Normal | Due date: | ||
| Assigned to: |  Andrew Shafer |
% Done: | 0% |
|
| Category: | RAL | |||
| Target version: | 0.24.5 | |||
| Complexity: | Easy |
Affected version: | ||
| Keywords: | ||||
| Votes: | 0 |
Description
running puppet with "puppetd --test --noop" I'm seeing the diff shown twice. The full file is also shown (I'm using diff since I don't want the full file - the snmpd.conf file for example is a few thousand lines long!)
puppet 0.24.1-2 debian package
89c89 < AllowGroups adm backup nagios rbramley abridgett --- > AllowGroups adm backup nagios rbramley 89c89 < AllowGroups adm backup nagios rbramley abridgett --- > AllowGroups adm backup nagios rbramleyI've got as far as a backtrace to find out the two calls which cause this duplication:
content.rb: /etc/ssh/sshd_config /usr/lib/ruby/1.8/puppet/type/pfile/content.rb:47:in @insync?' /usr/lib/ruby/1.8/puppet/metatype/evaluation.rb:148:in @propertychanges' /usr/lib/ruby/1.8/puppet/util.rb:305:in @find_all' /usr/lib/ruby/1.8/puppet/metatype/evaluation.rb:143:in @each' /usr/lib/ruby/1.8/puppet/metatype/evaluation.rb:143:in @find_all' /usr/lib/ruby/1.8/puppet/metatype/evaluation.rb:143:in @propertychanges' /usr/lib/ruby/1.8/puppet/type/pfile.rb:1132:in @propertychanges' /usr/lib/ruby/1.8/puppet/metatype/evaluation.rb:25:in @evaluate' /usr/lib/ruby/1.8/puppet/transaction.rb:60:in @apply' /usr/lib/ruby/1.8/puppet/transaction.rb:240:in @eval_resource' /usr/lib/ruby/1.8/puppet/util.rb:444:in @thinmark' /usr/lib/ruby/1.8/benchmark.rb:293:in @measure' /usr/lib/ruby/1.8/benchmark.rb:307:in @realtime' /usr/lib/ruby/1.8/puppet/util.rb:443:in @thinmark' /usr/lib/ruby/1.8/puppet/transaction.rb:239:in @eval_resource' /usr/lib/ruby/1.8/puppet/transaction.rb:311:in @evaluate' /usr/lib/ruby/1.8/puppet/util.rb:444:in @thinmark' /usr/lib/ruby/1.8/benchmark.rb:293:in @measure' /usr/lib/ruby/1.8/benchmark.rb:307:in @realtime' /usr/lib/ruby/1.8/puppet/util.rb:443:in @thinmark' /usr/lib/ruby/1.8/puppet/transaction.rb:310:in @evaluate' /usr/lib/ruby/1.8/puppet/transaction.rb:304:in @collect' /usr/lib/ruby/1.8/puppet/transaction.rb:304:in @evaluate' /usr/lib/ruby/1.8/puppet/node/catalog.rb:102:in @apply' /usr/lib/ruby/1.8/puppet/network/client/master.rb:260:in @run' /usr/lib/ruby/1.8/puppet/util.rb:212:in @benchmark' /usr/lib/ruby/1.8/benchmark.rb:293:in @measure' /usr/lib/ruby/1.8/benchmark.rb:307:in @realtime' /usr/lib/ruby/1.8/puppet/util.rb:211:in @benchmark' /usr/lib/ruby/1.8/puppet/network/client/master.rb:259:in @run' /usr/lib/ruby/1.8/sync.rb:229:in @synchronize' /usr/lib/ruby/1.8/puppet/network/client/master.rb:241:in @run' /usr/sbin/puppetd:42989c89 < AllowGroups adm backup nagios rbramley abridgett --- > AllowGroups adm backup nagios rbramley content.rb: /etc/ssh/sshd_config /usr/lib/ruby/1.8/puppet/type/pfile/content.rb:47:in @insync?' /usr/lib/ruby/1.8/puppet/propertychange.rb:117:in @skip?' /usr/lib/ruby/1.8/puppet/propertychange.rb:72:in @go' /usr/lib/ruby/1.8/puppet/propertychange.rb:109:in @forward' /usr/lib/ruby/1.8/puppet/transaction.rb:119:in @apply_changes' /usr/lib/ruby/1.8/puppet/transaction.rb:111:in @collect' /usr/lib/ruby/1.8/puppet/transaction.rb:111:in @apply_changes' /usr/lib/ruby/1.8/puppet/transaction.rb:83:in @apply' /usr/lib/ruby/1.8/puppet/transaction.rb:240:in @eval_resource' /usr/lib/ruby/1.8/puppet/util.rb:444:in @thinmark' /usr/lib/ruby/1.8/benchmark.rb:293:in @measure' /usr/lib/ruby/1.8/benchmark.rb:307:in @realtime' /usr/lib/ruby/1.8/puppet/util.rb:443:in @thinmark' /usr/lib/ruby/1.8/puppet/transaction.rb:239:in @eval_resource' /usr/lib/ruby/1.8/puppet/transaction.rb:311:in @evaluate' /usr/lib/ruby/1.8/puppet/util.rb:444:in @thinmark' /usr/lib/ruby/1.8/benchmark.rb:293:in @measure' /usr/lib/ruby/1.8/benchmark.rb:307:in @realtime' /usr/lib/ruby/1.8/puppet/util.rb:443:in @thinmark' /usr/lib/ruby/1.8/puppet/transaction.rb:310:in @evaluate' /usr/lib/ruby/1.8/puppet/transaction.rb:304:in @collect' /usr/lib/ruby/1.8/puppet/transaction.rb:304:in @evaluate' /usr/lib/ruby/1.8/puppet/node/catalog.rb:102:in @apply' /usr/lib/ruby/1.8/puppet/network/client/master.rb:260:in @run' /usr/lib/ruby/1.8/puppet/util.rb:212:in @benchmark' /usr/lib/ruby/1.8/benchmark.rb:293:in @measure' /usr/lib/ruby/1.8/benchmark.rb:307:in @realtime' /usr/lib/ruby/1.8/puppet/util.rb:211:in @benchmark' /usr/lib/ruby/1.8/puppet/network/client/master.rb:259:in @run' /usr/lib/ruby/1.8/sync.rb:229:in @synchronize' /usr/lib/ruby/1.8/puppet/network/client/master.rb:241:in @run' /usr/sbin/puppetd:42989c89 < AllowGroups adm backup nagios rbramley abridgett --- > AllowGroups adm backup nagios rbramley
History
Updated by Adrian Bridgett about 1 year ago
Perhaps the best solution here is to change propertychange.rb so that it calls a "diff" function instead if it exists, falling back on the is_to_s, should_to_s method? This also allows types to print changes in a format that suits them - e.g.:
/nfs should be mounted
rather than something like:
/nfs is missing, should be mounted
i.e. files aren't really a special case, although they are the most obvious case
Updated by Adrian Bridgett about 1 year ago
I've been having a look at this, but I don't understand the internals of puppet anything like well enough - there seems to be quite a bit of duplication and/or odd places things are done.
There are explicit change_to_s functions - so I'm not sure exactly why should_to_s and is_to_s is used for example
The attached patch does help matters for me - only tested with --test --noop.
From an end-user point of view, there are two situations - new file, changed file. Each of those has two situations - one where a diff is sensible and one where it's not (either it's huge, or it's not requested, or either the old or new version is binary).
I also think that printing what has changed is something you might want whether --noop is supplied or not, so anything checking --noop is wrong.
This patch removes quite a lot of file printing, however (at least in the noop case) nothing we want AFAICT.
One case this doesn't cope with is source files -
replacing from source puppet://puppet1.example.com/activemq/activemq.xml.mqcentral1.example.com with contents {md5}d648aa055d1faa83fe6be518f5299bbb
in type/file/source.rb currentvalue is an md5sum on entry to the function :(
Anyhow, I hope this is of some use!
Updated by Luke Kanies about 1 year ago
There's a lot of debugging in your patch, so it's a bit unclear to me exactly what it's doing.
The problem itself, I think, is actually pretty straightforward -- you get two diffs when using 'content' but only one when using 'source'. At one point, I think I tracked it down to the 'insync?' method in content being used twice in the course of the transaction, once normally and once when determining if the change should be skipped. This patch makes the problem go away:
diff --git a/lib/puppet/propertychange.rb b/lib/puppet/propertychange.rb
index 35bbede..99b52dc 100644
--- a/lib/puppet/propertychange.rb
+++ b/lib/puppet/propertychange.rb
@@ -114,11 +114,6 @@ module Puppet
end
def skip?
- if @property.insync?(@is)
- @property.info "Already in sync"
- return true
- end
-
if @property.noop
@property.log "is %s, should be %s (noop)" %
[property.is_to_s(@is), property.should_to_s(@should)]
As you can see, it's a very simple patch. It should be safe to apply, but it will require a good bit of testing to make sure that it doesn't change the behaviour of any resource types. Obviously all of the types are supposed to correctly be able to tell whether they're already in sync, but there have been cases where they could not.
Updated by Luke Kanies about 1 year ago
- Assigned to changed from Puppet Community to Andrew Shafer
- Target version changed from 0.25.0 to 0.24.5
- Complexity changed from Unknown to Easy
This is small enough that it should be easy to fix for the next release.
Updated by Luke Kanies 12 months ago
- Status changed from Accepted to Closed
This was fixed as part of #1393.