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

Add storage_type to aws_fsx_lustre_file_system #14727

Conversation

mtpdt
Copy link
Contributor

@mtpdt mtpdt commented Aug 18, 2020

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #14692

Release note for CHANGELOG:

resource/aws_fsx_lustre_file_system: Add `storage_type` and `drive_cache_type`

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCache*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCache* -timeout 120m
=== RUN   TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead
=== PAUSE TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead
=== RUN   TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone
=== PAUSE TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone
=== CONT  TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead
=== CONT  TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone
--- PASS: TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead (567.91s)
--- PASS: TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone (588.92s)
PASS
ok   github.com/terraform-providers/terraform-provider-aws/aws 588.977s

...

@ghost ghost added size/S Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. documentation Introduces or discusses updates to documentation. service/fsx Issues and PRs that pertain to the fsx service. labels Aug 18, 2020
@mtpdt mtpdt force-pushed the f-aws_fsx_lustre_file_system-storage_type branch from 2531b11 to a37e2bd Compare August 18, 2020 22:19
@ghost ghost added size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/S Managed by automation to categorize the size of a PR. labels Aug 18, 2020
@mtpdt mtpdt force-pushed the f-aws_fsx_lustre_file_system-storage_type branch 6 times, most recently from de85e3e to d290e63 Compare August 20, 2020 19:40
@mtpdt mtpdt marked this pull request as ready for review August 20, 2020 20:24
@mtpdt mtpdt requested a review from a team August 20, 2020 20:24
@mtpdt
Copy link
Contributor Author

mtpdt commented Aug 20, 2020

@DrFaust92 I have another feature PR for FSX lustre.

@DrFaust92 DrFaust92 added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Aug 24, 2020
@DrFaust92
Copy link
Collaborator

hey @mtpdt can you rebase?

@mtpdt mtpdt force-pushed the f-aws_fsx_lustre_file_system-storage_type branch 4 times, most recently from 20b38cf to 83f1101 Compare September 23, 2020 02:55
@mtpdt
Copy link
Contributor Author

mtpdt commented Sep 23, 2020

@DrFaust92 rebased

Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

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

some docs comments, also lets add some docs for the added possible values for PerUnitStorageThroughput

@@ -36,6 +36,8 @@ The following arguments are supported:
* `kms_key_id` - (Optional) ARN for the KMS Key to encrypt the file system at rest, applicable for `PERSISTENT_1`. Defaults to an AWS managed KMS Key.
* `per_unit_storage_throughput` - (Optional) - Describes the amount of read and write throughput for each 1 tebibyte of storage, in MB/s/TiB, required for the `PERSISTENT_1` deployment_type. For valid values, see the [AWS documentation](https://docs.aws.amazon.com/fsx/latest/APIReference/API_CreateFileSystemLustreConfiguration.html).
* `automatic_backup_retention_days` - (Optional) The number of days to retain automatic backups. Setting this to 0 disables automatic backups. You can retain automatic backups for a maximum of 35 days. only valid for `PERSISTENT_1` deployment_type.
* `storage_type` - (Optional) - The filesystem storage type. Either `SSD` or `HDD`, defaults to `SSD`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets add a comment that HDD is only relevant for persistent deployment type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -36,6 +36,8 @@ The following arguments are supported:
* `kms_key_id` - (Optional) ARN for the KMS Key to encrypt the file system at rest, applicable for `PERSISTENT_1`. Defaults to an AWS managed KMS Key.
* `per_unit_storage_throughput` - (Optional) - Describes the amount of read and write throughput for each 1 tebibyte of storage, in MB/s/TiB, required for the `PERSISTENT_1` deployment_type. For valid values, see the [AWS documentation](https://docs.aws.amazon.com/fsx/latest/APIReference/API_CreateFileSystemLustreConfiguration.html).
* `automatic_backup_retention_days` - (Optional) The number of days to retain automatic backups. Setting this to 0 disables automatic backups. You can retain automatic backups for a maximum of 35 days. only valid for `PERSISTENT_1` deployment_type.
* `storage_type` - (Optional) - The filesystem storage type. Either `SSD` or `HDD`, defaults to `SSD`.
* `drive_cache_type` - (Optional) - Required for `HDD` storage_type, whether to use an SSD drive cache, either `READ` or `NONE`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add some more info here: start with The type of drive cache used by PERSISTENT_1 file systems that are provisioned with HDD storage devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, added your sentence, and modified the end of it.

@DrFaust92
Copy link
Collaborator

tests are passing other than doc comments:

--- PASS: TestAccAWSFsxLustreFileSystem_basic (534.61s)
--- PASS: TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead (588.73s)
--- PASS: TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone (569.74s)

@mtpdt mtpdt force-pushed the f-aws_fsx_lustre_file_system-storage_type branch from 83f1101 to 554cf94 Compare September 24, 2020 03:27
@mtpdt mtpdt force-pushed the f-aws_fsx_lustre_file_system-storage_type branch from 554cf94 to 67e07a6 Compare September 24, 2020 03:29
@mtpdt
Copy link
Contributor Author

mtpdt commented Sep 24, 2020

Added documentation for the possible values for PerUnitStorageThroughput

Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

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

LGTM.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSFsxLustreFileSystem_basic\|TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCache'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSFsxLustreFileSystem_basic\|TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCache -timeout 120m
=== RUN   TestAccAWSFsxLustreFileSystem_basic
=== PAUSE TestAccAWSFsxLustreFileSystem_basic
=== RUN   TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead
=== PAUSE TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead
=== RUN   TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone
=== PAUSE TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone
=== CONT  TestAccAWSFsxLustreFileSystem_basic
=== CONT  TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone
=== CONT  TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead
--- PASS: TestAccAWSFsxLustreFileSystem_basic (440.00s)
--- PASS: TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone (517.70s)
--- PASS: TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead (600.53s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	600.578s

@breathingdust
Copy link
Member

LGTM! 🚀 Thanks @mtpdt

Verified Acceptance Tests in Commercial (us-west-2)

make testacc TEST=./aws TESTARGS='-run=TestAccAWSFsxLustreFileSystem_basic\|TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCache'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSFsxLustreFileSystem_basic\|TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCache -timeout 120m
=== RUN   TestAccAWSFsxLustreFileSystem_basic
=== PAUSE TestAccAWSFsxLustreFileSystem_basic
=== RUN   TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead
=== PAUSE TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead
=== RUN   TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone
=== PAUSE TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone
=== CONT  TestAccAWSFsxLustreFileSystem_basic
=== CONT  TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone
=== CONT  TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead
--- PASS: TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheNone (458.16s)
--- PASS: TestAccAWSFsxLustreFileSystem_basic (592.90s)
--- PASS: TestAccAWSFsxLustreFileSystem_StorageTypeHddDriveCacheRead (599.95s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	601.545s

@breathingdust breathingdust added this to the v3.9.0 milestone Sep 30, 2020
@breathingdust breathingdust merged commit dd05397 into hashicorp:master Sep 30, 2020
breathingdust added a commit that referenced this pull request Sep 30, 2020
@DrFaust92
Copy link
Collaborator

@breathingdust @YakDriver FYI, this is missing the precheck for regions the dont support FSx. ill take care of this.

@DrFaust92
Copy link
Collaborator

Handled in #15299

@ghost
Copy link

ghost commented Oct 2, 2020

This has been released in version 3.9.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Oct 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 31, 2020
@mtpdt mtpdt deleted the f-aws_fsx_lustre_file_system-storage_type branch January 4, 2021 19:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/fsx Issues and PRs that pertain to the fsx service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support storage_type for aws_fsx_lustre_file_system
4 participants