feat(aws-fsx): L2 construct for FSx for Lustre #6653

horsmand · 2020-03-10T17:12:03Z

Description

I wrote the L2 construct for FSx for Lustre. This can be found in lustre-file-system.ts. I didn't implement FSx for Windows, but tried to split out anything that would be shareable between the two implementations into file-system.ts.

I verified this through 100% unit test coverage, along with some integration tests. I've included one integration test in the code called integ.lustre-file-system.ts that does basic setup. I also wrote a more in-depth integ test that I haven't included here but used as example code in the Mounting section of the README. I did the same for the import instructions as well, there's no included integ test but I did write one to verify it and include parts of it in the README.

Commit Message

feat(aws-fsx): L2 construct for FSx for Lustre (#6653)

Created the L2 construct for FSx for Lustre with the ability to create or import a file system and give access to it through Connections.

End Commit Message

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

aws-cdk-automation · 2020-03-10T18:06:10Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 52fbf17
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-03-10T19:04:19Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 930e5e4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-03-12T17:19:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 361f9f2
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-03-13T16:12:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b627f3f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2020-03-16T09:00:22Z

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

+};
+
+const fileSystem = new FsxLustreFileSystem(stack, 'FsxLustreFileSystem', {
+  lustreConfiguration,


I think in terms of example I would prefer inlining the config here rather than put them in separate variables.

rix0rrr · 2020-03-16T09:00:57Z

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

+
+const fileSystem = new FsxLustreFileSystem(stack, 'FsxLustreFileSystem', {
+  lustreConfiguration,
+  storageCapacity,


A unitless number scares me. Can we make this storageCapacityGiB, storageCapacityGB, storageCapacityTiB, or whatever the case may be?

I've inlined this along with the lustreConfiguration so there is no variable and line is storageCapacity: 1200. Would you still rather have a variable named storageCapacityGiB?

Did you mean rename the attribute itself to storageCapacityGiB? I could do that but it would be inconsistent with how CloudFormation names it.

Yes, I meant rename the property. I'm okay taking the hit being not exactly the same. The prefix is the same (so people typing will autocomplete to the prop they expect) and I feel the additional clarity is worth it.

Not all AWS services are consistent with one another, but we can at least be consistent within the CDK.

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

rix0rrr · 2020-03-16T09:05:23Z

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

+const vpc = new Vpc(stack, 'VPC');
+
+// Create a security group to share across the file system and ec2 instance and open the FSx ports.
+const securityGroup = new SecurityGroup(stack, 'FsxLustreSecurityGroup', {


You seem to be going against your own advice of the previous section here. Quote:

FSx has a fixed default port, so you don't need to specify the port.

Also, generally you wouldn't share an SG across instance and filesystem. As soon as you attach your instance to a load balancer, you would implicitly open port 8000 (or whatever) on the filesystem as well.

This example is rather convoluted due to the way the EC2 instance needs to look the file system mount name up for itself.

The problem I was trying to avoid here was having the EC2 instance come up before the file system is up and the mount name is retrievable from the AWS CLI. To do that, I added the dependency on the file system from the instance. If I let both resources create their own security groups and tried to add this dependency, it would be circular, as the file system already attempts to add the instance's security group to its Connections.

Removing this dependency would simplify the example a fair bit, but there's always the chance the instance runs its UserData before the file system is initialized and the mount fails.

Oh I see. The security groups are accidentally included in the dependency set when you do addDependency huh?

Let's replace it with:

// Depend on the filesystem resource, not any of the security groups and security group rules instance.addDependency(fileSystem.node.defaultChild);

Which I agree is not great, but that's more of a symptom of the dependency system. We can address that in due time, but that's no reason to make this example make people do things we actually don't want them to do.

This worked.

rix0rrr · 2020-03-16T09:07:21Z

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

+inst.node.addDependency(fs);
+
+const mountPath = '/mnt/fsx';
+const dnsName = `${fs.fileSystemId}.fsx.${stack.region}.amazonaws.com`;


This could have been a property on the FsxFileSystem class, would come in very useful. Don't forget to use Aws.URL_SUFFIX for the amazonaws.com part.

I've added this property.

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

Pull request has been modified.

aws-cdk-automation · 2020-03-17T21:12:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ab44cff
Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2020-03-18T11:13:33Z

I think you might have missed some comments of mine as they were hidden by GitHub by default.

philxiao · 2020-03-31T21:36:46Z

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

+
+const fileSystem = new FsxLustreFileSystem(stack, 'FsxLustreFileSystem', {
+  lustreConfiguration: { deploymentType: FsxLustreDeploymentType.SCRATCH_2 },
+  storageCapacity: 1200,


I am not quite familiar with the use case. Just to clarify, did we choose 1200 because this is the smallest offering?

Yes, there was no other reason for using 1200.

philxiao · 2020-03-31T21:37:25Z

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

+securityGroup.addIngressRule(Peer.anyIpv4(), Port.tcp(988), 'Default FSx port ingress');
+securityGroup.addEgressRule(Peer.anyIpv4(), Port.tcp(988), 'Default FSx port egress');


This is not sufficient. Please add port 1021-1023 to it as well. See updated guidelines in https://docs.aws.amazon.com/fsx/latest/LustreGuide/limit-access-security-groups.html

philxiao · 2020-03-31T21:38:30Z

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

+inst.userData.addCommands(
+  'set -eux',
+  'yum update -y',
+  'amazon-linux-extras install -y lustre2.10',


The lustre version will change over time. Maybe worth thinking how you would want to version control you cdk to meet with the lustre version you need.

This is example code in a README, not managed on behalf of the user.

Looks like the current iteration of this is punting the version management to the user.

Is there a way the latest version could be automatically retrieved, which we can recommend or make available to users instead?

I tried looking into it a little more but couldn't find any documentation about why 2.10 is being used rather than a more recent version. There's also no way to tie it to the latest version, unless amazon-linux-extras ins't used. This will at least always pick up the most recent release of 2.10.

I got the following information about why AL2 is distributing Lustre 2.10:

FSx for Lustre currently serves only Lustre 2.10 and we only recommend Lustre 2.10 clients for our customers. The Lustre 2.10 kernel modules are baked directly into the AL2 kernel, which is why we currently only vend Lustre 2.10 in the amazon-linux-extras repo (only the Lustre 2.10 userspace tools are included in the Lustre2.10 package in the repo).
I think FSx for Lustre may be unique here. Using a Lustre 2.12 client, for example, may give you trouble when using FSx for Lustre backed by a 2.10 server.

philxiao · 2020-03-31T21:45:51Z

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

+  readonly perUnitStorageThroughput?: number;
+
+  /**
+   * The preferred time to perform weekly maintenance, in the UTC time zone, 24 hour clock. For example: '2:20:30'.


the first digit is the day of the week, say 2:20:30 means Tuesday at 20:30

aws-cdk-automation · 2020-04-02T16:05:31Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9578da1
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-03T18:06:07Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: be8c0f0
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-08T01:03:11Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 925c59d
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-08T01:49:29Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: a0189b6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-08T15:37:33Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 1dccc8a
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-09T16:06:32Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 619f340
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-13T22:10:58Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b0fcbb0
Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr

Some questions about the code in the example (I think it's doing a couple things it doesn't need to, or at least SHOULDN'T need to).

Other than that, as good as ready to ship!

(P.S: You don't have to keep merging from master by hand, we have a bot that will do that automatically when we approve)

rix0rrr · 2020-04-14T08:46:56Z

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

+    subnetType: SubnetType.PUBLIC,
+  }
+});
+inst.connections.securityGroups.forEach((securityGroup => {


Wait, what does this do? This seems very wrong.

I was trying to figure out a way to get all the ports allowed for a set of them that wasn't consecutive. The default port on Connections only accepts a single port or a range. FSx requires 988, 1021, 1022, 1023. To get around this I've changed it to open ports 988-1023. This isn't ideal, but I don't see a better solution.

rix0rrr · 2020-04-14T08:48:31Z

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

+
+// Add a dependency on the file system to the instance so that we can be sure the file system is instantiated before querying
+// for its mount name.
+if (fs.node.defaultChild) {


Is this necessary though? CloudFormation should be taking care of this automatically.

It doesn't automatically create the dependency, but it does seem to create the EC2 instance after the file system is created. I'll remove this.

Use of a { Ref } or { Fn::GetAtt } will automatically create a dependency. You only need to use DependsOn if the dependency is implicit, because creating the resource has some side effect (such as Policys usually do).

rix0rrr · 2020-04-14T08:52:11Z

packages/@aws-cdk/aws-fsx/lib/maintenance-time.ts

+/**
+ * Class for scheduling a weekly manitenance time.
+ */
+export class LustreMaintenanceTime {


Pull request has been modified.

aws-cdk-automation · 2020-04-22T15:05:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: e6a2fb7
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-23T15:43:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b45e364
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-23T22:46:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 0a906ee
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-04-30T16:31:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: c4588b2
Result: FAILED
Build Logs (available for 30 days)

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

horsmand · 2020-04-30T21:59:39Z

FSx has exposed the Lustre file system's mount name through CloudFormation, so I was able to add this as field to the LustreFileSystem Construct and then update my mounting example in the README to use this instead of having to update the AWS CLI version on the EC2 instance and then parse the mount name out of a CLI call.

aws-cdk-automation · 2020-04-30T22:38:55Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: d524edc
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-05-04T13:36:34Z

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-cdk-automation · 2020-05-04T14:21:55Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 38b4719
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-05-04T14:22:07Z

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

SomayaB assigned rix0rrr Mar 10, 2020

rix0rrr previously requested changes Mar 16, 2020

View reviewed changes

philxiao suggested changes Mar 31, 2020

View reviewed changes

feat(aws-fsx): L2 construct for FSx for Lustre

a0189b6

horsmand force-pushed the fsx-lustre-l2 branch from 925c59d to a0189b6 Compare April 8, 2020 01:18

Merge branch 'master' into fsx-lustre-l2

1dccc8a

Merge branch 'master' into fsx-lustre-l2

619f340

Merge branch 'master' into fsx-lustre-l2

b0fcbb0

rix0rrr previously requested changes Apr 14, 2020

View reviewed changes

Merge branch 'master' into fsx-lustre-l2

e6a2fb7

Merge branch 'master' into fsx-lustre-l2

b45e364

Addressing PR comments

0a906ee

Merge branch 'master' into fsx-lustre-l2

c4588b2

Added Support of LustreMountName

d524edc

rix0rrr approved these changes May 4, 2020

View reviewed changes

Merge branch 'master' into fsx-lustre-l2

38b4719

mergify bot merged commit 7363912 into aws:master May 4, 2020

		securityGroup.addIngressRule(Peer.anyIpv4(), Port.tcp(988), 'Default FSx port ingress');
		securityGroup.addEgressRule(Peer.anyIpv4(), Port.tcp(988), 'Default FSx port egress');

feat(aws-fsx): L2 construct for FSx for Lustre #6653

feat(aws-fsx): L2 construct for FSx for Lustre #6653

Conversation

horsmand commented Mar 10, 2020 • edited

Description

Commit Message

End Commit Message

aws-cdk-automation commented Mar 10, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 10, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 12, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 13, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Mar 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Mar 17, 2020

AWS CodeBuild CI Report

rix0rrr commented Mar 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 2, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 3, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 8, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 8, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 8, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 9, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 13, 2020

AWS CodeBuild CI Report

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 22, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 23, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 23, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 30, 2020

AWS CodeBuild CI Report

horsmand commented Apr 30, 2020

aws-cdk-automation commented Apr 30, 2020

AWS CodeBuild CI Report

mergify bot commented May 4, 2020

aws-cdk-automation commented May 4, 2020

AWS CodeBuild CI Report

mergify bot commented May 4, 2020

horsmand commented Mar 10, 2020 •

edited

rix0rrr Mar 18, 2020 •

edited