On Mar 15, 2010, at 2:06 AM, Brice Figureau wrote:
On Sun, 2010-03-14 at 15:49 -0700, Luke Kanies wrote:
On Mar 14, 2010, at 1:42 AM, Brice Figureau wrote:
On 14/03/10 08:48, Luke Kanies wrote:
On Mar 13, 2010, at 1:57 PM, Brice Figureau wrote:
On 13/03/10 21:33, Rein Henrichs wrote:
Brice,
I think the "real" fix for the underlying problem is fixing the
regression
bug introduced in the file serving refactor.
Indeed, but I think this patch is close to this.
That said, if this hot patch
works around the 100% CPU usage on recursive chowns bug in the
user list
thread, I think it's desirable to get this into the 0.25.x branch
ASAP.
The real underlying issue is that file metadata terminus doesn't
care
about the file {} checksum settings, it always compute the md5
checksum
of said files. Why? Because this is what is correct for remote
files
metadatas.
This actually shouldn't always need to be true - it's definitely
unlikely that we wouldn't be interested in the remote md5, but it's
at
least possible.
OK, it makes sense, but I have hard time thinking (in the current
puppet
system) when we wouldn't want to have remote metadatas including the
checksum?
*We* wouldn't, but there's someone out there who's using remote file
recursion just for setting owner/mode/etc, I promise. Crazy, but
probably true. (Note: I don't know that this is true, I'm just
estimating it based on experience.)
That being said, if we have a simple fix that only works for local
file serving, I'm perfectly happy with that and we should do it.
I think so too. I'd just appreciate if someone else could test the
current patch (which is available in my github repository at
tickets/0.25.x/2929).
I'll be testing it this week, definitely.
Unfortunately the same code is called in the direct file serving
terminus system, where the checksum should not be computed (it
will be
done by the checksum property).
This patch is not 100% correct (but my minimal testing show that
it
works), and I don't expect it to be merged directly in 0.25.x (on
which
it is based).
Why don't you expect that?
My sentence wasn't complete: it is missing an "as is".
When I wrote this I just had thought that directing the direct file
server to say to the fileset that they are local (and as such don't
have
to compute a checksum) would be a better direction.
I must also add that I did only minimal testing of said patch (ie
tried
only in the lab with various files (ie sourced, local, recursive).
But
the file resource is so vast, I didn't check managing links, copying
files, etc, were still working.
The basic model is sound; the only confusing bits are around how
checksum is used by source and content. That needs to be refactored
anyway (e.g., I somehow broke Puppet's Tripwire-like functionality in
0.25), partially because it can be so hard to track.
I've been thinking that 'checksum' needs the treatment 'source' got -
turned into a parameter instead of a property.
Yes, I think this would be much simpler.
Theoretically,
checksum and content should be isomorphic, as long as you have source
specified (or a filebucket), so checksum should just be a utility
parameter rather than a real property.
That would make sense. Checksum should just be an indication of the
checksum type. And the real checksum would just be driven by either
content or source.
Given that, we should probably
also fix the confusion where the checksum parameter sets the checksum
type but also determines and returns the actual checksum. This is
especially stupid now that we only ever print a file's checksum
rather
than its contents.
That's what made my second attempt at fixing this issue so difficult.
So, seems like we should switch the 'checksum' parameter to just
support the various values (md5 et al), and move all checksum
calculating into utility methods that the rest of the file resource
uses. This should hopefully clean everything up enough that it
actually becomes maintainable.
Then our tripwire-like behaviour (which is that it passively monitors
files, logging when a file's contents change) comes from tracking the
content rather than the checksum:
file { "/tmp/foobar": check => content }
I'm not sure about this syntax: do you mean we should add a new
property
(ie check) to reintroduce tripwire functionality?
This is actually a metaparam that's existed since the earliest days of
Puppet - its intent is that Puppet will not manage those properties
but will instead just track changes to them over time and alert when
changes happen.
This doesn't actually work right now, but I haven't had the time to
figure out why.
It basically does two things:
* allow setting checksum => none
* make sure file metadata doesn't compute anything if checksum is
none
(but only for the direct file server terminus).
A more correct patch would be to prevent checksums to be
computed in
local file metadata search.
I suppose that would be sufficient - just have a 'local' flag,
effectively, and an additional flag saying "i don't care about the
checksum", but... then the latter becomes a superset of the former,
so
skip the former, use what you have here, and move on.
Or am I missing something?
I don't think so, but I'm quite ignorant about the interaction
between
the checksum file property and the rest of the system, so I want to
make
sure I'm not introducing any new regressions.
Ok.
Do you want to attempt to tackle the above refactor, would you like
to
collaborate with me on it, or would you prefer to skip it for now and
just get this fix in?
Frankly I'd prefer we fix the issue in 0.25.x with this patch (granted
someone else can test it with success).
I don't really have lot of time until June for puppet dev (except
those
occasional fixes), but I can at least try to start something. If you
want to do it, then go ahead, that will certainly be much more
effective
than my procrastination :-)
Ok. I'll get this patch tested, and if I have some airplane time or
something see what a full checksum refactor would look like. The
'source' refactor ended up being relatively easy in the end and it had
a huge impact on maintainability.
--
The world is round; it has no point.
-- Adrienne E. Gusoff
---------------------------------------------------------------------
Luke Kanies -|- http://reductivelabs.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.
