New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

r/vapp_container: Add vapp_container resource #566

Merged

bill-rich merged 10 commits into master from f-vapp-container

Aug 6, 2018

Contributor

bill-rich commented Jul 13, 2018

No description provided.

bill-rich requested a review from a team

July 13, 2018 06:20

bill-rich force-pushed the f-vapp-container branch from 9405c9d to 2d8450d Compare

July 13, 2018 06:47

Contributor

vancluever commented Jul 13, 2018

@bill-rich could this PR be cleaned up a bit? I see a bunch of my commits in there. Maybe rebase your current changes on master?

git checkout f-vapp-container
git fetch origin master
git rebase master

That should make things a lot clearer and may resolve merge conflicts as well.


          r/vapp_container: Add vapp_container resource

ade70fd

bill-rich force-pushed the f-vapp-container branch from 2d8450d to ade70fd Compare

July 13, 2018 16:16

Contributor Author

bill-rich commented Jul 13, 2018

@vancluever Thanks! I missed that. Got it sorted now.

vancluever suggested changes

View reviewed changes

Contributor

vancluever left a comment

Hey @bill-rich! This looks mostly good. We should address these comments I think and then maybe I'll give a manual test a spin to see how it runs. Let me know!

vsphere/internal/helper/storagepod/storage_pod_helper.go Outdated

