Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/aws: Add resource_aws_volume_attachment #2050

Merged
merged 7 commits into from
May 28, 2015
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions builtin/providers/aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ func Provider() terraform.ResourceProvider {
"aws_security_group_rule": resourceAwsSecurityGroupRule(),
"aws_sqs_queue": resourceAwsSqsQueue(),
"aws_subnet": resourceAwsSubnet(),
"aws_volume_attachment": resourceAwsVolumeAttachment(),
"aws_vpc_dhcp_options_association": resourceAwsVpcDhcpOptionsAssociation(),
"aws_vpc_dhcp_options": resourceAwsVpcDhcpOptions(),
"aws_vpc_peering_connection": resourceAwsVpcPeeringConnection(),
Expand Down
36 changes: 36 additions & 0 deletions builtin/providers/aws/resource_aws_ebs_volume_test.go
Original file line number Diff line number Diff line change
@@ -1,23 +1,59 @@
package aws

import (
"fmt"
"testing"

"github.com/awslabs/aws-sdk-go/aws"
"github.com/awslabs/aws-sdk-go/service/ec2"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
)

func TestAccAWSEBSVolume(t *testing.T) {
var v ec2.Volume
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccAwsEbsVolumeConfig,
Check: resource.ComposeTestCheckFunc(
testAccCheckVolumeExists("aws_ebs_volume.test", &v),
),
},
},
})
}

func testAccCheckVolumeExists(n string, v *ec2.Volume) resource.TestCheckFunc {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upgraded this test while I was here, since I needed a testAccCheckVolumeExists method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📈 👍

return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
if !ok {
return fmt.Errorf("Not found: %s", n)
}

if rs.Primary.ID == "" {
return fmt.Errorf("No ID is set")
}

conn := testAccProvider.Meta().(*AWSClient).ec2conn

request := &ec2.DescribeVolumesInput{
VolumeIDs: []*string{aws.String(rs.Primary.ID)},
}

response, err := conn.DescribeVolumes(request)
if err == nil {
if response.Volumes != nil && len(response.Volumes) > 0 {
*v = *response.Volumes[0]
return nil
}
}
return fmt.Errorf("Error finding EC2 volume %s", rs.Primary.ID)
}
}

