feat(fsx): specify file system type version for the Lustre file system by badmintoncryer · Pull Request #31136 · aws/aws-cdk

badmintoncryer · 2024-08-18T00:59:00Z

Issue # (if applicable)

Closes #31130

Reason for this change

We cannot specify Lustre version for the file system.

Description of changes

Add fileSystemTypeVersion prop to LustreFileSystemProps.

There are some restrictions about fileSystemTypeVersion.

2.10 is supported by the Scratch and Persistent_1 Lustre deployment types.
2.12 is supported by all Lustre deployment types, except for PERSISTENT_2 with a metadata configuration mode.
2.15 is supported by all Lustre deployment types and is recommended for all new file systems.

https://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-resource-fsx-filesystem.html#cfn-fsx-filesystem-filesystemtypeversion

I've only added validation of V2.10 is supported by the Scratch and Persistent_1 deployment types because LustreFileSystemProps does not have metadata configuration props.

  private validateFiileSystemTypeVersion(deploymentType: LustreDeploymentType, fileSystemTypeVersion?: FileSystemTypeVersion): void {
    if (fileSystemTypeVersion === undefined) {
      return;
    }

    if (fileSystemTypeVersion === FileSystemTypeVersion.V_2_10) {
      if (!deploymentType.startsWith('SCRATCH') && deploymentType !== LustreDeploymentType.PERSISTENT_1) {
        throw new Error('fileSystemTypeVersion V_2_10 is only supported for SCRATCH and PERSISTENT_1 deployment types');
      }
    }
  }

Description of how you validated changes

Add both unit and integ tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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.

badmintoncryer · 2024-08-18T01:34:18Z

packages/aws-cdk-lib/aws-fsx/README.md

+**Note**: The `fileSystemTypeVersion` has a restrictions on the values that can be set based on the `deploymentType`.
+
+- `V_2_10` is supported by the Scratch and `PERSISTENT_1` deployment types.
+- `V_2_12` is supported by all Lustre deployment types, except for `PERSISTENT_2` with a metadata configuration mode.
+- `V_2_15` is supported by all Lustre deployment types and is recommended for all new file systems.
+
+**Note**: The default value of `fileSystemTypeVersion` is `V_2_10` except for the following cases:
+
+- Default value is `V_2_12` when `deploymentType` is set to `PERSISTENT_2` without a metadata configuration mode.
+- Default value is `V_2_15` when `deploymentType` is set to `PERSISTENT_2` with a metadata configuration mode.


I have included descriptions related to metadata configuration mode, but the current LustreFileSystem construct does not allow setting metadata. Should descriptions related to metadata be included?

I thought it might be good to either add a note stating that Metadata Configuration cannot be set yet, or alternatively, include the metadata configuration property in this PR.
What do you think?

The MetadataConfiguration property already exists, so it seems possible to add it. However, since it's a new property, it appears that descriptions haven't been defined for it yet.

Thank you for your opinion!

I think it is better to support MetadataConfiguration initially, I was planning to address the metadataConfiguration in this PR as well, but since all the CloudFormation documentation was marked as "not available," I decided to try again once that issue is resolved.

I have updated the README to reflect the assumption that metadataConfiguration is not configured.

mazyu36

Thank you for your contribution. I've made some comments.

mazyu36 · 2024-09-20T10:26:53Z

packages/aws-cdk-lib/aws-fsx/lib/lustre-file-system.ts

