r/virtual_machine: Split disk Diff

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,34 +1398,70 @@ 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) {
 		if version.Older(viapi.VSphereVersion{Product: version.Product, Major: 6}) {
 			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
 }
 

vsphere/resource_vsphere_virtual_machine_test.go

@@ -359,6 +359,66 @@ 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_highDiskUnitInsufficientBusAddAfterCreation(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck: func() {
+			testAccPreCheck(t)
+			testAccResourceVSphereVirtualMachinePreCheck(t)
+		},
+		Providers: testAccProviders,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccResourceVSphereVirtualMachineConfigBasic(),
+				Check:  testAccResourceVSphereVirtualMachineCheckExists(true),
+			},
+			{
+				Config:      testAccResourceVSphereVirtualMachineConfigMultiHighBusInsufficientBus(),
+				ExpectError: regexp.MustCompile("unit_number on disk \"disk1\" too high \\(15\\) - maximum value is 14 with 1 SCSI controller\\(s\\)"),
+			},
+		},
+	})
+}
+
+func TestAccResourceVSphereVirtualMachine_diskAddAfterCreation(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck: func() {
+			testAccPreCheck(t)
+			testAccResourceVSphereVirtualMachinePreCheck(t)
+		},
+		Providers: testAccProviders,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccResourceVSphereVirtualMachineConfigBasic(),
+				Check:  testAccResourceVSphereVirtualMachineCheckExists(true),
+			},
+			{
+				Config: testAccResourceVSphereVirtualMachineConfigMultiDevice(),
+				Check:  testAccResourceVSphereVirtualMachineCheckExists(true),
+			},
+		},
+	})
+}
+
 func TestAccResourceVSphereVirtualMachine_highDiskUnitsToRegularSingleController(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck: func() {
@@ -3908,6 +3968,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" {