module: Add EBS extra volume support #1
Conversation
modules/aws/ebs/variables.tf
Outdated
| description = "Number of instances" | ||
| } | ||
|
|
||
| variable "num" { |
There was a problem hiding this comment.
I see you have sizes and types variable, it seems like you may be able to determine the num based on these two variables. We just need to confirm that you will not run into this hashicorp/terraform#17421
There was a problem hiding this comment.
@bernadinm i hit exactly this limitation. Will add a comment about this limitation.
modules/aws/ebs/main.tf
Outdated
| * num = "3" | ||
| * sizes = ["500", "100", "1000"] | ||
| * types = ["", "", "gp2"] | ||
| * iopses = ["0", "300", "3000"] |
There was a problem hiding this comment.
My first impression is that three independent arrays are not good idea. I would prefer a list of maps. Any reason why you have chosen such a design?
There was a problem hiding this comment.
@fatz thanks for the review! Can we do that as a input variable?
There was a problem hiding this comment.
yes. an example are additional_listeners here https://github.com/dcos-terraform/terraform-aws-lb
However this also requires handling unset keys and defaults. But in general this is possible
There was a problem hiding this comment.
ah! very nice! i like that! I'll see what I can do.
modules/aws/ebs/main.tf
Outdated
| * module "instance-volume" { | ||
| * source = "dcos-terraform/ebs/aws" | ||
| * | ||
| * instances = ["id-ablsldhfa123", "id-aidrkdkek131"] |
There was a problem hiding this comment.
in generally do not like the idea of inter module resource changes. Volume management should be part of the instance module not an external one.
All modules are currently design to keep this approach and don't do intermodule changes. We should prefer buiding a similar solution in the instance module. I would only agree on such a solution if there is no useful way doing it with the instance module
There was a problem hiding this comment.
@fatz are you suggesting moving the code into terraform-aws-instance?
There was a problem hiding this comment.
if this should be a feature of the universal installer yes.
|
But as long as this is not part of the universal installer it could IMO be used beside the universal installer as this is only about attaching volumes. So you could take the output of the infrastructure module to attach these volumes without being implemented as a universal installer feature |
|
@fatz thanks for the review. I was also debating if this should be part of universal installer or not. I think the goal of UI is to make it easy for users to setup DC/OS clusters as well as the relevant infrastructure (e.g., DNS, load balancer, security groups, etc.). Given that, I am still leaning toward making this part of UI. This feature is optional anyway. |
jieyu
force-pushed
the
jie/ebs
branch
6 times, most recently
from
4944c59 to
51e6237
Compare
Allow extra EBS volumes to be created for each AWS instance.
|
@fatz all PRs referenced in this PR has been updated. PTAL again! dcos-terraform/terraform-aws-instance#6 |
Allow extra EBS volumes to be created for each AWS instance.
Created a new module under
modules/aws/ebs/. This will becomedcos-terraform/terraform-aws-ebseventually, and linked to this repo as a submodule similar to others.dcos-terraform/terraform-aws-instance#6
dcos-terraform/terraform-aws-infrastructure#20
dcos-terraform/terraform-aws-private-agents#8
dcos-terraform/terraform-aws-dcos#45