Fix several bugs in different resources

- Make sure attaching a disk or a NIC is tried a couple of times as this only works after the OS has fully booted;
- Stop using the device name instead of ID as the names differ depending on the hypervisor that you are using;
- VPC’s do not always have a source NAT IP;

Author: Sander van Harmelen

builtin/providers/cloudstack/resource_cloudstack_disk.go

@@ -28,8 +28,8 @@ func resourceCloudStackDisk() *schema.Resource {
 				Default:  false,
 			},
 
-			"device": &schema.Schema{
-				Type:     schema.TypeString,
+			"device_id": &schema.Schema{
+				Type:     schema.TypeInt,
 				Optional: true,
 				Computed: true,
 			},
@@ -116,7 +116,7 @@ func resourceCloudStackDiskCreate(d *schema.ResourceData, meta interface{}) erro
 	// Set the volume ID and partials
 	d.SetId(r.Id)
 	d.SetPartial("name")
-	d.SetPartial("device")
+	d.SetPartial("device_id")
 	d.SetPartial("disk_offering")
 	d.SetPartial("size")
 	d.SetPartial("virtual_machine_id")
@@ -163,28 +163,7 @@ func resourceCloudStackDiskRead(d *schema.ResourceData, meta interface{}) error
 	setValueOrID(d, "zone", v.Zonename, v.Zoneid)
 
 	if v.Attached != "" {
-		// Get the virtual machine details
-		vm, _, err := cs.VirtualMachine.GetVirtualMachineByID(
-			v.Virtualmachineid,
-			cloudstack.WithProject(d.Get("project").(string)),
-		)
-		if err != nil {
-			return err
-		}
-
-		// Get the guest OS type details
-		os, _, err := cs.GuestOS.GetOsTypeByID(vm.Guestosid)
-		if err != nil {
-			return err
-		}
-
-		// Get the guest OS category details
-		c, _, err := cs.GuestOS.GetOsCategoryByID(os.Oscategoryid)
-		if err != nil {
-			return err
-		}
-
-		d.Set("device", retrieveDeviceName(v.Deviceid, c.Name))
+		d.Set("device_id", int(v.Deviceid))
 		d.Set("virtual_machine_id", v.Virtualmachineid)
 	}
 
@@ -235,9 +214,9 @@ func resourceCloudStackDiskUpdate(d *schema.ResourceData, meta interface{}) erro
 		d.SetPartial("size")
 	}
 
-	// If the device changed, just detach here so we can re-attach the
+	// If the device ID changed, just detach here so we can re-attach the
 	// volume at the end of this function
