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(aws-fsx): construct for NetApp ONTAP file system #42

Merged
merged 49 commits into from
Oct 5, 2024

Conversation

badmintoncryer
Copy link
Contributor

@badmintoncryer badmintoncryer commented Jul 20, 2024

Issue # (if applicable)

Closes #41.

Reason for this change

There is no L2 construct for creating FSx for NetApp ONTAP file system.

Description of changes

  • Add OntapFileSystem class
  • Add MaintenanceTime class
    • However LusterMaintenanceTime class already exists in aws-cdk-lib.aws-fsx module , it seems not to be used in other file systems such as NetApp ONTAP. Therefore, I newly added MaintenanceTime class.
new ocf.aws_fsx.OntapFileSystem(stack, 'OntapMultiAzFileSystem', {
  vpc,
  vpcSubnets: vpc.privateSubnets,
  storageCapacityGiB: 5120,
  ontapConfiguration: {
    automaticBackupRetention: Duration.days(7),
    dailyAutomaticBackupStartTime: new aws_fsx.DailyAutomaticBackupStartTime({
      hour: 1,
      minute: 0,
    }),
    deploymentType: ocf.aws_fsx.OntapDeploymentType.MULTI_AZ_2,
    diskIops: 15360,
    endpointIpAddressRange: '192.168.39.0/24',
    fsxAdminPassword: 'fsxPassword1',
    haPairs: 1,
    preferredSubnet: vpc.privateSubnets[0],
    routeTables: [vpc.privateSubnets[0].routeTable, vpc.privateSubnets[1].routeTable],
    throughputCapacity: 384,
    weeklyMaintenanceStartTime: new ocf.aws_fsx.MaintenanceTime({
      day: aws_fsx.Weekday.SUNDAY,
      hour: 1,
      minute: 0,
    }),
  },
  removalPolicy: RemovalPolicy.DESTROY,
});

The CI job failed due to a conflict with the peer dependency of aws-cdk-lib.
I would like to set the peer dependency version to the newest one.

https://github.com/open-constructs/aws-cdk-library/actions/runs/10023311572/job/27704071917?pr=42

Description of how you validated changes

Added both unit and integ tests.

Checklist


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

@badmintoncryer badmintoncryer marked this pull request as ready for review July 25, 2024 12:34
Copy link
Contributor

@go-to-k go-to-k left a comment

Choose a reason for hiding this comment

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

Thanks for your great contribution!

The amount of changes is too large to look at all at once, so I have commented on the few superficial things that need to be fixed. I would like to look at the core later when I have more time, but it may take some time to look at everything.

If you have links to detailed documentation that we reviewers should have seen beforehand, or your design or implementation intentions, it would be helpful if you could include them in the PR description or code comments. This would improve the quality and speed of my review and may make it easier for other reviewers to participate in the review.

src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/aws-fsx/README.md Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
@badmintoncryer
Copy link
Contributor Author

badmintoncryer commented Aug 3, 2024

@go-to-k Thank you for your review! I've addressed most of your comments (some are still pending and will be updated later). I will update my PR description to include links to the documentation or other relevant information. Please bear with me for a moment.

src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
@badmintoncryer
Copy link
Contributor Author

I'll add comments later.

.projenrc.ts Outdated Show resolved Hide resolved
@go-to-k
Copy link
Contributor

go-to-k commented Sep 12, 2024

Hi, @badmintoncryer

Thanks for the references! It's very helpful!

How are the revisions coming along? I will resume the review when you are ready. (I'm aware that you are still in the process of correcting the following and other points.)

#42 (comment)

@badmintoncryer
Copy link
Contributor Author

@go-to-k I'm really sorry for forgetting this issue😓 I'll resolve it and get back to you later.

@go-to-k go-to-k changed the title feat(fsx): construct for NetApp ONTAP file system feat(aws-fsx): construct for NetApp ONTAP file system Sep 14, 2024
Co-authored-by: Kenta Goto (k.goto) <[email protected]>
Signed-off-by: Kazuho Cryer-Shinozuka <[email protected]>
@go-to-k
Copy link
Contributor

go-to-k commented Sep 23, 2024

Could you resolve the conflicts?

src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
@badmintoncryer
Copy link
Contributor Author

@go-to-k
I was planning to reach out once everything had settled, but I appreciate your prompt response.

I've implemented the ThroughputCapacityPerHaPair class and added validation logic.

However, I’m not very confident about the overall approach, so I’d like your feedback. In particular, I’m not sure about the method name MB_PER_SEC_NUMBER, and I think it could be improved.

@go-to-k
Copy link
Contributor

go-to-k commented Oct 2, 2024

In particular, I’m not sure about the method name MB_PER_SEC_NUMBER, and I think it could be improved.

  • MB_PER_SEC_128
  • PER_SEC_128_MB
  • PER_HA_PAIR_128_MB
  • VALUE_128_MB_PER_SEC
  • SIZE_128_MB_PER_SEC
  • CAPACITY_128_MB_PER_SEC
  • THROUGHPUT_128_MB_PER_SEC
  • THROUGHPUT_CAPACITY_128_MB_PER_SEC
  • etc...

Hmmm... That's difficult... I prefer the format of XX_128_MB_PER_SEC or XX_128_MB, but MB_PER_SEC_128 is also good...

@go-to-k
Copy link
Contributor

go-to-k commented Oct 2, 2024

Please also change the README, JSDOC, and tests later (if we need to change anything to match your implementation).

@go-to-k
Copy link
Contributor

go-to-k commented Oct 2, 2024

Hmmm... That's difficult... I prefer the format of XX_128_MB_PER_SEC or XX_128_MB, but MB_PER_SEC_128 is also good...

Let's go with MB_PER_SEC_NUMBER for now!

Copy link
Contributor

@go-to-k go-to-k left a comment

Choose a reason for hiding this comment

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

Great changes, very good! I have made some comments. I will approve this PR if they are modified.

src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
src/aws-fsx/ontap-file-system.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@go-to-k go-to-k left a comment

Choose a reason for hiding this comment

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

Could confirmed all changes. Thanks for this PR. I approve and merge.

@go-to-k go-to-k added this pull request to the merge queue Oct 5, 2024
Merged via the queue into open-constructs:main with commit 3fe93f5 Oct 5, 2024
6 checks passed
@badmintoncryer
Copy link
Contributor Author

@go-to-k Thank you very much for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fsx: L2 construct for the FSx for NetApp ONTAP file system
4 participants