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).
> >>> 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 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 :-)
--
Brice Figureau
Follow the latest Puppet Community evolutions on www.planetpuppet.org!
--
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.
