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(fsx): specify file system type version for the Lustre file system #31136

Merged
merged 22 commits into from
Oct 16, 2024
Merged
Changes from 1 commit
Commits
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
44 changes: 44 additions & 0 deletions packages/aws-cdk-lib/aws-fsx/lib/lustre-file-system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,26 @@ import { LustreMaintenanceTime } from './maintenance-time';
import { Connections, ISecurityGroup, ISubnet, Port, SecurityGroup } from '../../aws-ec2';
import { Aws, Duration, Token } from '../../core';

/**
* The Lustre version for the file system.
*/
export enum FileSystemTypeVersion {
/**
* Version 2.10
*/
V_2_10 = '2.10',

/**
* Version 2.12
*/
V_2_12 = '2.12',

/**
* Version 2.15
*/
V_2_15 = '2.15',
}

/**
* The different kinds of file system deployments used by Lustre.
*/
Expand Down Expand Up @@ -181,6 +201,13 @@ export interface LustreFileSystemProps extends FileSystemProps {
*/
readonly lustreConfiguration: LustreConfiguration;

/**
* The Lustre version for the file system.
*
* @default - V_2_10, except for PERSISTENT_2 deployment type, where it is V_2_12 without metadata configuration mode and V_2_15 with metadata configuration mode.
*/
readonly fileSystemTypeVersion?: FileSystemTypeVersion;

/**
* The subnet that the file system will be accessible from.
*/
Expand Down Expand Up @@ -289,6 +316,7 @@ export class LustreFileSystem extends FileSystemBase {
lustreConfiguration,
securityGroupIds: [securityGroup.securityGroupId],
storageCapacity: props.storageCapacityGiB,
fileSystemTypeVersion: props.fileSystemTypeVersion,
});
this.fileSystem.applyRemovalPolicy(props.removalPolicy);

Expand Down Expand Up @@ -316,6 +344,22 @@ export class LustreFileSystem extends FileSystemBase {
this.validateAutomaticBackupRetention(deploymentType, lustreConfiguration.automaticBackupRetention);

this.validateDailyAutomaticBackupStartTime(lustreConfiguration.automaticBackupRetention, lustreConfiguration.dailyAutomaticBackupStartTime);
this.validateFiileSystemTypeVersion(deploymentType, props.fileSystemTypeVersion);
}

/**
* Validates the file system type version
*/
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');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we also validate



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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}

/**
Expand Down
Loading