+  /**
+   * The Lustre version for the file system.
+   *
+   * @see https://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-resource-fsx-filesystem.html#cfn-fsx-filesystem-filesystemtypeversion


nit: should remove ja_jp

Thanks! I've removed ja_JP/

mazyu36 · 2024-09-21T11:01:26Z

packages/aws-cdk-lib/aws-fsx/README.md

+**Note**: The `fileSystemTypeVersion` has a restrictions on the values that can be set based on the `deploymentType`.
+
+- `V_2_10` is supported by the Scratch and `PERSISTENT_1` deployment types.
+- `V_2_12` is supported by all Lustre deployment types, except for `PERSISTENT_2` with a metadata configuration mode.
+- `V_2_15` is supported by all Lustre deployment types and is recommended for all new file systems.
+
+**Note**: The default value of `fileSystemTypeVersion` is `V_2_10` except for the following cases:
+
+- Default value is `V_2_12` when `deploymentType` is set to `PERSISTENT_2` without a metadata configuration mode.
+- Default value is `V_2_15` when `deploymentType` is set to `PERSISTENT_2` with a metadata configuration mode.


I thought it might be good to either add a note stating that Metadata Configuration cannot be set yet, or alternatively, include the metadata configuration property in this PR.
What do you think?

The MetadataConfiguration property already exists, so it seems possible to add it. However, since it's a new property, it appears that descriptions haven't been defined for it yet.

badmintoncryer · 2024-09-21T14:12:35Z

@mazyu36 Thank you for your review! I've addressed your comments.

mazyu36

Thanks! LGTM

badmintoncryer · 2024-09-22T03:51:57Z

@mazyu36 Thanks!

badmintoncryer · 2024-09-22T14:48:29Z

@pahud ~~mazyu36 (and I) has been selected as a community reviewer, but even after approving, the 'needs-community-review' label is not being updated. Do you know the reason for this?~~

I'm sorry. This problem has resolved.

packages/aws-cdk-lib/aws-fsx/lib/lustre-file-system.ts

GavinZZ · 2024-10-16T20:25:28Z

packages/aws-cdk-lib/aws-fsx/lib/lustre-file-system.ts

+    if (fileSystemTypeVersion === FileSystemTypeVersion.V_2_10) {
+      if (!deploymentType.startsWith('SCRATCH') && deploymentType !== LustreDeploymentType.PERSISTENT_1) {
+        throw new Error('fileSystemTypeVersion V_2_10 is only supported for SCRATCH and PERSISTENT_1 deployment types');
+      }


can we also validate

2.12 is supported by all Lustre deployment types, except for PERSISTENT_2 with a metadata configuration mode.

As I described above, LustreFileSystemProps does not have metadata configuration prop and I think it is difficult to add validation for that.

What do you think about this?

My bad. I totally missed the note in the PR description. Since this prop doesn't exists yet, I think it's fine to skip this validation now.

But can you add a TODO comment to indicate that we should add one once we support metadata config?

I've added TODO comment. Could you please confirm?

Pull request has been modified.

GavinZZ

Thank you!!

mergify · 2024-10-16T22:42:15Z

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

mergify · 2024-10-16T22:45:59Z

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 · 2024-10-16T23:43:38Z

AWS CodeBuild CI Report

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

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

mergify · 2024-10-16T23:44:01Z

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 · 2024-10-16T23:44:19Z

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.

validate

22f4228

aws-cdk-automation requested a review from a team August 18, 2024 00:59

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Aug 18, 2024

aws-cdk-automation previously requested changes Aug 18, 2024

View reviewed changes

badmintoncryer added 5 commits August 18, 2024 10:13

add integ test

d4fd1c9

add unit test

2c51c2f

chore

e761741

chore

531e6de

add readme

5ff5f18

badmintoncryer marked this pull request as ready for review August 18, 2024 01:32

badmintoncryer commented Aug 18, 2024

View reviewed changes

github-actions bot mentioned this pull request Aug 19, 2024

Weekly PR metrics report #31144

Closed

badmintoncryer added 2 commits August 19, 2024 21:13

Merge branch 'main' into 31130-luster

1f4deec

update readme

3a75639

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

badmintoncryer added 3 commits August 23, 2024 20:50

Merge branch 'main' into 31130-luster

1faa73f

Merge branch 'main' into 31130-luster

ae7f272

Merge branch 'main' into 31130-luster

3db6e3f

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

Monthly PR metrics report #31279

Closed

badmintoncryer added 2 commits September 7, 2024 00:46

Merge branch 'main' into 31130-luster

5040122

Merge branch 'main' into 31130-luster

0345d15

mazyu36 reviewed Sep 21, 2024

View reviewed changes

badmintoncryer added 2 commits September 21, 2024 23:05

update

85de35c

Merge branch 'main' into 31130-luster

e6d16a3

mazyu36 approved these changes Sep 22, 2024

View reviewed changes

badmintoncryer added 2 commits September 24, 2024 09:00

Merge branch 'main' into 31130-luster

0cad430

Merge branch 'main' into 31130-luster

b4f893e

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 Sep 24, 2024

Merge branch 'main' into 31130-luster

eed44aa

GavinZZ previously requested changes Oct 16, 2024

View reviewed changes

badmintoncryer added 2 commits October 17, 2024 06:45

update jsdoc

75dcaae

add comments

e040394

Merge remote-tracking branch 'origin/main' into 31130-luster

2f8987f

GavinZZ approved these changes Oct 16, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 16, 2024

Merge branch 'main' into 31130-luster

afbc949

mergify bot merged commit 252cca9 into aws:main Oct 16, 2024

github-actions bot locked as resolved and limited conversation to collaborators Oct 16, 2024

Conversation

badmintoncryer commented Aug 18, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazyu36 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

badmintoncryer commented Sep 21, 2024

Uh oh!

mazyu36 left a comment

Choose a reason for hiding this comment

Uh oh!

badmintoncryer commented Sep 22, 2024

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Oct 16, 2024

Uh oh!

mergify bot commented Oct 16, 2024

Uh oh!

aws-cdk-automation commented Oct 16, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Oct 16, 2024

Uh oh!

github-actions bot commented Oct 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

badmintoncryer commented Sep 22, 2024 •

edited

Loading