Reconfigure VM network connectivity aka. NICs #272

miha-plesko · 2018-05-22T13:09:23Z

With this commit we support reconfiguration of VM's network adapters - since UI only supports add and remove operations, we focus on these as well.

Turns out vCloud's cloud_networks (vApp networks) were not hooked to corresponding orchestration_stack (vApp) yet so we needed to update the parser. Also, we modify parser to omit subnet- prefix from cloud_subnet names because it looks strange on the reconfigure UI.

Fog version is shifted to 0.2.2 since only this version supports NIC reconfiguration.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1572086
Needed for: ManageIQ/manageiq-ui-classic#3972

Video: http://x.k00.fr/nicreconfigure

@miq-bot assign @agrare
@miq-bot add_label enhancement,gaprindashvili/yes

miha-plesko · 2018-05-22T14:24:56Z

Added some more tests, now I think it's ready for review.

agrare · 2018-05-23T15:19:54Z

app/models/manageiq/providers/vmware/cloud_manager/vm/reconfigure.rb

+    ['None'] + orchestration_stack.cloud_networks.map(&:name)
+  end
+
+  def available_adapter_names


Where are these two methods called? Is there a corresponding UI PR that is still pending?

Yes, ManageIQ/manageiq-ui-classic#3972

👍 thanks

agrare · 2018-05-23T15:48:12Z

app/models/manageiq/providers/vmware/cloud_manager/vm/reconfigure.rb

+  end
+
+  def available_adapter_names
+    available = (0..4).to_a - network_ports.map { |nic| nic_idx(nic.name) }.map(&:to_i)


Is this a vCloud limitation? Can you only have 5 NICs on a VM?

I think vCloud supports unlimited number of NICs. MIQ UI, however, hard-codes a limit of 4 when reconfiguring. I'm not sure why such limit is enforced, but I'm also not sure why vCloud user would ever want to assign more than 4 NICs so I didn't really go into that.

Let me update this to offer user only 4 indeces (0..3) so it will match UI limitation even better. If UI limit gets removed in future, we can add more here if needed.

agrare · 2018-05-23T15:51:15Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

+        :shared                 => network.is_shared,
+        :type                   => network_type,
+        :cloud_subnets          => [],
+        :orchestration_stack_id => network.try(:vapp).try(:id)


Hm I don't think this is the right id, the orchestration_stack_id should be our database id and the id you get from the vapp is probably more akin to our ems_ref.
If so you can set this to something like :orchestration_stack_ref and in the network_manager save_inventory in core you can pull that ref out, lookup the stack and put the ID there in the saver.

/cc @Ladas

yea, we should also have this tested in refresh spec.

This refresh parser is a bit special in a way it actually iterates orchestration stacks from VMDB and fetches networking information for each of them. This happens in L54 like this:

@ems.orchestration_stacks.each do |stack| # ... end

So vapp.id here is actually the VMDB id. Spec test is already in place and I've double tested in development environment prior opening PR and recording demo video 😇 But thanks for pointing out, renamed :vapp attribute into :orchestration_stack to make it less misleading.

miha-plesko

Thanks for review, I've pushed a slightly modified patch where I introduce additional change to the refresh parser. Currently, we inventory NIC name as

"{vm_name}#NIC#{nic_index}"

which is quite long and unusual. We now rather inventory it only as:

"NIC#{nic_index}"

@agrare if you think I should open a separate PR for parser changes just let me know. Thing is no one really used vCloud network inventory by now so I think we're safe to modify it just like this.

miha-plesko · 2018-05-24T06:25:52Z

app/models/manageiq/providers/vmware/cloud_manager/vm/reconfigure.rb

+  end
+
+  def available_adapter_names
+    available = (0..4).to_a - network_ports.map { |nic| nic_idx(nic.name) }.map(&:to_i)


I think vCloud supports unlimited number of NICs. MIQ UI, however, hard-codes a limit of 4 when reconfiguring. I'm not sure why such limit is enforced, but I'm also not sure why vCloud user would ever want to assign more than 4 NICs so I didn't really go into that.

