Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

r/virtual_machine: Remove OOB deleted virtual machines from state #321

Merged
merged 2 commits into from
Dec 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions vsphere/helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 values 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) {
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
}
}
21 changes: 20 additions & 1 deletion vsphere/internal/helper/virtualmachine/virtual_machine_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth making this message to be self-explanatory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an internal field on a very small object - and this generally matches the pattern you see in something like the errors package, so I think it's fine :)

}

// 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)
Expand All @@ -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
Expand Down
7 changes: 6 additions & 1 deletion vsphere/resource_vsphere_virtual_machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
37 changes: 36 additions & 1 deletion vsphere/resource_vsphere_virtual_machine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's just and Azure provider thing - but we'd generally make the deletion a Check function and assert it's been deleted, rather than crashing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PreConfig does not return error, so the only possible thing we can do is panic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: The intention here is to make sure this gets handled completely outside of the plan/apply testing cycle. PreConifg happens very early on, which is why I use it - Check on the other hand happens pretty much in the middle after the apply, but before the post-apply refresh to check for an empty plan. Having these OOB steps in PreConfig kind of gives them a more real-world workflow (ie: someone usually makes manual modifications between a TF run, versus in the middle of one).

What would be nicer is if the signature of PreConfig was altered to send an error back, but that's kind of a breaking change.

},
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{
Expand Down Expand Up @@ -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}"
}
Expand Down