Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[AWS SageMaker] Add working FSx setup and test #3831
[AWS SageMaker] Add working FSx setup and test #3831
Changes from 6 commits
1fc97d8
c198722
c7b941b
1c70e24
c7fdedd
386a44a
66e3899
27c61d2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think we might leak the resource here if this command fails for some reasons...
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.
That's a very real possibility at this point in the code. This is after our error trap, so if this command fails we would have to see it failing in the logs to catch it. We might need to set up some alarm to ensure we don't have too many FSx instances in the account.
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.
is there a better condition based solution for this?
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 have not had much of a chance to play around with this. I think maybe once the FSx file system changes status to "Deleting" it would have detached from the VPC and security groups. Not entirely sure, this was a transient error.
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.
why did this move?
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.
We want to launch EKS at the same time as creating FSx because both operations take a long time. However, when we use the
&
operator, it starts the shell in a different process and therefore any variables set in that shell are not set in the caller shell. So I bypass this by setting the name of the cluster before calling.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.
this needs to be fixed in future, the test should automatically be skipped if fsx-id is not defined rather than explicitly needing to set it