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 #30716

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

isker
Copy link
Contributor

@isker isker commented Jul 1, 2024

Issue # (if applicable)

Fixes #24341.

Reason for this change

You currently can't specify throughput on LaunchTemplate EBS volumes.

Description of changes

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

Description of how you validated changes

Unit and integration 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 effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jul 1, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team July 1, 2024 02:40
@isker
Copy link
Contributor Author

isker commented Jul 1, 2024

The build failure:

[4/4] Building fresh packages...
error /codebuild/output/src3794156442/src/github.com/aws/aws-cdk/node_modules/@lerna/create/node_modules/nx, /codebuild/output/src3794156442/src/github.com/aws/aws-cdk/node_modules/@nx/devkit/node_modules/nx, /codebuild/output/src3794156442/src/github.com/aws/aws-cdk/node_modules/lerna/node_modules/nx: Command failed.
Exit code: 135
Command: node ./bin/post-install
Arguments: 
Directory: /codebuild/output/src3794156442/src/github.com/aws/aws-cdk/node_modules/lerna/node_modules/nx
Output:
Bus error (core dumped)
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

[Container] 2024/07/01 02:44:29.448680 Command did not exit successfully /bin/bash ./build.sh --ci exit status 135
[Container] 2024/07/01 02:44:29.451865 Phase complete: BUILD State: FAILED

This is nowhere near the first spurious build failure I've seen in just my third PR to this project, and the problems are not limited to this main CI job: https://github.com/aws/aws-cdk/actions/runs/9737293345/job/26869186116?pr=30716 has also failed for reasons unclear.

It has proven to be challenging to contribute to aws-cdk. Contributors need to pass through an underspecified battery of PR label transitions in order that their PR be brought to a human being's attention so that it may be reviewed and merged. These transitions are automated and gated by CI success. But here is a PR where two of the gating CI jobs have spuriously failed, with no way for me to initiate a retry except by, I guess, rewording my commit message so that I get a different commit hash.

And if contributors don't teach themselves to do this song and dance then their PR will get closed by automation after a period of inactivity, as happened to this changeset in #30317.

Suggestions to maintainers:

  1. Please specify the actual process for PR review so that contributors understand what is expected of them. Having to spend substantial time trying to infer the process based upon my interactions with flakey CI jobs and occasionally maintainers is discouraging. https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#getting-a-review-from-a-maintainer does not specify:
    a. What to do if you can't get this project's flakey CI jobs to pass.
    b. What an exemption request is.
    c. What to do if your exemption requests are being ignored. I ended up having to repeatedly ping maintainers I thought could help me on feat(ec2): support throughput on LaunchTemplate EBS volumes #30317, to no avail. I hope this process was nowhere near as embarrassing to them as it was to me.
  2. If you cannot produce reliable CI, please stop gating PR review progression on it.
  3. The automated "close stale PRs" job leaves something to be desired. On feat(ec2): support throughput on LaunchTemplate EBS volumes #30317, this job closed the PR. I successfully begged a maintainer to reopen it. The job then quickly closed it again. (I gave up for a week, and am now back with a new PR.) Having been reopened by a maintainer, the job should not have quickly closed the PR again.

In aggregate, you have crafted a fully automated, drawn out process for contributors to be (incorrectly, I think) ignored. Ignoring contributors is your prerogative as maintainers, but you could be achieving such outcomes with much less effort than this.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jul 1, 2024
@isker
Copy link
Contributor Author

isker commented Jul 1, 2024

Screenshot 2024-06-30 at 23 42 01

Well, thanks, I guess.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 1, 2024
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍 Left some comments for adjustments

packages/aws-cdk-lib/aws-ec2/lib/volume.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ec2/lib/private/ebs-util.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ec2/README.md Outdated Show resolved Hide resolved
@isker isker force-pushed the ebs-throughput branch 3 times, most recently from dc030fd to 5899b6b Compare July 31, 2024 02:30
@isker
Copy link
Contributor Author

isker commented Jul 31, 2024

#30716 (comment) the log file is completely empty?

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍

packages/aws-cdk-lib/aws-ec2/lib/volume.ts Outdated Show resolved Hide resolved
@isker isker force-pushed the ebs-throughput branch 2 times, most recently from 7773043 to 8cd144f Compare August 1, 2024 01:47
@isker isker force-pushed the ebs-throughput branch 2 times, most recently from 49b2b42 to 05531b1 Compare August 1, 2024 03:09
@aws-cdk-automation aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Aug 1, 2024
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

Maybe I missed it, but I only saw the validation and definition of the property throughput, but I don't see where you assign it to CfnVolume?

packages/aws-cdk-lib/aws-ec2/lib/private/ebs-util.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ec2/lib/private/ebs-util.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ec2/lib/private/ebs-util.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-ec2/lib/volume.ts Show resolved Hide resolved
@aws-cdk-automation aws-cdk-automation removed pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Aug 27, 2024
@isker
Copy link
Contributor Author

isker commented Aug 27, 2024

Maybe I missed it, but I only saw the validation and definition of the property throughput, but I don't see where you assign it to CfnVolume?

https://github.com/aws/aws-cdk/pull/30716/files#diff-c1fd5c618720a81ee00a0360ca37364d5d5281c596a5bc8f63edb637607b6d54R64

This is not using CfnVolume. It's using a (nested) property of LaunchTemplate: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-launchtemplate-ebs.html
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ec2.CfnLaunchTemplate.EbsProperty.html#throughput.

@GavinZZ
Copy link
Contributor

GavinZZ commented Aug 28, 2024

Maybe I missed it, but I only saw the validation and definition of the property throughput, but I don't see where you assign it to CfnVolume?

https://github.com/aws/aws-cdk/pull/30716/files#diff-c1fd5c618720a81ee00a0360ca37364d5d5281c596a5bc8f63edb637607b6d54R64

This is not using CfnVolume. It's using a (nested) property of LaunchTemplate: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-launchtemplate-ebs.html https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ec2.CfnLaunchTemplate.EbsProperty.html#throughput.

Ah I see, I missed that line of code. Thanks for the clarification.

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.
@mergify mergify bot dismissed GavinZZ’s stale review August 29, 2024 00:03

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 29, 2024
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Contributor

mergify bot commented Aug 29, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 29, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 6ed0bed into aws:main Aug 29, 2024
12 checks passed
Copy link
Contributor

mergify bot commented Aug 29, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
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
4 participants