These are chat archives for ManageIQ/manageiq/rails5

9th
May 2016
Jason Frey
@Fryguy
May 09 2016 14:00
5! 5! 5! 5!
$5 FOOTLONG!
Joe Rafaniello
@jrafanie
May 09 2016 14:00
:wink:
Keenan Brock
@kbrock
May 09 2016 14:00
can I create a "pool" aka - temp database.yml entry that points to the credentials?
so it is as if I'm creating an entry in the text file
Julian Cheal
@juliancheal
May 09 2016 14:01
@Fryguy haha
Keenan Brock
@kbrock
May 09 2016 14:01
it will only be in memory
Oleg Barenboim
@chessbyte
May 09 2016 14:01
hate eating half-cooked food -- Rails5 feels half-cooked
Keenan Brock
@kbrock
May 09 2016 14:01
retrospective please
I want to fix this now
Jason Frey
@Fryguy
May 09 2016 14:01
I guess whatever works but I want to understand the change in the PR
Keenan Brock
@kbrock
May 09 2016 14:01
ok
Joe Rafaniello
@jrafanie
May 09 2016 14:01
can someone summarize the problem?
Jason Frey
@Fryguy
May 09 2016 14:01
one thing missing is the "remove_connection" bit
Julian Cheal
@juliancheal
May 09 2016 14:01
So I found out that RC1 is going to be replaced with RC2
Keenan Brock
@kbrock
May 09 2016 14:01
so rails says database passwords belong in database.yml
Joe Rafaniello
@jrafanie
May 09 2016 14:01
I think this breaks lots of thigns, not just us
Keenan Brock
@kbrock
May 09 2016 14:01
before, there was no good way to do this
Julian Cheal
@juliancheal
May 09 2016 14:02
As in asap.
Jason Frey
@Fryguy
May 09 2016 14:02
going to see if maybe @matthewd can join here
Keenan Brock
@kbrock
May 09 2016 14:02
well, I agree with the change that rails made with passwords
don't agree with HOW they did it thogh
anyway
sorry
Oleg Barenboim
@chessbyte
May 09 2016 14:02
@kbrock - eyes on the prize
Chris Arcand
@chrisarcand
May 09 2016 14:02
Retrospective please :trollface:
Keenan Brock
@kbrock
May 09 2016 14:02
store database passwords / info in database.yml
Jason Frey
@Fryguy
May 09 2016 14:03
does someone have the PR in question?
Joe Rafaniello
@jrafanie
May 09 2016 14:03
My only question is this breaking an api that was used by others
Keenan Brock
@kbrock
May 09 2016 14:03
ok
I have other things to do
if you all want to gripe
Jason Frey
@Fryguy
May 09 2016 14:03
well, breaking an API is technically ok because it's a major release, right? we just happened to be on beta
Chris Arcand
@chrisarcand
May 09 2016 14:03
I'm on my phone, sorry - is this the database url change that Sean talk about @Fryguy ?
Keenan Brock
@kbrock
May 09 2016 14:03
then take the PR and fix it
Joe Rafaniello
@jrafanie
May 09 2016 14:03
We can split off on fixing the issue /while others try to see if we can make it non-breaking on rails 5
Keenan Brock
@kbrock
May 09 2016 14:03
I don't have time to hear gripes
especially when I wanted to make this change for > 1 year
Jason Frey
@Fryguy
May 09 2016 14:04
@chrisarcand No, connection pooling was revamped
Chris Arcand
@chrisarcand
May 09 2016 14:04
Sigh. K, thanks
Joe Rafaniello
@jrafanie
May 09 2016 14:04

well, breaking an API is technically ok because it's a major release, right? we just happened to be on beta

without deprecating behavior?

Jason Frey
@Fryguy
May 09 2016 14:04
oh right...good point
here it is: rails/rails#24844
Joe Rafaniello
@jrafanie
May 09 2016 14:05
yup
Nick Carboni
@carbonin
May 09 2016 14:05
    begin
      remove_connection
      pool = establish_connection({
        :adapter  => adapter,
        :host     => host,
        :port     => port,
        :username => username,
        :password => password,
        :database => database
      }.delete_blanks)
      puts pool.inspect
      conn = pool.connection
      yield conn
    ensure
      remove_connection
      establish_connection
    end
