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

Allow custom AMI ID for EC2 workers #49

Merged
merged 5 commits into from
May 24, 2023
Merged

Conversation

zachmullen
Copy link
Member

Fixes #48

variables.tf Outdated

variable "ec2_worker_ami_id" {
type = string
default = ""
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to look into whether this should be null or just not defined here. The latest versions of TF have made some improvements to default values.

Copy link
Member

@brianhelba brianhelba left a comment

Choose a reason for hiding this comment

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

Seems reasonable in concept. I was a bit concerned that it was the edge of a slippery slope of wrapping more and more choices, but considering that GPU-enabled machines are one of the most common reasons for having an EC2 worker, I think it's foreseeable that a different AMI will be required to support the special hardware configuration.

I'd like to explore a few minor tweaks to improve validation, then I plan on merging this.

I plan to make the suggested changes myself; those are just notes to remind me.

Copy link
Member

@brianhelba brianhelba left a comment

Choose a reason for hiding this comment

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

The presence of

  lifecycle {
    ignore_changes = [
      # Allow an updated AMI be used for new provisions without always recreating the machine
      ami,
    ]
  }

is a significant barrier to making ami_id a useful variable. Once the module is created, incrementally setting or changing ami_id will not trigger the appropriate updates to the underlying resources. Worse, it'll do this silently, creating a very non-obvious source of bugs.

The underlying problem could be fixed once hashicorp/terraform#24188 is resolved, so we could only ignore_changes to ami if var.ami_id is not set.

If we want to try to release this now, I'd like to have some sort of workaround to address this bug.

@brianhelba
Copy link
Member

Maybe it's possible to have a postcondition to see if there's a mismatch in the actual AMI of the aws_instance.ec2_worker and the requested AMI. However, I'm not sure if the available resource attribute values can provide this information.

Copy link
Member

@brianhelba brianhelba left a comment

Choose a reason for hiding this comment

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

I don't think there's a technical way to solve my usability concerns. Even with the replace_triggered_by lifecycle option, I don't think it's possible to always reasonably predict a user's intent.

I've updated the variable name and docs to clarify the caveats of how AMIs interact with existing EC2 worker instances.

@brianhelba brianhelba merged commit 5d900b5 into master May 24, 2023
1 check passed
@brianhelba brianhelba deleted the custom-ec2-worker-ami branch May 24, 2023 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EC2 worker AMI should be a variable
2 participants