diff --git a/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go b/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go index 77bf66e1c..ac7e44124 100644 --- a/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go +++ b/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go @@ -597,6 +597,10 @@ func DiskDiffOperation(d *schema.ResourceDiff, c *govmomi.Client) error { } names[name] = struct{}{} units[nm["unit_number"].(int)] = struct{}{} + r := NewDiskSubresource(c, d, nm, nil, ni) + if err := r.DiffGeneral(); err != nil { + return fmt.Errorf("%s: %s", r.Addr(), err) + } } if _, ok := units[0]; !ok { return errors.New("at least one disk must have a unit_number of 0") @@ -624,7 +628,7 @@ nextNew: // We extrapolate using the label as a "primary key" of sorts. if nname == oname { r := NewDiskSubresource(c, d, nm, om, oi) - if err := r.Diff(); err != nil { + if err := r.DiffExisting(); err != nil { return fmt.Errorf("%s: %s", r.Addr(), err) } normalized[oi] = r.Data() @@ -1316,15 +1320,16 @@ func (r *DiskSubresource) Delete(l object.VirtualDeviceList) ([]types.BaseVirtua return deleteSpec, nil } -// Diff performs normalization and validation tasks for a specific disk -// sub-resource's diff. -func (r *DiskSubresource) Diff() error { - log.Printf("[DEBUG] %s: Beginning diff validation and normalization (device information may be incomplete)", r) +// DiffExisting validates and normalizes the fields for an existing disk +// sub-resource. It handles carrying over existing values, so this should not +// be used on disks that have not been successfully matched up between current +// and old diffs. +func (r *DiskSubresource) DiffExisting() error { + log.Printf("[DEBUG] %s: Beginning normalization of existing disk", r) name, err := diskLabelOrName(r.data) if err != nil { return err } - // Prevent a backward migration of label -> name. TODO: Remove this after // 2.0. olabel, nlabel := r.GetChange("label") @@ -1347,31 +1352,7 @@ func (r *DiskSubresource) Diff() error { r.Set("device_address", odaddr) r.Set("uuid", ouuid) - // Enforce the maximum unit number, which is the current value of - // scsi_controller_count * 15 - 1. - ctlrCount := r.rdd.Get("scsi_controller_count").(int) - maxUnit := ctlrCount*15 - 1 - currentUnit := r.Get("unit_number").(int) - if currentUnit > maxUnit { - return fmt.Errorf("unit_number on disk %q too high (%d) - maximum value is %d with %d SCSI controller(s)", name, currentUnit, maxUnit, ctlrCount) - } - - if r.Get("attach").(bool) { - switch { - case r.Get("datastore_id").(string) == "": - return fmt.Errorf("datastore_id for disk %q is required when attach is set", name) - case r.Get("size").(int) > 0: - return fmt.Errorf("size for disk %q cannot be defined when attach is set", name) - case r.Get("eagerly_scrub").(bool): - return fmt.Errorf("eagerly_scrub for disk %q cannot be defined when attach is set", name) - case r.Get("keep_on_remove").(bool): - return fmt.Errorf("keep_on_remove for disk %q is implicit when attach is set, please remove this setting", name) - } - } else { - // Enforce size as a required field when attach is not set - if r.Get("size").(int) < 1 { - return fmt.Errorf("size for disk %q: required option not set", name) - } + if !r.Get("attach").(bool) { // Carry forward path when attach is not set opath, _ := r.GetChange("path") r.Set("path", opath.(string)) @@ -1395,11 +1376,12 @@ func (r *DiskSubresource) Diff() error { r.Set("datastore_id", dsID) } default: - if err := r.normalizeDiskDatastore(); err != nil { + if err = r.normalizeDiskDatastore(); err != nil { return err } } } + // Preserve the share value if we don't have custom shares set osc, _ := r.GetChange("io_share_count") if r.Get("io_share_level").(string) != string(types.SharesLevelCustom) { @@ -1416,25 +1398,62 @@ func (r *DiskSubresource) Diff() error { // Ensure that there is no change in either eagerly_scrub or thin_provisioned // - these values cannot be changed once set. - if _, err := r.GetWithVeto("eagerly_scrub"); err != nil { + if _, err = r.GetWithVeto("eagerly_scrub"); err != nil { return fmt.Errorf("virtual disk %q: %s", name, err) } - if _, err := r.GetWithVeto("thin_provisioned"); err != nil { + if _, err = r.GetWithVeto("thin_provisioned"); err != nil { return fmt.Errorf("virtual disk %q: %s", name, err) } - // Same with attach - if _, err := r.GetWithVeto("attach"); err != nil { + if _, err = r.GetWithVeto("attach"); err != nil { return fmt.Errorf("virtual disk %q: %s", name, err) } // Validate storage vMotion if the datastore is changing if r.HasChange("datastore_id") { - if err := r.validateStorageRelocateDiff(); err != nil { + if err = r.validateStorageRelocateDiff(); err != nil { return err } } + log.Printf("[DEBUG] %s: Normalization of existing disk diff complete", r) + return nil +} +// DiffGeneral performs complex validation of an individual disk sub-resource +// that can't be done in schema alone. Should be run on new and existing disks. +func (r *DiskSubresource) DiffGeneral() error { + log.Printf("[DEBUG] %s: Beginning diff validation", r) + name, err := diskLabelOrName(r.data) + if err != nil { + return err + } + + // Enforce the maximum unit number, which is the current value of + // scsi_controller_count * 15 - 1. + ctlrCount := r.rdd.Get("scsi_controller_count").(int) + maxUnit := ctlrCount*15 - 1 + currentUnit := r.Get("unit_number").(int) + if currentUnit > maxUnit { + return fmt.Errorf("unit_number on disk %q too high (%d) - maximum value is %d with %d SCSI controller(s)", name, currentUnit, maxUnit, ctlrCount) + } + + if r.Get("attach").(bool) { + switch { + case r.Get("datastore_id").(string) == "": + return fmt.Errorf("datastore_id for disk %q is required when attach is set", name) + case r.Get("size").(int) > 0: + return fmt.Errorf("size for disk %q cannot be defined when attach is set", name) + case r.Get("eagerly_scrub").(bool): + return fmt.Errorf("eagerly_scrub for disk %q cannot be defined when attach is set", name) + case r.Get("keep_on_remove").(bool): + return fmt.Errorf("keep_on_remove for disk %q is implicit when attach is set, please remove this setting", name) + } + } else { + // Enforce size as a required field when attach is not set + if r.Get("size").(int) < 1 { + return fmt.Errorf("size for disk %q: required option not set", name) + } + } // Block certain options from being set depending on the vSphere version. version := viapi.ParseVersionFromClient(r.client) if r.Get("disk_sharing").(string) != string(types.VirtualDiskSharingSharingNone) { @@ -1442,8 +1461,7 @@ func (r *DiskSubresource) Diff() error { return fmt.Errorf("multi-writer disk_sharing is only supported on vSphere 6 and higher") } } - - log.Printf("[DEBUG] %s: Diff validation and normalization complete", r) + log.Printf("[DEBUG] %s: Diff validation complete", r) return nil } diff --git a/vsphere/resource_vsphere_virtual_machine_test.go b/vsphere/resource_vsphere_virtual_machine_test.go index 0d3168d9c..0c20cd98c 100644 --- a/vsphere/resource_vsphere_virtual_machine_test.go +++ b/vsphere/resource_vsphere_virtual_machine_test.go @@ -359,6 +359,26 @@ func TestAccResourceVSphereVirtualMachine_highDiskUnitNumbers(t *testing.T) { }) } +func TestAccResourceVSphereVirtualMachine_highDiskUnitInsufficientBus(t *testing.T) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { + testAccPreCheck(t) + testAccResourceVSphereVirtualMachinePreCheck(t) + }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: testAccResourceVSphereVirtualMachineConfigMultiHighBusInsufficientBus(), + ExpectError: regexp.MustCompile("unit_number on disk \"disk1\" too high \\(15\\) - maximum value is 14 with 1 SCSI controller\\(s\\)"), + }, + { + Config: testAccResourceVSphereEmpty, + Check: resource.ComposeTestCheckFunc(), + }, + }, + }) +} + func TestAccResourceVSphereVirtualMachine_highDiskUnitsToRegularSingleController(t *testing.T) { resource.Test(t, resource.TestCase{ PreCheck: func() { @@ -3908,6 +3928,94 @@ resource "vsphere_virtual_machine" "vm" { ) } +func testAccResourceVSphereVirtualMachineConfigMultiHighBusInsufficientBus() string { + return fmt.Sprintf(` +variable "datacenter" { + default = "%s" +} + +variable "resource_pool" { + default = "%s" +} + +variable "network_label" { + default = "%s" +} + +variable "datastore" { + default = "%s" +} + +data "vsphere_datacenter" "dc" { + name = "${var.datacenter}" +} + +data "vsphere_datastore" "datastore" { + name = "${var.datastore}" + datacenter_id = "${data.vsphere_datacenter.dc.id}" +} + +data "vsphere_resource_pool" "pool" { + name = "${var.resource_pool}" + datacenter_id = "${data.vsphere_datacenter.dc.id}" +} + +data "vsphere_network" "network" { + name = "${var.network_label}" + datacenter_id = "${data.vsphere_datacenter.dc.id}" +} + +resource "vsphere_virtual_machine" "vm" { + name = "terraform-test" + resource_pool_id = "${data.vsphere_resource_pool.pool.id}" + datastore_id = "${data.vsphere_datastore.datastore.id}" + + scsi_controller_count = 1 + + num_cpus = 2 + memory = 2048 + guest_id = "other3xLinux64Guest" + + network_interface { + network_id = "${data.vsphere_network.network.id}" + bandwidth_share_level = "normal" + } + + network_interface { + network_id = "${data.vsphere_network.network.id}" + bandwidth_share_level = "high" + } + + network_interface { + network_id = "${data.vsphere_network.network.id}" + bandwidth_share_level = "low" + } + + disk { + label = "disk0" + size = 20 + } + + disk { + label = "disk1" + unit_number = 15 + size = 10 + } + + disk { + label = "disk2" + unit_number = 31 + size = 5 + } +} +`, + os.Getenv("VSPHERE_DATACENTER"), + os.Getenv("VSPHERE_RESOURCE_POOL"), + os.Getenv("VSPHERE_NETWORK_LABEL_PXE"), + os.Getenv("VSPHERE_DATASTORE"), + ) +} + func testAccResourceVSphereVirtualMachineConfigMultiHighBus() string { return fmt.Sprintf(` variable "datacenter" {