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.

Sign up for GitHub

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

Jump to bottom

feat(ec2): Support for new EBS types #12074

Merged
merged 13 commits into from
Jan 21, 2021
Merged

feat(ec2): Support for new EBS types #12074

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ffef11e
feat(ec2): Support for new EBS types
leandrodamascena Dec 14, 2020
bf924f8
Merge branch 'master' into feature/new-ebs-types
leandrodamascena Dec 15, 2020
8614ac4
Merge branch 'master' into feature/new-ebs-types
leandrodamascena Dec 17, 2020
c2f20a9
Merge branch 'master' into feature/new-ebs-types
leandrodamascena Dec 28, 2020
baa39d6
Merge pull request #1 from aws/master
leandrodamascena Dec 28, 2020
e64ca4c
Merge branch 'master' of https://github.com/leandrodamascena/aws-cdk …
leandrodamascena Dec 28, 2020
f21617b
Merge branch 'feature/new-ebs-types' of https://github.com/leandrodam…
leandrodamascena Dec 28, 2020
d3ccef6
feat(ec2): Support for new EBS types
leandrodamascena Dec 28, 2020
d2a10da
Merge branch 'master' into feature/new-ebs-types
rix0rrr Jan 6, 2021
ab1edf3
Merge branch 'master' into feature/new-ebs-types
leandrodamascena Jan 20, 2021
9ecf7b1
feat(ec2): Support for new EBS types
leandrodamascena Jan 20, 2021
0fd879d
Merge branch 'master' into feature/new-ebs-types
leandrodamascena Jan 21, 2021
4279fba
Merge branch 'master' into feature/new-ebs-types
mergify[bot] Jan 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions packages/@aws-cdk/aws-autoscaling/lib/volume.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,15 +178,25 @@ export enum EbsDeviceVolumeType {
STANDARD = 'standard',

/**
* Provisioned IOPS SSD
* Provisioned IOPS SSD - IO1
*/
IO1 = 'io1',

/**
* General Purpose SSD
* Provisioned IOPS SSD - IO2
*/
IO2 = 'io2',

/**
* General Purpose SSD - GP2
*/
GP2 = 'gp2',

/**
* General Purpose SSD - GP3
*/
GP3 = 'gp3',

/**
* Throughput Optimized HDD
*/
Expand Down
28 changes: 24 additions & 4 deletions packages/@aws-cdk/aws-ec2/lib/volume.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,15 +205,25 @@ export enum EbsDeviceVolumeType {
STANDARD = 'standard',

/**
* Provisioned IOPS SSD
* Provisioned IOPS SSD - IO1
*/
IO1 = 'io1',

/**
* General Purpose SSD
* Provisioned IOPS SSD - IO2
*/
IO2 = 'io2',

/**
* General Purpose SSD - GP2
*/
GP2 = 'gp2',

/**
* General Purpose SSD - GP3
*/
GP3 = 'gp3',

/**
* Throughput Optimized HDD
*/
Expand All @@ -225,15 +235,25 @@ export enum EbsDeviceVolumeType {
SC1 = 'sc1',

/**
* General purpose SSD volume that balances price and performance for a wide variety of workloads.
* General purpose SSD volume (GP2) that balances price and performance for a wide variety of workloads.
*/
GENERAL_PURPOSE_SSD = GP2,

/**
* Highest-performance SSD volume for mission-critical low-latency or high-throughput workloads.
* General purpose SSD volume (GP3) that balances price and performance for a wide variety of workloads.
*/
GENERAL_PURPOSE_SSD_GP3 = GP3,

/**
* Highest-performance SSD volume (IO1) for mission-critical low-latency or high-throughput workloads.
*/
PROVISIONED_IOPS_SSD = IO1,

/**
* Highest-performance SSD volume (IO2) for mission-critical low-latency or high-throughput workloads.
*/
PROVISIONED_IOPS_SSD_IO2 = IO2,

/**
* Low-cost HDD volume designed for frequently accessed, throughput-intensive workloads.
*/
Expand Down
46 changes: 45 additions & 1 deletion packages/@aws-cdk/aws-ec2/test/instance.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,27 @@ nodeunitShim({
test.done();
},

'throws if volumeType === IO2 without iops'(test: Test) {
// THEN
test.throws(() => {
new Instance(stack, 'Instance', {
vpc,
machineImage: new AmazonLinuxImage(),
instanceType: InstanceType.of(InstanceClass.T3, InstanceSize.LARGE),
blockDevices: [{
deviceName: 'ebs',
volume: BlockDeviceVolume.ebs(15, {
deleteOnTermination: true,
encrypted: true,
volumeType: EbsDeviceVolumeType.IO2,
}),
}],
});
}, /ops property is required with volumeType: EbsDeviceVolumeType.IO1/);

test.done();
},

'warning if iops without volumeType'(test: Test) {
const instance = new Instance(stack, 'Instance', {
vpc,
Expand All @@ -226,7 +247,7 @@ nodeunitShim({

// THEN
test.deepEqual(instance.node.metadata[0].type, cxschema.ArtifactMetadataEntryType.WARN);
test.deepEqual(instance.node.metadata[0].data, 'iops will be ignored without volumeType: EbsDeviceVolumeType.IO1');
test.deepEqual(instance.node.metadata[0].data, 'iops will be ignored without volumeType: EbsDeviceVolumeType.IO1 or EbsDeviceVolumeType.IO2');
Copy link
Contributor

Choose a reason for hiding this comment

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

This warning is confusing. From the code it seems like setting volumeType to EbsDeviceVolumeType.IO2 without setting iops is not allowed, but the warning implies the other way around?

Same comment as below, if this test is not adding any value we shouldn't add it.

If these were added to satisfy the linter rule, note that I have added an exempt label so it will not block the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! As the comment above, I'll remove this test too.


test.done();
},
Expand All @@ -253,6 +274,29 @@ nodeunitShim({

test.done();
},

'warning if iops and volumeType !== IO2'(test: Test) {
const instance = new Instance(stack, 'Instance', {
vpc,
machineImage: new AmazonLinuxImage(),
instanceType: InstanceType.of(InstanceClass.T3, InstanceSize.LARGE),
blockDevices: [{
deviceName: 'ebs',
volume: BlockDeviceVolume.ebs(15, {
deleteOnTermination: true,
encrypted: true,
volumeType: EbsDeviceVolumeType.GP2,
iops: 5000,
}),
}],
});

// THEN
test.deepEqual(instance.node.metadata[0].type, cxschema.ArtifactMetadataEntryType.WARN);
test.deepEqual(instance.node.metadata[0].data, 'iops will be ignored without volumeType: EbsDeviceVolumeType.IO2');

test.done();
},
},

'instance can be created with Private IP Address'(test: Test) {
Expand Down
38 changes: 38 additions & 0 deletions packages/@aws-cdk/aws-ec2/test/volume.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,25 @@ nodeunitShim({
test.done();
},

'volume: io2'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is nothing special about the volume type, there is no need to add a unit test for it. Given the large number of types, adding a test for each one will increase the overall tests execution time without providing additional value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'm going to remove this test and all the others! As I saw tests with specific volume types, I considered it important for the process. But it makes sense not to increase the execution time of the tests.

// GIVEN
const stack = new cdk.Stack();

// WHEN
new Volume(stack, 'Volume', {
availabilityZone: 'us-east-1a',
size: cdk.Size.gibibytes(500),
volumeType: EbsDeviceVolumeType.PROVISIONED_IOPS_SSD,
});

// THEN
cdkExpect(stack).to(haveResourceLike('AWS::EC2::Volume', {
VolumeType: 'io2',
}, ResourcePart.Properties));

test.done();
},

'volume: gp2'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand All @@ -357,6 +376,25 @@ nodeunitShim({
test.done();
},

'volume: gp3'(test: Test) {
// GIVEN
const stack = new cdk.Stack();

// WHEN
new Volume(stack, 'Volume', {
availabilityZone: 'us-east-1a',
size: cdk.Size.gibibytes(500),
volumeType: EbsDeviceVolumeType.GENERAL_PURPOSE_SSD,
});

// THEN
cdkExpect(stack).to(haveResourceLike('AWS::EC2::Volume', {
VolumeType: 'gp3',
}, ResourcePart.Properties));

test.done();
},

'volume: st1'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-elasticsearch/lib/domain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ export interface EbsOptions {
readonly volumeSize?: number;

/**
* The EBS volume type to use with the Amazon ES domain, such as standard, gp2, io1, st1, or sc1.
* The EBS volume type to use with the Amazon ES domain, such as standard, gp2, io1.
* For more information, see[Configuring EBS-based Storage]
* (https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-createupdatedomains.html#es-createdomain-configure-ebs)
* in the Amazon Elasticsearch Service Developer Guide.
Expand Down
5 changes: 4 additions & 1 deletion packages/@aws-cdk/aws-rds/lib/instance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,10 +283,13 @@ export interface DatabaseInstanceNewProps {
readonly availabilityZone?: string;

/**
* The storage type.
* The storage type. Storage types supported are gp2, io1, standard.
*
* @see https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/CHAP_Storage.html#Concepts.Storage.GeneralSSD
*
* @default GP2
*/

readonly storageType?: StorageType;

/**
Expand Down