Merge pull request #8172 from jtopjian/openstack-volumeattach-fix

provider/openstack: Volume Attachment and Detachment Fixes

builtin/providers/openstack/resource_openstack_compute_instance_v2.go

@@ -24,7 +24,6 @@ import (
 	"github.com/rackspace/gophercloud/openstack/compute/v2/flavors"
 	"github.com/rackspace/gophercloud/openstack/compute/v2/images"
 	"github.com/rackspace/gophercloud/openstack/compute/v2/servers"
-	"github.com/rackspace/gophercloud/pagination"
 )
 
 func resourceComputeInstanceV2() *schema.Resource {
@@ -238,17 +237,16 @@ func resourceComputeInstanceV2() *schema.Resource {
 			"volume": &schema.Schema{
 				Type:     schema.TypeSet,
 				Optional: true,
-				Computed: true,
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
 						"id": &schema.Schema{
 							Type:     schema.TypeString,
+							Optional: true,
 							Computed: true,
 						},
 						"volume_id": &schema.Schema{
 							Type:     schema.TypeString,
-							Optional: true,
+							Required: true,
-							Computed: true,
 						},
 						"device": &schema.Schema{
 							Type:     schema.TypeString,
@@ -351,12 +349,6 @@ func resourceComputeInstanceV2Create(d *schema.ResourceData, meta interface{}) e
 		return err
 	}
 
-	// determine if volume configuration is correct
-	// this includes ensuring volume_ids are set
-	if err := checkVolumeConfig(d); err != nil {
-		return err
-	}
-
 	// determine if block_device configuration is correct
 	// this includes valid combinations and required attributes
 	if err := checkBlockDeviceConfig(d); err != nil {
@@ -712,16 +704,12 @@ func resourceComputeInstanceV2Update(d *schema.ResourceData, meta interface{}) e
 	}
 
 	if d.HasChange("volume") {
-		// ensure the volume configuration is correct
-		if err := checkVolumeConfig(d); err != nil {
-			return err
-		}
-
 		// old attachments and new attachments
 		oldAttachments, newAttachments := d.GetChange("volume")
 		// for each old attachment, detach the volume
 		oldAttachmentSet := oldAttachments.(*schema.Set).List()
 
+		log.Printf("[DEBUG] Attempting to detach the following volumes: %#v", oldAttachmentSet)
 		if blockClient, err := config.blockStorageV1Client(d.Get("region").(string)); err != nil {
 			return err
 		} else {
@@ -814,6 +802,22 @@ func resourceComputeInstanceV2Delete(d *schema.ResourceData, meta interface{}) e
 		return fmt.Errorf("Error creating OpenStack compute client: %s", err)
 	}
 
+	// Make sure all volumes are detached before deleting
+	volumes := d.Get("volume")
+	if volumeSet, ok := volumes.(*schema.Set); ok {
+		volumeList := volumeSet.List()
+		if len(volumeList) > 0 {
+			log.Printf("[DEBUG] Attempting to detach the following volumes: %#v", volumeList)
+			if blockClient, err := config.blockStorageV1Client(d.Get("region").(string)); err != nil {
+				return err
+			} else {
+				if err := detachVolumesFromInstance(computeClient, blockClient, d.Id(), volumeList); err != nil {
+					return err
+				}
+			}
+		}
+	}
+
 	if d.Get("stop_before_destroy").(bool) {
 		err = startstop.Stop(computeClient, d.Id()).ExtractErr()
 		if err != nil {
@@ -1397,6 +1401,7 @@ func detachVolumesFromInstance(computeClient *gophercloud.ServiceClient, blockCl
 		va := v.(map[string]interface{})
 		aId := va["id"].(string)
 
+		log.Printf("[INFO] Attempting to detach volume %s", va["volume_id"])
 		if err := volumeattach.Delete(computeClient, serverId, aId).ExtractErr(); err != nil {
 			return err
 		}
@@ -1420,68 +1425,43 @@ func detachVolumesFromInstance(computeClient *gophercloud.ServiceClient, blockCl
 }
 
 func getVolumeAttachments(computeClient *gophercloud.ServiceClient, d *schema.ResourceData) error {
-	var attachments []volumeattach.VolumeAttachment
+	var vols []map[string]interface{}
-
-	err := volumeattach.List(computeClient, d.Id()).EachPage(func(page pagination.Page) (bool, error) {
-		actual, err := volumeattach.ExtractVolumeAttachments(page)
-		if err != nil {
-			return false, err
-		}
-
-		attachments = actual
-		return true, nil
-	})
 
+	allPages, err := volumeattach.List(computeClient, d.Id()).AllPages()
 	if err != nil {
 		return err
 	}
 
-	var vols []map[string]interface{}
+	allVolumeAttachments, err := volumeattach.ExtractVolumeAttachments(allPages)
-	for _, attachment := range attachments {
+	if err != nil {
-		// ignore the volume if it is attached as a root device
+		return err
-		rootDevFound := false
-		for _, rootDev := range []string{"/dev/vda", "/dev/xda", "/dev/sda", "/dev/xvda"} {
-			if attachment.Device == rootDev {
-				rootDevFound = true
-			}
-		}
-
-		if rootDevFound {
-			continue
 	}
 
+	if v, ok := d.GetOk("volume"); ok {
+		volumes := v.(*schema.Set).List()
+		for _, volume := range volumes {
+			if volumeMap, ok := volume.(map[string]interface{}); ok {
+				if v, ok := volumeMap["volume_id"].(string); ok {
+					for _, volumeAttachment := range allVolumeAttachments {
+						if v == volumeAttachment.ID {
 							vol := make(map[string]interface{})
-		vol["id"] = attachment.ID
+							vol["id"] = volumeAttachment.ID
-		vol["volume_id"] = attachment.VolumeID
+							vol["volume_id"] = volumeAttachment.VolumeID
-		vol["device"] = attachment.Device
+							vol["device"] = volumeAttachment.Device
 							vols = append(vols, vol)
 						}
+					}
+				}
+			}
+		}
+	}
+
 	log.Printf("[INFO] Volume attachments: %v", vols)
 	d.Set("volume", vols)
 
 	return nil
 }
 
-func checkVolumeConfig(d *schema.ResourceData) error {
-	// Although a volume_id is required to attach a volume, in order to be able to report
-	// the attached volumes of an instance, it must be "computed" and thus "optional".
-	// This accounts for situations such as "boot from volume" as well as volumes being
-	// attached to the instance outside of Terraform.
-	if v := d.Get("volume"); v != nil {
-		vols := v.(*schema.Set).List()
-		if len(vols) > 0 {
-			for _, v := range vols {
-				va := v.(map[string]interface{})
-				if va["volume_id"].(string) == "" {
-					return fmt.Errorf("A volume_id must be specified when attaching volumes.")
-				}
-			}
-		}
-	}
-
-	return nil
-}
-
 func checkBlockDeviceConfig(d *schema.ResourceData) error {
 	if vL, ok := d.GetOk("block_device"); ok {
 		for _, v := range vL.([]interface{}) {

builtin/providers/openstack/resource_openstack_compute_instance_v2_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/terraform"
 
+	"github.com/rackspace/gophercloud"
 	"github.com/rackspace/gophercloud/openstack/blockstorage/v1/volumes"
 	"github.com/rackspace/gophercloud/openstack/compute/v2/extensions/floatingip"
 	"github.com/rackspace/gophercloud/openstack/compute/v2/extensions/secgroups"
@@ -148,6 +149,11 @@ func TestAccComputeV2Instance_volumeDetachPostCreation(t *testing.T) {
 		}`)
 
 	var testAccComputeV2Instance_volumeDetachPostCreationInstance = fmt.Sprintf(`
+		resource "openstack_blockstorage_volume_v1" "myvol" {
+			name = "myvol"
+			size = 1
+		}
+
 		resource "openstack_compute_instance_v2" "foo" {
 			name = "terraform-test"
 			security_groups = ["default"]
@@ -169,7 +175,7 @@ func TestAccComputeV2Instance_volumeDetachPostCreation(t *testing.T) {
 			resource.TestStep{
 				Config: testAccComputeV2Instance_volumeDetachPostCreationInstance,
 				Check: resource.ComposeTestCheckFunc(
-					testAccCheckBlockStorageV1VolumeDoesNotExist(t, "openstack_blockstorage_volume_v1.myvol", &volume),
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.myvol", &volume),
 					testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.foo", &instance),
 					testAccCheckComputeV2InstanceVolumesDetached(&instance),
 				),
@@ -178,11 +184,12 @@ func TestAccComputeV2Instance_volumeDetachPostCreation(t *testing.T) {
 	})
 }
 
-func TestAccComputeV2Instance_additionalVolumeDetachPostCreation(t *testing.T) {
+func TestAccComputeV2Instance_volumeDetachAdditionalVolumePostCreation(t *testing.T) {
 	var instance servers.Server
-	var volume volumes.Volume
+	var volume_1 volumes.Volume
+	var volume_2 volumes.Volume
 
-	var testAccComputeV2Instance_volumeDetachPostCreationInstanceAndAdditionalVolume = fmt.Sprintf(`
+	var testAccComputeV2Instance_volumeDetachAdditionalVolumePostCreationInstanceAndVolume = fmt.Sprintf(`
 
 		resource "openstack_blockstorage_volume_v1" "root_volume" {
 			name = "root_volume"
@@ -246,22 +253,22 @@ func TestAccComputeV2Instance_additionalVolumeDetachPostCreation(t *testing.T) {
 		CheckDestroy: testAccCheckComputeV2InstanceDestroy,
 		Steps: []resource.TestStep{
 			resource.TestStep{
-				Config: testAccComputeV2Instance_volumeDetachPostCreationInstanceAndAdditionalVolume,
+				Config: testAccComputeV2Instance_volumeDetachAdditionalVolumePostCreationInstanceAndVolume,
 				Check: resource.ComposeTestCheckFunc(
-					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.root_volume", &volume),
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.root_volume", &volume_1),
-					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.additional_volume", &volume),
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.additional_volume", &volume_2),
 					testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.foo", &instance),
-					testAccCheckComputeV2InstanceVolumeAttachment(&instance, &volume),
+					testAccCheckComputeV2InstanceVolumeAttachment(&instance, &volume_1),
-					testAccCheckComputeV2InstanceVolumeAttachment(&instance, &volume),
+					testAccCheckComputeV2InstanceVolumeAttachment(&instance, &volume_2),
 				),
 			},
 			resource.TestStep{
 				Config: testAccComputeV2Instance_volumeDetachPostCreationInstance,
 				Check: resource.ComposeTestCheckFunc(
-					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.root_volume", &volume),
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.root_volume", &volume_1),
-					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.additional_volume", &volume),
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.additional_volume", &volume_2),
 					testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.foo", &instance),
-					testAccCheckComputeV2InstanceVolumeAttachment(&instance, &volume),
+					testAccCheckComputeV2InstanceVolumeAttachment(&instance, &volume_1),
 					testAccCheckComputeV2InstanceVolumeDetached(&instance, "openstack_blockstorage_volume_v1.additional_volume"),
 				),
 			},
@@ -269,6 +276,159 @@ func TestAccComputeV2Instance_additionalVolumeDetachPostCreation(t *testing.T) {
 	})
 }
 
+func TestAccComputeV2Instance_volumeAttachInstanceDelete(t *testing.T) {
+	var instance servers.Server
+	var volume_1 volumes.Volume
+	var volume_2 volumes.Volume
+
+	var testAccComputeV2Instance_volumeAttachInstanceDelete_1 = fmt.Sprintf(`
+		resource "openstack_blockstorage_volume_v1" "root_volume" {
+			name = "root_volume"
+			size = 1
+			image_id = "%s"
+		}
+
+		resource "openstack_blockstorage_volume_v1" "additional_volume" {
+			name = "additional_volume"
+			size = 1
+		}
+
+		resource "openstack_compute_instance_v2" "foo" {
+			name = "terraform-test"
+			security_groups = ["default"]
+
+			block_device {
+				uuid = "${openstack_blockstorage_volume_v1.root_volume.id}"
+				source_type = "volume"
+				boot_index = 0
+				destination_type = "volume"
+				delete_on_termination = false
+			}
+
+			volume {
+				volume_id = "${openstack_blockstorage_volume_v1.additional_volume.id}"
+			}
+		}`,
+		os.Getenv("OS_IMAGE_ID"))
+
+	var testAccComputeV2Instance_volumeAttachInstanceDelete_2 = fmt.Sprintf(`
+		resource "openstack_blockstorage_volume_v1" "root_volume" {
+			name = "root_volume"
+			size = 1
+			image_id = "%s"
+		}
+
+		resource "openstack_blockstorage_volume_v1" "additional_volume" {
+			name = "additional_volume"
+			size = 1
+		}`,
+		os.Getenv("OS_IMAGE_ID"))
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckComputeV2InstanceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccComputeV2Instance_volumeAttachInstanceDelete_1,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.root_volume", &volume_1),
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.additional_volume", &volume_2),
+					testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.foo", &instance),
+					testAccCheckComputeV2InstanceVolumeAttachment(&instance, &volume_1),
+					testAccCheckComputeV2InstanceVolumeAttachment(&instance, &volume_2),
+				),
+			},
+			resource.TestStep{
+				Config: testAccComputeV2Instance_volumeAttachInstanceDelete_2,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.root_volume", &volume_1),
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.additional_volume", &volume_2),
+					testAccCheckComputeV2InstanceDoesNotExist(t, "openstack_compute_instance_v2.foo", &instance),
+					testAccCheckComputeV2InstanceVolumeDetached(&instance, "openstack_blockstorage_volume_v1.root_volume"),
+					testAccCheckComputeV2InstanceVolumeDetached(&instance, "openstack_blockstorage_volume_v1.additional_volume"),
+				),
+			},
+		},
+	})
+}
+
+func TestAccComputeV2Instance_volumeAttachToNewInstance(t *testing.T) {
+	var instance_1 servers.Server
+	var instance_2 servers.Server
+	var volume_1 volumes.Volume
+
+	var testAccComputeV2Instance_volumeAttachToNewInstance_1 = fmt.Sprintf(`
+		resource "openstack_blockstorage_volume_v1" "volume_1" {
+			name = "volume_1"
+			size = 1
+		}
+
+		resource "openstack_compute_instance_v2" "instance_1" {
+			name = "instance_1"
+			security_groups = ["default"]
+
+			volume {
+				volume_id = "${openstack_blockstorage_volume_v1.volume_1.id}"
+			}
+		}
+
+		resource "openstack_compute_instance_v2" "instance_2" {
+			depends_on = ["openstack_compute_instance_v2.instance_1"]
+			name = "instance_2"
+			security_groups = ["default"]
+		}`)
+
+	var testAccComputeV2Instance_volumeAttachToNewInstance_2 = fmt.Sprintf(`
+			resource "openstack_blockstorage_volume_v1" "volume_1" {
+				name = "volume_1"
+				size = 1
+			}
+
+			resource "openstack_compute_instance_v2" "instance_1" {
+				name = "instance_1"
+				security_groups = ["default"]
+			}
+
+			resource "openstack_compute_instance_v2" "instance_2" {
+				depends_on = ["openstack_compute_instance_v2.instance_1"]
+				name = "instance_2"
+				security_groups = ["default"]
+
+				volume {
+					volume_id = "${openstack_blockstorage_volume_v1.volume_1.id}"
+				}
+			}`)
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckComputeV2InstanceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccComputeV2Instance_volumeAttachToNewInstance_1,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.volume_1", &volume_1),
+					testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.instance_1", &instance_1),
+					testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.instance_2", &instance_2),
+					testAccCheckComputeV2InstanceVolumeAttachment(&instance_1, &volume_1),
+					testAccCheckComputeV2InstanceVolumeDetached(&instance_2, "openstack_blockstorage_volume_v1.volume_1"),
+				),
+			},
+			resource.TestStep{
+				Config: testAccComputeV2Instance_volumeAttachToNewInstance_2,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.volume_1", &volume_1),
+					testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.instance_1", &instance_1),
+					testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.instance_2", &instance_2),
+					testAccCheckComputeV2InstanceVolumeDetached(&instance_1, "openstack_blockstorage_volume_v1.volume_1"),
+					testAccCheckComputeV2InstanceVolumeAttachment(&instance_2, &volume_1),
+				),
+			},
+		},
+	})
+}
+
 func TestAccComputeV2Instance_floatingIPAttachGlobally(t *testing.T) {
 	var instance servers.Server
 	var fip floatingip.FloatingIP
@@ -928,6 +1088,30 @@ func testAccCheckComputeV2InstanceExists(t *testing.T, n string, instance *serve
 	}
 }
 
+func testAccCheckComputeV2InstanceDoesNotExist(t *testing.T, n string, instance *servers.Server) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		config := testAccProvider.Meta().(*Config)
+		computeClient, err := config.computeV2Client(OS_REGION_NAME)
+		if err != nil {
+			return fmt.Errorf("(testAccCheckComputeV2InstanceExists) Error creating OpenStack compute client: %s", err)
+		}
+
+		_, err = servers.Get(computeClient, instance.ID).Extract()
+		if err != nil {
+			errCode, ok := err.(*gophercloud.UnexpectedResponseCodeError)
+			if !ok {
+				return err
+			}
+			if errCode.Actual == 404 {
+				return nil
+			}
+			return err
+		}
+
+		return fmt.Errorf("Instance still exists")
+	}
+}
+
 func testAccCheckComputeV2InstanceMetadata(
 	instance *servers.Server, k string, v string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {