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

Support multiple EBS volume types & set default to less-expensive gp3 #77

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mbijon
Copy link

@mbijon mbijon commented May 27, 2021

This is a minor change that could offer a very small cost-savings for bastion hosts. We would like to see it reflected upstream if possible.

If the change to a gp3 default is concerning due to a large volume of TF updates, could the support for specifying volume_type still be added so we can manually specify gp3?

NOTE: This change overlaps partly with #72.

  1. That PR absorbed some other work that wasn't ready for upstream b/c we based it on master
  2. This change responds to the PR comment: Changes root volume default-type to gp3 #72 (comment)

what

why

  • The gp3 type now being set as default is both less expensive per-GB and more-performant for small volumes
    • Since bastion servers have small boot volumes they should never need the larger-sizes where gp2 is needed
    • This is mainly a cost-savings for bastion disk-patterns
  • Allowing volume-type to be specified allows gp2 or standard volume-types if required

references

@mbijon mbijon requested review from a team as code owners May 27, 2021 00:41
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

Bridgecrew has found 1 infrastructure configuration error in this PR ⬇️

main.tf Show resolved Hide resolved
@mbijon mbijon changed the title Volume type Support multiple EBS volume types & set default to less-expensive gp3 May 27, 2021
@mbijon
Copy link
Author

mbijon commented May 27, 2021

I think the "Auto Format" workflow failure is due to a configuration error or traffic-issue for your "cloudpossebot" @Gowiem. The repo it's getting a 403 from is publicly accessible: https://github.com/WorkingGroupLink/terraform-aws-ec2-bastion-server/

remote: Permission to WorkingGroupLink/terraform-aws-ec2-bastion-server.git denied to cloudpossebot.
fatal: unable to access 'https://github.com/WorkingGroupLink/terraform-aws-ec2-bastion-server/': The requested URL returned error: 403
Error: Process completed with exit code 128.

@Gowiem
Copy link
Member

Gowiem commented May 27, 2021

@mbijon happens sometimes. Mind pushing an empty commit? It will sometimes resolve itself --

git commit --allow-empty -m "Trigger CI"
git push

Thanks!

Gowiem
Gowiem previously approved these changes May 27, 2021
variables.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
@Gowiem
Copy link
Member

Gowiem commented May 27, 2021

/test all

@mergify mergify bot dismissed Gowiem’s stale review May 27, 2021 23:48

This Pull Request has been updated, so we're dismissing all reviews.

@mbijon
Copy link
Author

mbijon commented May 27, 2021

Done @Gowiem. Unfortunately it also triggered a merge-commit on my end & failed again anyway...

@mbijon happens sometimes. Mind pushing an empty commit? It will sometimes resolve itself --

git commit --allow-empty -m "Trigger CI"
git push

Thanks!

@Gowiem
Copy link
Member

Gowiem commented May 28, 2021

Ah @mbijon I think with you opening from another org ("WorkingGroupLink"), it's not getting the proper permissions... I'm not sure what the deal is there. @Nuru can I bug you to shed some knowledge on how this works when you have a minute? I know I've seen you poke people for this before.

@mbijon
Copy link
Author

mbijon commented May 28, 2021

@Gowiem wouldn't it work better, faster if you cherry-picked my code into a local branch?

If so, I pushed a cleaner branch without the merge commits to here. Just grab the 2 most-recent commit for the work that matters: https://github.com/WorkingGroupLink/terraform-aws-ec2-bastion-server/commits/mbijon/volume-type

@bmbferreira
Copy link

👋 I'm interested in this as well. Can this be merged any time soon please? Thanks for all the great work 🙇

nachovalera added a commit to Cyber-Duck/terraform-aws-ec2-bastion-server that referenced this pull request Apr 3, 2023
nachovalera added a commit to Cyber-Duck/terraform-aws-ec2-bastion-server that referenced this pull request Apr 3, 2023
@hans-d
Copy link

hans-d commented Mar 8, 2024

/terratest

@hans-d hans-d added stale This PR has gone stale terratest-failing labels Mar 8, 2024
Copy link

mergify bot commented Mar 9, 2024

Thanks @mbijon for creating this pull request!

A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

While you wait, make sure to review our contributor guidelines.

Tip

Need help or want to ask for a PR review to be expedited?

Join us on Slack in the #pr-reviews channel.

@mergify mergify bot removed the stale This PR has gone stale label Mar 15, 2024
@mergify mergify bot added the triage Needs triage label Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Needs triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants