Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix allowed_vlans to call preload correctly. #16702

Merged
merged 1 commit into from
Dec 21, 2017
Merged

Fix allowed_vlans to call preload correctly. #16702

merged 1 commit into from
Dec 21, 2017

Conversation

lfu
Copy link
Member

@lfu lfu commented Dec 20, 2017

The existing preload association option is not specified correctly.

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

@miq-bot assign @gmcculloug
@miq-bot add_label bug, provisioning, fine/yes, gaprindashvili/yes
cc @agrare

To verify the N+1 queries in Rails console.
With existing preload association option.

[1] pry(main)> hosts = [Host.first, Host.last]
[2] pry(main)> vlans = {}
[3] pry(main)> MiqPreloader.preload(hosts, :switches => :lans)
  HostSwitch Load (0.3ms)  SELECT "host_switches".* FROM "host_switches" WHERE "host_switches"."host_id" IN (1, 23)
  HostSwitch Inst Including Associations (30.1ms - 5rows)
  Switch Load (0.5ms)  SELECT "switches".* FROM "switches" WHERE "switches"."id" IN (1, 2, 47, 43, 44)
  Switch Inst Including Associations (8.5ms - 5rows)
  Lan Load (0.4ms)  SELECT "lans".* FROM "lans" WHERE "lans"."switch_id" IN (2, 1, 43, 44, 47)
  Lan Inst Including Associations (10.0ms - 10rows)
=> [#<ActiveRecord::Associations::Preloader::HasManyThrough:0x00007fa4a70cd9d0
  @klass=
......
[4] pry(main)> hosts.each do |h|
[4] pry(main)*   h.lans.each { |l| vlans[l.name] = l.name unless l.switch.shared? }
[4] pry(main)* end
  Lan Load (0.4ms)  SELECT "lans".* FROM "lans" INNER JOIN "switches" ON "lans"."switch_id" = "switches"."id" INNER JOIN "host_switches" ON "switches"."id" = "host_switches"."switch_id" WHERE "host_switches"."host_id" = $1  [["host_id", 1]]
  Lan Inst Including Associations (0.1ms - 4rows)
  Switch Load (0.2ms)  SELECT  "switches".* FROM "switches" WHERE "switches"."id" = $1 LIMIT $2  [["id", 2], ["LIMIT", 1]]
  Switch Inst Including Associations (0.1ms - 1rows)
  Switch Load (0.2ms)  SELECT  "switches".* FROM "switches" WHERE "switches"."id" = $1 LIMIT $2  [["id", 1], ["LIMIT", 1]]
  Switch Inst Including Associations (0.0ms - 1rows)
  Switch Load (0.2ms)  SELECT  "switches".* FROM "switches" WHERE "switches"."id" = $1 LIMIT $2  [["id", 1], ["LIMIT", 1]]
  Switch Inst Including Associations (0.0ms - 1rows)
  Switch Load (0.2ms)  SELECT  "switches".* FROM "switches" WHERE "switches"."id" = $1 LIMIT $2  [["id", 1], ["LIMIT", 1]]
  Switch Inst Including Associations (0.1ms - 1rows)
  Lan Load (0.4ms)  SELECT "lans".* FROM "lans" INNER JOIN "switches" ON "lans"."switch_id" = "switches"."id" INNER JOIN "host_switches" ON "switches"."id" = "host_switches"."switch_id" WHERE "host_switches"."host_id" = $1  [["host_id", 23]]
  Lan Inst Including Associations (0.1ms - 6rows)
  Switch Load (0.2ms)  SELECT  "switches".* FROM "switches" WHERE "switches"."id" = $1 LIMIT $2  [["id", 43], ["LIMIT", 1]]
  Switch Inst Including Associations (0.1ms - 1rows)
  Switch Load (0.2ms)  SELECT  "switches".* FROM "switches" WHERE "switches"."id" = $1 LIMIT $2  [["id", 43], ["LIMIT", 1]]
  Switch Inst Including Associations (0.0ms - 1rows)
  Switch Load (0.1ms)  SELECT  "switches".* FROM "switches" WHERE "switches"."id" = $1 LIMIT $2  [["id", 44], ["LIMIT", 1]]
  Switch Inst Including Associations (0.0ms - 1rows)
  Switch Load (0.2ms)  SELECT  "switches".* FROM "switches" WHERE "switches"."id" = $1 LIMIT $2  [["id", 44], ["LIMIT", 1]]
  Switch Inst Including Associations (0.0ms - 1rows)
  Switch Load (0.2ms)  SELECT  "switches".* FROM "switches" WHERE "switches"."id" = $1 LIMIT $2  [["id", 44], ["LIMIT", 1]]
  Switch Inst Including Associations (0.0ms - 1rows)
  Switch Load (0.2ms)  SELECT  "switches".* FROM "switches" WHERE "switches"."id" = $1 LIMIT $2  [["id", 47], ["LIMIT", 1]]
  Switch Inst Including Associations (0.0ms - 1rows)
=> [#<ManageIQ::Providers::Vmware::InfraManager::HostEsx:0x00007fa4a77e0bf0
  id: 1,
......

With modified preload association.

[1] pry(main)> hosts = [Host.first, Host.last]
[2] pry(main)> vlans = {}
[3] pry(main)> MiqPreloader.preload(hosts, :lans => :switches)
  HostSwitch Load (0.5ms)  SELECT "host_switches".* FROM "host_switches" WHERE "host_switches"."host_id" IN (1, 23)
  HostSwitch Inst Including Associations (4.8ms - 5rows)
  Switch Load (1.2ms)  SELECT "switches".* FROM "switches" WHERE "switches"."id" IN (1, 2, 47, 43, 44)
  Switch Inst Including Associations (7.5ms - 5rows)
  Lan Load (0.7ms)  SELECT "lans".* FROM "lans" WHERE "lans"."switch_id" IN (1, 2, 47, 43, 44)
  Lan Inst Including Associations (11.1ms - 10rows)
=> [#<ActiveRecord::Associations::Preloader::HasManyThrough:0x00007fb8d5456678
  @klass=
......
[4] pry(main)>
[5] pry(main)> hosts.each do |h|
[5] pry(main)*   h.lans.each { |l| vlans[l.name] = l.name unless l.switch.shared? }
[5] pry(main)* end
=> [#<ManageIQ::Providers::Vmware::InfraManager::HostEsx:0x00007fb8b66e7eb0
  id: 1,
........

@gmcculloug gmcculloug requested a review from agrare December 20, 2017 15:32
agrare
agrare approved these changes Dec 20, 2017
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this now matches the dvs method we have in the vwmare repo

@gmcculloug gmcculloug requested a review from Fryguy December 20, 2017 18:26
Fryguy
Fryguy reviewed Dec 20, 2017
View reviewed changes
spec/models/miq_provision_virt_workflow_spec.rb Outdated
hosts = [@host1]
MiqPreloader.preload(hosts, :lans => :switches)
expect(ActiveRecord::Base.connection).not_to receive(:exec_query)
workflow.load_hosts_vlans(hosts, {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't make sense. All the test is showing is that ActiveRecord works correctly. A better test would be to use the query counting helper.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/ManageIQ/manageiq/blob/master/spec/support/custom_matchers/exceed_query_limit.rb and https://github.com/ManageIQ/manageiq/blob/master/spec/support/custom_matchers/match_query_limit_of.rb custom matches for how to do query limit checks.

@lfu lfu force-pushed the allowed_vlans_1510069 branch from 7844a4c to 9c94f30 Compare December 21, 2017 15:30
@miq-bot
Copy link
Member

miq-bot commented Dec 21, 2017

Checked commit lfu@9c94f30 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@gmcculloug gmcculloug merged commit 09a566a into ManageIQ:master Dec 21, 2017
@gmcculloug gmcculloug added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 21, 2017
simaishi pushed a commit that referenced this pull request Jan 3, 2018
@gmcculloug @simaishi
Merge pull request #16702 from lfu/allowed_vlans_1510069
2c9776e 
Fix allowed_vlans to call preload correctly.
(cherry picked from commit 09a566a)

https://bugzilla.redhat.com/show_bug.cgi?id=1530674
@simaishi
Copy link
Contributor

simaishi commented Jan 3, 2018

Gaprindashvili backport details:

$ git log -1
commit 2c9776e58c605bd0c61b61ee513021b8f4bda0d8
Author: Greg McCullough <[email protected]>
Date:   Thu Dec 21 10:51:28 2017 -0500

    Merge pull request #16702 from lfu/allowed_vlans_1510069
    
    Fix allowed_vlans to call preload correctly.
    (cherry picked from commit 09a566a277934cc4e955d8fe42b64175e5949f42)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1530674

simaishi pushed a commit that referenced this pull request Apr 9, 2018
@gmcculloug @simaishi
Merge pull request #16702 from lfu/allowed_vlans_1510069
76497ff 
Fix allowed_vlans to call preload correctly.
(cherry picked from commit 09a566a)

https://bugzilla.redhat.com/show_bug.cgi?id=1565248
@simaishi
Copy link
Contributor

simaishi commented Apr 9, 2018

Fine backport details:

$ git log -1
commit 76497ffcd9f23eb9df6d499d901ac7c8422968e5
Author: Greg McCullough <[email protected]>
Date:   Thu Dec 21 10:51:28 2017 -0500

    Merge pull request #16702 from lfu/allowed_vlans_1510069
    
    Fix allowed_vlans to call preload correctly.
    (cherry picked from commit 09a566a277934cc4e955d8fe42b64175e5949f42)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1565248

d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
@gmcculloug @d-m-u
Merge pull request ManageIQ#16702 from lfu/allowed_vlans_1510069
3e93595 
Fix allowed_vlans to call preload correctly.
(cherry picked from commit 09a566a)

https://bugzilla.redhat.com/show_bug.cgi?id=1565248
@lfu lfu deleted the allowed_vlans_1510069 branch September 29, 2018 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

@agrare agrare agrare approved these changes

Assignees

@gmcculloug gmcculloug

Labels
bug fine/backported gaprindashvili/backported lifecycle/provisioning
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lfu @miq-bot @simaishi @Fryguy @agrare @gmcculloug