Seems to fix it. Then we can rely on the spec name being the model name and don't have to mess with connection_specification_name
Joe Rafaniello
@jrafanie
May 09 2016 14:05
all i'm saying is some people can try to fix it for us, others can see if we can make rails 5 NOT break everyone else
Jason Frey
@Fryguy
May 09 2016 14:06
so, the establish_connection part feels right, sort of (not sure about the thread local variables), but does the remove_connection work?
Oleg Barenboim
@chessbyte
May 09 2016 14:06
speaking of Rails5, should I merge ManageIQ/manageiq#8528 ?
Joe Rafaniello
@jrafanie
May 09 2016 14:07
Yes
Nick Carboni
@carbonin
May 09 2016 14:07
I can create a separate PR with that fix ... Thoughts?
Oleg Barenboim
@chessbyte
May 09 2016 14:07
@jrafanie yes to my merging ManageIQ/manageiq#8528 ?
Joe Rafaniello
@jrafanie
May 09 2016 14:07
Yes @chessbyte
it's failing for only the issue we know about so I'm good with merging it
Oleg Barenboim
@chessbyte
May 09 2016 14:08
merged. Now cherry-picking it to Darga
Jason Frey
@Fryguy
May 09 2016 14:09
@carbonin Why not embrace the connection_specification_name?
Also isn't the default for the connection_specification_name the model name?
Nick Carboni
@carbonin
May 09 2016 14:10
Yeah, that's why I removed the method that was defining it as something else.
Jason Frey
@Fryguy
May 09 2016 14:10
? I didn't see that in your diff there
Joe Rafaniello
@jrafanie
May 09 2016 14:11
@carbonin @Fryguy can I have link to the rubyrep code you're changing above? I'm trying to see all that they broke when that PR was merged
and @kbrock has a WIP PR here: ManageIQ/manageiq#8534
Joe Rafaniello
@jrafanie
May 09 2016 14:12
@Fryguy I thought there was something else in rubyrep that broke, thanks... that's the code that travis is failing on
Jason Frey
@Fryguy
May 09 2016 14:12
oh yeah...that's it I'm pretty sure
Nick Carboni
@carbonin
May 09 2016 14:13
ohhh now its just failing every once in a while .... :worried:
Jason Frey
@Fryguy
May 09 2016 14:13
I'm also concerned about our other usages or establish_connection/remove_connection
ok, need coffee :coffee:
Jason Frey
@Fryguy
May 09 2016 14:19
So @carbonin are you saying your change doesn't really work (i.e. it makes it sporadic?)
Nick Carboni
@carbonin
May 09 2016 14:19
It seems that the other spec fails with seed 20277
Failures:

  1) MiqRegionRemote.validate_connection_settings returns a message about indicating the port is missing when blank
     Failure/Error: raise ConnectionNotEstablished, "No connection pool with id #{spec_name} found." unless pool

     ActiveRecord::ConnectionNotEstablished:
       No connection pool with id primary found.
     # /home/ncarboni/.gem/ruby/2.2.4/bundler/gems/rails-a0b3de3002bd/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:876:in `retrieve_connection'
     # /home/ncarboni/.gem/ruby/2.2.4/bundler/gems/rails-a0b3de3002bd/activerecord/lib/active_record/connection_handling.rb:127:in `retrieve_connection'
     # /home/ncarboni/.gem/ruby/2.2.4/bundler/gems/rails-a0b3de3002bd/activerecord/lib/active_record/connection_handling.rb:91:in `connection'
     # /home/ncarboni/.gem/ruby/2.2.4/bundler/gems/rails-a0b3de3002bd/activerecord/lib/active_record/fixtures.rb:516:in `create_fixtures'
     # /home/ncarboni/.gem/ruby/2.2.4/bundler/gems/rails-a0b3de3002bd/activerecord/lib/active_record/fixtures.rb:1015:in `load_fixtures'
     # /home/ncarboni/.gem/ruby/2.2.4/bundler/gems/rails-a0b3de3002bd/activerecord/lib/active_record/fixtures.rb:977:in `setup_fixtures'
     # /home/ncarboni/.gem/ruby/2.2.4/bundler/gems/rails-a0b3de3002bd/activerecord/lib/active_record/fixtures.rb:852:in `before_setup'