-	if d.HasChange("device") || d.HasChange("virtual_machine") {
+	if d.HasChange("device_id") || d.HasChange("virtual_machine") {
 		// Detach the volume
 		if err := resourceCloudStackDiskDetach(d, meta); err != nil {
 			return fmt.Errorf("Error detaching disk %s from virtual machine: %s", name, err)
@@ -253,7 +232,7 @@ func resourceCloudStackDiskUpdate(d *schema.ResourceData, meta interface{}) erro
 
 		// Set the additional partials
 		d.SetPartial("attach")
-		d.SetPartial("device")
+		d.SetPartial("device_id")
 		d.SetPartial("virtual_machine_id")
 	} else {
 		// Detach the volume
@@ -304,21 +283,14 @@ func resourceCloudStackDiskAttach(d *schema.ResourceData, meta interface{}) erro
 		// Create a new parameter struct
 		p := cs.Volume.NewAttachVolumeParams(d.Id(), virtualmachineid.(string))
 
-		if device, ok := d.GetOk("device"); ok {
-			// Retrieve the device ID
-			deviceid := retrieveDeviceID(device.(string))
-			if deviceid == -1 {
-				return fmt.Errorf("Device %s is not a valid device", device.(string))
-			}
-
-			// Set the device ID
-			p.SetDeviceid(deviceid)
+		if deviceid, ok := d.GetOk("device_id"); ok {
+			p.SetDeviceid(int64(deviceid.(int)))
 		}
 
 		// Attach the new volume
-		r, err := Retry(4, retryableAttachVolumeFunc(cs, p))
+		r, err := Retry(10, retryableAttachVolumeFunc(cs, p))
 		if err != nil {
-			return err
+			return fmt.Errorf("Error attaching volume to VM: %s", err)
 		}
 
 		d.SetId(r.(*cloudstack.AttachVolumeResponse).Id)
@@ -397,115 +369,3 @@ func retryableAttachVolumeFunc(
 		return r, nil
 	}
 }
-
-func retrieveDeviceID(device string) int64 {
-	switch device {
-	case "/dev/xvdb", "D:":
-		return 1
-	case "/dev/xvdc", "E:":
-		return 2
-	case "/dev/xvde", "F:":
-		return 4
-	case "/dev/xvdf", "G:":
-		return 5
-	case "/dev/xvdg", "H:":
-		return 6
-	case "/dev/xvdh", "I:":
-		return 7
-	case "/dev/xvdi", "J:":
-		return 8
-	case "/dev/xvdj", "K:":
-		return 9
-	case "/dev/xvdk", "L:":
-		return 10
-	case "/dev/xvdl", "M:":
-		return 11
-	case "/dev/xvdm", "N:":
-		return 12
-	case "/dev/xvdn", "O:":
-		return 13
-	case "/dev/xvdo", "P:":
-		return 14
-	case "/dev/xvdp", "Q:":
-		return 15
-	default:
-		return -1
-	}
-}
-
-func retrieveDeviceName(device int64, os string) string {
-	switch device {
-	case 1:
-		if os == "Windows" {
-			return "D:"
-		}
-		return "/dev/xvdb"
-	case 2:
-		if os == "Windows" {
-			return "E:"
-		}
-		return "/dev/xvdc"
-	case 4:
-		if os == "Windows" {
-			return "F:"
-		}
-		return "/dev/xvde"
-	case 5:
-		if os == "Windows" {
-			return "G:"
-		}
-		return "/dev/xvdf"
-	case 6:
-		if os == "Windows" {
-			return "H:"
-		}
-		return "/dev/xvdg"
-	case 7:
-		if os == "Windows" {
-			return "I:"
-		}
-		return "/dev/xvdh"
-	case 8:
-		if os == "Windows" {
-			return "J:"
-		}
-		return "/dev/xvdi"
-	case 9:
-		if os == "Windows" {
-			return "K:"
-		}
-		return "/dev/xvdj"
-	case 10:
-		if os == "Windows" {
-			return "L:"
-		}
-		return "/dev/xvdk"
-	case 11:
-		if os == "Windows" {
-			return "M:"
-		}
-		return "/dev/xvdl"
-	case 12:
-		if os == "Windows" {
-			return "N:"
-		}
-		return "/dev/xvdm"
-	case 13:
-		if os == "Windows" {
-			return "O:"
-		}
-		return "/dev/xvdn"
-	case 14:
-		if os == "Windows" {
-			return "P:"
-		}
-		return "/dev/xvdo"
-	case 15:
-		if os == "Windows" {
-			return "Q:"
-		}
-		return "/dev/xvdp"
-	default:
-		return "unknown"
-	}
-}

builtin/providers/cloudstack/resource_cloudstack_disk_test.go

@@ -29,7 +29,7 @@ func TestAccCloudStackDisk_basic(t *testing.T) {
 	})
 }
 
-func TestAccCloudStackDisk_device(t *testing.T) {
+func TestAccCloudStackDisk_deviceID(t *testing.T) {
 	var disk cloudstack.Volume
 
 	resource.Test(t, resource.TestCase{
@@ -38,13 +38,13 @@ func TestAccCloudStackDisk_device(t *testing.T) {
 		CheckDestroy: testAccCheckCloudStackDiskDestroy,
 		Steps: []resource.TestStep{
 			resource.TestStep{
-				Config: testAccCloudStackDisk_device,
+				Config: testAccCloudStackDisk_deviceID,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckCloudStackDiskExists(
 						"cloudstack_disk.foo", &disk),
 					testAccCheckCloudStackDiskAttributes(&disk),
 					resource.TestCheckResourceAttr(
-						"cloudstack_disk.foo", "device", "/dev/xvde"),
+						"cloudstack_disk.foo", "device_id", "4"),
 				),
 			},
 		},
@@ -170,7 +170,7 @@ resource "cloudstack_disk" "foo" {
 	CLOUDSTACK_DISK_OFFERING_1,
 	CLOUDSTACK_ZONE)
 
-var testAccCloudStackDisk_device = fmt.Sprintf(`
+var testAccCloudStackDisk_deviceID = fmt.Sprintf(`
 resource "cloudstack_instance" "foobar" {
   name = "terraform-test"
   display_name = "terraform"
@@ -184,7 +184,7 @@ resource "cloudstack_instance" "foobar" {
 resource "cloudstack_disk" "foo" {
   name = "terraform-disk"
   attach = true
-  device = "/dev/xvde"
+  device_id = 4
   disk_offering = "%s"
   virtual_machine_id = "${cloudstack_instance.foobar.id}"
   zone = "${cloudstack_instance.foobar.zone}"

builtin/providers/cloudstack/resource_cloudstack_nic.go

@@ -53,13 +53,13 @@ func resourceCloudStackNICCreate(d *schema.ResourceData, meta interface{}) error
 	}
 
 	// Create and attach the new NIC
-	r, err := cs.VirtualMachine.AddNicToVirtualMachine(p)
+	r, err := Retry(10, retryableAddNicFunc(cs, p))
 	if err != nil {
 		return fmt.Errorf("Error creating the new NIC: %s", err)
 	}
 
 	found := false
-	for _, n := range r.Nic {
+	for _, n := range r.(*cloudstack.AddNicToVirtualMachineResponse).Nic {
 		if n.Networkid == d.Get("network_id").(string) {
 			d.SetId(n.Id)
 			found = true
@@ -133,3 +133,13 @@ func resourceCloudStackNICDelete(d *schema.ResourceData, meta interface{}) error
 
 	return nil
 }
+
+func retryableAddNicFunc(cs *cloudstack.CloudStackClient, p *cloudstack.AddNicToVirtualMachineParams) func() (interface{}, error) {
+	return func() (interface{}, error) {
+		r, err := cs.VirtualMachine.AddNicToVirtualMachine(p)
+		if err != nil {
+			return nil, err
+		}
+		return r, nil
+	}
+}

builtin/providers/cloudstack/resource_cloudstack_vpc.go

@@ -171,12 +171,10 @@ func resourceCloudStackVPCRead(d *schema.ResourceData, meta interface{}) error {
 		return err
 	}
 
-	if l.Count != 1 {
-		return fmt.Errorf("Unexpected number (%d) of source NAT IPs returned", l.Count)
+	if l.Count == 1 {
+		d.Set("source_nat_ip", l.PublicIpAddresses[0].Ipaddress)
 	}
 
-	d.Set("source_nat_ip", l.PublicIpAddresses[0].Ipaddress)
-
 	return nil
 }