Let me update this to offer user only 4 indeces (0..3) so it will match UI limitation even better. If UI limit gets removed in future, we can add more here if needed.

miha-plesko · 2018-05-24T13:15:42Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

+        :shared                 => network.is_shared,
+        :type                   => network_type,
+        :cloud_subnets          => [],
+        :orchestration_stack_id => network.try(:vapp).try(:id)


This refresh parser is a bit special in a way it actually iterates orchestration stacks from VMDB and fetches networking information for each of them. This happens in L54 like this:

@ems.orchestration_stacks.each do |stack| # ... end

So vapp.id here is actually the VMDB id. Spec test is already in place and I've double tested in development environment prior opening PR and recording demo video 😇 But thanks for pointing out, renamed :vapp attribute into :orchestration_stack to make it less misleading.

himdel · 2018-05-24T14:14:49Z

app/models/manageiq/providers/vmware/cloud_manager/vm/reconfigure.rb

@@ -28,6 +28,15 @@ def disk_default_type
    'LSI Logic Parallel SCSI'
  end

+  def available_networks
+    ['None'] + orchestration_stack.cloud_networks.map(&:name)


Not sure if 'None' should not be added by the UI.

But if it is here, it needs to be _('None') or N_('None') so that we have a hope of translating it.

@himdel yeah I agree adding None here seems like the wrong place

"None" is a special network name which we pass directly to vCloud API and vCloud understands it, so using _('None') won't work unless we perform some kind of reverse transformation prior passing to vCloud.

My thinking to add "None" here was that other providers don't seem to support it so I wasn't sure how to do it on UI (another IF statement with another function here on provider e.g. supports_none?). We can also make vLan field optional, but that would affect how reconfigure works for other providers.

That being said, please let me know if you'd still prefer to do it on UI and I'll try to do it.

Oh interesting, maybe make the network selection optional on the UI and set the network name to 'None' if options[:network].nil?

miha-plesko · 2018-05-25T09:28:44Z

Thanks @agrare and @himdel for comment on 'None' network, just moved it to to UI and must say it really feels better. Kindly ask for another pass on the two PRs.

miha-plesko · 2018-05-28T06:56:20Z

@agrare looking forward to another pass of review on this one.

With this commit we support reconfiguration of VM's network adapters - since UI only supports add and remove operations, we focus on these as well. Turns out vCloud's cloud_networks (vApp networks) were not hooked to corresponding orchestration_stack (vApp) yet so we needed to update the parser. Also, we modify parser to omit `subnet-` prefix from cloud_subnet names because it looks strange on the reconfigure UI. Fog version is shifted to 0.2.2 since only this version supports NIC reconfiguration. BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1572086 Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miq-bot · 2018-05-28T14:39:10Z

Checked commit miha-plesko@aef06b5 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. ⭐

agrare · 2018-05-30T12:50:05Z

app/models/manageiq/providers/vmware/cloud_manager/vm/reconfigure.rb

@@ -28,6 +28,11 @@ def disk_default_type
    'LSI Logic Parallel SCSI'
  end

+  def available_adapter_names
+    available = (0..3).to_a - network_ports.map { |nic| nic_idx(nic.name) }.map(&:to_i)


Just want to leave a comment here for the record that we believe 4 NICs is a current UI limitation, if this changes in the future we should come back to this.

Reconfigure VM network connectivity aka. NICs

Reconfigure VM network connectivity aka. NICs (cherry picked from commit b1dfc85) https://bugzilla.redhat.com/show_bug.cgi?id=1584699

simaishi · 2018-06-01T13:13:19Z

Gaprindashvili backport details:

$ git log -1
commit 5359c57a195fa9d5555581d480cd55c0be09e8be
Author: Adam Grare <agrare@redhat.com>
Date:   Wed May 30 08:50:37 2018 -0400

    Merge pull request #272 from miha-plesko/network-reconfigure
    
    Reconfigure VM network connectivity aka. NICs
    (cherry picked from commit b1dfc85630c7894848f97bfadab26c9e15cf4333)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1584699

Add changes for service, vm retire request approval

miq-bot assigned agrare May 22, 2018

miq-bot added enhancement gaprindashvili/yes labels May 22, 2018

