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

Conversation

catsby
Copy link
Contributor

@catsby catsby commented May 22, 2015

Allows you to attach an existing EBS volume to an existing Instance.

Usage:

provider "aws" {
  region="us-west-2"
}

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" # must be same as Instance
  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}"
}

Remaining:

  • docs
  • acceptance test(s)

},
},
})
}

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.

📈 👍

@ghost
Copy link

ghost commented May 26, 2015

@phinze @catsby

Great!

When do you think to merge this PR? ;) I can't wait for testing it!

@catsby
Copy link
Contributor Author

catsby commented May 26, 2015

I plan to review the feedback provided, and maybe make an adjustment or two. I suspect we can merge this afternoon or tomorrow 😄

@ghost
Copy link

ghost commented May 26, 2015

That’s wonderful !

great !

-- 
Julien Lavergne
Sent with Airmail

On May 26, 2015 at 4:37:17 PM, Clint ([email protected]) wrote:

I plan to review the feedback provided, and maybe make an adjustment or two. I suspect we can merge this afternoon or tomorrow


Reply to this email directly or view it on GitHub.

@ghost
Copy link

ghost commented May 26, 2015

Create ebs volume lifecycle is just like any other aws resources. TF just needs to wait for availability of any resources (e.g. aws eip ressource needs an aws instance).

Exist func must mean the ressource has been well created.

@catsby
Copy link
Contributor Author

catsby commented May 27, 2015

@phinze refactored – take another look please

ALSO – I think the acc. test leaks a volume, but I haven't had time to crack down on that.

vID, iID, awsErr.Message(), awsErr.Code())
}
return 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

@phinze
Copy link
Contributor

phinze commented May 27, 2015

Clean clean clean! 😻

One question, but if the answer is "nope" this LGTM.

@ghost
Copy link

ghost commented May 28, 2015

no need to wait for attachment; ebs volume needs just to be created above in the platform creation process.

…chment

* upstream/master: (21 commits)
  fix typo
  fix typo, use awslabs/aws-sdk-go
  Update CHANGELOG.md
  More internal links in template documentation.
  providers/aws: Requires ttl and records attributes if there isn't an ALIAS block.
  Condense switch fallthroughs into expr lists
  Fix docs for aws_route53_record params
  Update CHANGELOG.md
  provider/aws: Add IAM Server Certificate resource
  aws_db_instance docs updated per #2070
  providers/aws: Adds link to AWS docs about RDS parameters.
  Downgrade middleman to 3.3.12 as 3.3.13 does not exist
  providers/aws: Clarifies db_security_group usage.
  "More more" no more!
  Indentation issue
  Export ARN in SQS queue and SNS topic / subscription; updated tests for new AWS SDK errors; updated documentation.
  Changed Required: false to Optional: true in the SNS topic schema
  Initial SNS support
  correct resource name in example
  added attributes reference section for AWS_EBS_VOLUME
  ...
@catsby
Copy link
Contributor Author

catsby commented May 28, 2015

1f5c038: now with documentation!

@catsby
Copy link
Contributor Author

catsby commented May 28, 2015

ALSO – I think the acc. test leaks a volume, but I haven't had time to crack down on that.

I fixed the leak!

yeah

@mzupan
Copy link
Contributor

mzupan commented May 28, 2015

just used this and it works great

@phinze
Copy link
Contributor

phinze commented May 28, 2015

👍 x 💯 LGTM!

@ghost
Copy link

ghost commented May 28, 2015

Thx !

Julien

Le 28 mai 2015 à 22:46, Paul Hinze [email protected] a écrit :

x LGTM!


Reply to this email directly or view it on GitHub.

@catsby
Copy link
Contributor Author

catsby commented May 28, 2015

Thanks for the validation @mzupan ! Merging...

catsby added a commit that referenced this pull request May 28, 2015
provider/aws: Add resource_aws_volume_attachment
@catsby catsby merged commit 440537a into master May 28, 2015
@catsby catsby deleted the f-aws-volume-attachment branch May 28, 2015 21:24
@Pryz
Copy link
Contributor

Pryz commented Jul 22, 2015

It seems that the volume is attached after the provisioning of the instance. Is it expected ? If yes, how can we managed the bootstrap of the volume ?

@ghost
Copy link

ghost commented Jul 23, 2015

It's right and the expected behavior during the instance boot.

Envoyé de mon iPhone

Le 23 juil. 2015 à 00:57, Julien Fabre [email protected] a écrit :

It seems that the volume is attached after the provisioning of the instance. Is it expected ? If yes, how can we managed the bootstrap of the volume ?


Reply to this email directly or view it on GitHub.

@Pryz
Copy link
Contributor

Pryz commented Jul 24, 2015

Thanks. Do you have an example on how to manage the bootstrap of the volume ? (mkfs, etc etc)

@phinze
Copy link
Contributor

phinze commented Jul 27, 2015

@Pryz one way would be to add a remote-exec provisioner to the volume attachment itself and give it connection details for the instance, something like this:

resource "aws_volume_attachment" "ebs_att" {
  device_name = "/dev/sdh"
  volume_id = "${aws_ebs_volume.example.id}"
  instance_id = "${aws_instance.web.id}"
  // this will run after the attachment is made
  provisioner "remote-exec" {
    connection {
      host = "${aws_instance.web.public_ip}"
      // ...
    }
    inline = [ "mkfs -t ext4 /dev/sdh" /* ... */ ]
  }
}

@ghost
Copy link

ghost commented Jul 27, 2015

Cloudinit does the thing very well

-- 
Julien Lavergne
Sent with Airmail

On July 27, 2015 at 4:39:32 PM, Paul Hinze ([email protected]) wrote:

@Pryz one way would be to add a remote-exec provisioner to the volume attachment itself and give it connection details for the instance, something like this:

resource "aws_volume_attachment" "ebs_att" {
device_name = "/dev/sdh"
volume_id = "${aws_ebs_volume.example.id}"
instance_id = "${aws_instance.web.id}"
// this will run after the attachment is made
provisioner "remote-exec" {
connection {
host = "${aws_instance.web.public_ip}"
// ...
}
inline = [ "mkfs -t ext4 /dev/sdh" /* ... */ ]
}
}

Reply to this email directly or view it on GitHub.

@Pryz
Copy link
Contributor

Pryz commented Sep 4, 2015

The proposition of @phinze is working. I really think that if we can do that directly with a "ebs_block_device" resource things would be more readable and easier to maintain.

Also the @phinze's proposition involve code duplication (about the connection).

@julienlavergne The issue I have with cloudinit is that I have a lot of different instances to maintain. Most of the time the only differences (from Terraform) are about EBS. So having a "generic" cloud-init/user-data and manage the EBS with ebs_block_device would be easier :)

@ghost
Copy link

ghost commented Sep 4, 2015

I understand this feature.

-- 
Julien Lavergne
+33670273883
http://julienlavergne.com
Sent with Airmail

On September 4, 2015 at 12:05:26 PM, Julien Fabre ([email protected]) wrote:

The proposition of @phinze is working. I really think that if we can do that directly with a "ebs_block_device" resource things would be more readable and easier to maintain.

Also the @phinze's proposition involve code duplication (about the connection).

@julienlavergne The issue I have with cloudinit is that I have a lot of different instances to maintain. Most of the time the only differences (from Terraform) are about EBS. So having a "generic" cloud-init/user-data and manage the EBS with ebs_block_device would be easier :)


Reply to this email directly or view it on GitHub.

@ghost
Copy link

ghost commented Apr 15, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants