r/virtual_machine: add vApp properties #303

rowanjacobs · 2017-12-08T23:41:17Z

In order to support configurable vApp properties on VMs cloned from an OVF (#243), we have added a vapp subresource with a properties key-value map. In order to run acceptance tests, the template needs to have vApp properties with the keys example_key and another_key, whose initial and default values should be blank.

Note that we haven't been able to successfully implement zeroing out a property. This seems to likely be an issue with the vSphere API or client. But we think this should not block adding this feature in the mean time.

cc @mcwumbly @desmondrawls @genevievelesperance @evanfarrar

- Currently removing vApp properties doesn't work, which seems to potentially be a vSphere API or client issue. Signed-off-by: Rowan Jacobs <[email protected]>

rowanjacobs · 2017-12-13T19:04:21Z

@charandas I've verified that setting vapp.properties.guestinfo.coreos.config.data in coreos.config.data when cloning a VM sets the vApp property on the VM cloned from the CoreOS OVA. Are you using the clone setting? If not, how are you building the VM from the CoreOS OVA?

Here's what the VM configuration I'm using looks like:

resource "vsphere_virtual_machine" "vm" {
  name             = "terraform-test"
  resource_pool_id = "${data.vsphere_resource_pool.pool.id}"
  datastore_id     = "${data.vsphere_datastore.datastore.id}"

  num_cpus = 2
  memory   = 2048
  guest_id = "${data.vsphere_virtual_machine.template.guest_id}"

  network_interface {
    network_id   = "${data.vsphere_network.network.id}"
    adapter_type = "${data.vsphere_virtual_machine.template.network_interface_types[0]}"
  }

  disk {
    name = "terraform-test.vmdk"
    size = "${data.vsphere_virtual_machine.template.disks.0.size}"
  }

  vapp {
    properties {
      "guestinfo.hostname" = "khaleesi-terraform-test"
      "guestinfo.coreos.config.data" = "some guest data"
    }
  }

  clone {
    template_uuid = "${data.vsphere_virtual_machine.template.id}"
  }
}

rowanjacobs · 2017-12-13T19:59:37Z

I'm testing against vSphere 5.5.

charandas · 2017-12-13T20:16:49Z

I had missed the file changes in vsphere/virtual_machine_config_structure.go when building last night. Sorry for the false alarm. It works now. The reason I reverted to manually copying changes from your fork, is because of the following:

# would not build for me, with go 1.9.2
❯ go get -u github.com/rowanjacobs/terraform-provider-vsphere
# github.com/rowanjacobs/terraform-provider-vsphere
serve/gopath/src/github.com/rowanjacobs/terraform-provider-vsphere/main.go:10:15: cannot use vsphere.Provider (type func() "github.com/terraform-providers/terraform-provider-vsphere/vendor/github.com/hashicorp/terraform/terraform".ResourceProvider) as type "github.com/rowanjacobs/terraform-provider-vsphere/vendor/github.com/hashicorp/terraform/plugin".ProviderFunc in field value

# I basically tried to get it to build by copying over these changes
# into  github.com/terraform-providers/terraform-provider-vsphere in my gopath

vancluever · 2017-12-14T00:02:50Z

Hey @rowanjacobs, just wanted to say I have been watching this and it looks promising - I will give this a much closer look later this week!

One thing that I did notice when I looked earlier - does this support removal of the vApp properties? It looks like another attribute needs to be set to make this happen (vAppConfigRemoved). I'm kind of on the fence of how I would prefer this to be implemented as from what I remember, wiping the config removes them for good, and I don't know how easy they would be to re-construct after the fact. I'm wondering if maybe we can support that and source the proper settings from somewhere else, like a template?

In any case - will look later this week as mentioned.

tevaum · 2017-12-14T10:32:54Z

Just want to thank you guys for working on this stuff... I use terraform with vsphere to deploy some images that doesn't support guest customization and this is the only way to go for me, so I'm very excited seing you guys working on it!

vancluever · 2017-12-15T22:02:06Z

Hey @rowanjacobs, I'm about to give this a spin to see how it goes. I'll also see if I can solve the config zeroing problem as well.

Re: What I mentioned about removing vApp config, I don't think we need to concern ourselves with this right now 100%. A VM's OVF configuration is a complex thing that I think that we might not even want to touch with TF, treating vApp and OVF template configuration as kind of a template that that is out of our scope to manage, but rather something that would be up to something like Packer to configure as part of the image building process.

Anyway, will update soon!

vancluever · 2017-12-15T23:19:41Z

Hey @rowanjacobs, alright, finished checking, looks like there is an issue here.

Here is the vapp snippet of my config using the CoreOS OVA:

  vapp {
    properties {
      "guestinfo.hostname"                        = "${var.vm_name}.foobar.local"
      "guestinfo.interface.0.name"                = "ens192"
      "guestinfo.interface.0.ip.0.address"        = "172.16.1.150/24"
      "guestinfo.interface.0.route.0.gateway"     = "172.16.1.1"
      "guestinfo.interface.0.route.0.destination" = "0.0.0.0/0"
      "guestinfo.dns.server.0"                    = "172.16.1.1"
    }
  }

This is not writing a correct configuration. I think the best way to illustrate it is via how it seems to be overwriting some of the config parameters. Here is an example of the first three from the MOB.

The first parameter - guestinfo.hostname - seems to be writing correctly:

Before

After

However, the next couple get odd looking. This is the next parameter, the full cloud-config data, which should have not been touched. However if you look at the second picture here closely, you'll see that the key has been overwritten with the guestinfo.dns.server.0 key, which is empty:

Before

After

Finally, the last example overwrites the cloud-config URL property with the key and value for guestinfo.hostname.

Before

After

Finally, there is a non-empty diff on the next plan:

  ~ vsphere_virtual_machine.foo
      vapp.0.properties.guestinfo.dns.server.0:                    "" => "172.16.1.1"
      vapp.0.properties.guestinfo.interface.0.ip.0.address:        "" => "172.16.1.150/24"
      vapp.0.properties.guestinfo.interface.0.route.0.destination: "" => "0.0.0.0/0"
      vapp.0.properties.guestinfo.interface.0.route.0.gateway:     "" => "172.16.1.1"

Looking deeper, I'm guessing there has to be something up with the way the VAppPropertySpec/ArrayUpdateSpec is being built to have these kinds of conflicts in them and it needs to be fixed.

Also thinking about this a bit more, and given the potential for corruption here: I think we should be a bit more absolute when working with the vApp settings - we should only allow keys that are already in the vApp configuration (ie: are already valid parameters in the template). Also, we should make sure that we are saving non-zero values for any keys that are a part of the vApp configuration but are not defined in Terraform config (this way they can be properly zeroed out on the next run).

I'll add some notes in the code in the next couple of hours.

vancluever · 2017-12-15T23:36:15Z

vsphere/resource_vsphere_virtual_machine.go

+//
+// This sub-resource allows the customization of vApp properties
+// on cloned VMs.
+func VAppSubresourceSchema() map[string]*schema.Schema {


I think we could probably just put this directly into schemaVirtualMachineConfigSpec as it's so small, along with the vapp key. I'm almost tempted to also just say we flatten it and name it vapp_properties, but we might need to add more keys at a later time.

vancluever · 2017-12-15T23:37:35Z

vsphere/resource_vsphere_virtual_machine_test.go

@@ -836,6 +836,45 @@ func TestAccResourceVSphereVirtualMachine(t *testing.T) {
 				},
 			},
 		},
