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

feat: Add instance store volume option for instances with local disk #1213

Merged

Conversation

jwitko
Copy link
Contributor

@jwitko jwitko commented Feb 1, 2021

PR o'clock

Issue

Resolves #1212

Description

You can only supply EBS type volumes in worker launch templates. This PR aims to add the ability to also specify instance store volumes so that instances with local disk may be utilized efficiently.

The README did not need to be modified because it references local.tf where this change happens, but I did modify that section along with comments as seems to be the pattern. There are two values being added (Hard to see because of terraform fmt)

  1. additional_instance_store_volumes
  2. instance_store_virtual_name

Diff output without terraform fmt

diff --git a/local.tf b/local.tf
index 09d62f0..584ead4 100644
--- a/local.tf
+++ b/local.tf
@@ -40,6 +40,7 @@ locals {
     health_check_type             = null                        # Controls how health checking is done. Valid values are "EC2" or "ELB".
     health_check_grace_period     = null                        # Time in seconds after instance comes into service before checking health.
     instance_type                 = "m4.large"                  # Size of the workers instances.
+    instance_store_virtual_name   = "ephemeral0"                # "virtual_name" of the instance store volume.
     spot_price                    = ""                          # Cost of spot instance.
     placement_tenancy             = ""                          # The tenancy of the instance. Valid values are "default" or "dedicated".
     root_volume_size              = "100"                       # root volume size of workers instances.
@@ -71,6 +72,7 @@ locals {
     termination_policies          = []                          # A list of policies to decide how the instances in the auto scale group should be terminated.
     platform                      = "linux"                     # Platform of workers. either "linux" or "windows"
     additional_ebs_volumes        = []                          # A list of additional volumes to be attached to the instances on this Auto Scaling group. Each volume should be an object with the following: block_device_name (required), volume_size, volume_type, iops, encrypted, kms_key_id (only on launch-template), delete_on_termination. Optional values are grabbed from root volume or from defaults
+    additional_instance_store_volumes = []                      # A list of additional instance store (local disk) volumes to be attached to the instances on this Auto Scaling group. Each volume should be an object with the following: block_device_name (required), virtual_name.
     # Settings for launch templates
     root_block_device_name               = data.aws_ami.eks_worker.root_device_name # Root device name for workers. If non is provided, will assume default AMI was used.
     root_kms_key_id                      = ""                                       # The KMS key to use when encrypting the root storage device
diff --git a/workers_launch_template.tf b/workers_launch_template.tf
index f574fcc..52f40f2 100644
--- a/workers_launch_template.tf

# Worker Groups using Launch Templates

resource "aws_autoscaling_group" "workers_launch_template" {
  count = var.create_eks ? local.worker_group_launch_template_count : 0
  name_prefix = join(
    "-",
    compact(
      [
        coalescelist(aws_eks_cluster.this[*].name, [""])[0],
        lookup(var.worker_groups_launch_template[count.index], "name", count.index),
        lookup(var.worker_groups_launch_template[count.index], "asg_recreate_on_change", local.workers_group_defaults["asg_recreate_on_change"]) ? random_pet.workers_launch_template[count.index].id : ""
      ]
    )
  )
  desired_capacity = lookup(
    var.worker_groups_launch_template[count.index],
    "asg_desired_capacity",
    local.workers_group_defaults["asg_desired_capacity"],
  )
  max_size = lookup(
    var.worker_groups_launch_template[count.index],
    "asg_max_size",
    local.workers_group_defaults["asg_max_size"],
  )
  min_size = lookup(
    var.worker_groups_launch_template[count.index],
    "asg_min_size",
    local.workers_group_defaults["asg_min_size"],
  )
  force_delete = lookup(
    var.worker_groups_launch_template[count.index],
    "asg_force_delete",
    local.workers_group_defaults["asg_force_delete"],
  )
  target_group_arns = lookup(
    var.worker_groups_launch_template[count.index],
    "target_group_arns",
    local.workers_group_defaults["target_group_arns"]
  )
  load_balancers = lookup(
    var.worker_groups_launch_template[count.index],
:460
+++ b/workers_launch_template.tf
@@ -453,6 +453,21 @@ resource "aws_launch_template" "workers_launch_template" {

   }

+  dynamic "block_device_mappings" {
+    for_each = lookup(var.worker_groups_launch_template[count.index], "additional_instance_store_volumes", local.workers_group_defaults["additional_instance_store_volumes"])
+    content {
+      device_name = block_device_mappings.value.block_device_name
+       {
+         virtual_name = lookup(
+           block_device_mappings.value,
+           "virtual_name",
+           local.workers_group_defaults["instance_store_virtual_name"],
+         )
+       }
+    }
+
+  }
+
   tag_specifications {
     resource_type = "volume"

Checklist

@jwitko jwitko changed the title Add instance store volume option to EKS module feat: Add instance store volume option to EKS module Feb 1, 2021
@jwitko jwitko changed the title feat: Add instance store volume option to EKS module feat: Add instance store volume option for instances with local disk Feb 1, 2021
@rossigee
Copy link

This patch doesn't seem to have any effect for me. It still insists on creating an EBS volume using the default values. I think because there is a non-dynamic block_device_mappings block above the dynamic one you've added in your patch.

block_device_mappings {

@jwitko
Copy link
Contributor Author

jwitko commented Mar 17, 2021

This patch doesn't seem to have any effect for me. It still insists on creating an EBS volume using the default values. I think because there is a non-dynamic block_device_mappings block above the dynamic one you've added in your patch.

block_device_mappings {

@rossigee The additional non-dynamic block_device_mappings should not be an issue. This existed before my patch and a non-dynamic plus a dynamic existed before my patch too.

To make sure I fully understand the issue, are you saying that this patch does not work because it does not change the existing behavior of creating the EKS worker node root volumes as EBS volumes? If that is the case, this patch is not intended to modify that behavior. As you correctly noted, that non-dynamic block does indeed limit this module to creating EKS clusters which use EBS volumes as their root disk.

In the case that this is just a configuration issue, here is an example of my worker-group config relevant section:

    - name: "spot-colossal"
      override_instance_types: ["x1.16xlarge"]
      spot_instance_pools: 3
      asg_max_size: 2
      asg_desired_capacity: 0
      asg_min_size: 0
      kubelet_extra_args: "--root-dir /mnt/ephemeral/kubernetes --node-labels=${join(",", ["spot=true","node.kubernetes.io/lifecycle=spot","lifecycle=Ec2Spot","example.cloud/namespace=example-staging","example.cloud/node=owned"])} --register-with-taints=spotInstance=true:NoSchedule"
      additional_instance_store_volumes:
        - block_device_name: "/dev/sdb"

@funes79
Copy link

funes79 commented Apr 8, 2021

I would like to see the possibility to change the bootstrap (similar to eksctl preBootstrapCommands) and mount and format instance storage when the pod is provisioned, so thus I can use it in Spark on Kubernetes:
https://aws.amazon.com/blogs/containers/optimizing-spark-performance-on-kubernetes/

      - IDX=1
      - for DEV in /dev/disk/by-id/nvme-Amazon_EC2_NVMe_Instance_Storage_*-ns-1; do  mkfs.xfs ${DEV};mkdir -p /local${IDX};echo ${DEV} /local${IDX} xfs defaults,noatime 1 2 >> /etc/fstab; IDX=$((${IDX} + 1)); done
      - mount -a

Or is there any other workaround how to provide these instance volumes to pods?

@rossigee
Copy link

@rossigee The additional non-dynamic block_device_mappings should not be an issue. This existed before my patch and a non-dynamic plus a dynamic existed before my patch too.

@jwitko - my bad, i was completely misunderstanding how this works. Thanks for the explanation.

@barryib barryib self-assigned this May 21, 2021
@barryib
Copy link
Member

barryib commented May 21, 2021

@jwitko Thank you for opening this PR. Can you please rebase your branch from master ?

@jwitko
Copy link
Contributor Author

jwitko commented May 21, 2021

@jwitko Thank you for opening this PR. Can you please rebase your branch from master ?

No problem! Should be done now.

@barryib barryib merged commit 6aac9c4 into terraform-aws-modules:master May 27, 2021
@barryib
Copy link
Member

barryib commented May 27, 2021

Thanks @jwitko for your contribution.

spot_price = "" # Cost of spot instance.
placement_tenancy = "" # The tenancy of the instance. Valid values are "default" or "dedicated".
root_volume_size = "100" # root volume size of workers instances.
root_volume_type = "gp3" # root volume type of workers instances, can be "standard", "gp3", "gp2", or "io1"

Choose a reason for hiding this comment

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

was this change intended?

Copy link
Member

@barryib barryib May 28, 2021

Choose a reason for hiding this comment

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

Nope #1404

Good catch.

Copy link
Member

Choose a reason for hiding this comment

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

pushing a patch release.

Copy link
Member

Choose a reason for hiding this comment

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

Just released v17.0.1.

Thank you very much @james-callahan

Copy link
Contributor Author

@jwitko jwitko May 28, 2021

Choose a reason for hiding this comment

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

ahhh! So sorry about that @barryib @james-callahan
That was intended for my local fork only, apologies for missing reverting it.

@josecsotomorales
Copy link

I would like to see the possibility to change the bootstrap (similar to eksctl preBootstrapCommands) and mount and format instance storage when the pod is provisioned, so thus I can use it in Spark on Kubernetes: https://aws.amazon.com/blogs/containers/optimizing-spark-performance-on-kubernetes/

      - IDX=1
      - for DEV in /dev/disk/by-id/nvme-Amazon_EC2_NVMe_Instance_Storage_*-ns-1; do  mkfs.xfs ${DEV};mkdir -p /local${IDX};echo ${DEV} /local${IDX} xfs defaults,noatime 1 2 >> /etc/fstab; IDX=$((${IDX} + 1)); done
      - mount -a

Or is there any other workaround how to provide these instance volumes to pods?

I'm trying to configure something similar, is there a way to do that with this module?

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Launch templates do not support non-ebs volume types
7 participants