+              	// If the parent resource pool is a vApp, we need to create the VM using the
+              	// CreateChildVM vApp function instead of directly using SDRS recommendations.
+              	if sps.ResourcePool != nil {
+              		if vc, err := vappcontainer.FromID(client, sps.ResourcePool.Reference().Value); err == nil {

Contributor

vancluever Jul 16, 2018

This logic assumes that the only error that we are ever going to receive from vappcontainer.FromID will be ManagedObjectNotFound, which has implications further down the stack. You will want to handle the error, possibly in a switch:

vc, err := vappcontainer.FromID(client, sps.ResourcePool.Reference().Value)
switch {
case viapi.IsManagedObjectNotFoundError(err):
  // pass
case nil:
  // vApp container creation logic here
default:
  return err
}

vsphere/internal/helper/storagepod/storage_pod_helper.go Outdated

               	return err
               }
-              func recommendAndApplySDRS(

Contributor

vancluever Jul 16, 2018

Rather than deleting this function, I'd recommend taking the de-coupled logic that has been duplicated to other parts and just throwing that here. Then the rest of the places that reference recommendAndApplySDRS that don't require a special path to address vApp containers don't need changes at all.

Contributor Author

bill-rich Jul 17, 2018

The reason I did it this way was because I still need the recommendation in order to determine which datastore to use in the case where the VM is being created in a vApp. I didn't want to clutter up recommendAndApplySDRS with the vApp code. Does that make sense? Very possible I'm just overlooking something simple.

Contributor

vancluever Jul 19, 2018

I'm not saying to actually revert the de-coupling, but rather than removing the function altogether and then duplicating code a few places, just change recommendAndApplySDRS to:

func recommendAndApplySDRS(
	client *govmomi.Client,
	sps types.StoragePlacementSpec,
	timeout time.Duration,
) (*object.VirtualMachine, error) {
	placement, err := recommendSDRS(client, sps, time.Minute*time.Duration(timeout))
	if err != nil {
		return nil, err
	}
	return applySDRS(client, placement, time.Minute*time.Duration(timeout))
}

This way, you don't have to update the other places that this is being used (where you don't need the recommendation), such as #566 (comment), #566 (comment), or #566 (comment).

vsphere/internal/helper/storagepod/storage_pod_helper.go Outdated

@@ @@ -228,7 +250,11 @@ func CloneVM( @@
               		Type: string(types.StoragePlacementSpecPlacementTypeClone),
               	}
-              	return recommendAndApplySDRS(client, sps, time.Minute*time.Duration(timeout))

Contributor

vancluever Jul 16, 2018

See comment on recommendAndApplySDRS

vsphere/internal/helper/storagepod/storage_pod_helper.go Outdated

@@ @@ -268,7 +294,11 @@ func ReconfigureVM( @@
               		ConfigSpec: &spec,
               	}
-              	_, err = recommendAndApplySDRS(client, sps, provider.DefaultAPITimeout)

Contributor

vancluever Jul 16, 2018

See comment on recommendAndApplySDRS

vsphere/internal/helper/storagepod/storage_pod_helper.go Outdated

@@ @@ -305,16 +335,16 @@ func RelocateVM( @@
               		Type:         string(types.StoragePlacementSpecPlacementTypeRelocate),
               	}
-              	_, err = recommendAndApplySDRS(client, sps, time.Minute*time.Duration(timeout))

Contributor

vancluever Jul 16, 2018

See comment on recommendAndApplySDRS

website/docs/r/vapp_container.html.markdown Outdated

+              # vsphere\_vapp\_container
+              The `vsphere_vApp_container` resource can be used to create and manage
+              Virtual Appliances.

Contributor

vancluever Jul 16, 2018

This is not accurate, correct? Eventually we might allow OVAs through it (which could allow for the deploy of multi-VM vApps), but currently it's just a container.

Contributor Author

bill-rich Jul 17, 2018

It is not correct. Seems I got Virtual Appliance in my head when I was writing the docs. Thanks for catching that!

website/docs/r/vapp_container.html.markdown Outdated

+              The `vsphere_vApp_container` resource can be used to create and manage
+              Virtual Appliances.
+              For more information on vSphere Virtual Appliances, see [this

Contributor

vancluever Jul 16, 2018

Terminology also here. Might be worth it to scan the doc for all reference of "Virtual Appliance" and just replace with "vApp".

website/docs/r/vapp_container.html.markdown Outdated

+                or standalone host, or a resource pool itself. When moving a vApp container
+                from one parent resource pool to another, both must share a common root
+                resource pool or the move will fail.
+              * `parent_folder_id` - (Optional) The [manage object ID][docs-about-morefs] of

Contributor

vancluever Jul 16, 2018

s/manage/managed/

Contributor Author

bill-rich Jul 17, 2018

👍

vsphere/resource_vsphere_vapp_container_test.go Outdated

+              	}
+              }
+              func testGetVAppContainer(s *terraform.State, resourceName string) (*object.VirtualApp, error) {

Contributor

vancluever Jul 16, 2018

Can this be thrown into helper_test.go just in case it's needed by other tests other than vApp container tests?

This is where most of the other test helper code is stored.

Contributor Author

bill-rich Jul 17, 2018

Done

website/docs/r/vapp_container.html.markdown


		[ref-vsphere-vapp]: https://docs.vmware.com/en/VMware-vSphere/6.5/com.vmware.vsphere.vm_admin.doc/GUID-2A95EBB8-1779-40FA-B4FB-4D0845750879.html

		## Example Usage

This comment was marked as resolved.

# to view

Contributor Author

bill-rich Jul 17, 2018

Good idea. That probably isn't as obvious if you haven't been looking at it for hours.

bill-rich added 7 commits

July 17, 2018 15:57


          r/vapp_container: Raise unexpected errors

e0ee260


          r/vapp_container: Fix documentation terminology

3e8acf2


          r/resource_pool: Move testGetVAppContainer to helper_test.go

2832ea8


          r/vapp_container: Add a vApp_container example with a virtual machine

3ef2dcf


          r/vapp_container: Add resource pool checks to vApp container

edb1153


          r/vapp_container: Combine DRS recommend and apply

b22aa34

Also fix err shadowing and function complexity


          r/vapp_container: Handle InventoryPath for vApp VMs

b726036

bill-rich force-pushed the f-vapp-container branch 3 times, most recently from e630566 to 3cdb5b3 Compare

August 3, 2018 21:01


          r/vapp_container: Ignore VM folders in vApp Containers

04c0fbf

When a VM is part of a vApp, folder should be empty.
Add support moving into resource pools.

bill-rich force-pushed the f-vapp-container branch from 3cdb5b3 to 04c0fbf Compare

August 3, 2018 21:03

vancluever reviewed

View reviewed changes

vsphere/resource_vsphere_virtual_machine.go Outdated

               	if err := virtualdevice.DiskDiffOperation(d, client); err != nil {
               		return err
               	}
+              	// When a VM is a member of a vApp container, it is no longer part of the VM
+              	// tree, and therefor cannot have its VM folder set.

Contributor

vancluever Aug 6, 2018

s/therefor/therefore/

vancluever approved these changes

View reviewed changes


          /r/virtual_machine: Fix misspelling

c5f42b4

bill-rich merged commit d964862 into master

bill-rich deleted the f-vapp-container branch

August 6, 2018 17:43

ghost locked and limited conversation to collaborators

# for free to subscribe to this conversation on GitHub. Already have an account? #.

Labels

None yet