Facter bug causes puppet to hang

Pull request https://github.com/puppetlabs/facter/pull/174 was committed by upstream in https://github.com/puppetlabs/facter/commit/15b857472a75caf9cd596c2b33d285577382293a. I think it makes sense to SRU this.

This has been targeted in puppet labs for a 2.0.0 release, it would be counter-productive to merge before that, I'll try to get further feedback from puppet labs to try to take the best approach to this.

jamespage: ping, about bug #986973 after talking with upstream they reckon it's not worth fixing as of yet and recommend to wait to facter 2.0.

Marking 'Won't fix' for precise and raising task for quantal so we don't forget.

Quantal won't be shipping facter 2.0.0 as this is intended to support puppet 3.x (which we also won't ship).

Marking 'Won't Fix' for quantal and raising task for r-series.

Just a note that after this was WONTFIX'd, puppet labs moved the upstream bug back from 2.x to 1.7.0. Perhaps a reconsideration is in order?

@Chris Abbey

Thanks for pointing this out. I can't seem to find any corresponding commit related to a fix in 1.7.0 though. I'm looking at https://github.com/puppetlabs/facter/commits/1.7.1/lib/facter/memory.rb for example - given that the original pull request https://github.com/puppetlabs/facter/commit/15b857472a75caf9cd596c2b33d285577382293a involved a change to this file.

I think I've seen this issue myself in Precise. I am running "puppet apply" from a cronjob, and it occasionally hangs, eventually consuming all available memory. I am running the whole thing via the timeout command as a workaround. I think it is reasonable to fix this in Precise if there is a simple, obviously correct fix, but I can't find anything to cherry-pick.

Note that "zombie" has a specific meaning that doesn't apply here.

@Robie Basak

The copy of memory.rb that you linked in your update does contain the fix (removal of "require 'thread'" etc.) I just checked out the tag 1.7.0 and found the commit (https://github.com/puppetlabs/facter/commit/9ff4453b2f0843df887ae1ec2098a4336291df0c). Note: that commit is only for memory.rb. Patching of processor.rb may also necessary.

It'd be great if this were backported to the Ubuntu versions because right now it's necessary for us to maintain our own version with patched memory.rb and processor.rb. The code is also present in quantal and raring (facter 1.6.9), although I cannot say if this bug also affects those distributions.

I just did some digging and found that processor.rb isn't affected in versions 1.6.5 (precise) or 1.6.9. So, commit 9ff4453b2f0843df887ae1ec2098a4336291df0c is the only necessary one to cherry-pick here.

It would be fantastic if we could get memory.rb back ported to Precise, as this is really irritating to need to maintain a separate copy of memory.rb. Without the patched version, we experience many hangs per day with 'puppet apply' running via cron. Surprising to see a hang bug remain for so long with an LTS release.

@Shane

Could you please confirm the patch that you're using on memory.rb, and confirm that this fixes the problem for you?

I've also been seeing 'puppet apply' hanging via cron, but never figured out if this was the issue or if this patch fixed it. I've been working around it with a timeout.

If we can get some confidence in the fix, I'll put this forward as an SRU in Precise.

We'll also need this fixed in Saucy before we can commence an SRU. What is the fix needed in Saucy? Just memory.rb? The same patch? Or processor.rb as well?

Installing the patched version of memory.rb from https://github.com/puppetlabs/facter/commit/9ff4453b2f0843df887ae1ec2098a4336291df0c helped reduce the number of puppet hangs that we experience, but we were still getting hung processes. Attaching strace to a "hung" puppet process showed a constant output of the following lines repeated:

31024 futex(0x7f96be6ed5b0, FUTEX_WAKE_PRIVATE, 1) = 0
31024 futex(0x7f96be6ed584, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME, 231453, {1371588406, 113327589}, ffffffff) = -1 ETIMEDOUT (Connection timed out)
31024 clock_gettime(CLOCK_REALTIME, {1371588406, 113578908}) = 0
31024 futex(0x7f96be6ed5b0, FUTEX_WAKE_PRIVATE, 1) = 0
31024 futex(0x7f96be6ed584, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME, 231455, {1371588406, 123578908}, ffffffff) = -1 ETIMEDOUT (Connection timed out)
31024 clock_gettime(CLOCK_REALTIME, {1371588406, 123789011}) = 0

It turns out that puppet isn't actually hung, but is waiting forever for the futex() call to stop timing out.

Once I copied in the patched processor.rb from the same git changeset linked above, the hang problem stopped occurring for us. So from my perspective, both memory.pp and processor.rb need to be patched in the puppet 1.6.5 (precise) package.

@Shane

Yes - it appears you are correct and I was in error in my previous comment. The source file lib/facter/processor.rb does not make use of Thread::Exclusive (except for Windows), but lib/facter/util/processor.rb does, so it's necessary to patch that file also.

I found a machine exhibiting this problem. On Precise (facter 1.6.5-1ubuntu1), I ran the following experiments:

No patch: "puppet apply" failed 20 out of 100 runs.
Patched both memory.rb and processor.rb: "puppet apply" failed 0 out of 100 runs.
Patched only memory.rb: "puppet apply" failed 23 out of 100 runs.
Pathced only processor.rb: "puppet apply" failed 0 out of 100 runs.

So for me, patching just processor.rb fixed the problem. However, this being a race, it might be that I just wasn't able to trigger a race involving memory.rb in my specific circumstances.

The patch I used was https://github.com/puppetlabs/facter/commit/9ff4453b2f0843df887ae1ec2098a4336291df0c.patch. I can see that at least on Linux, all the affected areas do is parse /proc/meminfo and /proc/cpuinfo in a way that I don't see why any thread exclusive locks would be needed at all. Upstream seemed to agree in https://github.com/puppetlabs/facter/pull/174 and had no explanation for why they were put in either. So I think it's safe to backport all of the patch to Precise. I'll do this next week.

(this appears fixed in Saucy, so just the Precise SRU needed)

This is blocked on in-flight SRU bug 1170325.

Thanks

As discussed with Robie, this upload is being folded with the current one for dual verification.

Hello Jared, or anyone else affected,

Accepted facter into precise-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/facter/1.6.5-1ubuntu1.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

In the ten days before applying the -proposed update (1.6.5-1ubuntu1.2), one of my affected servers had nine failures (I haven't been testing any others). In the ten days since applying the -proposed update, the same server has had no failures at all. I haven't made any other changes in this time.

I still need to verify bug 1170325 by hand since it seems that nobody else will, and then this SRU can land.

tl;dr: I second the verification

Dear everyone,

I have used puppet to maintain a collection of some dozens of Precise servers. This bug, as described, has manifested regularly.

I've enabled the precise-proposed repository on a few test servers and updated facter to 1.6.5-1ubuntu1.2. Based on some hundreds of test runs, the issue is completely resolved.

This bug was fixed in the package facter - 1.6.5-1ubuntu1.2

---------------
facter (1.6.5-1ubuntu1.2) precise; urgency=low

  * d/p/thread-critical-freeze: fix "puppet apply" hang by avoiding
    unnecessary thread critical sections (LP: #986973).
 -- Robie Basak <email address hidden> Tue, 02 Jul 2013 03:21:36 +0000

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.