fpco / terraform-aws-foundation

Establish a solid Foundation on AWS with these modules for Terraform
MIT License
204 stars 98 forks source link

Asg additional block devices #249

Closed qrilka closed 4 years ago

qrilka commented 4 years ago

name: Pull request template about: Make a PR to terraform-aws-foundation

Please include the following in your PR:

Please also note that these are not hard requirements, but merely serve to define what maintainers are looking for in PR's. Including these will more likely lead to your PR being reviewed and accepted.

ketzacoatl commented 4 years ago

This change LGTM. Let's get it tested, merged, and released.

qrilka commented 4 years ago

I see additional block device available in out current project, do we need some additional testing here? I wanted to add a line into the changelog but I guess it makes sense to address #248 first

qrilka commented 4 years ago

@ketzacoatl we use this module for quite some time already and it seems to work fine. Are there any additional steps required to get this merged?

ketzacoatl commented 4 years ago

@qrilka apologies for overlooking the previous question. Can you paste/screenshot/etc a working example/etc that demonstrates the update is functional? That is what we're looking for in general, for all PRs (demonstrate the update works in common use cases, and doesn't break existing use by surprise).

ketzacoatl commented 4 years ago

Should I create a separate ASG in dev-sandbox?

Deploying an ASG to dev-sandbox is fine to do, sure.

Deploying the gitlab example ought to be a good test as well, that should be using all the pieces here and give you an easy way to confirm correctness/functioning. Using an ASG of your own is also ok if you prefer.

Will a plan be enough?

A plan is a good start, though that's not as ideal as a full test.

I'd even argue it's worth hooking up the gitlab or another couple examples to our Gitlab for CI and make the test easier to run a lot. We started down this path for foundation in the past (with some S3 tests), but we stopped short. I have also set this up for past projects (the legacy vpc example was one), and it worked sooooo great, I loved having that feedback. If we want to hookup one or more of the examples to Gitlab, we should scope that out in a new ticket.

qrilka commented 4 years ago

@ketzacoatl Gitlab examples seem to be not sufficient here as they don't use asg directly and also they don't use this new feature. So I went with a custom Terraform file - https://gist.github.com/qrilka/4e1f674ee28bd348e24fc3bd10318688

The resulting instances with (or without) extra EBS could be seen at https://imgur.com/a/4Q064jt

psibi commented 4 years ago

@qrilka / @ketzacoatl While we add volumes to it, this doesn't attach the volumes to the instances in the ASG. Also, it seems (for valid reasons) we can attach an ebs volume to only one instance at a time. Do you think we should handle this also ?

qrilka commented 4 years ago

@psibi EBS volume specified in a launch configuration creates a new one and attaches it automatically. See e.g. "Additional EBS block devices to attach to the instance. " for ebs_block_device at https://www.terraform.io/docs/providers/aws/r/launch_configuration.html With a launch configuration you attach a volume at launch time. I guess in your question with "attaching" you mean doing that at runtime (even during init)?

ketzacoatl commented 4 years ago

w/ Additional EBS Volumes:

image

No Additional Volumes:

image