feat(ec2): support for the credit configuration mode for burstable instances by badmintoncryer · Pull Request #28728 · aws/aws-cdk

badmintoncryer · 2024-01-16T18:07:32Z

In this PR, I have enabled the setting of the credit configuration mode for EC2 instances and NAT instances in burstable performance instances.

// for EC2 instance
new Instance(stack, 'Instance', {
  vpc,
  machineImage: new AmazonLinuxImage(),
  instanceType: InstanceType.of(InstanceClass.T3, InstanceSize.LARGE),
  creditSpecification: CpuCredits.STANDARD, // added
});

// for NAT instance
const natInstanceProvider = NatProvider.instance({
  instanceType: InstanceType.of(InstanceClass.T3, InstanceSize.LARGE),
  machineImage: new AmazonLinuxImage(),
  creditSpecification: CpuCredits.STANDARD, // added
});
new Vpc(stack, 'VPC', {
  natGatewayProvider: natInstanceProvider,
});

Closes #19166.

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

aws-cdk-automation

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.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

go-to-k

Thanks for this PR.
The implementation looks good. I made some comments.

go-to-k · 2024-01-21T09:04:33Z

packages/aws-cdk-lib/aws-ec2/lib/instance.ts

+
+  /**
+   * Specifying the CPU credit type for burstable EC2 instance types (T2, T3, T3a, etc).
+   * T3 instances with `host` tenancy do not support the unlimited CPU credit option.


This document appears to be taken from the L1 or CFn text.
The host here refers to the tenancy property of CfnInstanceProps, which is not visible in the L2 construct. Wouldn't it therefore be more appropriate to use the word dedicated host?

Otherwise, users may be get confused (like me).

@go-to-k
Indeed, you're right. I've updated the sentence, so please check it.

go-to-k · 2024-01-21T09:08:03Z

packages/aws-cdk-lib/aws-ec2/lib/nat.ts

+
+  /**
+   * Specifying the CPU credit type for burstable EC2 instance types (T2, T3, T3a, etc).
+   * T3 instances with `host` tenancy do not support the unlimited CPU credit option.


same as above

go-to-k · 2024-01-21T09:14:35Z

packages/aws-cdk-lib/aws-ec2/lib/instance.ts


+    // if credit specification is set, then the instance type must be burstable
+    if (props.creditSpecification && !props.instanceType.isBurstable()) {
+      throw new Error(`creditSpecification is not supported for ${props.instanceType.toString()} instance type`);


How about outputting which instance types are supported?

go-to-k · 2024-01-21T09:24:40Z

packages/aws-cdk-lib/aws-ec2/test/nat.test.ts

Since there are already tests for Nat instances, how about putting them in here instead of adding new files? If they are scattered, the contributors will not know which file to put them in when adding tests for Nat instances in the future.

(But if you have a particular intention, I will respect that.)

I hadn't noticed the existing test code. Thank you for your suggestion.

badmintoncryer · 2024-01-21T20:00:35Z

@go-to-k Thank you for your review! I have addressed your comments.

go-to-k

Thanks for your change, I made some comments again.

packages/aws-cdk-lib/aws-ec2/lib/instance.ts

badmintoncryer · 2024-01-22T17:10:10Z

@go-to-k Thank you! I have made the revisions again.

go-to-k

Thanks!

Co-authored-by: k.goto <24818752+go-to-k@users.noreply.github.com>

aws-cdk-automation · 2024-02-29T23:27:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 223e95b
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2024-02-29T23:27:39Z

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).

github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jan 16, 2024

aws-cdk-automation requested a review from a team January 16, 2024 18:07

aws-cdk-automation previously requested changes Jan 16, 2024

View reviewed changes

badmintoncryer marked this pull request as ready for review January 16, 2024 18:25

badmintoncryer marked this pull request as draft January 16, 2024 19:26

badmintoncryer marked this pull request as ready for review January 17, 2024 14:24

badmintoncryer changed the title ~~feat(ec2): support for the credit configuration mode for a EC2 instance including NAT instance.~~ feat(ec2): support for the credit configuration mode for a EC2 instance including NAT instance Jan 17, 2024

badmintoncryer changed the title ~~feat(ec2): support for the credit configuration mode for a EC2 instance including NAT instance~~ feat(ec2): support for the credit configuration mode for burstable instances Jan 17, 2024

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 17, 2024

go-to-k suggested changes Jan 21, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 21, 2024

badmintoncryer force-pushed the 19166-creditSpecification branch from 2f6b436 to 06a904c Compare January 21, 2024 18:15

go-to-k suggested changes Jan 22, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

go-to-k approved these changes Jan 22, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 22, 2024

github-actions bot mentioned this pull request Feb 1, 2024

Monthly PR metrics report - Jan 2024 #28952

Closed

badmintoncryer added 8 commits February 13, 2024 14:31

feat: add creditSpecification to InstanceProps

198d8e3

feat: add creditSpecification to NatInstanceProps

0b837f9

test: update unit test

70abcfe

test: integ test

69a645b

docs: updte readme

37cdf63

fix: import order

4cbd063

fix: unit test

f0760c3

test: integ test

338d85f

badmintoncryer and others added 9 commits February 13, 2024 14:31

fix: readme

16012bc

feat: throw for creditSpecification defined to non-burstable instance

2a5300a

fix: README

0c1d2ae

refactor: update unit test

d56e17d

docs: update comment

5af31ef

docs: update comment

7d8c659

instance.ts を更新

07221d4

Co-authored-by: k.goto <24818752+go-to-k@users.noreply.github.com>

instance.ts を更新

3ef1df5

Co-authored-by: k.goto <24818752+go-to-k@users.noreply.github.com>

fix: unit test

779fe6d

badmintoncryer force-pushed the 19166-creditSpecification branch from 0a2e48d to 779fe6d Compare February 13, 2024 05:31

badmintoncryer and others added 2 commits February 26, 2024 13:09

Merge branch 'main' into 19166-creditSpecification

f9e7793

Merge branch 'main' into 19166-creditSpecification

223e95b

aaythapa approved these changes Feb 29, 2024

View reviewed changes

mergify bot merged commit 3cbad4a into aws:main Feb 29, 2024

badmintoncryer deleted the 19166-creditSpecification branch March 1, 2024 01:34

Conversation

badmintoncryer commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

go-to-k left a comment

Choose a reason for hiding this comment

Uh oh!

go-to-k Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

badmintoncryer Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

go-to-k Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

go-to-k Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

go-to-k Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

badmintoncryer Jan 21, 2024

Choose a reason for hiding this comment

Uh oh!

badmintoncryer commented Jan 21, 2024

Uh oh!

go-to-k left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

badmintoncryer commented Jan 22, 2024

Uh oh!

go-to-k left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Feb 29, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Feb 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

badmintoncryer commented Jan 16, 2024 •

edited

Loading