[WIP] add hosts and vms as an association

[durandom]

some methods in ExtManagementSystem expect these to return an
association and as we can have NetworkManagers without a parent_manager (e.g. Nuage)
we must not delegate those methods

Links

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

@miq-bot add_labels bug, euwe/yes
@miq-bot assign Ladas

@Ladas please review

[durandom]

@chrisarcand do you have any idea how to handle this error?

ActiveRecord::HasManyThroughCantAssociateThroughHasOneOrManyReflection: Cannot modify association 'ManageIQ::Providers::Openstack::NetworkManager#hosts' because the source reflection class 'Host' is associated to 'ManageIQ::Providers::Openstack::InfraManager' via :has_many.

This is because I want to model the relation basically like this

class NetworkManager
  belongs_to :parent_manager, :class => 'ExtManagementSystem'
  has_many :hosts, :through => :parent_manager
end

class ExtManagementSystem
  has_many :hosts
end

[chrisarcand]

@durandom Something must be expecting that you can modify vms and hosts on NetworkManagers, which you can do with the delegation and not a :through relation (that's intended and unavoidable).

What's the stack trace on that error? Can you find the places where modification is happening and just avoid it? If so, callers can be updated and you can make this change.

Else, you won't be able to do it exactly this way and will have to make methods that return empty relations in the absence of a parent manager (icky...it'll make things work as normal before but that's pretty hacky)

Vague help I know; I need more direct experience with all these provider manager entities so I can be of more specific help.

[Ladas]

@durandom hm, weird thing is that this fails during @ems.destroy

could be it is trying to call e.g. the nullify defined on ems like: has_many :hosts, :foreign_key => "ems_id", :dependent => :nullify, :inverse_of => :ext_management_system

could you check the details of the NetworkManager hosts? It has some settings from the base class?

[durandom]

could you check the details of the NetworkManager hosts? It has some settings from the base class?

By redefining it on the NetworkManager it has only the options that are added there.
So its just :through => :parent_manager - and calling destroy on the network_manager I guess it's also calling .destroy on the :hosts. Which is indeed not what we want :(

@chrisarcand can I make the association somehow readonly? has_many :hosts, -> { readonly } as in the docs doesnt do the trick.

I also came up with adding methods like

def hosts
  network_manager ? network_manger.hosts : []
end

But that seems not as elegant as using a proper association.

[Ladas]

@durandom adding method, that is basically delegation (but delegation doesn't support default value when it's nil)

Also the default value would need to be Arel, not just []. We can still have calls hosts.where(xy)

---

But ^ seems to me like a bug in Rails really. It should not call the destroy on hosts, when you've overwritten the relation and it doesn't have a cascade delete.

[durandom]

Also the default value would need to be Arel, not just []. We can still have calls hosts.where(xy)

absolutely. But for lack of knowledge I just took the short path.
@Ladas do you know how I can return an empty relation?
And I wonder if that doesnt bring up new problems

[chrisarcand]

@durandom http://api.rubyonrails.org/v5.0.0.1/classes/ActiveRecord/QueryMethods.html#method-i-none

[durandom]

Here is a nice self contained test that @chrisarcand came up with.
Fails for rails 5 and 4
Works if you remove the nullify
@chrisarcand is researching if thats actually a bug.

Until then, dont know, go with the method solution.
@Ladas do you have an appliance with C&U enabled and a Nuage Networkmanager and can you check if https://bugzilla.redhat.com/show_bug.cgi?id=1393675 happens there too?

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  # Activate the gem you are reporting the issue against.
  gem "activerecord", "5.0.0.1"
  # gem "activerecord", "4.2.7.1"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table "hosts", force: :cascade do |t|
    t.integer  "manager_id"
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
    t.index ["manager_id"], name: "index_hosts_on_manager_id"
  end

  create_table "managers", force: :cascade do |t|
    t.integer  "manager_id"
    t.string   "type"
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
    t.index ["manager_id"], name: "index_managers_on_manager_id"
  end
end

class Host < ActiveRecord::Base
  belongs_to :manager
end

class Manager < ActiveRecord::Base
  has_one :network_manager, :dependent => :destroy
  has_many :hosts, :dependent => :nullify
end

class NetworkManager < Manager
  belongs_to :manager
  has_many :hosts, :through => :manager
end

class BugTest < Minitest::Test
  def test_association_stuff
    manager = Manager.create
    network_manager = NetworkManager.create
    network_manager.manager = manager
    network_manager.save
    assert manager.destroy
  end
end

[chrisarcand]

@durandom @Ladas I think I've found the issue here and it probably is a bug in Active Record. As I work on that, feel free to use normal methods.

[Ladas]

@durandom I don't have the Nuage manager currently, but from how it looks, the issue will be there

so for now, should we go with:

def hosts
  parent_manager ? parent_manager.hosts : Host.none
end

?

[Ladas]

hm, I was fairly sure this will fix https://bugzilla.redhat.com/show_bug.cgi?id=1383307

but seems like the through relation here is not really a through relation, but it does delegate instead? At least I don't see any join to parent_manager, I would expect here, but it directly uses the parent_manager id in the query.

@kbrock @chrisarcand ^ and that could be a reason why this fails on destroy, since this is not a proper has_many

[kbrock]

this is great marcel

just the spec change and I'll merge

[kbrock]

could you test that this is a relation instead of not_nil?

We are using all of these:

expect(manager.hosts).to be_a_kind_of(ActiveRecord::Relation)
expect(manager.hosts).to be_kind_of(ActiveRecord::Relation)
expect(manager.hosts).to be_a(ActiveRecord::Relation)

[miq-bot]

Checked commit durandom@3a86a9d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🏆

[durandom]

Yeah, unfortunately azure still fails. I'll add this very test here as well.
So once rails/rails#27364 produces some insights we might re-visit this...

Failures:

  1) ManageIQ::Providers::Azure::CloudManager does not create orphaned network_manager
     Failure/Error: ems.destroy

     ActiveRecord::HasManyThroughCantAssociateThroughHasOneOrManyReflection:
       Cannot modify association 'ManageIQ::Providers::Azure::NetworkManager#hosts' because the source reflection class 'Host' is associated to 'ManageIQ::Providers::BaseManager' via :has_many.
     # ./spec/models/manageiq/providers/azure/cloud_manager_spec.rb:21:in `block (2 levels) in <top (required)>'

[durandom]

@miq-bot add_label wip

[kbrock]

Really dislike overriding these models in subclasses.
When we are querying from the parent class, it will get these associations wrong. Especially as we are trying to remove all our N+1 code.

Having said that, can we keep as an association, just link via the parent_manager_id ?

class NetworkManager
  has_many :hosts, :foreign_key => 'parent_manager_id'
end

If we have to use a virtual association, lets default to Host.none

[kbrock]

@durandom looking into seeing if #14978 can solve this problem

[Ladas]

I'm gonna try to fix all of these using #14978 and #12884 (comment)

[miq-bot]

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!