+		{
+			"clone with vapp properties",


Some thoughts on testing this while enforcing present keys only - we might need to test this against a CoreOS template or something else well known, and work on setting well-known cloud-config keys.

I've started testing against the CoreOS OVA instead of against a custom-built template.

vancluever · 2017-12-15T23:41:14Z

vsphere/virtual_machine_config_structure.go

+				Info: &types.VAppPropertyInfo{
+					Key:   int32(i) + 1,
+					Id:    k,
+					Value: "",


Just noticed this here - the original comment said that there was problems zeroing out a property - is this code non-functional, or were you referring to something else (maybe deleting a key completely)?

The issues with zeroing out properties are due to the fact that at least in version 5.5, the vSphere API doesn't actually do anything when it receives an empty string for a vApp property value. We've gotten around this by changing the empty string to " ", and the corresponding portion of the new code should work properly now.

Awesome! This is great info actually as I can now update some of my code on the work I was doing on govc. Good find! 👍

vancluever · 2017-12-15T23:43:30Z

vsphere/virtual_machine_config_structure.go

+	}
+	vac := make(map[string]interface{})
+	vApp := d.Get("vapp").([]interface{})
+	if len(vApp) > 0 && vApp[0] != nil {


As mentioned in the comments before the change request, I think we need to add config values that have non-zero values set on them as well, that way, on the next apply, they get zeroed out and any defaults apply. That way, removing a key always removes the corresponding value from config, even if someone adds OOB changes.

vancluever · 2017-12-15T23:49:42Z

vsphere/virtual_machine_config_structure.go

+		_, isOld := oldMap[k]
+		_, isNew := newMap[k]
+
+		if isNew {


Annotating as I'm guessing it would be around here - just noting that we will want to somehow check to ensure the key is actually present in the vApp config before working with it. This may be here, or in another part of the function, or in another function altogether (maybe it actually is part of diff customization so that we can keep the method signature clean here and as consistent as possible with the rest of the expander/flattener logic).

Although it changes the signature, I decided to check if the key is present in this function because it does clean up the logic of the function significantly, now that we no longer have to merge two different lists of vApp properties and can rely on the canonical list of properties from the template.

vancluever · 2017-12-15T23:51:26Z

vsphere/virtual_machine_config_structure.go

+// We track changes to keys to determine if any have been removed from
+// configuration - if they have, we add them with an empty value to ensure
+// they are removed from vAppConfig on the update.
+func expandVAppConfig(d *schema.ResourceData) types.BaseVmConfigSpec {


BaseVmConfigSpec is an interface, but we aren't returning anything else other than VmConfigSpec so this is abstracting things in a way that they don't need to be abstracted. Can this be changed to return a *types.VmConfigSpec?

rowanjacobs · 2017-12-16T00:25:50Z

Thanks for getting back to us with your feedback! Unfortunately I'm about to go on vacation and the rest of the team will be short-staffed next week, so it's unlikely we'll be able to make required changes until the beginning of January. We'll definitely get to it when we can!

instead of getting them from the old version of the terraform template

vancluever · 2018-01-03T17:17:49Z

Hey @rowanjacobs - just FYI I will probably be reviewing this next week.

Thanks!

- so that terraform always knows about all keys in vApp properties Signed-off-by: Rowan Jacobs <[email protected]>

Signed-off-by: Desmond Pompa Alarcon Rawls <[email protected]>

vancluever · 2018-01-08T21:45:27Z

Hey all, looks like this is coming along great. A few things are still outstanding, save maybe documentation (still haven't looked at this part yet):

Changing vApp properties should require a restart - this is to ensure that they will be sent along on the next boot (this will allow things like reconfiguration of IP addresses in CoreOS). You can do this by setting the reboot_required computed attribute when values have changed (there is a getWithRestart helper as well).
While it looks like you can no longer add configuration keys to the vApp properties configuration, the operation still goes through in Terraform, and creates a perpetual diff as such. We need to ensure that these are blocked in the plan.

Do you know when you all will be able to work on these items? If it's going to be too much later, I can actually get these over the line for you all. We are actually doing some work here internally that could benefit from this and I'd be happy to finish it up. But let me know either way 🙂

rowanjacobs · 2018-01-08T21:51:52Z

I can definitely take care of the first change. I'll see if I can get to the second one this week, and if not then I'll pass it back to you since I'm going to be unavailable for a while starting next week.

vancluever · 2018-01-08T22:08:57Z

Fair enough! Will keep an eye out 👍

This is looking very good by the way! I was excited to see a CoreOS VM come up purely from guestinfo!

rowanjacobs · 2018-01-08T22:26:51Z

Thanks! Regarding the second change: should the plan fail and provide an error message indicating that you can't add new vApp property keys? Or should the new keys be silently removed from the plan? (Or, for my own reference, do you have an existing example somewhere in this or another provider that implements this in a preferred way?)

vancluever · 2018-01-11T17:10:40Z

Hey @rowanjacobs sorry for missing this.

It should be set up so that TF explicitly fails with an error. We don't want silent behaviour as that gives the wrong impression to the user, and the bad keys risk ending up propagating to state, which would be bad as well.

Signed-off-by: Genevieve LEsperance <[email protected]>

rowanjacobs · 2018-01-11T23:45:40Z

We were thinking of returning an error from expandVAppConfig, but none of the other expand functions, nor expandVirtualMachineConfigSpec, do this, so we'd have to change several function signatures. We can go ahead and add this to the PR; was this what you had in mind, or did you have another way you prefer of getting that error? We looked into adding a ValidatorFunc but it seems like that function wouldn't be able to access client or template information, and so wouldn't have access to the original list of vApp properties on the template.

Signed-off-by: Rowan Jacobs <[email protected]>

Signed-off-by: Genevieve LEsperance <[email protected]>

rowanjacobs · 2018-01-12T00:14:02Z

Here's an example of what the error looks like:

--- FAIL: TestAccResourceVSphereVirtualMachine_Basic (104.52s)
    --- FAIL: TestAccResourceVSphereVirtualMachine_Basic/clone_with_vapp_properties (104.52s)
    	testing.go:513: Step 2 error: Error applying: 1 error(s) occurred:

* vsphere_virtual_machine.vm: 1 error(s) occurred:

* vsphere_virtual_machine.vm: error expanding virtual machine config spec: Unsupported vApp properties in vapp.properties: [fake.property]

vancluever · 2018-01-12T16:01:34Z

Hey @rowanjacobs, the error handling looks good for now. I want to maybe move this over to CustomizeDiff in the long run both so that the validation can be done at plan-time, and also so that we keep the expander/flattener interface clean, but that can be done later.

I'm reviewing this today - if it looks good, is it ready to merge?

Thanks!

rowanjacobs · 2018-01-12T16:47:04Z

Yes, it should be ready to merge. Thanks!

vancluever · 2018-01-12T16:56:04Z

The interface for expandVirtualMachineConfigSpec has changed to accommodate errors during config expansion (namely to check for invalid vApp properties). However, as the expansion happens post-clone but before the initial configuration, errors risk putting the Terraform state in an irrecoverable situation should the configuration fail here. Moved the rollback functionality that we have for the post-clone reconfigure into a helper function so that we can leverage it here. Also, changed the wording of the error message so that it's a bit friendlier to the average user.

This blocks vApp properties from being set on virtual machines created from scratch. As this support is targeted towards VMs that haven been cloned from a template that has been imported from OVF/OVA, we expect the vApp configuration to exist (and further to that we don't support setting unknown keys). This basically means that it's impossible to set vApp properties on new virtual machines with this resource, or it should be. This commit corrects a few crash scenarios that are happening when trying to use this block on new virtual machines as it stands. It also blocks situations where someone tries to add configuration after the fact or on a virtual machine where no vApp configuration exists, which would be possible on both the from-scratch and clone paths.

* Unexported the schema helper for vApp configurations. * Fixed a hit on the standard "else" linter.

Also corrected some things that were actually valid for extra_config already, but had not been updated since discovering that vApp properties/OVF environment was blocking the ability to use guestinfo.

We have a different Linux template (Ubuntu) for base testing, I don't want to mess with that specifically. As such, I've added a variable exclusively for CoreOS templates and set the vApp tests to use those.

The tests were not working with our lab as the seemed to be expecting DHCP for network configuration. Added in the appropriate configuration options that are set up for the rest of the clone tests, with the customization options just moved to their appropriate cloud-config key/value settings. This may need to be changed if CoreOS ever drops support for the old cloud-init guestinfo keys in favour of Ignition.

Final update of the vApp property tests, adding tests for all determined failure scenarios to make sure we are trapping errors correctly. This should be the last commit before merge.

vancluever · 2018-01-13T03:43:13Z

FYI - this LGTM, I've made several updates and resolved the merge conflicts and I think we are in a good place now. I'm just waiting for the whole VM test suite to finish and then I'm merging!

Great job and hats off to everyone at @pivotal and @pivotal-cf that made this happen!

rowanjacobs and others added 3 commits December 8, 2017 12:24

Add VApp properties to r/virtual_machine

fe8c313

Nest properties under top-level vApp subresource

812d2d3

Test updating and removing vapp properties

53925ef

- Currently removing vApp properties doesn't work, which seems to potentially be a vSphere API or client issue. Signed-off-by: Rowan Jacobs <[email protected]>

vancluever added the enhancement Type: Enhancement label Dec 10, 2017

support empty vapp and vapp.properties blocks

050919e

vancluever self-assigned this Dec 15, 2017

vancluever suggested changes Dec 15, 2017

View reviewed changes

rowanjacobs added 5 commits January 2, 2018 11:48

Return *VmConfigSpec instead of BaseVmConfigSpec

6cc04ab

Use CoreOS OVA for testing

3e1d394

Get old vApp properties from VM template

8017ae4

instead of getting them from the old version of the terraform template

zero out unset properties

817a852

respect nonempty default values set in vapp properties

c27efe4

desmondrawls and others added 2 commits January 3, 2018 09:56

Read all non-default vapp property values

31881de

- so that terraform always knows about all keys in vApp properties Signed-off-by: Rowan Jacobs <[email protected]>

Move vapp to schemaVirtualMachineConfigSpec

4cb7e4f

Signed-off-by: Desmond Pompa Alarcon Rawls <[email protected]>

Reboot required for change in vApp config

032d70c

vancluever mentioned this pull request Jan 11, 2018

Feature request: vsphere_virtual_machine resource - ability to deploy to vApp instead of standard resource pool #61

Closed

vancluever mentioned this pull request Jan 11, 2018

OVA upload support for vsphere provider #74

Closed

Don't need to loop over new vApp map values

dfb9168

Signed-off-by: Genevieve LEsperance <[email protected]>

Genevieve LEsperance and others added 2 commits January 11, 2018 16:07

Unsupported vApp properties throw an error

3170e96

Signed-off-by: Rowan Jacobs <[email protected]>

Spelling

de8cf26

Signed-off-by: Genevieve LEsperance <[email protected]>

vancluever added 8 commits January 12, 2018 09:16

r/virtual_machine: Fix a couple of golint issues

c3fd7da

* Unexported the schema helper for vApp configurations. * Fixed a hit on the standard "else" linter.

r/virtual_machine: Document new vApp functionality

c309d3c

Also corrected some things that were actually valid for extra_config already, but had not been updated since discovering that vApp properties/OVF environment was blocking the ability to use guestinfo.

Merge branch 'master' into rowanjacobs/master

646758c

r/virtual_machine: vApp tests use CoreOS template

b19e6d3

We have a different Linux template (Ubuntu) for base testing, I don't want to mess with that specifically. As such, I've added a variable exclusively for CoreOS templates and set the vApp tests to use those.

r/virtual_machine: Add vApp property failure tests

ca780b0

Final update of the vApp property tests, adding tests for all determined failure scenarios to make sure we are trapping errors correctly. This should be the last commit before merge.

vancluever approved these changes Jan 13, 2018

View reviewed changes

vancluever merged commit 0e3cd6c into hashicorp:master Jan 13, 2018

rowanjacobs mentioned this pull request Jan 16, 2018

Support VAppConfig for vsphere_virtual_machine resource #243

Closed

charandas mentioned this pull request Jan 26, 2018

vapp property feature in vsphere_virtual_machine resource not working in v1.2.0 release #374

Closed

sdemura mentioned this pull request Feb 2, 2018

New 'vapp properties' do not work with Ubuntu 14.04/16.04 OVA #381

Closed

charandas mentioned this pull request Feb 8, 2018

vapp property changing restarts the VM, can it work like a null provider? #393

Closed

dhekimian mentioned this pull request Feb 8, 2018

Don't touch vapp properties (ovf) where ovf:userConfigurable="false" #394

Closed

vancluever mentioned this pull request Feb 13, 2018

WIP govc: Add virtual machine vApp configuration options vmware/govmomi#991

Closed

5 tasks

ghost locked and limited conversation to collaborators Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/virtual_machine: add vApp properties #303

r/virtual_machine: add vApp properties #303

rowanjacobs commented Dec 8, 2017

rowanjacobs commented Dec 13, 2017 •

edited

Loading

rowanjacobs commented Dec 13, 2017

charandas commented Dec 13, 2017

vancluever commented Dec 14, 2017

tevaum commented Dec 14, 2017

vancluever commented Dec 15, 2017

vancluever commented Dec 15, 2017

vancluever Dec 15, 2017

vancluever Dec 15, 2017

rowanjacobs Jan 2, 2018

vancluever Dec 15, 2017

rowanjacobs Jan 3, 2018

vancluever Jan 12, 2018

vancluever Dec 15, 2017

vancluever Dec 15, 2017

rowanjacobs Jan 2, 2018

vancluever Dec 15, 2017

rowanjacobs Jan 2, 2018

rowanjacobs commented Dec 16, 2017

vancluever commented Jan 3, 2018

vancluever commented Jan 8, 2018

rowanjacobs commented Jan 8, 2018

vancluever commented Jan 8, 2018

rowanjacobs commented Jan 8, 2018

vancluever commented Jan 11, 2018

rowanjacobs commented Jan 11, 2018

rowanjacobs commented Jan 12, 2018 •

edited

Loading

vancluever commented Jan 12, 2018

rowanjacobs commented Jan 12, 2018 •

edited

Loading

vancluever commented Jan 12, 2018 •

edited

Loading

vancluever commented Jan 13, 2018 •

edited

Loading

r/virtual_machine: add vApp properties #303

r/virtual_machine: add vApp properties #303

Conversation

rowanjacobs commented Dec 8, 2017

rowanjacobs commented Dec 13, 2017 • edited Loading

rowanjacobs commented Dec 13, 2017

charandas commented Dec 13, 2017

vancluever commented Dec 14, 2017

tevaum commented Dec 14, 2017

vancluever commented Dec 15, 2017

vancluever commented Dec 15, 2017

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

rowanjacobs commented Dec 16, 2017

vancluever commented Jan 3, 2018

vancluever commented Jan 8, 2018

rowanjacobs commented Jan 8, 2018

vancluever commented Jan 8, 2018

rowanjacobs commented Jan 8, 2018

vancluever commented Jan 11, 2018

rowanjacobs commented Jan 11, 2018

rowanjacobs commented Jan 12, 2018 • edited Loading

vancluever commented Jan 12, 2018

rowanjacobs commented Jan 12, 2018 • edited Loading

vancluever commented Jan 12, 2018 • edited Loading

vancluever commented Jan 13, 2018 • edited Loading

rowanjacobs commented Dec 13, 2017 •

edited

Loading

rowanjacobs commented Jan 12, 2018 •

edited

Loading

rowanjacobs commented Jan 12, 2018 •

edited

Loading

vancluever commented Jan 12, 2018 •

edited

Loading

vancluever commented Jan 13, 2018 •

edited

Loading