[PATCH/puppet 1/1] Fixing #2764 stale connections on ActiveRecord 2.1

[Jesse Wolfe]

Until recent releases, ActiveRecord did not automatically recover from
dropped connections to MySQL. puppetmasterd potentially exposes this by
attempting to reuse the same connection for clients after the first.

This patch asks ActiveRecord to check and reestablish the connection
whenever the model is retrieved from Puppet::Indirector::ActiveRecord.
This fixes the reported error, but I suspect that we cannot completely
guarantee that we'll never reach a stale MySQL connection without
patching or upgrading ActiveRecord itself.

I don't have a unit test for this because it doesn't change any behavior
of Puppet. Suggestions on how to automate a test of this bug/fix would
be appreciated.

Signed-off-by: Jesse Wolfe <jes...@gmail.com>
---
lib/puppet/indirector/active_record.rb | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lib/puppet/indirector/active_record.rb b/lib/puppet/indirector/active_record.rb
index 531109a..c6031c8 100644
--- a/lib/puppet/indirector/active_record.rb
+++ b/lib/puppet/indirector/active_record.rb
@@ -10,7 +10,9 @@ class Puppet::Indirector::ActiveRecord < Puppet::Indirector::Terminus
end

def ar_model
- self.class.ar_model
+ ar_model = self.class.ar_model
+ ar_model.connection.verify!(30)
+ ar_model
end

def initialize
--
1.6.3.3

[Markus Roberts]

I am not an ActiveRecord wizard, but I have a few (possibly misplaced) concerns about this approach.

-        self.class.ar_model
+        ar_model = self.class.ar_model
+        ar_model.connection.verify!(30)

1) Minor, probably just my ignorance--what does the 30 do?  Is it a time-value (e.g. 30 seconds or...?)

2) More serious: It's my understanding that the connection verify logic isn't thread safe and could mess things up if there are other threads using the connection.

3) That brings up an interesting question (sorry I didn't think of this when we were talking yesterday); from the description on the ticket it sounds as if the problem might be unmoderated connection sharing between threads rather than timeouts.  Did you look into that?

[Jesse A Wolfe]

1) Minor, probably just my ignorance--what does the 30 do?  Is it a time-value (e.g. 30 seconds or...?)

On some versions of ActiveRecord, it's a time-value - don't run this potentially expensive test again if it's been less than N seconds. On newer ones, it's ignored.

3) That brings up an interesting question (sorry I didn't think of this when we were talking yesterday); from the description on the ticket it sounds as if the problem might be unmoderated connection sharing between threads rather than timeouts.  Did you look into that?

Well, there may be more than one bug in there. I know that this obvious non-threading bug exists:
1) PuppetMasterD starts
2) PuppetD #1 connects
3) restart MySQL server (or let connection time out)
4) PuppetD #2 connects
5) "MySQL server has gone away" appears

 

2) More serious: It's my understanding that the connection verify logic isn't thread safe and could mess things up if there are other threads using the connection.

Honestly, I'm not certain - how did you come to that understanding? But that reminds me, ActiveRecord 2.1 has an "allow_concurrency" setting - should we be setting that?

[Markus Roberts]

>> 1) Minor, probably just my ignorance--what does the 30 do?  Is it a time-value (e.g. 30 seconds or...?)
>
> On some versions of ActiveRecord, it's a time-value - don't run this potentially expensive test again if it's been less than N seconds. On newer ones, it's ignored.

So yeah, 30 would seem a reasonable if the issue we're concerned about is connection timeout and not connection concurrency.

>> 3) That brings up an interesting question (sorry I didn't think of this when we were talking yesterday);
>> from the description on the ticket it sounds as if the problem might be unmoderated connection sharing
>> between threads rather than timeouts.  Did you look into that?

> Well, there may be more than one bug in there. I know that this obvious non-threading bug exists:
> 1) PuppetMasterD starts
> 2) PuppetD #1 connects
> 3) restart MySQL server (or let connection time out)
> 4) PuppetD #2 connects
> 5) "MySQL server has gone away" appears

