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 Intel Enhanced Optimizations for RDS MySQL Example #522

Conversation

lucasmelogithub
Copy link

Description

The PR adds an Intel Enhanced Optimizations for RDS MySQL example. The example implements:

  • Intel Xeon Instance Selection
  • Intel Xeon MySQL Optimizations via an Intel Parameter Group module

Please squash commits if approved/merged.

Motivation and Context

New Example

Breaking Changes

No

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
  • I have executed pre-commit run -a on my pull request

Successfully tested deployment:

  • TF Plan/Apply
  • TF Idempotency(terraform plan/apply a second time)
  • Destroy

image

@bryantbiggs
Copy link
Member

thank you for the PR - however, I don't know that this is the appropriate place for this. From what I can tell, this is mostly just using the module that you all have created for the parameter group. The examples we have provided here are demonstrations of the various configurations that this module supports - users are absolutely able to provide their own parameter groups and/or parameter group values, but it doesn't really have any bearing on the examples we've provided. Closing this for now

@JoshHilliker
Copy link

thank you for the PR - however, I don't know that this is the appropriate place for this. From what I can tell, this is mostly just using the module that you all have created for the parameter group. The examples we have provided here are demonstrations of the various configurations that this module supports - users are absolutely able to provide their own parameter groups and/or parameter group values, but it doesn't really have any bearing on the examples we've provided. Closing this for now


Hi Bryant, in our example we are showcasing the Intel Optimizations based on our Intel Xeon Tuning Guides. Our goal was not to show how to utilize parameter groups, but to highlight the specific optimizations/configuration that can increase RDS performance based on Intel Architecture. We spoke with @antonbabenko at HashiConf & then again at AWS re:Invent about showcasing these value add to customers. Please let me know if there are additional questions or if a quick meeting would be best. Thank you Josh H cc: @lucasmelogithub

@antonbabenko
Copy link
Member

antonbabenko commented Dec 11, 2023

@bryantbiggs What you say was also my point to Josh when I met him in October. :) Such examples are not as helpful since users can specify whatever they want in the argument. We know that, and usually, we don't accept such contributions.

On the other hand, let's consider some changes and review it once more if it makes sense to have it here or not. I recommend that the author make this example 100% self-contained without reference to external modules (like this) and unnecessary logos/images. Include the correct/optimal values for the parameter group as arguments to the module created by this repository - this can be the only interesting part of the example that differentiates it from other similar examples we already have. Leave links to the external repositories/sources, if necessary.

@bryantbiggs What do you think?

@bryantbiggs bryantbiggs reopened this Dec 11, 2023
@bryantbiggs
Copy link
Member

I haven't had great experiences with external parties providing contributions that point back to something they have created. Take EKS Blueprints for example - one of the major changes in the refactoring of that projects addons was due to partners not providing the level of support they had originally claimed and we cannot be experts of all the things. So when someone comes to our project and sees something - they will open an issue or raise questions here and we are not equipped to take on all of those requests (both from a resource perspective, but more importantly, from an expertise perspective).

And likewise, having these in actual code that is part of the CI test and validation process can quickly lead to problems (i.e. - externally linked code starts failing, which starts failing all of the CI checks here which is outside of our control).

However, I do agree that there is value in information sharing and I am sure there are users of this module that would be interested in this type of information. My proposal would be to not have it as functional code as part of the CI tests, but instead static code example that clearly states all questions/issues should be raised on the external project for its related components. For reference, this is what we do now for partner addons in EKS Blueprints https://aws-ia.github.io/terraform-aws-eks-blueprints-addons/main/aws-partner-addons/

We could create a separate docs section like https://github.com/terraform-aws-modules/terraform-aws-alb/blob/master/docs/patterns.md and have a code example and explanation on it there with any relevant links. So its more of a demonstration of how these other modules can further help users of this module but its not something we are actively testing and validating as part of this module. Thoughts?

@antonbabenko
Copy link
Member

