fix(spot): Prevent both instance_requirements and instance_type

remiflament commented 1 week ago

Description

I'm using this module to create ASG and resources associated to be used in my ECS cluster.

Motivation and Context

I searched to use for_each on this module to create on-demand and spot ASG/LaunchTemplate. I faced a problem because you can't declare instance_requirements if instance_type is set too.

To avoid this problem, I added a check that var.instance_type == null when you declare your instance_requirements.

Personal code example

if my asg is spot == false I keep the default instance_requirements as {} and set instance_type

if my asg isspot == true I set the instance_requirements, use_mixed_instances_policy and set the instance_type to null

locals.tf

auto_scaling_groups = {
        "on-demand" = {
          min_size     = 0
          max_size     = 5
          desired_size = 1 # only use at first deploy then manages by ECS CAS (Cluster Auto Scaling)
          spot         = false
          ec2_type     = "t3a.medium" # x64
        }
        "spot-burstable" = {
          min_size     = 0
          max_size     = 5
          desired_size = 1
          spot         = true
          spot_configuration = {
            allowed_instance_types = ["*"] # Must contains one or more wildcards (examples: m5.8xlarge, c5*.*, m5a.*, r*, *3*)
            memory_gb_min          = 4
            memory_gb_max          = 4
            vcpu_min               = 2
            vcpu_max               = 2
            burstable_performance  = "required"       # Valid Values: included | required | excluded
            cpu_manufacturers      = ["intel", "amd"] # Valid Values: intel | amd | amazon-web-services
          }
        }
      }

asg.tf

module "asg" {
  source = "./modules/terraform-aws-autoscaling/"

  for_each = local.env_vars.auto_scaling_groups

  # ... for brevity

  instance_type   = each.value.spot == false ? each.value.ec2_type : null

  # --------------------------------------------------
  # Spot configuration
  # --------------------
  instance_requirements = { 
    allowed_instance_types = each.value.spot ? each.value.spot_configuration.allowed_instance_types : null
    burstable_performance  = each.value.spot ? each.value.spot_configuration.burstable_performance : null # Valid Values: included | required | excluded
    cpu_manufacturers      = each.value.spot ? each.value.spot_configuration.cpu_manufacturers : null     # Valid Values: intel | amd | amazon-web-services
    memory_mib = {
      max = each.value.spot ? each.value.spot_configuration.memory_gb_max * 1024 : null
      min = each.value.spot ? each.value.spot_configuration.memory_gb_min * 1024 : null
    }
    vcpu_count = {
      max = each.value.spot ? each.value.spot_configuration.vcpu_max : null
      min = each.value.spot ? each.value.spot_configuration.vcpu_min : null
    }
  }
  use_mixed_instances_policy = each.value.spot ? true : false
  mixed_instances_policy = {
    instances_distribution = {
      spot_allocation_strategy = "lowest-price"
      spot_max_price           = "" # on-demand price
      spot_instance_pools      = 2
    }
  }
  capacity_rebalance = each.value.spot ? true : false
  # --------------------------------------------------

  # ... for brevity
}

Breaking Changes

No breaking changes.

How Has This Been Tested?

[ ] I have updated at least one of the examples/* to demonstrate and validate my change(s)
[ ] I have tested and validated these changes using one or more of the provided examples/* projects
[x] I have executed pre-commit run -a on my pull request
[x] I tested it locally in my stack

bryantbiggs commented 1 week ago

why do you need to use this in a for_each loop? seems like the problem is solved by having two ASG definitions, and will be much more stable

remiflament commented 1 week ago

@bryantbiggs yes it could be done with both ASG module declarations (150 lines for me right now for one ASG). In this example, I have two ASG, but you can go for example to 5 in your ECS cluster (small, medium, spot, on-demand instances)

The difference is not a huge change from the module perspective. And it fix an AWS limitation that when you declare instance_requirements you should not declare instance_type.

I hope you will find it helpful as it unlocks a new possibility.

bryantbiggs commented 1 week ago

but you can go for example to 5 in your ECS cluster (small, medium, spot, on-demand instances)

For what?

The difference is not a huge change from the module perspective. And it fix an AWS limitation that when you declare instance_requirements you should not declare instance_type.

There isn't anything broken so there isn't anything to fix in terms of limitations - its just that you are trying to force two very different configurations into one

remiflament commented 1 week ago

For what?

To place your ECS services depending on the application requirements in small, medium or large node. And if you are allowed to use spot, you can either run your workload 100% on spot or balance it between on-demand and spot. All these considerations are base on finOps. So it's a common thing to have multiple ASG in an ECS cluster.

its just that you are trying to force two very different configurations into one

Maybe, but I prefer using for_each loop when it's possible. You seem preferring duplicate a hundred lines of HCL.

Your comments are not based on the contribution, as it's just a small thing unlocking a huge possibility. It's more based on your HCL writing vision.

Any third opinion would be helpful @antonbabenko

terraform-aws-modules / terraform-aws-autoscaling