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

Prevent a DVPortGroup from overwriting a LAN with the same name in provisioning #14292

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Mar 13, 2017

During the provisioning workflow we return the list of allowed_vlans as a hash with the name of the network as the key.

If you have a DVPortGroup and a normal network with the same name they will have only one entry in the vlans hash.

Later we change the vlans key of a DVS to be "dvs_#{name} and delete the old key: https://github.com/ManageIQ/manageiq-providers-vmware/blob/master/app/models/manageiq/providers/vmware/infra_manager/provision_workflow.rb#L141-L142

This leads to only one entry in the vlans hash and it is the dvportgroup, the standard network is deleted.

This change will set the key of a dvportgroup to have the dvs_ prefix initially so it will not collide with non dvportgroups and both will be returned.

VC Networks:
screenshot from 2017-03-13 12-06-48

Before:
screenshot from 2017-03-13 12-08-09

After:
screenshot from 2017-03-13 12-09-17

Related: ManageIQ/manageiq-providers-vmware#25

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

h.lans.each do |l|
next if l.switch.shared? && !@vlan_options[:dvs]

key = if l.switch.shared?
Copy link
Member

Choose a reason for hiding this comment

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

@agrare The logic here looks correct, but I am wondering if it should be moved to the ManageIQ/manageiq-providers-vmware repo?

Is there any reason to keep this as base provision_workflow logic? No other providers need this that I am aware of.

Copy link
Member Author

Choose a reason for hiding this comment

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

The whole method or just the DVS handling?
The issue I had with just moving the DVS stuff was that by the time it got to manageiq-providers-vmware the regular network and the dvs network had already collided so there was only one entry in the hash.

Copy link
Member

Choose a reason for hiding this comment

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

DVS is specific to VMware so wondering if we can move that into that provider while retaining basic network logic here for the other infra providers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed that would be great...could we skip any shared switches here and handle them on the vmware side? Either that or use the uid_ems instead of the name as the key?

@blomquisg blomquisg assigned gmcculloug and unassigned blomquisg Mar 16, 2017
Keep DVPortGroups with the same name as a standard Network from
colliding in the vlans hash

https://bugzilla.redhat.com/show_bug.cgi?id=1430709
@agrare agrare force-pushed the bz_1430709_dvportgroup_vm_network_same_name branch from d02183e to 8eb552c Compare March 20, 2017 14:30
@miq-bot
Copy link
Member

miq-bot commented Mar 20, 2017

Checked commit agrare@8eb552c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

@agrare
Copy link
Member Author

agrare commented Mar 20, 2017

@gmcculloug I changed the base method to skip over all DVPortGroups so the ManageIQ::Providers::Vmware::InfraManager::ProvisionWorkflow#available_vlans_and_hosts adds all dvportgroups.

Can you take another look?

Copy link
Member

@gmcculloug gmcculloug left a comment

Choose a reason for hiding this comment

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

This ended up looking better then I thought it would. Nice job! 🌟

@gmcculloug gmcculloug merged commit f0ee797 into ManageIQ:master Mar 20, 2017
@gmcculloug gmcculloug added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 20, 2017
@agrare agrare deleted the bz_1430709_dvportgroup_vm_network_same_name branch March 20, 2017 19:31
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.

5 participants