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.
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. 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. 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.
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 }
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?
--
It's not that I'm afraid to die. I just don't want to be there when it
happens. -- Woody Allen
---------------------------------------------------------------------
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.
