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

Reconfigure VM: Add / Remove Network Adapters #25

Merged
merged 3 commits into from
Jan 2, 2018

Conversation

kruge002
Copy link
Contributor

@kruge002 kruge002 commented Dec 20, 2017

This PR adds the method getDeviceKeysByLabel to retrieve the network adapters device keys for the given network adapter label. This method is part of the new functionality added to the Reconfigure VM webpage.

This PR is part of a set of PRs:
#25
ManageIQ/manageiq-providers-vmware#163
ManageIQ/manageiq#16700
ManageIQ/manageiq-ui-classic#3121

More info: ManageIQ/manageiq-ui-classic#3119

@agrare agrare self-assigned this Dec 20, 2017
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.

Thanks for the PR @kruge002 !

Couple of comments, I think it would be great if you could model what we have for getScsiControllers but for ethernet cards.

def getDeviceKeysByNetwork(networkName)
devs = getProp("config.hardware")["config"]["hardware"]["device"]
devs.each do |dev|
next if !["VirtualVmxnet3","VirtualE1000e","VirtualPCNet32"].include? dev.xsiType
Copy link
Member

@agrare agrare Dec 20, 2017

Choose a reason for hiding this comment

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

This leaves out a few NIC types (VirtualE1000, VirtualVmxnet2), ideally what you'd want to do is dev.kind_of?(VirtualEthernetCard) but since xsiType is just a string that's not possible.

What we have instead to track VIM class inheritance is VimClass so you could add all of these as child classes of VirtualEthernetCard and do next unless VimClass.child_classes(:VirtualEthernetCard).include? dev.xsiType

Easier to just do what we do with VirtualScsiControllers

devs = getProp("config.hardware")["config"]["hardware"]["device"]
devs.each do |dev|
next if !["VirtualVmxnet3","VirtualE1000e","VirtualPCNet32"].include? dev.xsiType
next if dev["deviceInfo"]["label"] != networkName
Copy link
Member

Choose a reason for hiding this comment

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

deviceInfo.label is "Network adapter 1", summary is what has the network name if it is a VM Network. This won't work if the NIC is on a dvPortGroup however because the deviceInfo.summary is e.g.: "DVSwitch: 50 3c b7 67 59 58 cf ce-75 16 2f e0 2b 6a d8 3c"

What you could do is check the backing, so e.g.

next unless case dev["backing"].xsiType
when "VirtualEthernetCardNetworkBackingInfo"
  dev["backing"]["deviceName"] == networkName
when "VirtualEthernetCardDistributedVirtualPortBackingInfo"
  dev["backing"]["port"]["portgroupKey"] == networkName # NOTE this will need to be the dvPortGroup ManagedObjectReference but this is the `Lan.uid_ems` for MIQ anyway
end

@@ -925,6 +925,20 @@ def getDeviceKeysByBacking(backingFile, hardware = nil)
([nil, nil])
end # def getDeviceKeysByBacking

def getDeviceKeysByNetwork(networkName)
devs = getProp("config.hardware")["config"]["hardware"]["device"]
Copy link
Member

Choose a reason for hiding this comment

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

You can use getHardware which does getProp("config.hardware").try(:fetch_path, "config", "hardware") || {} for you

@kruge002
Copy link
Contributor Author

@agrare Thanks for the feedback. I am working on it. You will hear back from me in the new year.

@miq-bot
Copy link
Member

miq-bot commented Dec 21, 2017

Checked commits kruge002/vmware_web_service@d017d23~...af8b1cd with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 6 offenses detected

lib/VMwareWebService/MiqVimVm.rb

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.

Looking good 😄 I still think you need to use the backing to find the network instead of the description so that it supports dvportgroups.

@kruge002
Copy link
Contributor Author

kruge002 commented Jan 2, 2018

@agrare I renamed the method getDeviceKeysByNetwork to getDeviceKeysByLabel because the method needs to find the device keys for the given Network Adapter label e.g. 'Network adapter 1'. The network name is not needed here so it will work for dvportgroups as well.

I will add a commit to solve the rubocop issues. I hope you will accept the codeclimate issue (Cognitive Complexity of 8 exceeds 5).

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.

LGTM thanks @kruge002 !

@agrare agrare merged commit 57936b7 into ManageIQ:master Jan 2, 2018
@agrare
Copy link
Member

agrare commented Jan 3, 2018

@kruge002 I released v0.2.5 with these changes, thanks for your contributions!

@kruge002
Copy link
Contributor Author

kruge002 commented Jan 3, 2018

@agrare Thanks for the merge! One PR ready, three to go to completely add the "Add / Remove Network Adapter" functionality to "Reconfigure VM":
ManageIQ/manageiq-providers-vmware#163
ManageIQ/manageiq#16700
ManageIQ/manageiq-ui-classic#3121

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.

3 participants