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.

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

Fix the VM Provisioning issue with auto replacement in selected dvPortGroup network. #78

Merged
merged 1 commit into from
Jul 20, 2017

Conversation

lfu
Copy link
Member

@lfu lfu commented Jul 18, 2017

Fix the VM Provisioning issue with auto replacement in selected dvPortGroup network.

Issue is introduced in ManageIQ/manageiq#14946.
https://bugzilla.redhat.com/show_bug.cgi?id=1467399

@miq-bot assign @gmcculloug
@miq-bot add_label bug, fine/yes

_log.info("Filtering hosts with the following network: <#{vlan_name}>")
shared = !vlan_name.match(/dvs_/).nil?
vlan_name.sub!(/^dvs_/, '') if shared
all_hosts.select { |h| h.lans.any? { |lan| lan.name == vlan_name && lan.switch.shared == shared } }
Copy link
Member

Choose a reason for hiding this comment

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

I don't think lan.switch.shared == shared will work since we don't set switch.shared to false, its just nil: https://github.com/ManageIQ/manageiq-providers-vmware/blob/master/app/models/manageiq/providers/vmware/infra_manager/refresh_parser.rb#L428-L438

Copy link
Member

Choose a reason for hiding this comment

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

Not really "clean" but you could do && !!lan.switch.shared == shared so a nil is treated as false

let(:s13) { FactoryGirl.create(:switch, :name => "C") }
let(:s11) { FactoryGirl.create(:switch, :name => "A", :shared => false) }
let(:s12) { FactoryGirl.create(:switch, :name => "B", :shared => false) }
let(:s13) { FactoryGirl.create(:switch, :name => "C", :shared => false) }
Copy link
Member

Choose a reason for hiding this comment

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

Should probably leave shared nil in the tests since that matches what we'll see on a real system.

_log.info("Filtering hosts with the following network: <#{vlan_name}>")
shared = !vlan_name.match(/dvs_/).nil?
vlan_name.sub!(/^dvs_/, '') if shared
all_hosts.select { |h| h.lans.any? { |lan| lan.name == vlan_name && !lan.switch.shared.nil? == shared } }
Copy link
Member

Choose a reason for hiding this comment

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

Well now this counts on switch.shared being nil and will break if we update the parser in the future to (I think) correctly set shared to false.
!!lan.switch.shared will treat nil and false as "falsy" which is what we want.

_log.info("Filtering hosts with the following network: <#{vlan_name}>")
shared = !vlan_name.match(/dvs_/).nil?
vlan_name.sub!(/^dvs_/, '') if shared
all_hosts.select { |h| h.lans.any? { |lan| lan.name == vlan_name && !lan.switch.shared.blank? == shared } }
Copy link
Contributor

Choose a reason for hiding this comment

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

is this doing n+1 queries possibly (n+1)^2 ? Or all_hosts already has all_hosts.include(:lans => switch).references(:lans => switch) #haven't tested the query

@miq-bot
Copy link
Member

miq-bot commented Jul 19, 2017

Checked commit lfu@4b7cddc with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@agrare agrare merged commit 5e17387 into ManageIQ:master Jul 20, 2017
@agrare agrare added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 20, 2017
simaishi pushed a commit that referenced this pull request Aug 4, 2017
Fix the VM Provisioning issue with auto replacement in selected dvPortGroup network.
(cherry picked from commit 5e17387)

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

simaishi commented Aug 4, 2017

Fine backport details:

$ git log -1
commit 6a23cc6decacc268d98d484c7004bac520333cb7
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Jul 20 13:29:03 2017 -0400

    Merge pull request #78 from lfu/DVPortGroup_1467399
    
    Fix the VM Provisioning issue with auto replacement in selected dvPortGroup network.
    (cherry picked from commit 5e173878b02e61367e6d8fcebfd4ea8ad4c80b51)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1478562

@lfu lfu deleted the DVPortGroup_1467399 branch September 29, 2018 14:35
agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants