These are chat archives for ManageIQ/manageiq/rails5

10th
May 2016
Joe Rafaniello
@jrafanie
May 10 2016 15:49
Ok, after working with @carbonin much of yesterday on the connection pool refactor, we have a failing test on rails master/5-0-stable that seems to indicate either there was a regression or we're not doing it right. I put a gist together that hopefully narrows the problem down. https://gist.github.com/jrafanie/a27f713e791875a45a007aebb013e539. @matthewd, if you're available, what do yo think?
Note, we worked around the issue for now on manageiq but are now tracking down if that workaround should be needed
Keenan Brock
@kbrock
May 10 2016 15:50
I think the remove_connection is confusing. WHICH are we removing?
Joe Rafaniello
@jrafanie
May 10 2016 15:51
the class's connection pool
Jason Frey
@Fryguy
May 10 2016 15:51
remove_connection does now take a name...not sure what the default is
Keenan Brock
@kbrock
May 10 2016 15:51
+1
so with that gone
in theory, it should use "primary"
Jason Frey
@Fryguy
May 10 2016 15:51
I think, but not sure
Nick Carboni
@carbonin
May 10 2016 15:51
@Fryguy The default is "primary" but the call to establish_connection creates a pool keyed by the class name
Keenan Brock
@kbrock
May 10 2016 15:52
how does the class know that klass2.establish_connection should not use the new connection params?
Joe Rafaniello
@jrafanie
May 10 2016 15:52
and once you go custom pool, you can't go back to the primary one
Keenan Brock
@kbrock
May 10 2016 15:52
+1
THAT is the bug - right?
Nick Carboni
@carbonin
May 10 2016 15:52
Then remove_connection removes the connection pool keyed by the class name but does not reconnect the the "primary" pool
Keenan Brock
@kbrock
May 10 2016 15:53
so it is as if we want to connect a model to a temporary pool
Nick Carboni
@carbonin
May 10 2016 15:53
Exactly
Keenan Brock
@kbrock
May 10 2016 15:53
but we want all others to use a standard connection
I mean
if we could have all connections use a different pool
even when we have the establish_connection in effect, then that would be good / even better?
Jason Frey
@Fryguy
May 10 2016 15:54
So, MiqRegionRemote in particular is weird because we derive from ApplicationRecord for the connection methods
Joe Rafaniello
@jrafanie
May 10 2016 15:54
well, yes, establish_connection sets @connection_specification_name and so the "use primary pool if unset logic isn't used", see here and here
Jason Frey
@Fryguy
May 10 2016 15:54
but we don't really want or need the "default" connection we get
we always want to use a remote connection that we are told about at run-time
Keenan Brock
@kbrock
May 10 2016 15:54
it is kinda a race condition - it would be best if we could say "connect using this temporary secondary credentials" but only this connection
Nick Carboni
@carbonin
May 10 2016 15:55
The issue is that we have no connection pool after calling remove_connection
because it looks for a pool with the name as the class name
Joe Rafaniello
@jrafanie
May 10 2016 15:55
because of :point_up: May 10, 2016 11:54 AM
Keenan Brock
@kbrock
May 10 2016 15:56
shouldn't the second establish_connection reset to primary?
Joe Rafaniello
@jrafanie
May 10 2016 15:56
you shouldn't need to establish_connection a second time
Nick Carboni
@carbonin
May 10 2016 15:56
No it creates a new pool with the class name
We can never get back to "primary" unless we set the name explicitly
Joe Rafaniello
@jrafanie
May 10 2016 15:58
yeah, see the two workarounds, lines 30 and 31 in the gist
Keenan Brock
@kbrock
May 10 2016 15:58
yea, that was the talk of defining a function for that and returning a value ( or a thread local value)
the main reason why establish_connection fixes it is because of the implicit klass2.connection_specification_name = "primary"?
do we do anything AR with this connection?
Nick Carboni
@carbonin
May 10 2016 16:00
The additional establish_connection creates a new pool with the class name and leaves the pool with the name "primary" around
Keenan Brock
@kbrock
May 10 2016 16:00
would it make sense for us to do AR direct, and not use the model abstraction?
@carbonin ooh - I don't like that
why are we using rails to do AR connections for us?
Nick Carboni
@carbonin
May 10 2016 16:01
Yeah, that's why we went this route ManageIQ/manageiq#8560
Joe Rafaniello
@jrafanie
May 10 2016 16:01
@kbrock, I don't think we're discussing going forward. This is "is this a regression in rails"?
Keenan Brock
@kbrock
May 10 2016 16:01
why not just create an AR connection? would it be that much work?
@carbonin would it make sense to go further? establish_connection has that remove_connection in it
we're not really using the model's connection_pool / handler from what I can tell
so we're plugging into this caching / pooling layer that seems just poised to bite us
Jason Frey
@Fryguy
May 10 2016 16:04
Last I talked to @matthewd (before this rewrite), the contract is use establish_connection/remove_connection
so if that pattern isn't working, then I consider it a Rails bug
Keenan Brock
@kbrock
May 10 2016 16:05
that contract is if you want to use AR::Base methods
but we don't really
we just want a connection
Jason Frey
@Fryguy
May 10 2016 16:05
maybe
that's where it's kind of a gray area
Keenan Brock
@kbrock
May 10 2016 16:05
maybe we do
Jason Frey
@Fryguy
May 10 2016 16:06
I'm still wondering if this is also just a bug in our tests in that the tests rely on it getting back to the "primary"
Keenan Brock
@kbrock
May 10 2016 16:06
using AR to manage a pool that we are trying to swap out the instant this is done, and then just using the pool to fetch the connection seems odd
Jason Frey
@Fryguy
May 10 2016 16:06
so we can verify before/after behavior
Keenan Brock
@kbrock
May 10 2016 16:06
yea
Jason Frey
@Fryguy
May 10 2016 16:07
so, if we change the test to verify that the expected pool is gone, is that Good Enough™ ?
I'm not really sure
Nick Carboni
@carbonin
May 10 2016 16:07
I think we do. For example, we are using that connection in https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_region.rb#L123 which uses tables, primary_key, and columns
(need AR methods)
Jason Frey
@Fryguy
May 10 2016 16:07
AR methods, but not "model" stuff, right?
Joe Rafaniello
@jrafanie
May 10 2016 16:08
yeah, connection methods
Not model
Nick Carboni
@carbonin
May 10 2016 16:08
I use MiqRegionRemote.region_number_from_sequence once ...
And expect that to be for the remote connection
There are other ways to do that though
if we want to change
Keenan Brock
@kbrock
May 10 2016 16:10
the way we are modifying the model's global pool makes me nervious. changing global params like this. and then closing to clean up. are we cleaning up our own pool, or another one?
granted - very low odds. but we are dealing with global data that is not meant to just be changed haphazardly.
Nick Carboni
@carbonin
May 10 2016 16:11
Who is "our" in that case?
I think the pool belongs to the model
Keenan Brock
@kbrock
May 10 2016 16:11
+1
pool in the model
ooh
no
we change the model's pool
Nick Carboni
@carbonin
May 10 2016 16:11
Right
Keenan Brock
@kbrock
May 10 2016 16:11
then we take the connection and do stuff
if anyone else does a remote call
Nick Carboni
@carbonin
May 10 2016 16:12
So yeah not threadsafe
Keenan Brock
@kbrock
May 10 2016 16:12
they will change the pool information
we already have a connection, so not a problem
but the remove_connection
not sure who gets removed
probably not going to happen
Nick Carboni
@carbonin
May 10 2016 16:12
What will happen is the pool keyed by the model name will get replaced with a new pool when the second establish_connection is called
Keenan Brock
@kbrock
May 10 2016 16:13
yes
which is no biggie
but the remove
if the first process calls that
which is more probable
the second connection (linked to the pool)
will that get collected?
(while the second process is using it)
Nick Carboni
@carbonin
May 10 2016 16:13
Will remove the pool now keyed by the model name and the second call will blow up
Keenan Brock
@kbrock
May 10 2016 16:14
the remove blowing up
eh
but will it expire the connection that the second one is using?
so the transaction "throw away this region" die in the middle?
a few of those methods sound pretty hard core. destroy_entire_region
Joe Rafaniello
@jrafanie
May 10 2016 16:15
So, is what we were doing before in with_remote_connection a valid use case for swapping the connection even if it's not threadsafe? It's a bit late but I'd rather not flood Matthew's gitter with non-rails discussion... we can do that in yet another room
Nick Carboni
@carbonin
May 10 2016 16:17
What happens to the connection will likely depend on what https://github.com/arthurnn/rails/blob/master/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb#L585 does. And yeah, that is the main question @jrafanie
Jason Frey
@Fryguy
May 10 2016 16:17

So, is what we were doing before in with_remote_connection a valid use case for swapping the connection even if it's not threadsafe

We don't do those things in threads, so it should be fine

Joe Rafaniello
@jrafanie
May 10 2016 16:18
Yeah, at this point, I'd like to just answer the question "is this a regression in rails or are we doing it wrong"
Jason Frey
@Fryguy
May 10 2016 16:19
or both :P
Keenan Brock
@kbrock
May 10 2016 16:19
yea both
the assumed use case is "connect to 2 databases" swap between them
we are using rails to generate connections to random databases
the model is not made for that
Jason Frey
@Fryguy
May 10 2016 16:20
no, the use case is "connect to a different database, then throw it away"
the test cares about going back to the original in order to verify
Keenan Brock
@kbrock
May 10 2016 16:21
ok, well, the use case is doing it wrong
MiqRegionRemote.region_number_from_sequence seems to be legit
Jason Frey
@Fryguy
May 10 2016 16:21
but Rails may still have a regression anyway
Keenan Brock
@kbrock
May 10 2016 16:21
but that is not throwing it away
yes
Joe Rafaniello
@jrafanie
May 10 2016 16:21
So, it seemsremove_connection is failing to allow the default "primary" pool to be used once you configure a custom pool
Keenan Brock
@kbrock
May 10 2016 16:21
agreed, think rails has a regression
but what is remove_connection?
it says "I'm done with this connection for this request"
not "reset my db credentials"
Joe Rafaniello
@jrafanie
May 10 2016 16:23
Its's an unfortunate name for a method that removes the connection pool under the covers (disconnecting any connections in the pool)
Keenan Brock
@kbrock
May 10 2016 16:24
yes, my interpretation for that method is probably off
Jason Frey
@Fryguy
May 10 2016 16:25
it is literally the opposite of establish_connection
Keenan Brock
@kbrock
May 10 2016 16:25
can we establish a connection in another way?
for everyone but replication, we just need a connection. right?
Joe Rafaniello
@jrafanie
May 10 2016 16:26

