Add VM specific customisations to vApp orchestration #12273

gberginc · 2016-10-28T14:53:03Z

Previously users were only allowed to modify the name of the provisioned vApp and parameters that control the power state of the vApp.

With this change we are adding support for changing the name of the instance as well as the ability to select a different VDC network to which instances may be connected.

Currently we are focusing on the UI part of the changes. I am also adding the spec file for these kinds of tests (it's not complete yet as more different cases are needed). The spec file was previously not used because I was mainly following other orchestration templates.

@miq-bot add_label providers/vmware/cloud, enhancement, ui, wip

gberginc · 2016-10-28T14:57:18Z

app/models/manageiq/providers/vmware/cloud_manager/orchestration_template.rb

+            :name          => "vdc_network-#{vm_id}",
+            :label         => "Network",
+            :data_type     => "string",
+            :default_value => "(default)",


I am still not sure whether I should be using i18n here or not. Other templates will output whatever there is in the template itself, but this is a special case and I believe we could use it...

If it will show up on the UI I'd put _() around the string to be safe

gberginc · 2016-10-28T15:01:36Z

app/models/manageiq/providers/vmware/cloud_manager/orchestration_template.rb

+    return "(default)" if cloud_networks.empty?
+
+    # Get network names.
+    vdc_network_names = cloud_networks.collect(&:name)


Here I am only collecting names of the networks, however I'd rather see if I could create key,value pairs. The list of names is used here and converted into the actual dialog field in OrchestrationTemplateDialogService class.

What I am thinking is if it would be possible to change the code so that it would allow setting up key, value and the corresponding code would use keys if they exist, otherwise it would use the same logic as today.

@gberginc I like the idea but what keys were you thinking of using and what would it make simpler? I could see :default => "(default)" instead of using the string directly.
We'll want to check with @bzwei about changing OrchestrationParameterAllowed to support this

@agrare currently I can only return the names of the VDC networks that are to be displayed in the dropdown. The logic that will create the actual request to the VMware API I'll need to find the IDs of these networks so I thought that the dropdown would have the ability to set these from the code, i.e. {nil => (default), id1 => VDC net 1, ...}.

If we just pass ["(default)", "VDC net 1", ...], the code would work exactly as it does now.

@agrare this class is what I am referring to in my previous comments. This class receives values from the form shown in the above screenshot and uses those values to construct provider-specific options struct.

Currently, this class uses only those parameters in "Options". In this class I'd like to get IDs (ems_id), instead of network names so that I don't need to lookup in the database for these IDs.

I am ok to change OrchestrationParameterAllowed to be a hash rather than an array. We may accept both formats for backward comparability purpose.

gberginc · 2016-10-28T15:02:44Z

spec/models/manageiq/providers/vmware/cloud_manager/orchestration_template_spec.rb

+      expect(orchestration_template.content.include?('ovf:Envelope')).to be_truthy
+    end
+
+    it "is parsed using MmeiqXml" do


Ooops :-). The typo will be fixed.

miq-bot · 2016-11-09T13:32:30Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

bzwei · 2016-11-29T14:24:29Z

app/models/manageiq/providers/vmware/cloud_manager/orchestration_service_option_converter.rb

      }
+      options[:vdc_id] = @dialog_options['dialog_availability_zone'] if @dialog_options.key?("dialog_availability_zone") && !@dialog_options["dialog_availability_zone"].nil? && !@dialog_options["dialog_availability_zone"].empty?


can be simplified as

options[:vdc_id] = @dialog_options['dialog_availability_zone'] unless @dialog_options['dialog_availability_zone'].blank?

bzwei · 2016-11-29T14:38:06Z

app/models/manageiq/providers/vmware/cloud_manager/orchestration_service_option_converter.rb

+      network_config = {}
+      source_vms = []
+
+      vm_params.each do |vm_id, vm_opts|


prefer source_vms = vm_params.collect

bzwei · 2016-11-29T14:40:24Z

app/models/manageiq/providers/vmware/cloud_manager/orchestration_service_option_converter.rb

