From e75553fd9d4793871ba391dbf8c2edf283f74975 Mon Sep 17 00:00:00 2001 From: Joe Topjian Date: Sat, 12 Sep 2015 04:15:21 +0000 Subject: [PATCH 1/3] provider/openstack: Fixes Boot From Volume This commit fixes the previously broken "boot from volume" feature. It also adds an acceptance test to ensure the feature continues to work. The "delete_on_termination" option was also added. --- .../resource_openstack_compute_instance_v2.go | 63 +++++++++++++++---- ...urce_openstack_compute_instance_v2_test.go | 61 ++++++++++++++++++ 2 files changed, 112 insertions(+), 12 deletions(-) diff --git a/builtin/providers/openstack/resource_openstack_compute_instance_v2.go b/builtin/providers/openstack/resource_openstack_compute_instance_v2.go index e30295e44602..5e578b45366f 100644 --- a/builtin/providers/openstack/resource_openstack_compute_instance_v2.go +++ b/builtin/providers/openstack/resource_openstack_compute_instance_v2.go @@ -176,7 +176,7 @@ func resourceComputeInstanceV2() *schema.Resource { ForceNew: true, }, "block_device": &schema.Schema{ - Type: schema.TypeList, + Type: schema.TypeSet, Optional: true, ForceNew: true, Elem: &schema.Resource{ @@ -201,12 +201,22 @@ func resourceComputeInstanceV2() *schema.Resource { Type: schema.TypeInt, Optional: true, }, + "delete_on_termination": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + Default: false, + }, }, }, + Set: func(v interface{}) int { + // there can only be one bootable block device; no need to hash anything + return 0 + }, }, "volume": &schema.Schema{ Type: schema.TypeSet, Optional: true, + Computed: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "id": &schema.Schema{ @@ -215,7 +225,8 @@ func resourceComputeInstanceV2() *schema.Resource { }, "volume_id": &schema.Schema{ Type: schema.TypeString, - Required: true, + Optional: true, + Computed: true, }, "device": &schema.Schema{ Type: schema.TypeString, @@ -325,11 +336,19 @@ func resourceComputeInstanceV2Create(d *schema.ResourceData, meta interface{}) e } } - if blockDeviceRaw, ok := d.Get("block_device").(map[string]interface{}); ok && blockDeviceRaw != nil { - blockDevice := resourceInstanceBlockDeviceV2(d, blockDeviceRaw) - createOpts = &bootfromvolume.CreateOptsExt{ - createOpts, - blockDevice, + if v, ok := d.GetOk("block_device"); ok { + vL := v.(*schema.Set).List() + if len(vL) > 1 { + return fmt.Errorf("Can only specify one block device to boot from.") + } + for _, v := range vL { + blockDeviceRaw := v.(map[string]interface{}) + blockDevice := resourceInstanceBlockDeviceV2(d, blockDeviceRaw) + createOpts = &bootfromvolume.CreateOptsExt{ + createOpts, + blockDevice, + } + log.Printf("[DEBUG] Create BFV Options: %+v", createOpts) } } @@ -343,6 +362,23 @@ func resourceComputeInstanceV2Create(d *schema.ResourceData, meta interface{}) e } } + // Boot From Volume makes the root volume/disk appear as an attached volume. + // Because of that, and in order to accurately report volume status, the volume_id + // of the "volume" parameter must be computed and optional. + // However, a volume_id, of course, is required to attach a volume. We do the check + // here to fail early (before the instance is created) if a volume_id was not specified. + 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.") + } + } + } + } + log.Printf("[DEBUG] Create Options: %#v", createOpts) server, err := servers.Create(computeClient, createOpts).Extract() if err != nil { @@ -388,6 +424,7 @@ func resourceComputeInstanceV2Create(d *schema.ResourceData, meta interface{}) e if blockClient, err := config.blockStorageV1Client(d.Get("region").(string)); err != nil { return fmt.Errorf("Error creating OpenStack block storage client: %s", err) } else { + if err := attachVolumesToInstance(computeClient, blockClient, d.Id(), vols); err != nil { return err } @@ -919,11 +956,12 @@ func resourceInstanceBlockDeviceV2(d *schema.ResourceData, bd map[string]interfa sourceType := bootfromvolume.SourceType(bd["source_type"].(string)) bfvOpts := []bootfromvolume.BlockDevice{ bootfromvolume.BlockDevice{ - UUID: bd["uuid"].(string), - SourceType: sourceType, - VolumeSize: bd["volume_size"].(int), - DestinationType: bd["destination_type"].(string), - BootIndex: bd["boot_index"].(int), + UUID: bd["uuid"].(string), + SourceType: sourceType, + VolumeSize: bd["volume_size"].(int), + DestinationType: bd["destination_type"].(string), + BootIndex: bd["boot_index"].(int), + DeleteOnTermination: bd["delete_on_termination"].(bool), }, } @@ -1046,6 +1084,7 @@ func resourceComputeVolumeAttachmentHash(v interface{}) int { var buf bytes.Buffer m := v.(map[string]interface{}) buf.WriteString(fmt.Sprintf("%s-", m["volume_id"].(string))) + return hashcode.String(buf.String()) } diff --git a/builtin/providers/openstack/resource_openstack_compute_instance_v2_test.go b/builtin/providers/openstack/resource_openstack_compute_instance_v2_test.go index b2160f2670fb..5d44b6bedb9b 100644 --- a/builtin/providers/openstack/resource_openstack_compute_instance_v2_test.go +++ b/builtin/providers/openstack/resource_openstack_compute_instance_v2_test.go @@ -143,6 +143,39 @@ func TestAccComputeV2Instance_multi_secgroups(t *testing.T) { }) } +func TestAccComputeV2Instance_bootFromVolume(t *testing.T) { + var instance servers.Server + var testAccComputeV2Instance_bootFromVolume = fmt.Sprintf(` + resource "openstack_compute_instance_v2" "foo" { + name = "terraform-test" + security_groups = ["default"] + block_device { + uuid = "%s" + source_type = "image" + volume_size = 5 + boot_index = 0 + destination_type = "volume" + delete_on_termination = true + } + }`, + 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_bootFromVolume, + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.foo", &instance), + testAccCheckComputeV2InstanceBootVolumeAttachment(&instance), + ), + }, + }, + }) +} + func testAccCheckComputeV2InstanceDestroy(s *terraform.State) error { config := testAccProvider.Meta().(*Config) computeClient, err := config.computeV2Client(OS_REGION_NAME) @@ -249,6 +282,34 @@ func testAccCheckComputeV2InstanceVolumeAttachment( } } +func testAccCheckComputeV2InstanceBootVolumeAttachment( + instance *servers.Server) resource.TestCheckFunc { + return func(s *terraform.State) error { + var attachments []volumeattach.VolumeAttachment + + config := testAccProvider.Meta().(*Config) + computeClient, err := config.computeV2Client(OS_REGION_NAME) + if err != nil { + return err + } + err = volumeattach.List(computeClient, instance.ID).EachPage(func(page pagination.Page) (bool, error) { + actual, err := volumeattach.ExtractVolumeAttachments(page) + if err != nil { + return false, fmt.Errorf("Unable to lookup attachment: %s", err) + } + + attachments = actual + return true, nil + }) + + if len(attachments) == 1 { + return nil + } + + return fmt.Errorf("No attached volume found.") + } +} + func testAccCheckComputeV2InstanceFloatingIPAttach( instance *servers.Server, fip *floatingip.FloatingIP) resource.TestCheckFunc { return func(s *terraform.State) error { From 3d3f8122a95ad54dc220654d50f08da72ede0dfe Mon Sep 17 00:00:00 2001 From: Joe Topjian Date: Sat, 12 Sep 2015 17:56:38 +0000 Subject: [PATCH 2/3] provider/openstack: Volume Cleanup This commit cleans up the volume and block device handling in the instance resource. It also adds more acceptance tests to deal with different workflows of attaching and detaching a volume through the instance's lifecycle. No new functionality has been added. --- ...e_openstack_blockstorage_volume_v1_test.go | 25 ++ .../resource_openstack_compute_instance_v2.go | 236 +++++++++--------- ...urce_openstack_compute_instance_v2_test.go | 153 ++++++++++-- 3 files changed, 286 insertions(+), 128 deletions(-) diff --git a/builtin/providers/openstack/resource_openstack_blockstorage_volume_v1_test.go b/builtin/providers/openstack/resource_openstack_blockstorage_volume_v1_test.go index acd9ccd22ece..3900e2331e35 100644 --- a/builtin/providers/openstack/resource_openstack_blockstorage_volume_v1_test.go +++ b/builtin/providers/openstack/resource_openstack_blockstorage_volume_v1_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" ) @@ -106,6 +107,30 @@ func testAccCheckBlockStorageV1VolumeExists(t *testing.T, n string, volume *volu } } +func testAccCheckBlockStorageV1VolumeDoesNotExist(t *testing.T, n string, volume *volumes.Volume) resource.TestCheckFunc { + return func(s *terraform.State) error { + config := testAccProvider.Meta().(*Config) + blockStorageClient, err := config.blockStorageV1Client(OS_REGION_NAME) + if err != nil { + return fmt.Errorf("Error creating OpenStack block storage client: %s", err) + } + + _, err = volumes.Get(blockStorageClient, volume.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("Volume still exists") + } +} + func testAccCheckBlockStorageV1VolumeMetadata( volume *volumes.Volume, k string, v string) resource.TestCheckFunc { return func(s *terraform.State) error { diff --git a/builtin/providers/openstack/resource_openstack_compute_instance_v2.go b/builtin/providers/openstack/resource_openstack_compute_instance_v2.go index 5e578b45366f..36f3fcc15e8c 100644 --- a/builtin/providers/openstack/resource_openstack_compute_instance_v2.go +++ b/builtin/providers/openstack/resource_openstack_compute_instance_v2.go @@ -176,6 +176,11 @@ func resourceComputeInstanceV2() *schema.Resource { ForceNew: true, }, "block_device": &schema.Schema{ + // TODO: This is a set because we don't support singleton + // sub-resources today. We'll enforce that the set only ever has + // length zero or one below. When TF gains support for + // sub-resources this can be converted. + // As referenced in resource_aws_instance.go Type: schema.TypeSet, Optional: true, ForceNew: true, @@ -307,6 +312,13 @@ func resourceComputeInstanceV2Create(d *schema.ResourceData, meta interface{}) e return err } + // determine if volume/block_device configuration is correct + // this includes ensuring volume_ids are set + // and if only one block_device was specified. + if err := checkVolumeConfig(d); err != nil { + return err + } + networks := make([]servers.Network, len(networkDetails)) for i, net := range networkDetails { networks[i] = servers.Network{ @@ -338,9 +350,6 @@ func resourceComputeInstanceV2Create(d *schema.ResourceData, meta interface{}) e if v, ok := d.GetOk("block_device"); ok { vL := v.(*schema.Set).List() - if len(vL) > 1 { - return fmt.Errorf("Can only specify one block device to boot from.") - } for _, v := range vL { blockDeviceRaw := v.(map[string]interface{}) blockDevice := resourceInstanceBlockDeviceV2(d, blockDeviceRaw) @@ -362,23 +371,6 @@ func resourceComputeInstanceV2Create(d *schema.ResourceData, meta interface{}) e } } - // Boot From Volume makes the root volume/disk appear as an attached volume. - // Because of that, and in order to accurately report volume status, the volume_id - // of the "volume" parameter must be computed and optional. - // However, a volume_id, of course, is required to attach a volume. We do the check - // here to fail early (before the instance is created) if a volume_id was not specified. - 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.") - } - } - } - } - log.Printf("[DEBUG] Create Options: %#v", createOpts) server, err := servers.Create(computeClient, createOpts).Extract() if err != nil { @@ -417,17 +409,14 @@ func resourceComputeInstanceV2Create(d *schema.ResourceData, meta interface{}) e } } - // were volume attachments specified? - if v := d.Get("volume"); v != nil { + // if volumes were specified, attach them after the instance has launched. + if v, ok := d.GetOk("volume"); ok { vols := v.(*schema.Set).List() - if len(vols) > 0 { - if blockClient, err := config.blockStorageV1Client(d.Get("region").(string)); err != nil { - return fmt.Errorf("Error creating OpenStack block storage client: %s", err) - } else { - - if err := attachVolumesToInstance(computeClient, blockClient, d.Id(), vols); err != nil { - return err - } + if blockClient, err := config.blockStorageV1Client(d.Get("region").(string)); err != nil { + return fmt.Errorf("Error creating OpenStack block storage client: %s", err) + } else { + if err := attachVolumesToInstance(computeClient, blockClient, d.Id(), vols); err != nil { + return err } } } @@ -578,21 +567,9 @@ func resourceComputeInstanceV2Read(d *schema.ResourceData, meta interface{}) err d.Set("image_name", image.Name) // volume attachments - vas, err := getVolumeAttachments(computeClient, d.Id()) - if err != nil { + if err := getVolumeAttachments(computeClient, d); err != nil { return err } - if len(vas) > 0 { - attachments := make([]map[string]interface{}, len(vas)) - for i, attachment := range vas { - attachments[i] = make(map[string]interface{}) - attachments[i]["id"] = attachment.ID - attachments[i]["volume_id"] = attachment.VolumeID - attachments[i]["device"] = attachment.Device - } - log.Printf("[INFO] Volume attachments: %v", attachments) - d.Set("volume", attachments) - } return nil } @@ -702,30 +679,31 @@ 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() - if len(oldAttachmentSet) > 0 { - if blockClient, err := config.blockStorageV1Client(d.Get("region").(string)); err != nil { + if blockClient, err := config.blockStorageV1Client(d.Get("region").(string)); err != nil { + return err + } else { + if err := detachVolumesFromInstance(computeClient, blockClient, d.Id(), oldAttachmentSet); err != nil { return err - } else { - if err := detachVolumesFromInstance(computeClient, blockClient, d.Id(), oldAttachmentSet); err != nil { - return err - } } } // for each new attachment, attach the volume newAttachmentSet := newAttachments.(*schema.Set).List() - if len(newAttachmentSet) > 0 { - if blockClient, err := config.blockStorageV1Client(d.Get("region").(string)); err != nil { + if blockClient, err := config.blockStorageV1Client(d.Get("region").(string)); err != nil { + return err + } else { + if err := attachVolumesToInstance(computeClient, blockClient, d.Id(), newAttachmentSet); err != nil { return err - } else { - if err := attachVolumesToInstance(computeClient, blockClient, d.Id(), newAttachmentSet); err != nil { - return err - } } } @@ -1112,81 +1090,78 @@ func resourceComputeSchedulerHintsHash(v interface{}) int { } func attachVolumesToInstance(computeClient *gophercloud.ServiceClient, blockClient *gophercloud.ServiceClient, serverId string, vols []interface{}) error { - if len(vols) > 0 { - for _, v := range vols { - va := v.(map[string]interface{}) - volumeId := va["volume_id"].(string) - device := va["device"].(string) - - s := "" - if serverId != "" { - s = serverId - } else if va["server_id"] != "" { - s = va["server_id"].(string) - } else { - return fmt.Errorf("Unable to determine server ID to attach volume.") - } - - vaOpts := &volumeattach.CreateOpts{ - Device: device, - VolumeID: volumeId, - } + for _, v := range vols { + va := v.(map[string]interface{}) + volumeId := va["volume_id"].(string) + device := va["device"].(string) + + s := "" + if serverId != "" { + s = serverId + } else if va["server_id"] != "" { + s = va["server_id"].(string) + } else { + return fmt.Errorf("Unable to determine server ID to attach volume.") + } - if _, err := volumeattach.Create(computeClient, s, vaOpts).Extract(); err != nil { - return err - } + vaOpts := &volumeattach.CreateOpts{ + Device: device, + VolumeID: volumeId, + } - stateConf := &resource.StateChangeConf{ - Pending: []string{"attaching", "available"}, - Target: "in-use", - Refresh: VolumeV1StateRefreshFunc(blockClient, va["volume_id"].(string)), - Timeout: 30 * time.Minute, - Delay: 5 * time.Second, - MinTimeout: 2 * time.Second, - } + if _, err := volumeattach.Create(computeClient, s, vaOpts).Extract(); err != nil { + return err + } - if _, err := stateConf.WaitForState(); err != nil { - return err - } + stateConf := &resource.StateChangeConf{ + Pending: []string{"attaching", "available"}, + Target: "in-use", + Refresh: VolumeV1StateRefreshFunc(blockClient, va["volume_id"].(string)), + Timeout: 30 * time.Minute, + Delay: 5 * time.Second, + MinTimeout: 2 * time.Second, + } - log.Printf("[INFO] Attached volume %s to instance %s", volumeId, serverId) + if _, err := stateConf.WaitForState(); err != nil { + return err } + + log.Printf("[INFO] Attached volume %s to instance %s", volumeId, serverId) } return nil } func detachVolumesFromInstance(computeClient *gophercloud.ServiceClient, blockClient *gophercloud.ServiceClient, serverId string, vols []interface{}) error { - if len(vols) > 0 { - for _, v := range vols { - va := v.(map[string]interface{}) - aId := va["id"].(string) + for _, v := range vols { + va := v.(map[string]interface{}) + aId := va["id"].(string) - if err := volumeattach.Delete(computeClient, serverId, aId).ExtractErr(); err != nil { - return err - } + if err := volumeattach.Delete(computeClient, serverId, aId).ExtractErr(); err != nil { + return err + } - stateConf := &resource.StateChangeConf{ - Pending: []string{"detaching", "in-use"}, - Target: "available", - Refresh: VolumeV1StateRefreshFunc(blockClient, va["volume_id"].(string)), - Timeout: 30 * time.Minute, - Delay: 5 * time.Second, - MinTimeout: 2 * time.Second, - } + stateConf := &resource.StateChangeConf{ + Pending: []string{"detaching", "in-use"}, + Target: "available", + Refresh: VolumeV1StateRefreshFunc(blockClient, va["volume_id"].(string)), + Timeout: 30 * time.Minute, + Delay: 5 * time.Second, + MinTimeout: 2 * time.Second, + } - if _, err := stateConf.WaitForState(); err != nil { - return err - } - log.Printf("[INFO] Detached volume %s from instance %s", va["volume_id"], serverId) + if _, err := stateConf.WaitForState(); err != nil { + return err } + log.Printf("[INFO] Detached volume %s from instance %s", va["volume_id"], serverId) } return nil } -func getVolumeAttachments(computeClient *gophercloud.ServiceClient, serverId string) ([]volumeattach.VolumeAttachment, error) { +func getVolumeAttachments(computeClient *gophercloud.ServiceClient, d *schema.ResourceData) error { var attachments []volumeattach.VolumeAttachment - err := volumeattach.List(computeClient, serverId).EachPage(func(page pagination.Page) (bool, error) { + + err := volumeattach.List(computeClient, d.Id()).EachPage(func(page pagination.Page) (bool, error) { actual, err := volumeattach.ExtractVolumeAttachments(page) if err != nil { return false, err @@ -1197,8 +1172,45 @@ func getVolumeAttachments(computeClient *gophercloud.ServiceClient, serverId str }) if err != nil { - return nil, err + return err + } + + vols := make([]map[string]interface{}, len(attachments)) + for i, attachment := range attachments { + vols[i] = make(map[string]interface{}) + vols[i]["id"] = attachment.ID + vols[i]["volume_id"] = attachment.VolumeID + vols[i]["device"] = attachment.Device } + log.Printf("[INFO] Volume attachments: %v", vols) + d.Set("volume", vols) - return attachments, nil + 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.") + } + } + } + } + + if v, ok := d.GetOk("block_device"); ok { + vL := v.(*schema.Set).List() + if len(vL) > 1 { + return fmt.Errorf("Can only specify one block device to boot from.") + } + } + + return nil } diff --git a/builtin/providers/openstack/resource_openstack_compute_instance_v2_test.go b/builtin/providers/openstack/resource_openstack_compute_instance_v2_test.go index 5d44b6bedb9b..fa5533508f0f 100644 --- a/builtin/providers/openstack/resource_openstack_compute_instance_v2_test.go +++ b/builtin/providers/openstack/resource_openstack_compute_instance_v2_test.go @@ -51,6 +51,20 @@ func TestAccComputeV2Instance_volumeAttach(t *testing.T) { var instance servers.Server var volume volumes.Volume + var testAccComputeV2Instance_volumeAttach = fmt.Sprintf(` + resource "openstack_blockstorage_volume_v1" "myvol" { + name = "myvol" + size = 1 + } + + resource "openstack_compute_instance_v2" "foo" { + name = "terraform-test" + security_groups = ["default"] + volume { + volume_id = "${openstack_blockstorage_volume_v1.myvol.id}" + } + }`) + resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, @@ -68,6 +82,102 @@ func TestAccComputeV2Instance_volumeAttach(t *testing.T) { }) } +func TestAccComputeV2Instance_volumeAttachPostCreation(t *testing.T) { + var instance servers.Server + var volume volumes.Volume + + var testAccComputeV2Instance_volumeAttachPostCreationInstance = fmt.Sprintf(` + resource "openstack_compute_instance_v2" "foo" { + name = "terraform-test" + security_groups = ["default"] + }`) + + var testAccComputeV2Instance_volumeAttachPostCreationInstanceAndVolume = fmt.Sprintf(` + resource "openstack_blockstorage_volume_v1" "myvol" { + name = "myvol" + size = 1 + } + + resource "openstack_compute_instance_v2" "foo" { + name = "terraform-test" + security_groups = ["default"] + volume { + volume_id = "${openstack_blockstorage_volume_v1.myvol.id}" + } + }`) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeV2InstanceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccComputeV2Instance_volumeAttachPostCreationInstance, + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.foo", &instance), + ), + }, + resource.TestStep{ + Config: testAccComputeV2Instance_volumeAttachPostCreationInstanceAndVolume, + Check: resource.ComposeTestCheckFunc( + testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.myvol", &volume), + testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.foo", &instance), + testAccCheckComputeV2InstanceVolumeAttachment(&instance, &volume), + ), + }, + }, + }) +} + +func TestAccComputeV2Instance_volumeDetachPostCreation(t *testing.T) { + var instance servers.Server + var volume volumes.Volume + + var testAccComputeV2Instance_volumeDetachPostCreationInstanceAndVolume = fmt.Sprintf(` + resource "openstack_blockstorage_volume_v1" "myvol" { + name = "myvol" + size = 1 + } + + resource "openstack_compute_instance_v2" "foo" { + name = "terraform-test" + security_groups = ["default"] + volume { + volume_id = "${openstack_blockstorage_volume_v1.myvol.id}" + } + }`) + + var testAccComputeV2Instance_volumeDetachPostCreationInstance = fmt.Sprintf(` + resource "openstack_compute_instance_v2" "foo" { + name = "terraform-test" + security_groups = ["default"] + }`) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeV2InstanceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccComputeV2Instance_volumeDetachPostCreationInstanceAndVolume, + Check: resource.ComposeTestCheckFunc( + testAccCheckBlockStorageV1VolumeExists(t, "openstack_blockstorage_volume_v1.myvol", &volume), + testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.foo", &instance), + testAccCheckComputeV2InstanceVolumeAttachment(&instance, &volume), + ), + }, + resource.TestStep{ + Config: testAccComputeV2Instance_volumeDetachPostCreationInstance, + Check: resource.ComposeTestCheckFunc( + testAccCheckBlockStorageV1VolumeDoesNotExist(t, "openstack_blockstorage_volume_v1.myvol", &volume), + testAccCheckComputeV2InstanceExists(t, "openstack_compute_instance_v2.foo", &instance), + testAccCheckComputeV2InstanceVolumesDetached(&instance), + ), + }, + }, + }) +} + func TestAccComputeV2Instance_floatingIPAttach(t *testing.T) { var instance servers.Server var fip floatingip.FloatingIP @@ -282,6 +392,33 @@ func testAccCheckComputeV2InstanceVolumeAttachment( } } +func testAccCheckComputeV2InstanceVolumesDetached(instance *servers.Server) resource.TestCheckFunc { + return func(s *terraform.State) error { + var attachments []volumeattach.VolumeAttachment + + config := testAccProvider.Meta().(*Config) + computeClient, err := config.computeV2Client(OS_REGION_NAME) + if err != nil { + return err + } + err = volumeattach.List(computeClient, instance.ID).EachPage(func(page pagination.Page) (bool, error) { + actual, err := volumeattach.ExtractVolumeAttachments(page) + if err != nil { + return false, fmt.Errorf("Unable to lookup attachment: %s", err) + } + + attachments = actual + return true, nil + }) + + if len(attachments) > 0 { + return fmt.Errorf("Volumes are still attached.") + } + + return nil + } +} + func testAccCheckComputeV2InstanceBootVolumeAttachment( instance *servers.Server) resource.TestCheckFunc { return func(s *terraform.State) error { @@ -321,19 +458,3 @@ func testAccCheckComputeV2InstanceFloatingIPAttach( } } - -var testAccComputeV2Instance_volumeAttach = fmt.Sprintf(` - resource "openstack_blockstorage_volume_v1" "myvol" { - name = "myvol" - size = 1 - } - - resource "openstack_compute_instance_v2" "foo" { - region = "%s" - name = "terraform-test" - security_groups = ["default"] - volume { - volume_id = "${openstack_blockstorage_volume_v1.myvol.id}" - } - }`, - OS_REGION_NAME) From 4a5cd0b4154cd380edce857c91736298250491d8 Mon Sep 17 00:00:00 2001 From: Joe Topjian Date: Sat, 12 Sep 2015 20:21:55 +0000 Subject: [PATCH 3/3] provider/openstack: Fixing Image ID/Name areas This commit cleans up areas that configure the image_id and image_name. It enables the ability to not have to specify an image_id or image_name when booting from a volume. It also prevents Terraform from reporting an error when an image name is no longer able to be resolved from an image ID. This usually happens when the image has been deleted, but there are still running instances that were based off of it. The image_id and image_name parameters no longer immediately take a default value from the OS_IMAGE_ID and OS_IMAGE_NAME environment variables. If no other resolution of an image_id or image_name were found, then these variables will be referenced. This further supports booting from a volume. Finally, documentation was updated to take into account booting from a volume. --- .../resource_openstack_compute_instance_v2.go | 131 +++++++++++------- .../r/compute_instance_v2.html.markdown | 10 +- 2 files changed, 88 insertions(+), 53 deletions(-) diff --git a/builtin/providers/openstack/resource_openstack_compute_instance_v2.go b/builtin/providers/openstack/resource_openstack_compute_instance_v2.go index 36f3fcc15e8c..4cf68de038ab 100644 --- a/builtin/providers/openstack/resource_openstack_compute_instance_v2.go +++ b/builtin/providers/openstack/resource_openstack_compute_instance_v2.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "fmt" "log" + "os" "time" "github.com/hashicorp/terraform/helper/hashcode" @@ -45,18 +46,16 @@ func resourceComputeInstanceV2() *schema.Resource { ForceNew: false, }, "image_id": &schema.Schema{ - Type: schema.TypeString, - Optional: true, - ForceNew: true, - Computed: true, - DefaultFunc: envDefaultFunc("OS_IMAGE_ID"), + Type: schema.TypeString, + Optional: true, + ForceNew: true, + Computed: true, }, "image_name": &schema.Schema{ - Type: schema.TypeString, - Optional: true, - ForceNew: true, - Computed: true, - DefaultFunc: envDefaultFunc("OS_IMAGE_NAME"), + Type: schema.TypeString, + Optional: true, + ForceNew: true, + Computed: true, }, "flavor_id": &schema.Schema{ Type: schema.TypeString, @@ -297,7 +296,11 @@ func resourceComputeInstanceV2Create(d *schema.ResourceData, meta interface{}) e var createOpts servers.CreateOptsBuilder - imageId, err := getImageID(computeClient, d) + // Determines the Image ID using the following rules: + // If a bootable block_device was specified, ignore the image altogether. + // If an image_id was specified, use it. + // If an image_name was specified, look up the image ID, report if error. + imageId, err := getImageIDFromConfig(computeClient, d) if err != nil { return err } @@ -372,7 +375,16 @@ func resourceComputeInstanceV2Create(d *schema.ResourceData, meta interface{}) e } log.Printf("[DEBUG] Create Options: %#v", createOpts) - server, err := servers.Create(computeClient, createOpts).Extract() + + // If a block_device is used, use the bootfromvolume.Create function as it allows an empty ImageRef. + // Otherwise, use the normal servers.Create function. + var server *servers.Server + if _, ok := d.GetOk("block_device"); ok { + server, err = bootfromvolume.Create(computeClient, createOpts).Extract() + } else { + server, err = servers.Create(computeClient, createOpts).Extract() + } + if err != nil { return fmt.Errorf("Error creating OpenStack server: %s", err) } @@ -554,17 +566,10 @@ func resourceComputeInstanceV2Read(d *schema.ResourceData, meta interface{}) err } d.Set("flavor_name", flavor.Name) - imageId, ok := server.Image["id"].(string) - if !ok { - return fmt.Errorf("Error setting OpenStack server's image: %v", server.Image) - } - d.Set("image_id", imageId) - - image, err := images.Get(computeClient, imageId).Extract() - if err != nil { + // Set the instance's image information appropriately + if err := setImageInformation(computeClient, server, d); err != nil { return err } - d.Set("image_name", image.Name) // volume attachments if err := getVolumeAttachments(computeClient, d); err != nil { @@ -980,44 +985,72 @@ func resourceInstanceSchedulerHintsV2(d *schema.ResourceData, schedulerHintsRaw return schedulerHints } -func getImageID(client *gophercloud.ServiceClient, d *schema.ResourceData) (string, error) { - imageId := d.Get("image_id").(string) +func getImageIDFromConfig(computeClient *gophercloud.ServiceClient, d *schema.ResourceData) (string, error) { + // If block_device was used, an Image does not need to be specified. + // If an Image was specified, ignore it + if _, ok := d.GetOk("block_device"); ok { + return "", nil + } - if imageId != "" { + if imageId := d.Get("image_id").(string); imageId != "" { return imageId, nil + } else { + // try the OS_IMAGE_ID environment variable + if v := os.Getenv("OS_IMAGE_ID"); v != "" { + return v, nil + } } - imageCount := 0 imageName := d.Get("image_name").(string) + if imageName == "" { + // try the OS_IMAGE_NAME environment variable + if v := os.Getenv("OS_IMAGE_NAME"); v != "" { + imageName = v + } + } + if imageName != "" { - pager := images.ListDetail(client, &images.ListOpts{ - Name: imageName, - }) - pager.EachPage(func(page pagination.Page) (bool, error) { - imageList, err := images.ExtractImages(page) - if err != nil { - return false, err - } + imageId, err := images.IDFromName(computeClient, imageName) + if err != nil { + return "", err + } + return imageId, nil + } - for _, i := range imageList { - if i.Name == imageName { - imageCount++ - imageId = i.ID - } - } - return true, nil - }) + return "", fmt.Errorf("Neither a boot device, image ID, or image name were able to be determined.") +} - switch imageCount { - case 0: - return "", fmt.Errorf("Unable to find image: %s", imageName) - case 1: - return imageId, nil - default: - return "", fmt.Errorf("Found %d images matching %s", imageCount, imageName) +func setImageInformation(computeClient *gophercloud.ServiceClient, server *servers.Server, d *schema.ResourceData) error { + // If block_device was used, an Image does not need to be specified. + // If an Image was specified, ignore it + if _, ok := d.GetOk("block_device"); ok { + d.Set("image_id", "Attempt to boot from volume - no image supplied") + return nil + } + + imageId := server.Image["id"].(string) + if imageId != "" { + d.Set("image_id", imageId) + if image, err := images.Get(computeClient, imageId).Extract(); err != nil { + errCode, ok := err.(*gophercloud.UnexpectedResponseCodeError) + if !ok { + return err + } + if errCode.Actual == 404 { + // If the image name can't be found, set the value to "Image not found". + // The most likely scenario is that the image no longer exists in the Image Service + // but the instance still has a record from when it existed. + d.Set("image_name", "Image not found") + return nil + } else { + return err + } + } else { + d.Set("image_name", image.Name) } } - return "", fmt.Errorf("Neither an image ID nor an image name were able to be determined.") + + return nil } func getFlavorID(client *gophercloud.ServiceClient, d *schema.ResourceData) (string, error) { diff --git a/website/source/docs/providers/openstack/r/compute_instance_v2.html.markdown b/website/source/docs/providers/openstack/r/compute_instance_v2.html.markdown index 27ea7fe64f28..73e66364618b 100644 --- a/website/source/docs/providers/openstack/r/compute_instance_v2.html.markdown +++ b/website/source/docs/providers/openstack/r/compute_instance_v2.html.markdown @@ -35,11 +35,13 @@ The following arguments are supported: * `name` - (Required) A unique name for the resource. -* `image_id` - (Optional; Required if `image_name` is empty) The image ID of - the desired image for the server. Changing this creates a new server. +* `image_id` - (Optional; Required if `image_name` is empty and not booting + from a volume) The image ID of the desired image for the server. Changing + this creates a new server. -* `image_name` - (Optional; Required if `image_id` is empty) The name of the - desired image for the server. Changing this creates a new server. +* `image_name` - (Optional; Required if `image_id` is empty and not booting + from a volume) The name of the desired image for the server. Changing this + creates a new server. * `flavor_id` - (Optional; Required if `flavor_name` is empty) The flavor ID of the desired flavor for the server. Changing this resizes the existing server.