My present thinking, from the wording on the ticket, is that any subsequent connections are failing,not just ones after a long wait--and that sounds more like a concurrency issue than a timeout problem.  But ultimately I'm just speculating; the description on the ticket could be read either way.

>> 2) More serious: It's my understanding that the connection verify logic isn't thread safe
>> and could mess things up if there are other threads using the connection.

> Honestly, I'm not certain - how did you come to that understanding? But that reminds me,
> ActiveRecord 2.1 has an "allow_concurrency" setting - should we be setting that?

As I said (and should repeat periodically) I'm not an AR guru.  I think my impression comes from various discussions of the closely related "verify_active_connections!" such as this:

> It can only be used if you can call it at a time when you know all threads will NOT be making
> any queries over their DB connection. The way it verifies if the connection is active or not is
> by making a query. So if you have two threads trying to make two queries over the same
> connection you could get some weird behavior.

 from http://coderrr.wordpress.com/2009/01/08/activerecord-threading-issues-and-resolutions/#comments and other similar discussions.

"allow_concurrency" may be worth following up on as well.

Given all that, perhaps putting your patch up and seeing if Frank can test it might be the best solution.  If it fixes the problem for him, fine.  If not, look into the concurrency question. 

If anyone lurking on this thread can replicate the problem that could be a big hlp as well.

-- Markus

[Luke Kanies]

On Nov 6, 2009, at 12:00 PM, Markus Roberts wrote:

> "allow_concurrency" may be worth following up on as well.

I'm nearly positive we took this out because we were getting warnings
from Rails saying it had been deprecated.

--
The people who are regarded as moral luminaries are those who forego
ordinary pleasures themselves and find compensation in interfering
with the pleasures of others. -- Bertrand Russell
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

[Jesse A Wolfe]

On Fri, Nov 6, 2009 at 5:52 PM, Luke Kanies <lu...@madstop.com> wrote:

 

> "allow_concurrency" may be worth following up on as well.

I'm nearly positive we took this out because we were getting warnings
from Rails saying it had been deprecated.

It is indeed deprecated... as of ActiveRecord 2.2.x
This ticket was filed for AR 2.1.x, and I was under the impression Puppet supported that - I see now that the wiki ( http://reductivelabs.com/trac/puppet/wiki/UsingStoredConfiguration ) says it requires at least 2.2.2 . If that's true, then let's withdraw this patch and ask people to upgrade ActiveRecord.
 
~
Jesse Wolfe

[Markus Roberts]

> "allow_concurrency" may be worth following up on as well.

I'm nearly positive we took this out because we were getting warnings
from Rails saying it had been deprecated.

It is indeed deprecated... as of ActiveRecord 2.2.x
This ticket was filed for AR 2.1.x, and I was under the impression Puppet supported that - I see now that the wiki ( http://reductivelabs.com/trac/puppet/wiki/UsingStoredConfiguration ) says it requires at least 2.2.2 . If that's true, then let's withdraw this patch and ask people to upgrade ActiveRecord.

Don't forget to add a note to that effect to the ticket and change it from "Investigating" to "Rejected."

[Todd Zullinger]

Markus Roberts wrote:
>> It is indeed deprecated... as of ActiveRecord 2.2.x This ticket was
>> filed for AR 2.1.x, and I was under the impression Puppet supported
>> that - I see now that the wiki (
>> http://reductivelabs.com/trac/puppet/wiki/UsingStoredConfiguration
>> ) says it requires at least 2.2.2 . If that's true, then let's
>> withdraw this patch and ask people to upgrade ActiveRecord.
>
> Don't forget to add a note to that effect to the ticket and change
> it from "Investigating" to "Rejected."

Is the wiki really authoritative here? I thought that 2.1 was still
supported based on Luke's change in 1a5c5b3 (Fixing #2508 - removing
mention of ActiveRecord 2.3), which changed:

- raise "StoreConfigs not supported without ActiveRecord 2.3" unless Puppet.features.rails?
+ raise "StoreConfigs not supported without ActiveRecord 2.1 or higher" unless Puppet.features.rails?

If the wiki is accurate and 0.24.8 requires <= 2.2.2 and 0.25.x
requires >= 2.2.2, it seems like it will be difficult to support both
Fedora and RHEL/CentOS, as Fedora is at 2.3.4 and EPEL is at 2.1.1.

For distribution packagers, asking folks to install via gem is not an
option. And if EPEL can only support 0.24.8, updating Fedora to
0.25.1 gets fun, as many users likely have the puppetmaster on
RHEL/CentOS.

Please forgive me if I'm misunderstanding.

--
Todd OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The most overlooked advantage to owning a computer is that if they
foul up, there's no law against whacking them around a little.
-- Eric Porterfield

[Ohad Levy]

Maybe its time to ping David to upgrade rails... ?

[Markus Roberts]

>>> that - I see now that the wiki (

>> http://reductivelabs.com/trac/puppet/wiki/UsingStoredConfiguration
>> ) says it requires at least 2.2.2 . If that's true, then let's
>> withdraw this patch and ask people to upgrade ActiveRecord.
>
> Don't forget to add a note to that effect to the ticket and change
> it from "Investigating" to "Rejected."

Is the wiki really authoritative here?  I thought that 2.1 was still
supported based on Luke's change in 1a5c5b3 (Fixing #2508 - removing
mention of ActiveRecord 2.3), which changed:

