From 6649b938da2efedf3d52acd9d46fcb681c73027d Mon Sep 17 00:00:00 2001 From: Paul Stack Date: Wed, 2 Nov 2016 15:29:37 +0000 Subject: [PATCH] provider/aws: Provide the option to skip_destroy on aws_volume_attachment (#9792) * provider/aws: Provide the option to skip_destroy on aws_volume_attachment When you want to attach and detach pre-existing EBS volumes to an instance, we would do that as follows: ``` resource "aws_instance" "web" { ami = "ami-21f78e11" availability_zone = "us-west-2a" instance_type = "t1.micro" tags { Name = "HelloWorld" } } data "aws_ebs_volume" "ebs_volume" { filter { name = "size" values = ["${aws_ebs_volume.example.size}"] } filter { name = "availability-zone" values = ["${aws_ebs_volume.example.availability_zone}"] } filter { name = "tag:Name" values = ["TestVolume"] } } resource "aws_volume_attachment" "ebs_att" { device_name = "/dev/sdh" volume_id = "${data.aws_ebs_volume.ebs_volume.id}" instance_id = "${aws_instance.web.id}" skip_destroy = true } ``` The issue here is that when we run a terraform destroy command, the volume tries to get detached from a running instance and goes into a non-responsive state. We would have to force_destroy the volume at that point and risk losing any data on it. This PR introduces the idea of `skip_destroy` on a volume attachment. tl;dr: We want the volume to be detached from the instane when the instance itself has been destroyed. This way the normal shut procedures will happen and protect the disk for attachment to another instance Volume Attachment Tests: ``` % make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSVolumeAttachment_' ==> Checking that code complies with gofmt requirements... go generate $(go list ./... | grep -v /terraform/vendor/) 2016/11/02 00:47:27 Generated command/internal_plugin_list.go TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSVolumeAttachment_ -timeout 120m === RUN TestAccAWSVolumeAttachment_basic --- PASS: TestAccAWSVolumeAttachment_basic (133.49s) === RUN TestAccAWSVolumeAttachment_skipDestroy --- PASS: TestAccAWSVolumeAttachment_skipDestroy (119.64s) PASS ok github.com/hashicorp/terraform/builtin/providers/aws 253.158s ``` EBS Volume Tests: ``` % make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEBSVolume_' ==> Checking that code complies with gofmt requirements... go generate $(go list ./... | grep -v /terraform/vendor/) 2016/11/02 01:00:18 Generated command/internal_plugin_list.go TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSEBSVolume_ -timeout 120m === RUN TestAccAWSEBSVolume_importBasic --- PASS: TestAccAWSEBSVolume_importBasic (26.38s) === RUN TestAccAWSEBSVolume_basic --- PASS: TestAccAWSEBSVolume_basic (26.86s) === RUN TestAccAWSEBSVolume_NoIops --- PASS: TestAccAWSEBSVolume_NoIops (27.89s) === RUN TestAccAWSEBSVolume_withTags --- PASS: TestAccAWSEBSVolume_withTags (26.88s) PASS ok github.com/hashicorp/terraform/builtin/providers/aws 108.032s ``` * Update volume_attachment.html.markdown --- .../providers/aws/resource_aws_ebs_volume.go | 28 +++++--- .../aws/resource_aws_volume_attachment.go | 19 +++-- .../resource_aws_volume_attachment_test.go | 69 ++++++++++++++++++- .../aws/r/volume_attachment.html.markdown | 5 ++ 4 files changed, 108 insertions(+), 13 deletions(-) diff --git a/builtin/providers/aws/resource_aws_ebs_volume.go b/builtin/providers/aws/resource_aws_ebs_volume.go index 550a9c696b..691cd85d12 100644 --- a/builtin/providers/aws/resource_aws_ebs_volume.go +++ b/builtin/providers/aws/resource_aws_ebs_volume.go @@ -203,15 +203,27 @@ func resourceAwsEbsVolumeRead(d *schema.ResourceData, meta interface{}) error { func resourceAwsEbsVolumeDelete(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).ec2conn - request := &ec2.DeleteVolumeInput{ - VolumeId: aws.String(d.Id()), - } + return resource.Retry(5*time.Minute, func() *resource.RetryError { + request := &ec2.DeleteVolumeInput{ + VolumeId: aws.String(d.Id()), + } + _, err := conn.DeleteVolume(request) + if err == nil { + return nil + } + + ebsErr, ok := err.(awserr.Error) + if ebsErr.Code() == "VolumeInUse" { + return resource.RetryableError(fmt.Errorf("EBS VolumeInUse - trying again while it detaches")) + } + + if !ok { + return resource.NonRetryableError(err) + } + + return resource.NonRetryableError(err) + }) - _, err := conn.DeleteVolume(request) - if err != nil { - return fmt.Errorf("Error deleting EC2 volume %s: %s", d.Id(), err) - } - return nil } func readVolume(d *schema.ResourceData, volume *ec2.Volume) error { diff --git a/builtin/providers/aws/resource_aws_volume_attachment.go b/builtin/providers/aws/resource_aws_volume_attachment.go index c586e0d2fe..bde1a7c1f5 100644 --- a/builtin/providers/aws/resource_aws_volume_attachment.go +++ b/builtin/providers/aws/resource_aws_volume_attachment.go @@ -21,25 +21,30 @@ func resourceAwsVolumeAttachment() *schema.Resource { Delete: resourceAwsVolumeAttachmentDelete, Schema: map[string]*schema.Schema{ - "device_name": &schema.Schema{ + "device_name": { Type: schema.TypeString, Required: true, ForceNew: true, }, - "instance_id": &schema.Schema{ + "instance_id": { Type: schema.TypeString, Required: true, ForceNew: true, }, - "volume_id": &schema.Schema{ + "volume_id": { Type: schema.TypeString, Required: true, ForceNew: true, }, - "force_detach": &schema.Schema{ + "force_detach": { + Type: schema.TypeBool, + Optional: true, + Computed: true, + }, + "skip_destroy": { Type: schema.TypeBool, Optional: true, Computed: true, @@ -156,6 +161,12 @@ func resourceAwsVolumeAttachmentRead(d *schema.ResourceData, meta interface{}) e func resourceAwsVolumeAttachmentDelete(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).ec2conn + if _, ok := d.GetOk("skip_destroy"); ok { + log.Printf("[INFO] Found skip_destroy to be true, removing attachment %q from state", d.Id()) + d.SetId("") + return nil + } + vID := d.Get("volume_id").(string) iID := d.Get("instance_id").(string) diff --git a/builtin/providers/aws/resource_aws_volume_attachment_test.go b/builtin/providers/aws/resource_aws_volume_attachment_test.go index 72dbeebb83..0b99d1ffad 100644 --- a/builtin/providers/aws/resource_aws_volume_attachment_test.go +++ b/builtin/providers/aws/resource_aws_volume_attachment_test.go @@ -19,7 +19,7 @@ func TestAccAWSVolumeAttachment_basic(t *testing.T) { Providers: testAccProviders, CheckDestroy: testAccCheckVolumeAttachmentDestroy, Steps: []resource.TestStep{ - resource.TestStep{ + { Config: testAccVolumeAttachmentConfig, Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr( @@ -36,6 +36,32 @@ func TestAccAWSVolumeAttachment_basic(t *testing.T) { }) } +func TestAccAWSVolumeAttachment_skipDestroy(t *testing.T) { + var i ec2.Instance + var v ec2.Volume + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckVolumeAttachmentDestroy, + Steps: []resource.TestStep{ + { + Config: testAccVolumeAttachmentConfigSkipDestroy, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "aws_volume_attachment.ebs_att", "device_name", "/dev/sdh"), + testAccCheckInstanceExists( + "aws_instance.web", &i), + testAccCheckVolumeExists( + "aws_ebs_volume.example", &v), + testAccCheckVolumeAttachmentExists( + "aws_volume_attachment.ebs_att", &i, &v), + ), + }, + }, + }) +} + func testAccCheckVolumeAttachmentExists(n string, i *ec2.Instance, v *ec2.Volume) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -91,3 +117,44 @@ resource "aws_volume_attachment" "ebs_att" { instance_id = "${aws_instance.web.id}" } ` + +const testAccVolumeAttachmentConfigSkipDestroy = ` +resource "aws_instance" "web" { + ami = "ami-21f78e11" + availability_zone = "us-west-2a" + instance_type = "t1.micro" + tags { + Name = "HelloWorld" + } +} + +resource "aws_ebs_volume" "example" { + availability_zone = "us-west-2a" + size = 1 + tags { + Name = "TestVolume" + } +} + +data "aws_ebs_volume" "ebs_volume" { + filter { + name = "size" + values = ["${aws_ebs_volume.example.size}"] + } + filter { + name = "availability-zone" + values = ["${aws_ebs_volume.example.availability_zone}"] + } + filter { + name = "tag:Name" + values = ["TestVolume"] + } +} + +resource "aws_volume_attachment" "ebs_att" { + device_name = "/dev/sdh" + volume_id = "${data.aws_ebs_volume.ebs_volume.id}" + instance_id = "${aws_instance.web.id}" + skip_destroy = true +} +` diff --git a/website/source/docs/providers/aws/r/volume_attachment.html.markdown b/website/source/docs/providers/aws/r/volume_attachment.html.markdown index a9a71f5b1b..406cbf1814 100644 --- a/website/source/docs/providers/aws/r/volume_attachment.html.markdown +++ b/website/source/docs/providers/aws/r/volume_attachment.html.markdown @@ -49,6 +49,11 @@ example, `/dev/sdh` or `xvdh`) volume to detach. Useful if previous attempts failed, but use this option only as a last resort, as this can result in **data loss**. See [Detaching an Amazon EBS Volume from an Instance][1] for more information. +* `skip_destroy` - (Optional, Boolean) Set this to true if you do not wish +to detach the volume from the instance to which it is attached at destroy +time, and instead just remove the attachment from Terraform state. This is +useful when destroying an instance which has volumes created by some other +means attached. ## Attributes Reference