Finished in 0.4174 seconds (files took 5.47 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./spec/models/miq_region_remote_spec.rb:3 # MiqRegionRemote.validate_connection_settings returns a message about indicating the port is missing when blank

Randomized with seed 20277
Looks like some cache clearing was removed also
Jason Frey
@Fryguy
May 09 2016 14:27
So, I don't understand why our original code doesn't work
words are hard :(
Joe Rafaniello
@jrafanie
May 09 2016 14:29
FYI, @Fryguy I pinged metaskills because that PR will probably break his "multiple db with rails" gem secondbase: https://github.com/customink/secondbase
Jason Frey
@Fryguy
May 09 2016 14:29
oh good point
Joe Rafaniello
@jrafanie
May 09 2016 14:29
My gut is they need to deprecate the behavior with rails 5
Jason Frey
@Fryguy
May 09 2016 14:29
it was designed for backward compatilibty though, so I don't think they think they need it
which is why I can't understand why ours doesn't work
I'm wondering if it's just our test
Nick Carboni
@carbonin
May 09 2016 14:33
Just adding an establish_connection (no args) after the original remove seems to do the trick. I think that's because the remove connection is actually getting rid of the MiqRegionRemote key in the hash, while the establish_connection is just updating the value
Chris Arcand
@chrisarcand
May 09 2016 14:48
@Fryguy I'm also concerned about our other usages or establish_connection/remove_connection :+1: I was thinking about this at the conf. I recall somebody talking about a new API for cleanly opening and closing it that I was going to add to ovirt_metrics stuffs.
Jason Frey
@Fryguy
May 09 2016 14:49
that must be the thing that just got merged, then?
Chris Arcand
@chrisarcand
May 09 2016 14:49
I dunno, I’m just catching up at my machine now. I’ll shush until I’m caught up ;)
Jason Frey
@Fryguy
May 09 2016 14:49
haha
Chris Arcand
@chrisarcand
May 09 2016 14:49
Sounds right though
Jason Frey
@Fryguy
May 09 2016 14:55
ok, I have to go in a hole for a bit and finish some stuff up...let me know what you find @jrafanie and @carbonin
Keenan Brock
@kbrock
May 09 2016 15:14
ManageIQ/manageiq#8545
Jason Frey
@Fryguy
May 09 2016 15:18
@kbrock Thanks...good idea...I'll merge and then we can work on this separately
Chris Arcand
@chrisarcand
May 09 2016 15:18
:+1:
Joe Rafaniello
@jrafanie
May 09 2016 15:18
:clap:
Oleg Barenboim
@chessbyte
May 09 2016 15:18
:+1: love the teamwork
Keenan Brock
@kbrock
May 09 2016 15:19
what does establish_connection do?
I undertand the old version - that associated a pool with a name
but the new version only takes a hash
what pool does that go into?
Jason Frey
@Fryguy
May 09 2016 15:19
there are 2 establish_connection methods
Keenan Brock
@kbrock
May 09 2016 15:19
ooh - thanks
Oleg Barenboim
@chessbyte
May 09 2016 15:19
@kbrock - your PR already failed https://travis-ci.org/ManageIQ/manageiq/jobs/128878684 -- but I think it is totally unrelated
Jason Frey
@Fryguy
May 09 2016 15:19
one is the "user facing" one that takes a hash, and then that works its way down to a lower level one
I merged ManageIQ/manageiq#8545 ... @chessbyte can you bounce the affected PRs?
@chessbyte Not sure if you want to darga/yes that one
I'm thinking yes so at least the darga branch goes green as well
Oleg Barenboim
@chessbyte
May 09 2016 15:20
is darga red too?
Keenan Brock
@kbrock
May 09 2016 15:20
lol. when it rains, it pours
(and auto correct changed the word rain for me to rails...)
Chris Arcand
@chrisarcand
May 09 2016 15:20
Undoubtedly.
Jason Frey
@Fryguy
May 09 2016 15:20
I would assume it would be
Oleg Barenboim
@chessbyte
May 09 2016 15:21
right - I am on it
Jason Frey
@Fryguy
May 09 2016 15:21
hahahaha "when it rails, it pours"
Oleg Barenboim
@chessbyte
May 09 2016 15:21
backporting and bouncing red PRs
Jason Frey
@Fryguy
May 09 2016 15:22
ok, I marked it darga/yes
Keenan Brock
@kbrock
May 09 2016 15:22
aah - of course - thanks
hopefully, we'll be throwing that away soon enough
Jason Frey
@Fryguy
May 09 2016 15:22
yeah
Keenan Brock
@kbrock
May 09 2016 15:26
wow - evmdatabase uses connections all over the place
Chris Arcand
@chrisarcand
May 09 2016 15:27
Yarp.
Keenan Brock
@kbrock
May 09 2016 15:27
looks like pglogical uses this remote stuff in 1 place too
Chris Arcand
@chrisarcand
May 09 2016 15:27
and ovirt metrics.
Jason Frey
@Fryguy
May 09 2016 15:27
yeah, that's why I'm concerned
Keenan Brock
@kbrock
May 09 2016 15:27
miqregion remote - that is an application record
but it is just used to pass a connection to other models
Jason Frey
@Fryguy
May 09 2016 15:28
yes, but it is only ApplicationRecord for the connection pooling logic
right
Keenan Brock
@kbrock
May 09 2016 15:28
seems the other models could just be run with the correct db in the first place
Jason Frey
@Fryguy
May 09 2016 15:28
it's not for other models....it's to connect to a remote database
Keenan Brock
@kbrock
May 09 2016 15:28
hmm
instead of miq remote -> connection -> other model
put this logic into AR_remote.rb
not sure if that opens us up security wise though :(
Jason Frey
@Fryguy
May 09 2016 15:29
it's no different than opening any other kind of connection
Keenan Brock
@kbrock
May 09 2016 15:29
MiqRegion.destroy_entire_region ?
whoa
tricky to pull reslover away from the establish connection
they really want to hide that
Keenan Brock
@kbrock
May 09 2016 15:48
I think I like nick's solution - it is a race condition, but not sure if that is the end of the world. (multiple threads changing out the config information)
they really want the class to have a database
having trouble registering the new credentials with a new name
Joe Rafaniello
@jrafanie
May 09 2016 16:23
Nick and I drafted a test on rails to recreate it (passing before the infamous refactoring PR)
Jason Frey
@Fryguy
May 09 2016 16:24
oh good
Joe Rafaniello
@jrafanie
May 09 2016 16:24
      def test_stuff
        klass2     = Class.new(Base)   { def self.name; 'klass2';    end }
        connection = klass2.connection
        assert_equal(connection.pool.size, 5)
        pool = klass2.establish_connection(adapter: 'sqlite3', pool: 10, :database => "activerecord_unittest")
        begin
          conn = pool.connection
          assert_equal(conn.pool.size, 10)
        ensure
          klass2.remove_connection
          assert_equal(klass2.connection.pool.size, 5)
        end
      end
Jason Frey
@Fryguy
May 09 2016 16:24
so it's something on their end?
I don't think they accept anonymous classes...is self.name sufficient for that?
Joe Rafaniello
@jrafanie
May 09 2016 16:24
yes
Jason Frey
@Fryguy
May 09 2016 16:24
k
Joe Rafaniello
@jrafanie
May 09 2016 16:25
it's how they get around anonymous classes in other tests
Jason Frey
@Fryguy
May 09 2016 16:25
so does your tests create 2 pools?
on the same class?
Joe Rafaniello
@jrafanie
May 09 2016 16:25
So, that fails with
  1) Error:
ActiveRecord::ConnectionAdapters::ConnectionHandlerTest#test_stuff:
ActiveRecord::ConnectionNotEstablished: No connection pool with id klass2 found.
    /Users/joerafaniello/Code/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:876:in `retrieve_connection'
    /Users/joerafaniello/Code/rails/activerecord/lib/active_record/connection_handling.rb:128:in `retrieve_connection'
    /Users/joerafaniello/Code/rails/activerecord/lib/active_record/connection_handling.rb:91:in `connection'
    test/cases/connection_adapters/connection_handler_test.rb:34:in `test_stuff'
Jason Frey
@Fryguy
May 09 2016 16:26
hmmmm
Joe Rafaniello
@jrafanie
May 09 2016 16:26
Yes, and the mapping to pool lookup is not reset after the remove_connection
Jason Frey
@Fryguy
May 09 2016 16:27
looked?
Joe Rafaniello
@jrafanie
May 09 2016 16:27
it's failing on the connection call in assert_equal(klass2.connection.pool.size, 5)
Jason Frey
@Fryguy
May 09 2016 16:27
gotcha
yeah having a test on their stuff is much better than trying to explain with ours
Joe Rafaniello
@jrafanie
May 09 2016 16:27
ok, grabbing lunch, we'll try to dig into it. It seems like the cache needs to be reset or something
Jason Frey
@Fryguy
May 09 2016 16:28
k cool
let me know how it goes
Joe Rafaniello
@jrafanie
May 09 2016 16:28
ok
Jason Frey
@Fryguy
May 09 2016 16:28
I'm sure you and nick can solve it :+1: :)