Allow VMware provision without needing to specify a host #11437

agrare · 2016-09-21T23:05:01Z

This will allow a user to provision a VM to a VMware DRS-enabled cluster without having to specify a specific host. This aligns MIQ provision workflow with the VMware cloning workflow when you have a DRS cluster.

Lookup a host in the cluster to find the right storage ems_ref (https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/vmware/infra_manager/provision/cloning.rb#L113)
Be able to use VDS switches/portgroups without having to lookup by host ems_ref (https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/vmware/infra_manager/provision/configuration/network.rb#L83)

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

agrare · 2016-09-22T14:17:37Z

app/models/manageiq/providers/vmware/infra_manager/provision/cloning.rb

  end

  def dest_folder
    folder_id = get_option(:placement_folder_name)
    return EmsFolder.find_by(:id => folder_id) if folder_id

-    host_dc = dest_host.parent_datacenter || dest_host.ems_cluster.parent_datacenter
+    dc = dest_cluster ? dest_cluster.parent_datacenter : dest_host.parent_datacenter


A host always has at least a ComputeResource but for some reason we only inventory ClusterComputeResource
This line could be simplified to just dc = dest_cluster.parent_datacenter if we change that, might revisit later.

same simplification

gmcculloug · 2016-09-22T16:51:23Z

@bdunne Please review

agrare · 2016-09-22T16:53:22Z

app/models/manageiq/providers/vmware/infra_manager/provision/placement.rb

+    datastore = selected_placement_obj(:placement_ds_name, Storage)
+
+    raise MiqException::MiqProvisionError, "Destination placement_ds_name not provided" if datastore.nil?
+    raise MiqException::MiqProvisionError, "Destination placement_host_name and placement_cluster_name not provided" if host.nil? && cluster.nil?


One test relies on this method raising an exception if these are missing, not sure if its better to do the checks here or in prepare_for_clone_task though

The selected_placement_obj method used by both the manual_placement and automatic_placement methods use to raise an error if the object was not defined. Now the manual_placement method is checking here and the automatic_placement will not catch the error until prepare_for_clone_task.

Can we consolidate these checks in the placement method above? I would move the new raise logic in prepare_for_clone_task and do that in the placement method as well. It makes more sense to do all our placement checking in that one place.

Moved all new checks to placement

bdunne · 2016-09-22T17:38:15Z

app/models/manageiq/providers/vmware/infra_manager/provision/cloning.rb

@@ -54,18 +57,17 @@ def dest_resource_pool
    resource_pool = ResourcePool.find_by(:id => respool_id) unless respool_id.nil?
    return resource_pool unless resource_pool.nil?

-    cluster = dest_host.owning_cluster
-    cluster ? cluster.default_resource_pool : dest_host.default_resource_pool
+    dest_cluster ? dest_cluster.default_resource_pool : dest_host.default_resource_pool


Could be simplified to:

dest_cluster.try(:default_resource_pool) || dest_host.default_resource_pool

bdunne · 2016-09-22T17:38:48Z

app/models/manageiq/providers/vmware/infra_manager/provision/cloning.rb

  end

  def dest_folder
    folder_id = get_option(:placement_folder_name)
    return EmsFolder.find_by(:id => folder_id) if folder_id

-    host_dc = dest_host.parent_datacenter || dest_host.ems_cluster.parent_datacenter
+    dc = dest_cluster ? dest_cluster.parent_datacenter : dest_host.parent_datacenter


same simplification

bdunne · 2016-09-22T17:43:14Z

app/models/manageiq/providers/vmware/infra_manager/provision/cloning.rb

+    host_ids = if clone_options[:host]
+                 clone_options[:host].id
+               else
+                 clone_options[:cluster].hosts.collect(&:id)


Is .hosts an ActiveRecord Collection? If so, pluck(:id) would be slightly more efficient.

bdunne · 2016-09-22T17:48:29Z

app/models/manageiq/providers/vmware/infra_manager/provision/placement.rb

    datastore ||= selected_placement_obj(:placement_ds_name, Storage)
-    return host, datastore
+    cluster     = selected_placement_obj(:placement_cluster_name, EmsCluster) || (host.ems_cluster if host)


Prefer host.try(:ems_cluster) over (host.ems_cluster if host)

bdunne · 2016-09-22T17:52:18Z

product/dialogs/miq_dialogs/miq_provision_vmware_dialogs_template.yaml

@@ -435,7 +435,7 @@
          :auto_select_single: false
          :description: Name
          :required_method: :validate_placement
-          :required: true
+          :required: false


Since this is no longer required, the :required_method: will not run. I would suggest changing it to a validation_method and verify that validate_placement returns the expected result with and without a host selected.

@gmcculloug mentioned not wanting this in here at all yet so it might be moot

I would suggest creating a separate PR for the dialog/workflow validation changes. I suggested this change to @agrare for local testing but did not expect it to be part of the PR.

Yeah my misunderstanding didn't realize it was for local dev, removed.

agrare · 2016-09-22T20:13:16Z

Got rid of a VIM API call here now that we have dvSwitches in VMDB 😄

How bad would it be to have allowed_vlans return [lan.id, lan.name] in options[:network] similar to hosts/clusters/folders? Right now it just passes the name, that way we could avoid having to do this lookup

agrare · 2016-09-26T12:40:59Z

@gmcculloug can you review?

@miq-bot remove-label wip

gmcculloug

@agrare Do not change the return values for allowed_vlans in this PR since that will cause issues for automate users. We can look into doing that in a separate PR where we can focus on backwards compatibility. The details would get lost in this PR and we would also not want to back port that change to the Darga branch.

gmcculloug · 2016-09-26T20:28:31Z

app/models/manageiq/providers/vmware/infra_manager/provision/cloning.rb

+    # Find a host in the cluster that has this storage mounted to get the right ems_ref for this
+    # datastore in the datacenter
+    datastore = HostStorage.find_by(:storage_id => clone_options[:datastore].id, :host_id => host_ids)
+    vim_clone_options[:datastore] = datastore.ems_ref if datastore


Now that this logic is more than a single line it would be clearer to have it in a separate method.

vim_clone_options[:datastore] = datastore_ems_ref(clone_options)

gmcculloug · 2016-09-26T21:31:02Z

app/models/manageiq/providers/vmware/infra_manager/provision/placement.rb

+    datastore = selected_placement_obj(:placement_ds_name, Storage)
+
+    raise MiqException::MiqProvisionError, "Destination placement_ds_name not provided" if datastore.nil?
+    raise MiqException::MiqProvisionError, "Destination placement_host_name and placement_cluster_name not provided" if host.nil? && cluster.nil?


The selected_placement_obj method used by both the manual_placement and automatic_placement methods use to raise an error if the object was not defined. Now the manual_placement method is checking here and the automatic_placement will not catch the error until prepare_for_clone_task.

Can we consolidate these checks in the placement method above? I would move the new raise logic in prepare_for_clone_task and do that in the placement method as well. It makes more sense to do all our placement checking in that one place.

gmcculloug · 2016-09-26T21:34:14Z

product/dialogs/miq_dialogs/miq_provision_vmware_dialogs_template.yaml

@@ -435,7 +435,7 @@
          :auto_select_single: false
          :description: Name
          :required_method: :validate_placement
-          :required: true
+          :required: false


I would suggest creating a separate PR for the dialog/workflow validation changes. I suggested this change to @agrare for local testing but did not expect it to be part of the PR.

agrare · 2016-09-27T17:32:47Z

@gmcculloug made requested changes and re-tested

miq-bot · 2016-09-27T17:37:42Z

Checked commits agrare/manageiq@cc6906c~...dca8127 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
6 files checked, 5 offenses detected

app/models/manageiq/providers/vmware/infra_manager/provision/cloning.rb

❕ - Line 81, Col 121 - Metrics/LineLength - Line is too long. [144/120]
❕ - Line 82, Col 121 - Metrics/LineLength - Line is too long. [141/120]

app/models/manageiq/providers/vmware/infra_manager/provision/placement.rb

❕ - Line 14, Col 121 - Metrics/LineLength - Line is too long. [145/120]
❕ - Line 15, Col 121 - Metrics/LineLength - Line is too long. [134/120]

spec/models/manageiq/providers/vmware/infra_manager/provision/placement_spec.rb

❕ - Line 9, Col 121 - Metrics/LineLength - Line is too long. [124/120]

gmcculloug · 2016-09-27T18:31:12Z

@agrare Thanks. The two remaining tasks in the description will be moved to a separate PR, correct?

@bdunne Changes look good to me. Please give this one final review.

agrare · 2016-09-27T18:31:49Z

@gmcculloug correct, I'll take them out of the TODO list

bdunne · 2016-09-27T18:53:13Z

👍 LGTM

agrare · 2016-09-27T18:53:51Z

Thanks guys!

bdunne · 2016-09-27T18:55:13Z

@agrare This needs to go back to the euwe branch, right?

agrare · 2016-09-27T18:56:22Z

@bdunne yes, there's a 5.6.z clone also (https://bugzilla.redhat.com/show_bug.cgi?id=1378116)

Allow VMware provision without needing to specify a host (cherry picked from commit de4f24f)

chessbyte · 2016-09-29T22:50:04Z

Euwe Backport details:

$ git log
commit 92faa3252b44f9b8227537d037349c6ae68fa9b7
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Tue Sep 27 14:53:16 2016 -0400

    Merge pull request #11437 from agrare/vmware_infra_provision_no_host

    Allow VMware provision without needing to specify a host
    (cherry picked from commit de4f24f31c66681066a0b85931653d872818c4ab)

chessbyte · 2016-10-07T18:56:10Z

Darga BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1378116

chessbyte · 2016-10-07T19:22:12Z

This should be backported AFTER #11135

Allow VMware provision without needing to specify a host (cherry picked from commit de4f24f) https://bugzilla.redhat.com/show_bug.cgi?id=1378116

chessbyte · 2016-10-11T13:17:23Z

Darga Backport details:

$ git log
commit a6a212a019cb8191c022dbe97d6496aa20fd482e
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Tue Sep 27 14:53:16 2016 -0400

    Merge pull request #11437 from agrare/vmware_infra_provision_no_host

    Allow VMware provision without needing to specify a host
    (cherry picked from commit de4f24f31c66681066a0b85931653d872818c4ab)

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

chessbyte added enhancement lifecycle/provisioning providers/vmware/infra labels Sep 22, 2016

chessbyte assigned gmcculloug Sep 22, 2016

chessbyte added the wip label Sep 22, 2016

agrare force-pushed the vmware_infra_provision_no_host branch from 887cdf6 to 1e044b5 Compare September 22, 2016 13:13

agrare commented Sep 22, 2016

View reviewed changes

agrare force-pushed the vmware_infra_provision_no_host branch from 008c857 to 2845e48 Compare September 22, 2016 15:54

agrare commented Sep 22, 2016

View reviewed changes

bdunne reviewed Sep 22, 2016

View reviewed changes

agrare force-pushed the vmware_infra_provision_no_host branch 2 times, most recently from 562db92 to a10e83c Compare September 22, 2016 19:55

agrare changed the title ~~[WIP] Allow VMware provision without needing to specify a host~~ Allow VMware provision without needing to specify a host Sep 26, 2016

miq-bot removed the wip label Sep 26, 2016

gmcculloug reviewed Sep 26, 2016

View reviewed changes

agrare force-pushed the vmware_infra_provision_no_host branch 2 times, most recently from a9b0be1 to c557796 Compare September 27, 2016 12:47

gmcculloug mentioned this pull request Sep 27, 2016

fix vmware provision bug #11531

Closed

agrare added 9 commits September 27, 2016 11:54

Successfully provisioned without a host

cc6906c

Use dest_cluster

9618843

Check for non-DRS enabled clusters

44e85f7

Select a cluster if host is given in placement

5ccecd3

Handle provisioning to a host not in a cluster

e216aba

Lookup datastore on any host in the cluster

c2c90b3

Fix provision spec tests

0498c78

Lookup network in DB instead of asking the vCenter

4570d2b

Move datastore ems_ref to a function

51e70c8

Move placement checks to common location

dca8127

agrare force-pushed the vmware_infra_provision_no_host branch from c557796 to dca8127 Compare September 27, 2016 17:35

bdunne merged commit de4f24f into ManageIQ:master Sep 27, 2016

bdunne added this to the Sprint 47 Ending Oct 3, 2016 milestone Sep 27, 2016

agrare deleted the vmware_infra_provision_no_host branch September 27, 2016 18:53

gmcculloug added darga/yes labels Sep 27, 2016

chessbyte pushed a commit that referenced this pull request Sep 29, 2016

Merge pull request #11437 from agrare/vmware_infra_provision_no_host

92faa32

Allow VMware provision without needing to specify a host (cherry picked from commit de4f24f)

chessbyte added euwe/backported and removed euwe/yes labels Sep 29, 2016

gmcculloug mentioned this pull request Oct 5, 2016

Update changelog for Sprint 47 #11624

Merged

agrare mentioned this pull request Oct 7, 2016

Validate either a host or cluster is selected when provisioning on VMware #11665

Merged

chessbyte added darga/backported and removed darga/yes labels Oct 11, 2016

Fryguy added the providers/infrastructure label Jan 24, 2020

Allow VMware provision without needing to specify a host #11437

Allow VMware provision without needing to specify a host #11437

Conversation

agrare commented Sep 21, 2016 • edited

agrare Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcculloug commented Sep 22, 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

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

agrare commented Sep 22, 2016 • edited

agrare commented Sep 26, 2016

gmcculloug 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

agrare commented Sep 27, 2016

miq-bot commented Sep 27, 2016

gmcculloug commented Sep 27, 2016

agrare commented Sep 27, 2016

bdunne commented Sep 27, 2016

agrare commented Sep 27, 2016

bdunne commented Sep 27, 2016

agrare commented Sep 27, 2016

chessbyte commented Sep 29, 2016

chessbyte commented Oct 7, 2016

chessbyte commented Oct 7, 2016

chessbyte commented Oct 11, 2016

agrare commented Sep 21, 2016 •

edited

agrare Sep 22, 2016 •

edited

agrare commented Sep 22, 2016 •

edited