Skip to content

Commit

Permalink
r/virtual_machine: Validate exact disk type when using linked_clone
Browse files Browse the repository at this point in the history
A VM that's cloned with linked_clone and different disk mode options
that what is set on the source template will end up with inconsistent
settings post-clone, causing a read-update failure that will leave the
VM in a state that is irrecoverable to Terrform - the only way to
rectify the situation is to delete the VM and start over with a new
state.

This adds extra validation to make sure that thin_provisioned and
eagerly_scrub are set to the same settings as the source template's
counterpart disk, along with validating that the disk is the exact same
size (something that wasn't done yet).

Fixes #275.
  • Loading branch information
vancluever committed Dec 6, 2017
1 parent 482c9c7 commit a88129e
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 8 deletions.
38 changes: 33 additions & 5 deletions vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,7 @@ nextNew:
// This function is meant to be called during diff customization. It is a
// subset of the normal refresh behaviour as we don't worry about checking
// existing state.
func DiskCloneValidateOperation(d *schema.ResourceDiff, c *govmomi.Client, l object.VirtualDeviceList) error {
func DiskCloneValidateOperation(d *schema.ResourceDiff, c *govmomi.Client, l object.VirtualDeviceList, linked bool) error {
log.Printf("[DEBUG] DiskCloneValidateOperation: Checking existing virtual disk configuration")
devices := selectDisks(l, d.Get("scsi_controller_count").(int))
// Sort the device list, in case it's not sorted already.
Expand Down Expand Up @@ -614,11 +614,39 @@ func DiskCloneValidateOperation(d *schema.ResourceDiff, c *govmomi.Client, l obj
if tr.Get("name").(string) != expected {
return fmt.Errorf("%s: invalid disk name %q for cloning. Please rename this disk to %q", tr.Addr(), tr.Get("name").(string), expected)
}
// Quickly compare size as well, as disks need to be at least the same size
// as the source disks, or else the operation will fail on the reconfigure.
if tr.Get("size").(int) < r.Get("size").(int) {
return fmt.Errorf("%s: disk name %s must have a minimum size of %d GiB", tr.Addr(), tr.Get("name").(string), r.Get("size").(int))

// Do some pre-clone validation. This is mainly to make sure that the disks
// clone in a way that is consistent with configuration.
targetName := tr.Get("name").(string)
sourceSize := r.Get("size").(int)
targetSize := tr.Get("size").(int)
targetThin := tr.Get("thin_provisioned").(bool)
targetEager := tr.Get("eagerly_scrub").(bool)

var sourceThin, sourceEager bool
if b := r.Get("thin_provisioned"); b != nil {
sourceThin = b.(bool)
}
if b := r.Get("eagerly_scrub"); b != nil {
sourceEager = b.(bool)
}

switch {
case linked:
switch {
case sourceSize != targetSize:
return fmt.Errorf("%s: disk name %s must be the exact size of source when using linked_clone (expected: %d GiB)", tr.Addr(), targetName, sourceSize)
case sourceThin != targetThin:
return fmt.Errorf("%s: disk name %s must have same value for thin_provisioned as source when using linked_clone (expected: %t)", tr.Addr(), targetName, sourceThin)
case sourceEager != targetEager:
return fmt.Errorf("%s: disk name %s must have same value for eagerly_scrub as source when using linked_clone (expected: %t)", tr.Addr(), targetName, sourceEager)
}
default:
if sourceSize > targetSize {
return fmt.Errorf("%s: disk name %s must be at least the same size of source when cloning (expected: >= %d GiB)", tr.Addr(), targetName, sourceSize)
}
}

// Finally, we don't support non-SCSI (ie: SATA, IDE, NVMe) disks, so kick
// back an error if we see one of those.
ct, _, _, err := splitDevAddr(r.DevAddr())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ func ValidateVirtualMachineClone(d *schema.ResourceDiff, c *govmomi.Client) erro
}
// If linked clone is enabled, check to see if we have a snapshot. There need
// to be a single snapshot on the template for it to be eligible.
if d.Get("clone.0.linked_clone").(bool) {
linked := d.Get("clone.0.linked_clone").(bool)
if linked {
log.Printf("[DEBUG] ValidateVirtualMachineClone: Checking snapshots on %s for linked clone eligibility", tUUID)
if err := validateCloneSnapshots(vprops); err != nil {
return err
Expand All @@ -94,7 +95,7 @@ func ValidateVirtualMachineClone(d *schema.ResourceDiff, c *govmomi.Client) erro
// in the configuration. This is in the virtual device package, so pass off
// to that now.
l := object.VirtualDeviceList(vprops.Config.Hardware.Device)
if err := virtualdevice.DiskCloneValidateOperation(d, c, l); err != nil {
if err := virtualdevice.DiskCloneValidateOperation(d, c, l, linked); err != nil {
return err
}

Expand Down
4 changes: 3 additions & 1 deletion website/docs/r/virtual_machine.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,9 @@ both the resource configuration and source template:
this.
* The `size` of a virtual disk must be at least the same size as its
counterpart disk in the template.
* When using `linked_clone`, the `size` has to be an exact match.
* When using `linked_clone`, the `size`, `thin_provisioned`, and
`eagerly_scrub` settings for each disk must be an exact match to the
individual disk's counterpart in the source template.
* The [`scsi_controller_count`](#scsi_controller_count) setting should be
configured as necessary to cover all of the disks on the template. For best
results, only configure this setting for the amount of controllers you will
Expand Down

0 comments on commit a88129e

Please # to comment.