From ea3711d28f5bd82b5a32123927cc668b388eeeef Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Thu, 14 Dec 2017 10:28:46 -0800 Subject: [PATCH 1/2] r/virtual_machine: Remove OOB deleted virtual machines from state This was incorrectly changed from v0.4.2. While failing on read errors is normal, it's a TF convention to check to see if the read error was due to a resource being deleted outside of the TF workflow, and if it has, mark that resource as gone by blanking out its ID. This update restores that behavior to the VM resource. Other resources in the provider will need to have this functionality added, although most of those items are core infrastructure resources and are not as ephemeral as a virtual machine is, so they are probably lower priority to update. They will be done on an as needed basis. Fixes #299. --- vsphere/helper_test.go | 47 +++++++++++++++++++ .../virtualmachine/virtual_machine_helper.go | 21 ++++++++- vsphere/resource_vsphere_virtual_machine.go | 7 ++- .../resource_vsphere_virtual_machine_test.go | 37 ++++++++++++++- 4 files changed, 109 insertions(+), 3 deletions(-) diff --git a/vsphere/helper_test.go b/vsphere/helper_test.go index 427d1937b..952213236 100644 --- a/vsphere/helper_test.go +++ b/vsphere/helper_test.go @@ -115,6 +115,17 @@ func copyStatePtr(t **terraform.State) resource.TestCheckFunc { } } +// copyState returns a TestCheckFunc that returns a deep copy of the state. +// Unlike copyStatePtr, this state has de-coupled from the in-flight state, so +// it will not be modified on subsequent steps and hence will possibly drift. +// It can be used to access vales of the state at a certain step. +func copyState(t **terraform.State) resource.TestCheckFunc { + return func(s *terraform.State) error { + *t = s.DeepCopy() + return nil + } +} + // testGetPortGroup is a convenience method to fetch a static port group // resource for testing. func testGetPortGroup(s *terraform.State, resourceName string) (*types.HostPortGroup, error) { @@ -342,6 +353,27 @@ func testDeleteVMDisk(s *terraform.State, resourceName string, name string) erro return virtualdisk.Delete(tVars.client, p.String(), dc) } +// testDeleteVM deletes the virtual machine. This is used to test resource +// re-creation if TF cannot locate a VM that is in state any more. +func testDeleteVM(s *terraform.State, resourceName string) error { + if err := testPowerOffVM(s, resourceName); err != nil { + return err + } + vm, err := testGetVirtualMachine(s, resourceName) + if err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), defaultAPITimeout) + defer cancel() + task, err := vm.Destroy(ctx) + if err != nil { + return fmt.Errorf("error destroying virtual machine: %s", err) + } + tctx, tcancel := context.WithTimeout(context.Background(), defaultAPITimeout) + defer tcancel() + return task.Wait(tctx) +} + // testGetTagCategory gets a tag category by name. func testGetTagCategory(s *terraform.State, resourceName string) (*tags.Category, error) { tVars, err := testClientVariablesForResource(s, fmt.Sprintf("vsphere_tag_category.%s", resourceName)) @@ -532,3 +564,18 @@ func testGetDVPortgroupProperties(s *terraform.State, resourceName string) (*mo. } return dvportgroup.Properties(dvs) } + +// testCheckResourceNotAttr is an inverse check of TestCheckResourceAttr. It +// checks to make sure the resource attribute does *not* match a certain value. +func testCheckResourceNotAttr(name, key, value string) resource.TestCheckFunc { + return func(s *terraform.State) error { + err := resource.TestCheckResourceAttr(name, key, value)(s) + if err != nil { + if regexp.MustCompile("[-_.a-zA-Z0-9]\\: Attribute '.*' expected .*, got .*").MatchString(err.Error()) { + return nil + } + return err + } + return fmt.Errorf("%s: Attribute '%s' expected to not match %#v", name, key, value) + } +} diff --git a/vsphere/internal/helper/virtualmachine/virtual_machine_helper.go b/vsphere/internal/helper/virtualmachine/virtual_machine_helper.go index c01c7d023..9871a2e53 100644 --- a/vsphere/internal/helper/virtualmachine/virtual_machine_helper.go +++ b/vsphere/internal/helper/virtualmachine/virtual_machine_helper.go @@ -21,6 +21,25 @@ import ( var errGuestShutdownTimeout = errors.New("the VM did not power off within the specified amount of time") +// UUIDNotFoundError is an error type that is returned when a +// virtual machine could not be found by UUID. +type UUIDNotFoundError struct { + s string +} + +// Error implements error for UUIDNotFoundError. +func (e *UUIDNotFoundError) Error() string { + return e.s +} + +// newUUIDNotFoundError returns a new UUIDNotFoundError with the +// text populated. +func newUUIDNotFoundError(s string) *UUIDNotFoundError { + return &UUIDNotFoundError{ + s: s, + } +} + // FromUUID locates a virtualMachine by its UUID. func FromUUID(client *govmomi.Client, uuid string) (*object.VirtualMachine, error) { log.Printf("[DEBUG] Locating virtual machine with UUID %q", uuid) @@ -34,7 +53,7 @@ func FromUUID(client *govmomi.Client, uuid string) (*object.VirtualMachine, erro } if result == nil { - return nil, fmt.Errorf("virtual machine with UUID %q not found", uuid) + return nil, newUUIDNotFoundError(fmt.Sprintf("virtual machine with UUID %q not found", uuid)) } // We need to filter our object through finder to ensure that the diff --git a/vsphere/resource_vsphere_virtual_machine.go b/vsphere/resource_vsphere_virtual_machine.go index bdb6d9345..23a98db6c 100644 --- a/vsphere/resource_vsphere_virtual_machine.go +++ b/vsphere/resource_vsphere_virtual_machine.go @@ -239,7 +239,12 @@ func resourceVSphereVirtualMachineRead(d *schema.ResourceData, meta interface{}) id := d.Id() vm, err := virtualmachine.FromUUID(client, id) if err != nil { - return fmt.Errorf("cannot locate virtual machine with UUID %q: %s", id, err) + if _, ok := err.(*virtualmachine.UUIDNotFoundError); ok { + log.Printf("[DEBUG] %s: Virtual machine not found, marking resource as gone: %s", resourceVSphereVirtualMachineIDString(d), err) + d.SetId("") + return nil + } + return fmt.Errorf("error searching for with UUID %q: %s", id, err) } vprops, err := virtualmachine.Properties(vm) if err != nil { diff --git a/vsphere/resource_vsphere_virtual_machine_test.go b/vsphere/resource_vsphere_virtual_machine_test.go index dd49e35ad..2fc9c61fd 100644 --- a/vsphere/resource_vsphere_virtual_machine_test.go +++ b/vsphere/resource_vsphere_virtual_machine_test.go @@ -114,6 +114,41 @@ func TestAccResourceVSphereVirtualMachine(t *testing.T) { }, }, }, + { + "re-create on deletion", + resource.TestCase{ + PreCheck: func() { + testAccPreCheck(tp) + testAccResourceVSphereVirtualMachinePreCheck(tp) + }, + Providers: testAccProviders, + CheckDestroy: testAccResourceVSphereVirtualMachineCheckExists(false), + Steps: []resource.TestStep{ + { + Config: testAccResourceVSphereVirtualMachineConfigBasic(), + Check: resource.ComposeTestCheckFunc( + copyState(&state), + testAccResourceVSphereVirtualMachineCheckExists(true), + ), + }, + { + PreConfig: func() { + if err := testDeleteVM(state, "vm"); err != nil { + panic(err) + } + }, + Config: testAccResourceVSphereVirtualMachineConfigBasic(), + Check: resource.ComposeTestCheckFunc( + testAccResourceVSphereVirtualMachineCheckExists(true), + func(s *terraform.State) error { + oldID := state.RootModule().Resources["vsphere_virtual_machine.vm"].Primary.ID + return testCheckResourceNotAttr("vsphere_virtual_machine.vm", "id", oldID)(s) + }, + ), + }, + }, + }, + }, { "multi-device", resource.TestCase{ @@ -2107,7 +2142,7 @@ resource "vsphere_virtual_machine" "vm" { num_cpus = 2 memory = 2048 guest_id = "other3xLinux64Guest" - + network_interface { network_id = "${data.vsphere_network.network.id}" } From 62f51a05ed28e4479b95a77d7da90b4eb96a39f8 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Thu, 14 Dec 2017 12:21:23 -0800 Subject: [PATCH 2/2] testing: vale -> value --- vsphere/helper_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vsphere/helper_test.go b/vsphere/helper_test.go index 952213236..6c001aead 100644 --- a/vsphere/helper_test.go +++ b/vsphere/helper_test.go @@ -118,7 +118,7 @@ func copyStatePtr(t **terraform.State) resource.TestCheckFunc { // copyState returns a TestCheckFunc that returns a deep copy of the state. // Unlike copyStatePtr, this state has de-coupled from the in-flight state, so // it will not be modified on subsequent steps and hence will possibly drift. -// It can be used to access vales of the state at a certain step. +// It can be used to access values of the state at a certain step. func copyState(t **terraform.State) resource.TestCheckFunc { return func(s *terraform.State) error { *t = s.DeepCopy()