it is literally the opposite of establish_connection

In fact, you can feed the return from remove_connection directly into establish_connection

Jason Frey
@Fryguy
May 10 2016 16:26
@jrafanie yes, for "reconnect"
Nick Carboni
@carbonin
May 10 2016 16:26
It seems more likely that https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_handling.rb#L97 needs to allow for any model to "fallback" to the "primary" spec name
Jason Frey
@Fryguy
May 10 2016 16:26
@kbrock No, establish_connection/remove_connection is the expected API for establishing a connection as far as I know
Keenan Brock
@kbrock
May 10 2016 16:27
@carbonin yes - that is a regression / bug
@Fryguy that is the api to hook up a connection to a model
but are we trying to do that?
ugh
nick - that code has a race condition bug :(
Jason Frey
@Fryguy
May 10 2016 16:28

that is the api to hook up a connection to a model

That is a good point...is it?

Keenan Brock
@kbrock
May 10 2016 16:29
there must be an api to say "get a connection to the database"
not "lets establish a connection so anyone accessing this model will use this connection"
we want a "last man out"
but that is determined when you leave, not when you come in
but as jason said, maybe we don't care about changing this connection
Matthew Draper
@matthewd
May 10 2016 19:28

is this a regression in rails or are we doing it wrong

+1 both

Jason Frey
@Fryguy
May 10 2016 19:29
Oh hai :wave:
Chris Arcand
@chrisarcand
May 10 2016 19:29
zing
Joe Rafaniello
@jrafanie
May 10 2016 19:29
Yes, both is the winner
Jason Frey
@Fryguy
May 10 2016 19:29
great! now what? :)
Matthew Draper
@matthewd
May 10 2016 19:29
establish_connection / remove_connection aren't thread safe, so really shouldn't be used the way MIQ is using them — they're supposed to be ~once-off calls to tell the model how to connect
Jason Frey
@Fryguy
May 10 2016 19:30
luckily we aren't using them in threads
Matthew Draper
@matthewd
May 10 2016 19:30
But, MIQ gets away with what it's doing because there is only a single thread in play, and it's guaranteed to put the connection back again (ensure) before anything else notices
So, that remove_connection isn't enough to get the model back to using whatever its ancestor does, is a bug
.. but ultimately, MIQ should be skipping establish_connection, and going a level further down, to just make a connection without ever assigning it to a model
Jason Frey
@Fryguy
May 10 2016 19:32
yeah, I think that's what we really want...is there an API for that (that's not considered Rails-internal-only)?
Matthew Draper
@matthewd
May 10 2016 19:34
I don't think so, but that sounds fixable
Jason Frey
@Fryguy
May 10 2016 19:34
cool
yeah, we would need a way to get a connection, and then a proper way to close the connection
though, we do have some code that would need to change to accept a connection (and not rely on .connection returning the right thing)
Oleg Barenboim
@chessbyte
May 10 2016 19:35
@Fryguy can we just go raw SQL?
Jason Frey
@Fryguy
May 10 2016 19:36
yeah, probably if necessary, though I'd prefer to avoid the duplication
most of what we do with the connection is raw SQL anyway, I think
Joe Rafaniello
@jrafanie
May 10 2016 19:36
Yeah
Matthew Draper
@matthewd
May 10 2016 19:37
If it worked with anonymous classes, you could create an anonymous subclass, then establish_connection on that (to avoid the thread-safety issue with manipulating a shared class instance)
Jason Frey
@Fryguy
May 10 2016 19:37
there are a few other places that we use a model (e.g. git grep ModelWithNoBackingTable), but those are transient, so I'm not too concerned, and that could probably still use establish_connection
@matthewd Yeah I was thinking that as well...shouldn't be too hard to change to an anonymous subclass
Joe Rafaniello
@jrafanie
May 10 2016 19:38
yeah, it wants a class name in some cases and a table_name in others
Jason Frey
@Fryguy
May 10 2016 19:38
although I think the new Rails connection refactoring stuff bails on anonymous classes ¯\_(ツ)_/¯
Joe Rafaniello
@jrafanie
May 10 2016 19:39
yes, because of the missing name
Matthew Draper
@matthewd
May 10 2016 19:39
Yeah, that was the "if it worked" :)
Jason Frey
@Fryguy
May 10 2016 19:39
haha
thanks for the insights @matthewd
Matthew Draper
@matthewd
May 10 2016 19:39
But I suspect it could be convinced to do so
Jason Frey
@Fryguy
May 10 2016 19:40
what are the next steps for us...should @jrafanie open an issue wrt the regression?
I can't think of anything else on the Rails side immediately
Matthew Draper
@matthewd
May 10 2016 19:40
Thanks for the report! I was nervous about letting it in so late, but it seemed safe. :neutral_face:
Joe Rafaniello
@jrafanie
May 10 2016 19:40
not sure if you saw the gist in this wall of words
Jason Frey
@Fryguy
May 10 2016 19:40
yeah, I was surprised it came in right before the branch cut
Joe Rafaniello
@jrafanie
May 10 2016 19:41
no gitter, don't expand a gist that's more than 10 lines, it's not very helpful
Matthew Draper
@matthewd
May 10 2016 19:42
Yeah, you should be able to use the AR repro script thing to.. ah perfect: do exactly what that gist does :)
Joe Rafaniello
@jrafanie
May 10 2016 19:43
ok, I'll do the AR repro thing too
FYI, this was our workaround to get us green: ManageIQ/manageiq#8560
Matthew Draper
@matthewd
May 10 2016 19:44
Just the test should be fine, as you've already written it that way
Joe Rafaniello
@jrafanie
May 10 2016 19:44
Ok, thanks @matthewd
Matthew Draper
@matthewd
May 10 2016 19:45
(esp as we already know where the regression was… 'real' tests are harder to move around when bisecting etc)
Joe Rafaniello
@jrafanie
May 10 2016 20:13
Thanks @matthewd, rails/rails#24959, feel free to throw :tomato:
Matthew Draper
@matthewd
May 10 2016 20:15
Thanks! Arthur said he'll have a look this evening.
I haven't reread the implementation, but it sounds like it should be an easy fix :+1:
Joe Rafaniello
@jrafanie
May 10 2016 20:15
Great, no rush. We know how to get around it for now.