miha-plesko mentioned this pull request May 22, 2018

Network adapter reconfiguration for vCloud provider ManageIQ/manageiq-ui-classic#3972

Merged

miha-plesko force-pushed the network-reconfigure branch 2 times, most recently from 6860ad6 to 8e2b911 Compare May 22, 2018 14:20

agrare reviewed May 23, 2018

View reviewed changes

miha-plesko force-pushed the network-reconfigure branch from 8e2b911 to c6b9dae Compare May 24, 2018 13:22

miha-plesko commented May 24, 2018

View reviewed changes

himdel reviewed May 24, 2018

View reviewed changes

miha-plesko force-pushed the network-reconfigure branch 2 times, most recently from 41548b1 to 35b5e6d Compare May 25, 2018 09:19

miha-plesko force-pushed the network-reconfigure branch 2 times, most recently from 9f08cc3 to 732339c Compare May 25, 2018 11:01

miha-plesko changed the title ~~Reconfigure VM network connectivity aka. NICs~~ [WIP] Reconfigure VM network connectivity aka. NICs May 28, 2018

miq-bot added the wip label May 28, 2018

miha-plesko force-pushed the network-reconfigure branch from 732339c to aef06b5 Compare May 28, 2018 14:35

miha-plesko changed the title ~~[WIP] Reconfigure VM network connectivity aka. NICs~~ Reconfigure VM network connectivity aka. NICs May 28, 2018

miq-bot removed the wip label May 28, 2018

agrare reviewed May 30, 2018

View reviewed changes

agrare approved these changes May 30, 2018

View reviewed changes

agrare merged commit aef06b5 into ManageIQ:master May 30, 2018

agrare added a commit that referenced this pull request May 30, 2018

Merge pull request #272 from miha-plesko/network-reconfigure

b1dfc85

Reconfigure VM network connectivity aka. NICs

agrare added this to the Sprint 87 Ending Jun 4, 2018 milestone May 30, 2018

simaishi pushed a commit that referenced this pull request Jun 1, 2018

Merge pull request #272 from miha-plesko/network-reconfigure

5359c57

Reconfigure VM network connectivity aka. NICs (cherry picked from commit b1dfc85) https://bugzilla.redhat.com/show_bug.cgi?id=1584699

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jun 1, 2018

This was referenced Jun 1, 2018

Reconfigure VM: Add / Remove Network Adapters ManageIQ/manageiq-ui-classic#3121

Merged

Reconfigure VM: Add / Remove Network Adapters #163

Merged

Reconfigure VM: Add / Remove Network Adapters ManageIQ/manageiq#16700

Merged

miha-plesko deleted the network-reconfigure branch January 7, 2019 08:26

agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019

Merge pull request ManageIQ#272 from d-m-u/request_approval

995e7cb

Add changes for service, vm retire request approval

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconfigure VM network connectivity aka. NICs #272

Reconfigure VM network connectivity aka. NICs #272

miha-plesko commented May 22, 2018 •

edited

Loading

miha-plesko commented May 22, 2018

agrare May 23, 2018

miha-plesko May 23, 2018

agrare May 23, 2018

agrare May 23, 2018

miha-plesko May 24, 2018

agrare May 23, 2018

Ladas May 24, 2018

miha-plesko May 24, 2018

miha-plesko left a comment

miha-plesko May 24, 2018

miha-plesko May 24, 2018

himdel May 24, 2018

agrare May 24, 2018

miha-plesko May 24, 2018

agrare May 24, 2018

miha-plesko commented May 25, 2018

miha-plesko commented May 28, 2018

miq-bot commented May 28, 2018

agrare May 30, 2018

simaishi commented Jun 1, 2018

Reconfigure VM network connectivity aka. NICs #272

Reconfigure VM network connectivity aka. NICs #272

Conversation

miha-plesko commented May 22, 2018 • edited Loading

miha-plesko commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented May 25, 2018

miha-plesko commented May 28, 2018

miq-bot commented May 28, 2018

Choose a reason for hiding this comment

simaishi commented Jun 1, 2018

miha-plesko commented May 22, 2018 •

edited

Loading