I like referring to text documents with or without the code inside. I also don't want us to maintain code where we depend on external repositories we don't control.

@lucasmelogithub @JoshHilliker What do you think? Will you be able to do the change?

@JoshHilliker
Copy link

I like referring to text documents with or without the code inside. I also don't want us to maintain code where we depend on external repositories we don't control.

@lucasmelogithub @JoshHilliker What do you think? Will you be able to do the change?

@antonbabenko @bryantbiggs - Lucas & I were talking, what are your thoughts about moving the Intel MySQL parameter group module into this repo, that way it's managed under the same tree, but we can still use the sub modular approach.

@antonbabenko
Copy link
Member

I looked into the Intel MySQL parameter group module once more, and the only piece of code that can be interesting to have in our module is this variable - https://github.com/intel/terraform-intel-aws-mysql-parameter-group/blob/main/variables.tf#L1-L165 .

I don't see a need to make a separate submodule in our repository just for this variable, but instead, I prefer to add a small example or just a markdown file with such values listed. Everyone interested in knowing how to use this module to deploy RDS resources with an opinionated config provided by Intel can just look there.

I will let @bryantbiggs decide if he is open to "a small opinionated example" or a "markdown file" (as Bryant mentioned earlier). I am more in favor of a solution that requires less maintenance (=markdown file).

@JoshHilliker
Copy link

I think what is important as well is the benchmarking and branding of Intel behind these data points in the variables.tf. We kept this as a separate module to have updates/consumption via git tags w/ the standard tf source version control. I think having this a separate module with reference to Intel (we can remove image branding) would be of high value to consumers of the module. We plan to continue to update the module as new Xeon generations land on AWS. @antonbabenko @bryantbiggs @lucasmelogithub

@JoshHilliker
Copy link

@bryantbiggs @antonbabenko - Happy New Year! Love to pick this back up and get your feedback on my latest comment. Thank You

@bryantbiggs
Copy link
Member

Hi @JoshHilliker - so if you wish to keep the module separate and as a standalone module, then I think that we are discussing here is just a markdown file that demonstrates how to use the module with this module plus any other details might need to be aware of and links back to your module/docs.

@JoshHilliker
Copy link

Lucas & I talked more. Here's our proposal: 1) removal of Intel branding from Enhanced-Optimization example 2) pull in all parameter group configuration into the stand alone example, so no external dependencies. What do you think?

@JoshHilliker
Copy link

Lucas & I talked more. Here's our proposal: 1) removal of Intel branding from Enhanced-Optimization example 2) pull in all parameter group configuration into the stand alone example, so no external dependencies. What do you think?

@antonbabenko @bryantbiggs

@bryantbiggs
Copy link
Member

But we (Anton and I, and other contributors) do not have knowledge or experience with these Intel enhanced optimizations - so it doesn't make sense to have code here for that because we will be unable to troubleshoot it. This is why the static markdown is recommended - we can inform users of these configurations and of the module that will do this for them, but we're not testing and validating that

@JoshHilliker
Copy link

But we (Anton and I, and other contributors) do not have knowledge or experience with these Intel enhanced optimizations - so it doesn't make sense to have code here for that because we will be unable to troubleshoot it. This is why the static markdown is recommended - we can inform users of these configurations and of the module that will do this for them, but we're not testing and validating that

Okay.. here's my proposal. we can create an example directory for this and put in the markdown file. good?

@antonbabenko
Copy link
Member

@JoshHilliker Yes, please do that, and link to such an example from the main README.md file in the root.

@lucasmelogithub
Copy link
Author

@JoshHilliker Yes, please do that, and link to such an example from the main README.md file in the root.

Closing PR. A new one will be created per discussion above.

@lucasmelogithub lucasmelogithub deleted the intel-enhanced-mysql-example-1 branch March 1, 2024 22:28
Copy link

github-actions bot commented Apr 1, 2024

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 Apr 1, 2024
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.

4 participants