-                    raise "StoreConfigs not supported without ActiveRecord 2.3" unless Puppet.features.rails?
+                    raise "StoreConfigs not supported without ActiveRecord 2.1 or higher" unless Puppet.features.rails?

Please forgive me if I'm misunderstanding.

No worries.  I was under the same impression.  And note that the comment that kicked off this sub-thread is qualified "If that is true, then..."

So perhaps we should address that head on.  Is Rails 2.1 supported with 0.25.x?

-- Markus

[Brice Figureau]

Right now, no (which is the bug in question that this whole thread is
trying to address).

Should it be supported, I'd say yes, since this is a regression from
0.24.8 and it's not in the release notes.

Of course the workaround is to upgrade rails to 2.2 or 2.3, it's just
that it generates support request, and/or request rails >= 2.2 in the
rails feature.

--
Brice Figureau
My Blog: http://www.masterzen.fr/

[Luke Kanies]

I thought we were compatible, but I guess having removed this line
(which got rid of a warning with newer versions of Rails) we've hurt
performance on older versions.

I'm comfortable adding it back in, but it should be done in a way
that's version-sensitive.

And ftr, the reason I added 2.1 support back in was 1) it seemed to
actually work for everyone and 2) it's quite difficult to use Puppet
to upgrade Rails without having multiple versions supported at once.

--
You wake me up early in the morning to tell me I am right? Please
wait until I am wrong. -- Johann von Neumann

[Markus Roberts]

> So perhaps we should address that head on.  Is Rails 2.1 supported with
> 0.25.x?

Right now, no (which is the bug in question that this whole thread is
trying to address).

Should it be supported, I'd say yes, since this is a regression from
0.24.8 and it's not in the release notes.

*laugh* So I may be more cynical, but to me "supported" just means "it's supposed to work and we'll take steps to fix it if it turns out otherwise."  Things sometimes work in circumstances that aren't supported (you're lucky) or fail in circumstances where they are supported (you open a ticket).

-- Markus

[Todd Zullinger]

Ohad Levy wrote:
> Maybe its time to ping David to upgrade rails... ?

I don't think that's likely to happen in EPEL, or even within a
released version of Fedora (though only the soon to be EOL Fedora 10
has rails < 2.2.2).