+      end
+
+      # Create options suitable for VMware vCloud provider.
+      custom_opts = {


The hash key naming is very confusing all over this class. Why a mix with CamelCase, camelCase, and snack_case?

It's how fog library that is used to connect to vCloud names these parameters. I had the same issues, but in the end I wanted to make minimal changes to fog.

bzwei · 2016-11-29T14:51:22Z

app/models/manageiq/providers/vmware/cloud_manager/orchestration_service_option_converter.rb

+          next if param_match.nil?
+
+          vm_id = param_match.captures.first
+          vm_params[vm_id] = {} unless vm_params.key?(vm_id)


seems to me that you always want to initialize vm_params[vm_id] = {} and in next line set [param] conditionally

@bzweil can you explain a bit more? Line 72 creates new empty object in vm_params with vm_id as its key, but only if it wasn't created before. The next line will store specific parameter value for this VM.

@gberginc You're right. I misinterpreted it. But this line can be simplified as vm_params[vm_id] ||= {}

gberginc · 2016-11-29T19:03:07Z

@bzwei All comments addressed, apart from the one on the hash key names due to the external requirement from the fog library.

bzwei · 2016-11-30T13:52:27Z

app/models/manageiq/providers/vmware/cloud_manager/orchestration_service_option_converter.rb

+          } unless network_config.key?(network_id)
+
+          # Add network configuration to the source VM.
+          src_vm[:networks] = [


should it be an array [] or hash {}?

It has to be an array of objects. Since all elements are :key => value, this implicitly creates an object. Would you prefer that I do it explicitly, so that I create it like

src_vm[:networks] = [ { :networkName => network_id, :IsConnected => true, :IpAddressAllocationMode => "DHCP" } ]

Your original code is OK. Just want to make sure it is according to design.

Does this mean the API takes multiple networks for a single VM but your application limits it to one?

We are trying to mimic vCloud Director UI, where users seem to be able to only specify a single network.

bzwei · 2016-11-30T14:13:22Z

app/models/manageiq/providers/vmware/cloud_manager/orchestration_service_option_converter.rb

+        network_id = vm_opts["vdc_network"]
+        unless network_id.nil?
+          # Create new network config if it hasn't been created before.
+          network_config[network_id] = {


network_config[network_id] ||= { and no need for unless check.

I really appreciate these kind of comments! I will wait for the comment about explicitly storing the object in the array above and then resubmit.

Previously users were only allowed to modify the name of the provisioned vApp and parameters that control the power state of the vApp. With this change we are adding support for changing the name of the instances that are deployed as part of the vApp as well as the ability to select a different VDC network to which instances may be connected. Similarly to the vCloud Director, each instance may be connected to a different network. Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>

miq-bot · 2016-11-30T15:57:43Z

Checked commit xlab-si@20b9aa3 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
5 files checked, 8 offenses detected

app/models/manageiq/providers/vmware/cloud_manager/orchestration_service_option_converter.rb

❕ - Line 10, Col 121 - Metrics/LineLength - Line is too long. [126/120]
❕ - Line 23, Col 5 - Metrics/MethodLength - Method has too many lines. [26/25]

app/models/manageiq/providers/vmware/cloud_manager/orchestration_template.rb

❕ - Line 40, Col 3 - Metrics/MethodLength - Method has too many lines. [26/25]
❕ - Line 65, Col 121 - Metrics/LineLength - Line is too long. [154/120]

spec/models/manageiq/providers/vmware/cloud_manager/orchestration_template_spec.rb

❕ - Line 103, Col 121 - Metrics/LineLength - Line is too long. [123/120]
❕ - Line 37, Col 121 - Metrics/LineLength - Line is too long. [125/120]
❕ - Line 65, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for assert_vapp_parameter_group is too high. [43/20]
❕ - Line 86, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for assert_vm_parameter_group is too high. [24.02/20]

gberginc · 2016-12-14T14:53:44Z

@agrare I'was able to test this PR using the existing testbed and works as expected. It properly configures the name of VMs being deployed as part of vApp template instantiation. It also connects VMs to the specified VDC networks.

I am thus removing WIP label.

@miq-bot remove_label wip

agrare · 2016-12-14T15:51:36Z

LGTM 👍

chargio · 2016-12-15T14:52:39Z

Hi @agrare, it looks like a nice enhancement to the provider. Should it not be backported?

agrare · 2016-12-15T16:21:54Z

@sergio-ocon generally speaking enhancements aren't backported, if we have a BZ and it gets all ack's for a z-stream clone we'll backport it.

miq-bot added enhancement providers/vmware/cloud ui wip labels Oct 28, 2016

gberginc commented Oct 28, 2016

View reviewed changes

agrare self-assigned this Nov 1, 2016

gberginc force-pushed the add_vmware_cloud_orchestration_customisation branch from 8f5e4f2 to 1b2aef5 Compare November 7, 2016 19:15

gberginc mentioned this pull request Nov 11, 2016

Support hash values in dropdown orchestration dialog fields #12570

Merged

gberginc force-pushed the add_vmware_cloud_orchestration_customisation branch 2 times, most recently from e4000aa to c2c2eb3 Compare November 11, 2016 10:17

gberginc mentioned this pull request Nov 16, 2016

Add dynamic dropdown list support for orchestration service dialog #12693

Merged

gberginc force-pushed the add_vmware_cloud_orchestration_customisation branch from c2c2eb3 to edce1c1 Compare November 29, 2016 10:44

miq-bot added the dependencies label Nov 29, 2016

gberginc force-pushed the add_vmware_cloud_orchestration_customisation branch 2 times, most recently from 3c05d8c to 507408b Compare November 29, 2016 11:00

bzwei reviewed Nov 29, 2016

View reviewed changes

gberginc force-pushed the add_vmware_cloud_orchestration_customisation branch from 507408b to 17659d6 Compare November 29, 2016 19:01

gberginc force-pushed the add_vmware_cloud_orchestration_customisation branch from 17659d6 to 19bdbee Compare November 29, 2016 21:16

bzwei reviewed Nov 30, 2016

View reviewed changes

gberginc force-pushed the add_vmware_cloud_orchestration_customisation branch from 19bdbee to 20b9aa3 Compare November 30, 2016 15:53

gberginc changed the title ~~[WIP] Add VM specific customisations to vApp orchestration~~ Add VM specific customisations to vApp orchestration Dec 14, 2016

agrare removed the wip label Dec 14, 2016

agrare merged commit cebf291 into ManageIQ:master Dec 14, 2016

agrare added the euwe/no label Dec 14, 2016

agrare added this to the Sprint 51 Ending Jan 2, 2017 milestone Dec 14, 2016

tadeboro deleted the add_vmware_cloud_orchestration_customisation branch March 30, 2018 07:57

Fryguy added the providers/cloud label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VM specific customisations to vApp orchestration #12273

Add VM specific customisations to vApp orchestration #12273

gberginc commented Oct 28, 2016 •

edited

Loading

gberginc Oct 28, 2016

agrare Nov 7, 2016

gberginc Oct 28, 2016

agrare Nov 7, 2016

gberginc Nov 7, 2016

gberginc Nov 7, 2016

bzwei Nov 7, 2016

gberginc Oct 28, 2016

miq-bot commented Nov 9, 2016

bzwei Nov 29, 2016

bzwei Nov 29, 2016

bzwei Nov 29, 2016

gberginc Nov 29, 2016

bzwei Nov 29, 2016

gberginc Nov 29, 2016

bzwei Nov 29, 2016

gberginc commented Nov 29, 2016

bzwei Nov 30, 2016

gberginc Nov 30, 2016

bzwei Nov 30, 2016

bzwei Nov 30, 2016

gberginc Nov 30, 2016

bzwei Nov 30, 2016

gberginc Nov 30, 2016

miq-bot commented Nov 30, 2016

gberginc commented Dec 14, 2016

agrare commented Dec 14, 2016

chargio commented Dec 15, 2016 •

edited

Loading

agrare commented Dec 15, 2016

		}
		options[:vdc_id] = @dialog_options['dialog_availability_zone'] if @dialog_options.key?("dialog_availability_zone") && !@dialog_options["dialog_availability_zone"].nil? && !@dialog_options["dialog_availability_zone"].empty?

Add VM specific customisations to vApp orchestration #12273

Add VM specific customisations to vApp orchestration #12273

Conversation

gberginc commented Oct 28, 2016 • edited Loading

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

miq-bot commented Nov 9, 2016

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

gberginc commented Nov 29, 2016

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

miq-bot commented Nov 30, 2016

gberginc commented Dec 14, 2016

agrare commented Dec 14, 2016

chargio commented Dec 15, 2016 • edited Loading

agrare commented Dec 15, 2016

gberginc commented Oct 28, 2016 •

edited

Loading

chargio commented Dec 15, 2016 •

edited

Loading