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(ec2): support throughput on LaunchTemplate EBS volumes #30317

Closed
wants to merge 1 commit into from

Conversation

isker
Copy link
Contributor

@isker isker commented May 23, 2024

Issue # (if applicable)

Fixes #24341.

Reason for this change

This support was simply not included in ec2 when it was added to autoscaling in #22441.

Description of changes

I have copied that PR's implementation implementation to ec2 and similarly adapted its tests.

Description of how you validated changes

Unit tests.

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels May 23, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team May 23, 2024 02:08
@isker isker force-pushed the ebs-throughput branch from 6dde01b to e1d73f1 Compare May 23, 2024 02:09
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@isker
Copy link
Contributor Author

isker commented May 23, 2024

Exemption Request

  1. The EC2 README already has nothing to say about GP3 on LaunchTemplate BlockDevice volumes. It does talk about GP3 on ec2.Volumes already. I am not sure it's worth showing GP3 usage for LaunchTemplates as well.
  2. The PR that added general throughput support to EC2 feat(autoscaling): support for throughput on GP3 volumes #22441 declined to add integration tests to EC2 and was still merged. I guess I'm similarly declining 🌞.

Perhaps this PR is better characterized as a fix? I'm not sure.

@isker
Copy link
Contributor Author

isker commented May 29, 2024

Hi @pahud, since you have context on the linked issue, could you give me your thoughts on the above exemption request? Thanks!

@isker
Copy link
Contributor Author

isker commented Jun 6, 2024

Hi @pahud sorry to ping you once again but I would appreciate your advice on how to proceed. Thanks.

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@isker
Copy link
Contributor Author

isker commented Jun 14, 2024

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@pahud any help you can give me on this?

@GavinZZ
Copy link
Contributor

GavinZZ commented Jun 17, 2024

Hi @isker, EC2 is also a stable module so I would highly recommend adding an simple integration test to use the throughput property to make sure it's deployable. It should be quite straightforward. For the linked similar PR you mentioned, it also added an integ test.

@isker
Copy link
Contributor Author

isker commented Jun 17, 2024

@GavinZZ I see, I think I got confused because that PR added its test to packages/@aws-cdk/aws-autoscaling, and I saw no integration tests in packages/@aws-cdk/aws-ec2, but that's because everything got moved in a2c633f. I see where I can add integration test coverage for aws-ec2 now. I will do that.

This support was simply not included in ec2 when it was added to
autoscaling in aws#22441. I have copied that PR's implementation
implementation to ec2 and similarly adapted its tests.

Fixes aws#24341.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 672fd0d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@isker
Copy link
Contributor Author

isker commented Jun 18, 2024

Alright, there's the integ coverage. I'm still not sure that this should be added to any README, for the reasons I already mentioned.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jun 22, 2024
@isker
Copy link
Contributor Author

isker commented Jun 22, 2024

@GavinZZ help 😢

@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.

@GavinZZ GavinZZ reopened this Jun 23, 2024
@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EbsDeviceProps missing support for setting throughput on GP3 volumes, yet VolumeProps has support for it
3 participants