In just the past month or so, a security bug in ActiveSupport was
fixed by updating from 2.3.2 to 2.3.3 and this exposed a problem in
that rails apps hardcode the version by default (why they do this, I
don't think I want to know ;).

There was much discussion of this in the bug report, starting around:

https://bugzilla.redhat.com/show_bug.cgi?id=520843

In the end, the update was reverted to not break backwards
compatibility on released versions. I would suspect the same argument
holds for not updating the ActiveRecord package in Fedora and EPEL?

If so, I think it would mean that bumping the required version to
2.2.2 prevents us from pushing 0.25.1 to EPEL. :/ But maybe David or
others know differently -- I surely don't know the rails stack well.

But I can understand the pickle that trying to support older versions
puts everyone in. (I do wish that the rails folks wouldn't make
things so ...interesting.)

--
Todd OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Going to trial with a lawyer who considers your whole life-style a
Crime in Progress is not a happy prospect.
-- Hunter S. Thompson

[Markus Roberts]

But I can understand the pickle that trying to support older versions
puts everyone in.  (I do wish that the rails folks wouldn't make
things so ...interesting.)

+10

[Jesse Wolfe]

Suprisingly, I found that setting allow_concurrency made the
"MySQL server has gone away" stop occuring even if the MySQL server
drops connections.
This may be the only change needed to restore compatibility with
ActiveRecord 2.1.x

Signed-off-by: Jesse Wolfe <jes...@gmail.com>
---

lib/puppet/rails.rb | 4 ++++
spec/unit/rails.rb | 11 +++++++++++
2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/lib/puppet/rails.rb b/lib/puppet/rails.rb
index fc8eacd..87f1bb1 100644
--- a/lib/puppet/rails.rb
+++ b/lib/puppet/rails.rb
@@ -22,6 +22,10 @@ module Puppet::Rails
ActiveRecord::Base.logger.level = Logger::DEBUG
end

+ if (::ActiveRecord::VERSION::MAJOR == 2 and ::ActiveRecord::VERSION::MINOR <= 1)
+ ActiveRecord::Base.allow_concurrency = true
+ end
+
ActiveRecord::Base.verify_active_connections!

begin
diff --git a/spec/unit/rails.rb b/spec/unit/rails.rb
index d98c887..6dee55b 100755
--- a/spec/unit/rails.rb
+++ b/spec/unit/rails.rb
@@ -39,6 +39,7 @@ describe Puppet::Rails, "when initializing any connection" do
ActiveRecord::Base.stubs(:logger).returns(logger)
logger.expects(:level=).with(Logger::DEBUG)

+ ActiveRecord::Base.stubs(:allow_concurrency=)
ActiveRecord::Base.stubs(:verify_active_connections!)
ActiveRecord::Base.stubs(:establish_connection)
Puppet::Rails.stubs(:database_arguments)
@@ -46,6 +47,16 @@ describe Puppet::Rails, "when initializing any connection" do
Puppet::Rails.connect
end

+ describe "on ActiveRecord 2.1.x" do
+ confine "ActiveRecord 2.1.x" => (::ActiveRecord::VERSION::MAJOR == 2 and ::ActiveRecord::VERSION::MINOR <= 1)
+
+ it "should set ActiveRecord::Base.allow_concurrency" do
+ ActiveRecord::Base.expects(:allow_concurrency=).with(true)
+
+ Puppet::Rails.connect
+ end
+ end
+
it "should call ActiveRecord::Base.verify_active_connections!" do
ActiveRecord::Base.expects(:verify_active_connections!)

--
1.6.3.3

[Jesse A Wolfe]

Wish I had spellcheck in VIM.

"Surprisingly". "occurring".

[Markus Roberts]

+1

On Sun, Nov 8, 2009 at 4:01 PM, Jesse Wolfe <jes...@gmail.com> wrote:

[Markus Roberts]

> Wish I had spellcheck in VIM.
> "Surprisingly". "occurring".

:set spell

and

:set nospell

at least with reasonably modern vims.

[Luke Kanies]

I seem to remember something about these constants not always existing
in different versions of Rails. I assume they're in every version
since 2.1, but this might fail in interesting and unhelpful ways in
older releases. It might be worth throwing a 'defined?' in there,
although you don't have to take my word for it that this is a real
problem.

--
SELF-EVIDENT, adj. Evident to one's self and to nobody else.
-- Ambrose Bierce

[Jesse A Wolfe]

lib/puppet/feature/rails.rb already has a check to make sure we've got at least Rails 2.1, and both Puppet::Rails and the specs check Puppet.features.rails? before running this code.

[Luke Kanies]

Ah, ok; that should be fine, then.

--
Honest criticism is hard to take, particularly from a relative, a
friend, an acquaintance, or a stranger. -- Franklin P. Jones