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.

Sign up for GitHub

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

Jump to bottom

feat(core): add FSx for Lustre integration #461

Merged
merged 5 commits into from
Jul 28, 2021
Merged

feat(core): add FSx for Lustre integration #461

merged 5 commits into from
Jul 28, 2021

Conversation

jericht
Copy link
Contributor

@jericht jericht commented Jun 7, 2021 •
edited
Loading

Notes

Testing

  • Deployed most Linux distribution/versions (at least 1 per distribution) from the above link and verified that they are able to install the Lustre client and mount an FSxL.
  • Deployed the Repository construct with an FSxL as the backing filesystem and ensured the repository files were owned by UID:GID=1000:1000 and that the RCS had access to them

I deployed & tested:

  • Amazon Linux 1
  • Amazon Linux 2
  • RHEL 7.5
  • RHEL 7.6
  • RHEL 7.7
  • RHEL 7.8
  • RHEL 7.9
  • RHEL 8.2
  • RHEL 8.3
  • CentOS 7.7
  • Ubuntu 16.04
  • Ubuntu 18.04
  • Ubuntu 20.04
  • SUSE 12 SP5
  • Amazon Linux 2 (ARM)
  • Ubuntu 18.04 (ARM)
  • Ubuntu 20.04 (ARM)

I was not able to test the following:

  • SUSE 12 SP3 & SP4
    • I wasn't able to find AMIs with these versions of SUSE Linux and wasn't able to find a way to downgrade from 12 SP5. Only alternative I know of is to install the operating system from scratch.
  • RHEL 7.7, 7.8, and 7.9 (ARM)
    • I wasn't able to find AMIs with these versions and wasn't able to find a way to upgrade a RHEL 7.6 ARM-based AMI to the above versions.

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

@jericht jericht force-pushed the jericht/add_mountable_fsx branch 3 times, most recently from 9a653d6 to 82cfda6 Compare June 7, 2021 19:01
@jericht jericht marked this pull request as ready for review June 7, 2021 20:32
@jericht jericht force-pushed the jericht/add_mountable_fsx branch from 82cfda6 to 1c52c41 Compare June 7, 2021 20:41
@jusiskin jusiskin added the contribution/core This is a PR that came from AWS. label Jun 7, 2021
@jericht jericht force-pushed the jericht/add_mountable_fsx branch from 1c52c41 to 814213c Compare June 7, 2021 23:52
@horsmand horsmand self-requested a review June 16, 2021 22:22
horsmand
horsmand suggested changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@horsmand horsmand left a comment

Choose a reason for hiding this comment

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

Looks good for the most part, I just found the bash a little hard to follow at some points. Additional comments would help.

packages/aws-rfdk/lib/core/lib/mountable-fsx-lustre.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/core/lib/mountable-fsx-lustre.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/core/lib/mountable-fsx-lustre.ts Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/core/scripts/bash/installLustreClient.sh Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/core/scripts/bash/installLustreClient.sh Show resolved Hide resolved
packages/aws-rfdk/lib/core/scripts/bash/installLustreClient.sh Outdated
8.2|8.[1-9]*([0-9]))
case $kernel_version in
${rhel_kernel_versions[8.2]}) replace_ver="8.2";;
${rhel_kernel_versions[8.3]} replace_ver="8.3";;
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no 8.3 in the array. I also see the RHEL article you linked to has an entry for 8.4, should we add that too? Will we need to manually update this whenever a new version of RHEL is released?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah I missed that, added in an entry for 8.3. I went ahead and added an entry for 8.4, but we technically don't need it yet since it's the latest release, so we don't have to download a specific version yet.

Unfortunately, yes. We will need to manually update this whenever a new version of RHEL is released, assuming the FSx Lustre docs also update the instructions with the new kernel versions added.

packages/aws-rfdk/lib/core/scripts/bash/installLustreClient.sh Show resolved Hide resolved
packages/aws-rfdk/lib/core/scripts/bash/installLustreClient.sh Outdated
Comment on lines 163 to 173
if [[ -n "${replace_ver+x}" ]]; then
sudo sed -i "s#8#${replace_ver}#" /etc/yum.repos.d/aws-fsx.repo
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment on what is going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added a brief comment explaining what it does. For more context, the contents of aws-fsx.repo will look something like this:

[aws-fsx]
name=AWS FSx Packages - $basearch
baseurl=https://fsx-lustre-client-repo.s3.amazonaws.com/el/8/$basearch/
enabled=1
gpgcheck=1

[aws-fsx-src]
name=AWS FSx Source
baseurl=https://fsx-lustre-client-repo.s3.amazonaws.com/el/8/SRPMS/
enabled=1
gpgcheck=1

This changes the baseurl to point to a URL that has the right versions of the lustre client packages for the kernel we're on

packages/aws-rfdk/lib/core/scripts/bash/installLustreClient.sh Show resolved Hide resolved
@jericht jericht force-pushed the jericht/add_mountable_fsx branch from 814213c to 2f470f1 Compare July 8, 2021 21:43
@jericht jericht requested a review from horsmand July 8, 2021 23:28
@jericht jericht force-pushed the jericht/add_mountable_fsx branch from 11f82ca to 09d74b6 Compare July 27, 2021 18:56
@jericht jericht force-pushed the jericht/add_mountable_fsx branch from 09d74b6 to eb9ec56 Compare July 27, 2021 21:52
ddneilson
ddneilson approved these changes Jul 28, 2021
View reviewed changes
Copy link
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

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

LGTM

@horsmand horsmand merged commit bf5bbb9 into aws:mainline Jul 28, 2021
@jericht jericht mentioned this pull request Jul 30, 2021
Closed
@jusiskin jusiskin mentioned this pull request Aug 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@horsmand horsmand horsmand approved these changes

@ddneilson ddneilson ddneilson approved these changes

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jericht @ddneilson @horsmand @jusiskin