Markus Roberts <[email protected]> writes:
> On Sun, Apr 4, 2010 at 6:52 AM, Daniel Pittman <[email protected]> wrote:
>> Markus Roberts <[email protected]> writes:
>>
>> >> > I understand the file write to temp/rename pattern, as it is a mean to
>> >> > produce atomic content writing.
>>
>> [...]
>>
>> >> (Oh, and one incidental "future-proof" fix for binary data, setting
>> binmode >> on the file we use to write content to avoid any risk of newline
>> >> translation.) >> > > This of course only applies to the future MS
>> Windows port, as ruby's binmode > flag has no effect on *nix systems.
>>
>> Uh-huh. I put "future-proof" in scare-quotes because it is only a passing
>> fantasy that it does anything other than document the intention for the
>> programmers porting it to Windows. (...and I probably shouldn't have
>> bothered. ;)
>
> It's a frustrating line, I know; you don't want to write things incompatibly
> and yet, when we don't have compatibility, it seem like clutter to bend over
> backwards to "future-proof" them. So how would I do it? Probably I'd add a
> wrapped File#open method that called binmode (it's a method call, BTW, not a
> setting, so you write f.binmode, not f.binmode=blah) on the way in--and we
> might as well call fsync on the way out.
*nod* Not the way I would do it, but a perfectly reasonable approach.
[...]
>> Well, you lost both the old *and* new data, so the only thing that the use
>> of a temporary file got you was the atomic replacement when everything
>> worked right.
>
> Not quite (or at least you're overstating): it only failed to give you
> atomicity when everything went wrong--the perfect storm; if even one of your
> worst-case assumptions had gone the other way we would have been in a known
> state.
That is probably a fair statement.
[...]
> I think, though, that we are in general agreement about the issue, and all
> this is just haggling over the details and hand-waving about the broader
> context.
I think so.
> Only the details turn into code, so rather than arguing phylosophy further,
> let me put some out there:
>
> • MS Windows is a different beast and we should not attempt to solve it
> here; however,
> any code changes we make based on this thread should be clearly marked
> as to intent so they won't get optimized away in a MS Windows
> compatibility pass.
*nod*
> • The problem with not being able to fsync a directory in ruby has been
> hashed over
> for years (the whole read-pointer write-pointer thing), and there are
> too many puppet installations on older versions of ruby to expect much
> on that front. The best I can think of is to shell out and doing
> something too clever by half rather than renaming the files from within
> ruby, however...
I don't see this as important enough to do more than comment on, really. It
would be nice, but is more icing on the cake, really.
[...]
> • The best place to deal with these issues is in the utils, rather than
> cluttering the code with them. For an example of what I'm talking
> about, see secure_open in lib/ puppet/util.rb; I think a wrapper such as
> this to impose an fsync might be a reasonable solution.
Hmmm. I am not sure about that: there are some other issues in the area,
and I should probably go look at things other than the basic fsync safety.
At the moment there is a time-of-check, time-of-use rack that allows an
attacker to truncate and overwrite, any file on the system if puppet manages a
file in a directory they can write to. (Fixed with O_EXCL.)
Given the number of manifests that use /var/tmp or /tmp, that looks like an
issue that most folks out there are not aware of. :/
So, I think that you end up either moving the entire "atomic_replace" process
into Puppet::Util, including the checksum verification, or you end up having
to interweave code in two modules substantially.
Anyway, given the parts I didn't pay attention to before which are also
security risks I might actually go and audit the use of this stuff a bit more
thoroughly and develop some more reasonable patches.
Er, is there a good "getting started for puppet development" guide somewhere?
I failed to find one with the new documentation stuff, and while I used to
have a testable repository for puppet I didn't touch if for a major release
and now I can't make it work. :/
That way I should be able to find stupid things like the binmode method
mistake that are syntax-correct but not actually, y'know, working.
> • There is a tradeoff involved. I haven't done any performance testing of
> fsync on
> modern hardware, but there have been times and places where the cost
> outweighed the benefits. We should also drag Brice into this, as the
> file streaming patch may add twists to the issue we aren't considering.
If it helps, I am at least somewhat familiar with those trade-offs. The place
it is likely to hurt is on Linux systems using ext3 — there, fsync(2) is pretty
much identical to sync(2), which means it can do nasty things to overall
performance.
Mozilla ran into that when they started using sqlite to store data, and
updated that every time you did something with the "omnibar" address bar
thing. Essentially, complaints that killed interactivity across the system,
which are probably fair, because it would fsync ... frequently.
With puppet that should be mitigated: (I assume) replacing a file is an
uncommon operation, so the overall cost should be reasonably low.
On almost any other combination: another OS, another file-system, fsync is a
reasonably low cost operation. fdatasync would be even better, but we take
what we can get, I guess. :)
Daniel
--
✣ Daniel Pittman ✉ [email protected] ☎ +61 401 155 707
♽ made with 100 percent post-consumer electrons
--
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.
