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

chore: Replace local EMR on EKS module with virtual cluster sub-module from terraform-aws-emr #249

Merged
merged 2 commits into from
Jul 15, 2023

Conversation

bryantbiggs
Copy link
Contributor

What does this PR do?

  • Replace local EMR on EKS module with virtual cluster sub-module from terraform-aws-emr
  • Update EMR patterns to use EKS Blueprints addons module; align on Terraform required versions, EBS CSI IRSA
  • Update variables to follow convention of description, type, default ordering

Motivation

  • Removes used of local EMR on EKS sub-module and use publicly available EMR Virtual Cluster module
  • Standardization

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Mandatory for new blueprints. Yes, I have added a example to support my blueprint PR
  • Mandatory for new blueprints. Yes, I have updated the website/docs or website/blog section for this feature
  • Yes, I ran pre-commit run -a with this PR. Link for installing pre-commit locally

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

  • I deployed the emr-eks-ack pattern to check and validate changes

@bryantbiggs bryantbiggs temporarily deployed to DoEKS Test July 14, 2023 15:32 — with GitHub Actions Inactive
@@ -1,3 +1,55 @@
provider "aws" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started updating based on the Spark pattern but the change was getting big rather quick so I didn't carry it across all the EMR patterns (so far). Do we have a standard layout for DoEKS? I'd like to move EKS Blueprints to something along the lines of:

  • main.tf - has the provider blocks, common data and locals
  • eks.tf - eks configs
  • add-ons.tf - add-on configs
  • vpc.tf - vpc configs

Then depending on the pattern there may be more additional files such as:

  • emr.tf
  • ?

Which I think is mostly whats shown in DoEKS if the cluster moved to its own file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely, this aligns well with our established practice within the Data on EKS repository. This folder/file structure's primary objective is to offer straightforward navigation across different implementation parts like VPC, EKS, add-ons, emr-eks, FSx-for-Lustre, etc.

This enhances code readability, leading to easier and more effective maintenance. Each section's segregation into individual files allows us to make adjustments or troubleshoot specific sectors without impacting the overall infrastructure. Additionally, it streamlines the onboarding process for newcomers, enabling them to swiftly grasp the project's structure and functionality. This setup also promotes modularity, facilitating the reuse of certain configurations other blueprints(e.g., copy and paste vpc.tf file ), potentially increasing consistency and minimizing errors.

I propose we structure it as follows, but let's discuss this internally to finalize a pattern that is beneficial for both EKS Blueprints and Data on EKS:

main.tf - This can stay as it is, containing our provider blocks, locals, and data among other things. or remove it completely. Many terraform users try to look for main.tf file in terraform blueprints or modules and they might wonder where to start for reading the code. Let's discuss this a bit whether its a good idea to remove it or not.
eks.tf - I'm in favour of shifting EKS cluster resources from main.tf to eks.tf.
addons.tf - This will carry all the EKS blueprints and data addons, same as before.
vpc.tf - This will hold our VPC configurations.
emr-eks.tf - For the EMR teams. Let's keep it as emr-eks.tf to steer clear of any confusion with the EMR service.
fsx-for-lustre.tf - This will be the module for FSx.
versions.tf - Versions
providers.tf - I don't have a strong preference on keeping this as a separate file or moving it to main.tf.
and variables.tf, outputs.tf

We can remove locals.tf and data.tf files and add these to individual files that requires these local variables and data resources.

@bryantbiggs bryantbiggs temporarily deployed to DoEKS Test July 14, 2023 15:44 — with GitHub Actions Inactive
@bryantbiggs bryantbiggs temporarily deployed to DoEKS Test July 14, 2023 15:55 — with GitHub Actions Inactive
Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@bryantbiggs Thanks! We will try split the future PRs to individual blueprints so that we can easily review and test it.

@vara-bonthu vara-bonthu merged commit 5edce0c into awslabs:main Jul 15, 2023
45 checks passed
@bryantbiggs bryantbiggs deleted the chore/emr-scratch branch July 15, 2023 12:26
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.

None yet

2 participants