feat(fsx): add support for FSx Lustre Persistent_2 deployment type #18626
Conversation
AWS FSx Lustre released a new deployment type PERSISTENT_2 during re:Invent 2021. It is a newer deployment type that provides long term storage with higher throughput tiers (125, 250, 500, 1000). Data is replicated and file servers are replaced if they fail.
|
|
There was a problem hiding this comment.
Thanks for the contribution @siddharthsalot! In general looks good, two tiny comments before we merge this in.
| * Newer type of long term storage with higher throughput tiers. | ||
| * Data is replicated and file servers are replaced if they fail. | ||
| */ | ||
| PERSISTENT_2 = 'PERSISTENT_2' |
There was a problem hiding this comment.
Let's add a trailing comma here, to avoid the ugly diff that just happened above 🙂.
| PERSISTENT_2 = 'PERSISTENT_2' | |
| PERSISTENT_2 = 'PERSISTENT_2', |
skinny85
changed the title
Adds unit test cases for FSx Lustre Persistent_2 deployment type.
siddharthsalot
force-pushed
the
master
branch
from
3be7785 to
9814a7f
Compare
mergify
bot
dismissed
skinny85’s stale review
Pull request has been modified.
siddharthsalot
force-pushed
the
master
branch
from
953d1c4 to
a00efcd
Compare
|
Unit test results: |
skinny85
added
the
pr-linter/exempt-readme
There was a problem hiding this comment.
Looks great @siddharthsalot! A few tiny comments before we merge this in.
| if (deploymentType !== LustreDeploymentType.PERSISTENT_1) { | ||
| throw new Error('perUnitStorageThroughput can only be set for the PERSISTENT_1 deployment type'); | ||
| if (deploymentType !== LustreDeploymentType.PERSISTENT_1 && deploymentType !== LustreDeploymentType.PERSISTENT_2) { | ||
| throw new Error('perUnitStorageThroughput can only be set for the PERSISTENT_1/PERSISTENT_2 deployment types'); |
There was a problem hiding this comment.
Let's add what was provided here (this is a good practice with all error messages):
| throw new Error('perUnitStorageThroughput can only be set for the PERSISTENT_1/PERSISTENT_2 deployment types'); | |
| throw new Error('perUnitStorageThroughput can only be set for the PERSISTENT_1/PERSISTENT_2 deployment types, received: ' + deploymentType); |
There was a problem hiding this comment.
Agreed. Updated as part of Revision 4
|
|
||
| if (deploymentType === LustreDeploymentType.PERSISTENT_1) { | ||
| if (![50, 100, 200].includes(perUnitStorageThroughput)) { | ||
| throw new Error('perUnitStorageThroughput must be 50, 100, or 200 MB/s/TiB'); |
There was a problem hiding this comment.
Same note here:
| throw new Error('perUnitStorageThroughput must be 50, 100, or 200 MB/s/TiB'); | |
| throw new Error('perUnitStorageThroughput must be 50, 100, or 200 MB/s/TiB for PERSISTENT_1 deployment type, got: ' + perUnitStorageThroughput); |
There was a problem hiding this comment.
Agreed. Updated as part of Revision 4
| throw new Error('perUnitStorageThroughput must be 50, 100, or 200 MB/s/TiB'); | ||
| if (deploymentType === LustreDeploymentType.PERSISTENT_2) { | ||
| if (![125, 250, 500, 1000].includes(perUnitStorageThroughput)) { | ||
| throw new Error('perUnitStorageThroughput must be 125, 250, 500 or 1000 MB/s/TiB'); |
There was a problem hiding this comment.
Same here:
| throw new Error('perUnitStorageThroughput must be 125, 250, 500 or 1000 MB/s/TiB'); | |
| throw new Error('perUnitStorageThroughput must be 125, 250, 500 or 1000 MB/s/TiB for PERSISTENT_2 storage type, got: ' + perUnitStorageThroughput); |
There was a problem hiding this comment.
Agreed. Updated as part of Revision 4
| test('setting perUnitStorageThroughput on wrong deploymentType', () => { | ||
| lustreConfiguration = { | ||
| deploymentType: LustreDeploymentType.SCRATCH_2, | ||
| perUnitStorageThroughput: 125, | ||
| }; | ||
|
|
||
| expect(() => { | ||
| new LustreFileSystem(stack, 'FsxFileSystem', { | ||
| lustreConfiguration, | ||
| storageCapacityGiB: storageCapacity, | ||
| vpc, | ||
| vpcSubnet, | ||
| }); | ||
| }).toThrowError('perUnitStorageThroughput can only be set for the PERSISTENT_1/PERSISTENT_2 deployment types'); |
There was a problem hiding this comment.
I have to say, I don't quite understand why this test is here...? It's under a describe('perUnitStorageThroughput_Persistent_2', () => {, but it doesn't use DeploymentTime. PERSISTENT_2 at all?
We have a test above called test('setting perUnitStorageThroughput on wrong deploymentType' which I think covers this already...?
I think we can safely remove this test.
There was a problem hiding this comment.
Agreed. Updated as part of Revision 4
| test('invalid perUnitStorageThroughput', () => { | ||
| lustreConfiguration = { | ||
| deploymentType: LustreDeploymentType.PERSISTENT_2, | ||
| perUnitStorageThroughput: 1, | ||
| }; | ||
|
|
||
| expect(() => { | ||
| new LustreFileSystem(stack, 'FsxFileSystem', { | ||
| lustreConfiguration, | ||
| storageCapacityGiB: storageCapacity, | ||
| vpc, | ||
| vpcSubnet, | ||
| }); | ||
| }).toThrowError('perUnitStorageThroughput must be 125, 250, 500 or 1000 MB/s/TiB'); | ||
| }); |
There was a problem hiding this comment.
I would probably make this test also parametrized, so that it covers more scenarios than just 1, like the tests for invalid storageCapacity below.
There was a problem hiding this comment.
Agreed. Updated as part of Revision 4
siddharthsalot
force-pushed
the
master
branch
from
7f5bfdc to
2d0a33a
Compare
mergify
bot
dismissed
skinny85’s stale review
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
* origin/master: (27 commits) chore(eks): deprecate older versions of EKS (aws#18842) fix(tooling): update vscode devcontainer image (aws#18455) chore: npm-check-updates && yarn upgrade (aws#18832) chore(docs): Fix broken md links (aws#18384) chore(lambda-layer-awscli): install awscli with pip and requirements.txt (aws#18800) fix(aws-appsync): Strip unsupported characters from Lambda DataSource (aws#18765) feat(cfnspec): cloudformation spec v55.0.0 (aws#18827) docs(cfnspec): update CloudFormation documentation (aws#18826) chore(cxapi): plugin context provider limited by cx schema (aws#18709) feat(iotevents): add grant method to Input class (aws#18617) chore(cx-api): break circular dependencies (aws#18767) docs(core): clarify that `addOverride` does not change property casing (aws#18687) feat(s3-deployment): deploy data with deploy-time values (aws#18659) docs(cfnspec): update CloudFormation documentation (aws#18808) feat(cli): `cdk diff` works for Nested Stacks (aws#18207) docs(cfnspec): update CloudFormation documentation (aws#18783) chore(lambda-layer-awscli): add update mechanism for AWS CLI (aws#18780) chore(release): 1.143.0 feat(fsx): add support for FSx Lustre Persistent_2 deployment type (aws#18626) feat(amplify): support performance mode in Branch (aws#18598) ...
…ws#18626) FSx Lustre released support for a new deployment type "Persistent_2" at re:Invent 2021. It provides long term storage with higher throughput tiers (125, 250, 500, 1000 MiB/s/TiB). See https://docs.aws.amazon.com/fsx/latest/LustreGuide/using-fsx-lustre.html for more details. As part of this commit, we want to add support for this new deployment type in LustreDeploymentType enum. This will allow customers to create FSx Lustre file systems by specifying this new deployment type. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
FSx Lustre released support for a new deployment type "Persistent_2" at re:Invent 2021.
It provides long term storage with higher throughput tiers (125, 250, 500, 1000 MiB/s/TiB).
See https://docs.aws.amazon.com/fsx/latest/LustreGuide/using-fsx-lustre.html for more details.
As part of this commit, we want to add support for this new deployment type in LustreDeploymentType enum.
This will allow customers to create FSx Lustre file systems by specifying this new deployment type.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license