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 FSx for Lustre AutoImport support #158
Add FSx for Lustre AutoImport support #158
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @gkao123! |
Hi @gkao123. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
5985697
to
a932e67
Compare
a932e67
to
93ac0ff
Compare
/ok-to-test |
s3ImportPath: s3://ml-training-data-000 | ||
s3ExportPath: s3://ml-training-data-000/export | ||
deploymentType: SCRATCH_2 | ||
``` | ||
* subnetId - the subnet ID that the FSx for Lustre filesystem should be created inside. | ||
* securityGroupIds - a common separated list of security group IDs that should be attached to the filesystem. | ||
* autoImportPolicy - the policy FSx will follow that determines how the filesystem is automatically updated with changes made in the linked data repository. For list of acceptable policies, please view the official FSx for Lustre documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried looking for the documentation myself but couldn't find it. I found https://docs.aws.amazon.com/fsx/latest/LustreGuide/autoimport-data-repo.html but not the strings that each policy corresponds to. Is it OK to link to documentation here or is it a moving target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can link to this documentation: https://docs.aws.amazon.com/fsx/latest/APIReference/API_CreateFileSystemLustreConfiguration.html!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be perfect thanks
lgtm I just have one nitpick about the doc on what values are valid because I couldn't find it myself ... /approve |
@@ -158,6 +158,7 @@ var _ = Describe("[fsx-csi-e2e] Dynamic Provisioning with s3 data repository", f | |||
Parameters: map[string]string{ | |||
"subnetId": subnetId, | |||
"securityGroupIds": strings.Join(securityGroupIds, ","), | |||
"autoImportPolicy": "NEW_CHANGED" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think a comma is missing here.
Also might need to run gofmt, the "verify" test complained about it
93ac0ff
to
8c82a3d
Compare
go.mod
Outdated
@@ -1,7 +1,7 @@ | |||
module github.com/kubernetes-sigs/aws-fsx-csi-driver | |||
|
|||
require ( | |||
github.com/aws/aws-sdk-go v1.29.9 | |||
github.com/aws/aws-sdk-go v1.35.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try also to run 'go mod tidy', i think go.sum should have been updated too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran go mod tidy and updated the pr with go.sum. I also verified go make still works.
8c82a3d
to
c659501
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gkao123, wongma7 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @wongma7, the e2e tests seem to have failed due to the following error:
This seems unrelated to my code change. How should we resolve this error? |
I'm cleaning up the test account, some old file systems are still in Deleting state, I'll retest when they're deleted |
/retest should be enough room already |
Thank you! |
One of the test failed, not sure if it's a flake because it does involve importing from S3: "Dynamic Provisioning with s3 data repository should create a volume on demand with s3 as data repository" /retest |
/retest |
@wongma7 I noticed in https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-fsx-csi-driver/158/pull-aws-fsx-csi-driver-e2e/1314354691160674304 that the resources are not being successfully deleted after the test is completed. Do you know if this could cause the test failure? |
/retest |
c659501
to
1f43285
Compare
1f43285
to
44cc44b
Compare
/lgtm |
Is this a bug fix or adding new feature?
This pull request adds support for AutoImport, a new FSxL feature. For more details, please view: https://docs.aws.amazon.com/fsx/latest/LustreGuide/autoimport-data-repo.html
What testing is done?
End to end testing