const testAccAwsEbsVolumeConfig = `
resource "aws_ebs_volume" "test" {
availability_zone = "us-west-2a"
Expand Down
158 changes: 158 additions & 0 deletions builtin/providers/aws/resource_aws_volume_attachment.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
package aws

import (
"bytes"
"fmt"
"time"

"github.com/awslabs/aws-sdk-go/aws"
"github.com/awslabs/aws-sdk-go/aws/awserr"
"github.com/awslabs/aws-sdk-go/service/ec2"
"github.com/hashicorp/terraform/helper/hashcode"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
)

func resourceAwsVolumeAttachment() *schema.Resource {
return &schema.Resource{
Create: resourceAwsVolumeAttachmentCreate,
Read: resourceAwsVolumeAttachmentRead,
Delete: resourceAwsVolumeAttachmentDelete,

Schema: map[string]*schema.Schema{
"device_name": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
},

"instance_id": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
},

"volume_id": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
},

"force_detach": &schema.Schema{
Type: schema.TypeBool,
Optional: true,
Computed: true,
},
},
}
}

func resourceAwsVolumeAttachmentCreate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ec2conn
name := d.Get("device_name").(string)
iID := d.Get("instance_id").(string)
vID := d.Get("volume_id").(string)

opts := &ec2.AttachVolumeInput{
Device: aws.String(name),
InstanceID: aws.String(iID),
VolumeID: aws.String(vID),
}
stateConf := &resource.StateChangeConf{
Pending: []string{"pending"},
Target: "attaching",
Refresh: attachVolumeFunc(conn, opts),
Timeout: 1 * time.Minute,
Delay: 5 * time.Second,
MinTimeout: 3 * time.Second,
}

_, err := stateConf.WaitForState()
if err != nil {
return fmt.Errorf("Error attaching volume %s to instance %s: %s", vID, iID, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now that we loop above, there's no eventual consistency here? No need to loop and wait for "attached" or anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added polling for attached9329073 , JUST FOR YOU @phinze


d.SetId(volumeAttachmentID(name, vID, iID))
return resourceAwsVolumeAttachmentRead(d, meta)
}

func attachVolumeFunc(conn *ec2.EC2, opts *ec2.AttachVolumeInput) resource.StateRefreshFunc {
return func() (interface{}, string, error) {
va, err := conn.AttachVolume(opts)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not used to WaitForState funcs actually performing the operation like this.

It seems weird that VolumeIsInUse is interpreted as "attaching" below - I'd expect the flow to be more like:

  • AttachVolume once
  • Handle any errors from that call.
  • Then do a StateRefreshFunc that loops DescribeVolumes and waits for it to look right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so, this is a bit weird and maybe you can help me clarify things.
When you first call AttachVolume, it's possible that the volume you reference isn't totally ready.

In TF's case, say that volume is another resource. It gets created, but it's create func doesn't wait for it to be "ready", just "exist".

So when the AttachVolume comes along, the volume is still creating (or something), so the call simply fails. Here we try to account for that and retry up to 1 minute.

Here in this block I retry until I get that it's attached or attaching.

It's kind of an edge case, maybe. Could I clear this up? Unfortunately I ran into this issue early on, and never hit the error case later in development :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In TF's case, say that volume is another resource. It gets created, but it's create func doesn't wait for it to be "ready", just "exist".

Seems like it should be the volume's responsibility to wait to return until the volume is ready, such that subsequent resources don't have to worry about it.

So then we can expect a single call to AttachVolumes to do its thing (or be able to error right away), then we just poll to wait for its thing to be done.

Let me know if that makes sense.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phinze

Currently aws_ebs_volume doesn’t wait if the volume is ready.

That makes sense that resource aws_volume_attachment doesn’t wait for the availability of the volume but that aws_ebs_volume guarantees it by its return.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if that makes sense.

👍

if err != nil {
awsErr, ok := err.(awserr.Error)
if ok && awsErr.Code() == "VolumeInUse" && *va.InstanceID == *opts.InstanceID {
return nil, "attaching", nil
}
return nil, "error", err
}
return va, *va.State, nil
}
}

func resourceAwsVolumeAttachmentRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ec2conn

request := &ec2.DescribeVolumesInput{
VolumeIDs: []*string{aws.String(d.Get("volume_id").(string))},
Filters: []*ec2.Filter{
&ec2.Filter{
Name: aws.String("attachment.instance-id"),
Values: []*string{aws.String(d.Get("instance_id").(string))},
},
},
}

_, err := conn.DescribeVolumes(request)
if err != nil {
if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidVolume.NotFound" {
d.SetId("")
return nil
}
return fmt.Errorf("Error reading EC2 volume %s for instance: %s: %#v", d.Get("volume_id").(string), d.Get("instance_id").(string), err)
}
return nil
}

func resourceAwsVolumeAttachmentDelete(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ec2conn

volume := d.Get("volume_id").(string)
instance := d.Get("instance_id").(string)

opts := &ec2.DetachVolumeInput{
Device: aws.String(d.Get("device_name").(string)),
InstanceID: aws.String(instance),
VolumeID: aws.String(volume),
Force: aws.Boolean(d.Get("force_detach").(bool)),
}

return resource.Retry(1*time.Minute, func() error {
resp, err := conn.DetachVolume(opts)
if err != nil {
awsErr, ok := err.(awserr.Error)
if ok && (awsErr.Code() == "IncorrectState" || awsErr.Code() == "InvalidVolume.NotFound") {
// volume attachment is not in a valid "attachment state"
return nil
}

return err
}

if resp.State != nil && *resp.State == "detaching" {
return fmt.Errorf("waiting for volume %s to detach from instance %s", volume, instance)
} else if *resp.State == "detached" {
return nil
}
return fmt.Errorf("Error detaching volume %s from instance %s", volume, instance)
})
}

func volumeAttachmentID(name, volumeID, instanceID string) string {
var buf bytes.Buffer
buf.WriteString(fmt.Sprintf("%s-", name))
buf.WriteString(fmt.Sprintf("%s-", instanceID))
buf.WriteString(fmt.Sprintf("%s-", volumeID))

return fmt.Sprintf("vai-%d", hashcode.String(buf.String()))
}
93 changes: 93 additions & 0 deletions builtin/providers/aws/resource_aws_volume_attachment_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package aws

import (
"fmt"
"log"
"testing"

"github.com/awslabs/aws-sdk-go/service/ec2"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
)

func TestAccAWSVolumeAttachment_basic(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{
resource.TestStep{
Config: testAccVolumeAttachmentConfig,
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]
if !ok {
return fmt.Errorf("Not found: %s", n)
}

if rs.Primary.ID == "" {
return fmt.Errorf("No ID is set")
}

for _, b := range i.BlockDeviceMappings {
if rs.Primary.Attributes["device_name"] == *b.DeviceName {
if b.EBS.VolumeID != nil && rs.Primary.Attributes["volume_id"] == *b.EBS.VolumeID {
// pass
return nil
}
}
}

return fmt.Errorf("Error finding instance/volume")
}
}

func testAccCheckVolumeAttachmentDestroy(s *terraform.State) error {
for _, rs := range s.RootModule().Resources {
log.Printf("\n\n----- This is never called")
if rs.Type != "aws_volume_attachment" {
continue
}
}
return nil
}

const testAccVolumeAttachmentConfig = `
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 = 5
}

resource "aws_volume_attachment" "ebs_att" {
device_name = "/dev/sdh"
volume_id = "${aws_ebs_volume.example.id}"
instance_id = "${aws_instance.web.id}"
}
`